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.

Sign up for GitHub

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

Jump to bottom

cmd/cgo: offset generated as padding #61707

Closed
cosnicolaou opened this issue Aug 2, 2023 · 11 comments
Closed

cmd/cgo: offset generated as padding #61707

cosnicolaou opened this issue Aug 2, 2023 · 11 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@cosnicolaou
Copy link
Contributor

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

$ go version
go version go1.20.2 darwin/arm64

Does this issue reproduce with the latest release?

Yes, and with head.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/cnicolaou/Library/Caches/go-build"
GOENV="/Users/cnicolaou/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/cnicolaou/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/cnicolaou/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/homebrew/Cellar/go/1.20.2/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.20.2/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20.2"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/cnicolaou/Dropbox/go-dev/go/src/go.mod"
GOWORK="/Users/cnicolaou/Dropbox/go-dev/go/go.work"
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/d6/chtwn8h176z9_pf85ym_f_600000gn/T/go-build3910481843=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

While developing https://go-review.googlesource.com/c/go/+/503919 it is necessary to generate the go struct corresponding to the C struct vm_region_basic_info_data_64_t. Unfortunately the generated output is incorrect forcing a hand-edit of the generated code.

To reproduce the problem use:

cd $GOROOT/src/runtime/pprof
go tool cgo -godefs defs_darwin.go > tmp-go
diff defs_darwin_arm64.go tmp-go

What did you expect to see?

The offset field in the original C struct should appear as Offset uint64 in the generated output.

What did you see instead?

The generated code containsPad_cgo_0 [8]byte rather than the named field.
@seankhliao seankhliao changed the title affected/package: cmd/cgo cmd/cgo: offset generated as padding Aug 2, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 2, 2023
@mknyszek
Copy link
Contributor

mknyszek commented Aug 2, 2023

In triage, our theory is that because the offset field is defined as memory_object_offset_t (https://github.com/apple/darwin-xnu/blob/main/osfmk/mach/vm_region.h#L81C2-L81C24), cgo is just giving up because it can't find a header file or something.

@mknyszek mknyszek added this to the Backlog milestone Aug 2, 2023
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 2, 2023
@cherrymui
Copy link
Member

cherrymui commented Aug 2, 2023 •
edited
Loading

But that is defined in mach/memory_object_types.h, which is #include'd in vm_region.h. I also tried manually #include mach/memory_object_types.h in defs_darwin.go but that didn't help either. Other fields also have types from #include'd files, like vm_prot_t. Not sure why that one is different...

@ianlancetaylor
Copy link
Contributor

It's an alignment problem. The offset field is type unsigned long long. In the C struct it appears at byte offset 20. In Go the corresponding type is uint64. A uint64 field in Go is always aligned at an 8 byte boundary. Therefore, the field can't be represented correctly in a Go struct, so cgo falls back to adding padding bytes.

The reason C is using a 4 byte alignment rather than an 8 alignment is because the struct is defined within the scope of a #pragma pack(push, 4). You can see this near the top of <mach/vm_region.h>.

Hand editing the code to change the type of field to uint64 is going to produce a Go struct that does not match the C struct. That is, passing an instance of the Go struct to C code will not pass the right values. I see that a pointer is currently being passed to mach_vm_region; I would guess that Go sees the wrong values for the Offset, Behavior, and User_wired_count fields.

There is no really good workaround for this kind of thing. One approach is to hand write the struct in defs_darwin.go, but write the Offset field as [2]uint32. Then also pick up the struct from the C file as we do today. Then write an init function that uses unsafe.Offsetof to verify that the field offsets are the same in the hand written struct and the C struct, and panic if they differ. Normally the init function will compile down to nothing. In the code that uses the Offset field, manually convert from [2]uint32 to uint64.

Closing this issue because I don't think there is anything to do in the Go toolchain. Please comment if you disagree.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Aug 4, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Aug 4, 2023
@cherrymui
Copy link
Member

cherrymui commented Aug 4, 2023 •
edited
Loading

Thanks. This makes sense. I forgot to check the alignment. I'll send a CL to change it back to [8]byte and convert to uint64 manually.

That said, would it be better for the cgo command to preserve the name Offset even if the type is [8]byte, instead of Pad_cgo_0?

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/516156 mentions this issue: runtime/pprof: correct field alignment in machVMRegionBasicInfoData

@ianlancetaylor
Copy link
Contributor

That said, would it be better for the cgo command to preserve the name Offset even if the type is [8]byte, instead of Pad_cgo_0?

What's happening internally is that cgo says "I can't represent this C field, so I'm just going to drop it." Then it moves to the next field, and says "this field is at offset 28, and to make that work I need to insert 8 bytes of padding." That's why we wind up with Pad_cgo_0 as the field name.

That said, sure, I think that for a case like this it would be feasible to keep the name Offset. It might break some existing code, though.

gopherbot pushed a commit that referenced this issue Aug 4, 2023
@cherrymui
runtime/pprof: correct field alignment in machVMRegionBasicInfoData
a0c02df 
The type machVMRegionBasicInfoData is generated from C type
vm_region_basic_info_data_64_t, which is a packed struct with a
64-bit field at offset 20. We cannot use uint64 as the field type
in the Go struct, as that will be aligned at offset 24, which does
not match the C struct. Change back to [8]byte (which is what the
cgo command generates), but keep the name Offset.

Updates #61707.
Updates #50891.

Change-Id: I2932328d7f9dfe9d79cff89752666c794d4d3788
Reviewed-on: https://go-review.googlesource.com/c/go/+/516156
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cherry Mui <cherryyz@google.com>
@cosnicolaou
Copy link
Contributor Author

At the very least a warning is warranted, either as a comment in the generated code or as an error message. If there is a lot of legacy code then maybe generate the new code as a comment with a flag to switch to the new behaviour.

eg.

// is skipped because it cannot be represented in go as a due to alignment rules, use -flag to include as a byte array
Pad [8]byte

@cosnicolaou
Copy link
Contributor Author

Or alternatively, add a -explain-alignments, that doesn't generate code, just explanations for the alignment decisions made. Either of these would have avoided the confusion here which thankfully you caught quickly, but I'd worry that similar errors are made in the broader go codebase.

@ianlancetaylor
Copy link
Contributor

I honestly don't know how likely errors are, because -godefs is rarely used, and the default generated code should be safe to use. The problem only arises when somebody hand edits the generated code.

@cosnicolaou
Copy link
Contributor Author

cosnicolaou commented Aug 6, 2023 via email

@cosnicolaou
Copy link
Contributor Author

cosnicolaou commented Aug 6, 2023 via email

@golang golang locked and limited conversation to collaborators Aug 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@mknyszek @ianlancetaylor @gopherbot @cosnicolaou @cherrymui