Skip to content

cmd/cgo: define exported Go func as function #44352

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
Closed
wants to merge 13 commits into from
Closed

cmd/cgo: define exported Go func as function #44352

wants to merge 13 commits into from

Conversation

derekparker
Copy link
Contributor

@derekparker derekparker commented Feb 17, 2021 •
edited
Loading

In order to resolve symbols correctly during C compilation with CGO
the compiler emits a _cgo_main.c file which holds definitions for
symbols. The problem is that the definitions in the _cgo_main.c do not
match up with what's declared in _cgo_export.c which causes compilation
to fail if using CGO_CFLAGS="-flto -ffat-lto-objects" due to an
incompatibility with link time optimization in GCC.

This patch makes it so that the symbol emitted in _cgo_main.c lines up
with the type declared in _cgo_export.c. This should be safe to do as
the actual exported C symbol ends up as an argument to crosscall2 which
defines the function signature for these exported symbols. The function
sig will be consistent in that way since CGO generates wrappers and
structs to handle argument / return value passing.

Fixes #43830

@google-cla google-cla bot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Feb 17, 2021
@gopherbot
Copy link
Contributor

This PR (HEAD: cdc4a03) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/293290 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 1: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 1:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=3f73c437

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 1: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 1: Run-TryBot+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@derekparker derekparker changed the title cmd/cgo: Define exported Go func as function cmd/cgo: define exported Go func as function Feb 18, 2021
@derekparker
cmd/cgo: Define exported Go func as function
fce06f5 
In order to resolve symbols correctly during C compilation with CGO
the compiler emits a _cgo_main.c file which holds definitions for
symbols. The problem is that the definitions in the _cgo_main.c do not
match up with what's declared in _cgo_export.c which causes compilation
to fail if using CGO_CFLAGS="-flto -ffat-lto-objects" due to an
incompatibility with link time optimization in GCC.

This patch makes it so that the symbol emitted in _cgo_main.c lines up
with the type declared in _cgo_export.c. This should be safe to do as
the actual exported C symbol ends up as an argument to crosscall2 which
defines the function signature for these exported symbols. The function
sig will be consistent in that way since CGO generates wrappers and
structs to handle argument / return value passing.
@derekparker derekparker force-pushed the fix-cgo-export-lto-fail branch from cdc4a03 to fce06f5 Compare February 22, 2021 20:59
@derekparker
cmd/cgo: Fix CGO related LTO bug
f3b2c46 
This patch fixes another CGO related LTO bug. This seems to come
when the programmer tries to assign a C function pointer to a local Go
variable as seen in the reproducer. It turns out we do not need to emit
_cgo_hack or our own definition of the symbol(s) within the _cgo_main.c
file as was previously done.

Fixes golang#43830
@derekparker
Copy link
Contributor Author

I've added a test case for the first issue and included a second, related fix with a test case as well. Not seeing them in gerrit right now but I'm not sure how quick the sync works.

@gopherbot
Copy link
Contributor

This PR (HEAD: f3b2c46) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/293290 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Derek Parker:

Patch Set 2:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 3:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Derek Parker:

Patch Set 3:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 3:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Derek Parker:

Patch Set 3:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 3:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 7ce5095) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/293290 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Derek Parker:

Patch Set 4:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Derek Parker:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Derek Parker:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Derek Parker:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 049eea1) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/293290 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Derek Parker:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 11:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 11:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 11: TryBot-Result-1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 4d8e1bf) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/293290 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 12: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 12:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 12:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 12: TryBot-Result-1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: ac10278) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/293290 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Derek Parker:

Patch Set 12:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Michael Hudson-Doyle:

Patch Set 13: Run-TryBot+1 Code-Review+2

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 13:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 13:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 13: TryBot-Result-1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 13:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 13:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 13:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Derek Parker:

Patch Set 13:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 9957918) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/293290 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 14: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 14:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 14:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 14: TryBot-Result-1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 14:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Derek Parker:

Patch Set 14:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/293290.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/293290 has been abandoned.

Replaced by CL 322614

@gopherbot gopherbot closed this May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cmd/cgo: Conflicting types for generated symbols prevent LTO-enabled compilation
2 participants
@derekparker @gopherbot