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

runtime: defs_linux.go doesn't work anymore #8477

Closed
mdempsky opened this issue Aug 5, 2014 · 6 comments
Closed

runtime: defs_linux.go doesn't work anymore #8477

mdempsky opened this issue Aug 5, 2014 · 6 comments
Labels
FrozenDueToAge

Comments

@mdempsky
Copy link
Contributor

mdempsky commented Aug 5, 2014 

Running "GOARCH=amd64 go tool cgo -debug-gcc=true -cdefs defs_linux.go" on
Ubuntu 14.04 no longer works; GCC complains about multiple definitions of sigset_t,
timespec, etc.

I suspect this is because of revision 46fd4ef6c0deeb184aa0b843dfc080cc869a7e19, which
added "#include <sys/types.h>" to ensure size_t was available.  On Linux
this will define the userspace-compatible sigset_t/timespec/etc struct definitions, but
defs_linux.go has already included headers to define the *kernel*-compatible definitions.

I suspect the proper fix is to replace cgo's #include <sys/types.h> with #include
<stddef.h>, which is the ISO C standard header for "size_t" and defines
far fewer additional/unnecessary types than <sys/types.h> does.

defs_linux.go should probably also #undef size_t at the end of its "import
"C"" preamble.

I'll send a patch for both of these.
@mdempsky
Copy link
Contributor Author

mdempsky commented Aug 5, 2014

Comment 1:

Having fixed this locally, here's the resulting diff I get between the current
defs_linux_amd64.h and a freshly generated one:
--- defs_linux_amd64.h  2014-08-05 13:42:59.683096049 -0700
+++ -   2014-08-05 13:51:18.314042923 -0700
@@ -122,7 +122,7 @@
 };
 struct EpollEvent {
        uint32  events;
-       uint64  data;
+       byte    Pad_cgo_0[8];
 };
 
 
@@ -246,7 +246,7 @@
        uint64  trapno;
        uint64  oldmask;
        uint64  cr2;
-       Fpstate1        *fpstate;
+       byte    anon0[8];
        uint64  __reserved1[8];
 };
 
The EpollEvent struct change is because it's a packed structure, so data is supposed to
be at a 4-byte offset, but cgo wants to put it at an 8-byte offset.  This makes me a
little surprised if the integrated network poller is currently working correctly on
linux/amd64, but I haven't looked into it further yet.
The Sigcontext struct change is because Linux has moved fpstate into an anonymous union.
 The Go runtime doesn't touch fpstate anyway, so that seems harmless.

@gopherbot
Copy link
Contributor

Comment 2:

CL https://golang.org/cl/120610043 mentions this issue.

@ianlancetaylor
Copy link
Member

Comment 3:

The integrated network poller is written in C, so the incorrect Go struct does not
affect it.  The C struct is declared in runtime/defs_linux_amd64.h within the scope of a
#pragma pack on.

@mdempsky
Copy link
Contributor Author

mdempsky commented Aug 5, 2014

Comment 4:

Ah, I missed the #pragma pack directives.  That makes sense then why the current
EpollEvent struct definition works then.
I'll dig into why "cgo -cdefs" has started replacing "data" with padding instead of
emitting it as a "uint64".

@mdempsky
Copy link
Contributor Author

mdempsky commented Aug 5, 2014

Comment 5:

Seems like the culprit is revision 2fb7165d5fdcd4141eff4cc3dc4bbba5a6728272.  Updated
https://golang.org/cl/120610043 to fix the EpollEvent issue too.

@ianlancetaylor
Copy link
Member

Comment 6:

This issue was closed by revision f7a8adb.

Status changed to Fixed.

@mdempsky mdempsky added the fixed label Aug 6, 2014
@golang golang locked and limited conversation to collaborators Jun 25, 2016
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
@mdempsky @wheatman
cmd/cgo: fix handling of defs_linux.go

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
bd30af7 
Instead of including <sys/types.h> to get size_t, instead include
the ISO C standard <stddef.h> header, which defines fewer additional
types at risk of colliding with the user code.  In particular, this
prevents collisions between <sys/types.h>'s userspace definitions with
the kernel definitions needed by defs_linux.go.

Also, -cdefs mode uses #pragma pack, so we can keep misaligned fields.

Fixes golang#8477.

LGTM=iant
R=golang-codereviews, iant
CC=golang-codereviews
https://golang.org/cl/120610043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
@mdempsky @wheatman
cmd/cgo: fix handling of defs_linux.go
c9d044e 
Instead of including <sys/types.h> to get size_t, instead include
the ISO C standard <stddef.h> header, which defines fewer additional
types at risk of colliding with the user code.  In particular, this
prevents collisions between <sys/types.h>'s userspace definitions with
the kernel definitions needed by defs_linux.go.

Also, -cdefs mode uses #pragma pack, so we can keep misaligned fields.

Fixes golang#8477.

LGTM=iant
R=golang-codereviews, iant
CC=golang-codereviews
https://golang.org/cl/120610043
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mdempsky @ianlancetaylor @gopherbot