Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/sys/unix: possible missed change in generated file ztypes_solaris_amd64.go #55997

Closed
nshalman opened this issue Oct 2, 2022 · 10 comments
Closed
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@nshalman
Copy link

nshalman commented Oct 2, 2022

What version of Go are you using (go version)?

$ go version
go1.19.1 solaris/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/nshalman/.cache/go-build"
GOENV="/home/nshalman/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="solaris"
GOINSECURE=""
GOMODCACHE="/home/nshalman/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="solaris"
GOPATH="/home/nshalman/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/nshalman/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/nshalman/go/pkg/tool/solaris_amd64"
GOVCS=""
GOVERSION="go1.19.1"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/nshalman/sys/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2608192519=/tmp/go-build -gno-record-gcc-switches"

What did you do?

As of commit f11e5e49a4ec85883999349b0dc6373df034397b I ran the following command in an unmodified checkout:

go tool cgo -godefs types_solaris.go | go run mkpost.go > ztypes_solaris_amd64.go

What did you expect to see?

No change to working tree.

What did you see instead?

I get the follow output from git diff:

diff --git a/unix/ztypes_solaris_amd64.go b/unix/ztypes_solaris_amd64.go
index c1a9b83..f1cb9ee 100644
--- a/unix/ztypes_solaris_amd64.go
+++ b/unix/ztypes_solaris_amd64.go
@@ -195,7 +195,7 @@ type IPv6Mreq struct {
 type Msghdr struct {
        Name         *byte
        Namelen      uint32
-       Iov          *Iovec
+       Iov          *_Ctype_struct_iovec
        Iovlen       int32
        Accrights    *int8
        Accrightslen int32

Is that a change that should be / have been applied to that file?

I was about to touch types_solaris.go for a different change and found this bit confusing.

I think this is related to go #52885 / https://go-review.googlesource.com/c/sys/+/412496

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 2, 2022
@gopherbot gopherbot added this to the Unreleased milestone Oct 2, 2022
@nshalman
Copy link
Author

nshalman commented Oct 2, 2022

Experimentation suggests that the autogenerated change is incorrect. I think this is something we would fix in mkpost.go, perhaps like so? :

diff --git a/unix/mkpost.go b/unix/mkpost.go
index 76075c3..5337eca 100644
--- a/unix/mkpost.go
+++ b/unix/mkpost.go
@@ -96,6 +96,11 @@ func main() {
                })
        }

+       if goos == "solaris" {
+               iovecspec := regexp.MustCompile(`_Ctype_struct_iovec`)
+               b = iovecspec.ReplaceAll(b, []byte("Iovec"))
+       }
+
        // Intentionally export __val fields in Fsid and Sigset_t
        valRegex := regexp.MustCompile(`type (Fsid|Sigset_t) struct {(\s+)X__(bits|val)(\s+\S+\s+)}`)
        b = valRegex.ReplaceAll(b, []byte("type $1 struct {${2}Val$4}"))

nshalman added a commit to nshalman/sys that referenced this issue Oct 2, 2022
Change-Id: Iaf1b90486c2b4c8227a81aa06abec8d26176566d
nshalman added a commit to nshalman/sys that referenced this issue Oct 2, 2022
Fixes golang/go#55997

Change-Id: Iaf1b90486c2b4c8227a81aa06abec8d26176566d
@nshalman
Copy link
Author

nshalman commented Oct 2, 2022

I will open a CR with nshalman/sys@7bb33b5

@gopherbot
Copy link

Change https://go.dev/cl/437356 mentions this issue: unix: add solaris Iovec cleanup in mkpost.go

@tklauser
Copy link
Member

tklauser commented Oct 3, 2022

/cc @ianlancetaylor

@tklauser
Copy link
Member

tklauser commented Oct 3, 2022

I wonder whether instead of defining type goIovec like https://go.dev/cl/412496 did and then modifying its use, we should instead directly modify the Base field of the original type Iovec (generated from struct iovec) in mkpost.go, akin to how we already change []int8 to []byte for several other types, e.g. here:

https://github.com/golang/sys/blob/f11e5e49a4ec85883999349b0dc6373df034397b/unix/mkpost.go#L135-L142

@nshalman
Copy link
Author

nshalman commented Oct 4, 2022

I wonder whether instead of defining type goIovec like https://go.dev/cl/412496 did and then modifying its use, we should instead directly modify the Base field of the original type Iovec (generated from struct iovec) in mkpost.go, akin to how we already change []int8 to []byte for several other types, e.g. here:

https://github.com/golang/sys/blob/f11e5e49a4ec85883999349b0dc6373df034397b/unix/mkpost.go#L135-L142

I defer to the expertise of others what the right solution to this is. I'd really love independent confirmation that this issue is real and not just something quirky on my system. If my solution is acceptable, the CR is there to be merged, but if a different approach is preferred, I'd love to let someone else fix it so that I can focus my limited spare time on the porting work that this interrupted.

Thank you for your time and attention!!

@ianlancetaylor
Copy link
Contributor

I think that @tklauser is right and that CL 412496 (which I wrote) was wrong. We should try to copy the existing int8 -> byte code in mkpost.go.

@tklauser tklauser removed their assignment Oct 5, 2022
@nshalman
Copy link
Author

nshalman commented Oct 6, 2022

I think that @tklauser is right and that CL 412496 (which I wrote) was wrong. We should try to copy the existing int8 -> byte code in mkpost.go.

I've abandoned https://go.dev/cl/437356 accordingly.

@cagedmantis cagedmantis added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 6, 2022
@tklauser tklauser self-assigned this Apr 13, 2023
@gopherbot
Copy link

Change https://go.dev/cl/484635 mentions this issue: unix: convert Iovec.Base to *byte in mkpost.go on solaris

@nshalman
Copy link
Author

Thank you, @tklauser!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants