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

cmd/link: -buildmode=c-shared exports many functions, not just //export functions [1.15 backport] #43591

Closed
gopherbot opened this issue Jan 8, 2021 · 14 comments

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 8, 2021

@networkimprov requested issue #30674 (fixed by 6f7b553 in Go 1.16) to be considered for backport to the next 1.15 minor release.

@gopherbot please backport to 1.15

buildmode=pie became the default in 1.15, so folks have to manually set buildmode=exe to work around this bug.

@networkimprov
Copy link

@networkimprov networkimprov commented Jan 8, 2021

Filed in case fix for #40795 depends on this.

Loading

@dmitshur dmitshur removed this from the Go1.15.7 milestone Jan 19, 2021
@dmitshur dmitshur added this to the Go1.15.8 milestone Jan 19, 2021
@cagedmantis cagedmantis removed this from the Go1.15.8 milestone Feb 4, 2021
@cagedmantis cagedmantis added this to the Go1.15.9 milestone Feb 4, 2021
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Feb 18, 2021

We've discussed this in a release meeting and it didn't seem like this is needed for #43592. CC @ianlancetaylor, @jeremyfaller.

@networkimprov If you still think that CL 262797 (fix for #30674) should be backported, please provide a rationale as described in the third paragraph of https://golang.org/wiki/MinorReleases. It seems to be about buildmode=c-shared, which doesn't match the current backport rationale.

Loading

@networkimprov
Copy link

@networkimprov networkimprov commented Feb 18, 2021

You need to ask the authors of the fixes for this and #40795; somewhere I read this might be needed for its backport.

cc @qmuntal

Loading

@qmuntal
Copy link
Contributor

@qmuntal qmuntal commented Feb 18, 2021

e463c28 adds a hardcoded dummy variable tagged with __declspec(dllexport) and removes the -Wl,--export-all-symbols flag from the linker arguments. This tells mingw-w64 to only add the symbols tagged with __declspec(dllexport) into the DLL export table. The dummy variable is necessary because the default behavior of mingw-w64 is to export everything, even if -Wl,--export-all-symbols is not present, unless there is a symbol tagged with __declspec(dllexport).

What 6f7b553 does is add the __declspec(dllexport) tag to all methods that should be exported.

Backporting e463c28 without either backporting 6f7b553 or setting again -Wl,--export-all-symbols for c-shared means that the generated binary won't honor the //export comment and that will only export the dummy variable (_cgo_dummy_export).

Loading

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Mar 8, 2021

Thank you for the rationale, it's very helpful.

We've discussed this candidate and agreed it is also okay to backport, so both e463c28 (#43592) and 6f7b553 (this issue) will be backported. Only Go 1.15 needs this backport.

Loading

@networkimprov
Copy link

@networkimprov networkimprov commented Mar 8, 2021

Loading

@toothrot toothrot removed this from the Go1.15.9 milestone Mar 10, 2021
@toothrot toothrot added this to the Go1.15.10 milestone Mar 10, 2021
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Mar 10, 2021

Change https://golang.org/cl/300693 mentions this issue: [release-branch.go1.15] [release-branch.go.1.15] cmd/link,cmd/cgo: avoid exporting all symbols on windows buildmode=c-shared/pie

Loading

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Mar 10, 2021

Change https://golang.org/cl/300694 mentions this issue: [release-branch.go1.15] cmd/cgo: remove unnecessary space in cgo export header

Loading

@cagedmantis cagedmantis removed this from the Go1.15.10 milestone Mar 11, 2021
@cagedmantis cagedmantis added this to the Go1.15.11 milestone Mar 11, 2021
@networkimprov
Copy link

@networkimprov networkimprov commented Mar 31, 2021

Should https://golang.org/cl/300692 reference this issue?

Loading

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Mar 31, 2021

CL 300693 is a cherry-pick of CL 262797 and includes "Fixes #43591", the backport issue for CL 262797.
CL 300692 is a cherry-pick of CL 264459 and includes "Fixes #43592", the backport issue for CL 264459.

It seems okay to me that CL 300692 doesn't reference this issue, since CL 300693 does.

Loading

@networkimprov
Copy link

@networkimprov networkimprov commented Mar 31, 2021

I'm referring to #40795 which this is the backport for.

Loading

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Mar 31, 2021

I'm not sure what you mean. This is a 1.15 backport issue for #30674. The 1.15 backport issue for #40795 is #43592. If it's important, can you please leave a review comment on the affected CL?

Loading

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Mar 31, 2021

Closed by merging 5055314 to release-branch.go1.15.

Loading

@gopherbot gopherbot closed this Mar 31, 2021
gopherbot pushed a commit that referenced this issue Mar 31, 2021
…ws buildmode=c-shared

Disable default symbol auto-export behaviour by marking exported
function with the __declspec(dllexport) attribute. Old behaviour can
still be used by setting -extldflags=-Wl,--export-all-symbols.

See https://sourceware.org/binutils/docs/ld/WIN32.html for more info.

This change cuts 50kb of a "hello world" dll.

Updates #6853.
Updates #30674.
Fixes #43591.

Change-Id: I9c7fb09c677cc760f24d0f7d199740ae73981413
Reviewed-on: https://go-review.googlesource.com/c/go/+/262797
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Trust: Alex Brainman <alex.brainman@gmail.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/300693
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@gopherbot

This comment has been minimized.

gopherbot pushed a commit that referenced this issue Mar 31, 2021
…rt header

The cgo header has an unnecessary space in the exported function
definition on non-windows goos.

This was introduced in go1.16 so it would be good to fix it before
release.

Example:

// Current behavior, notice there is an unecessary space
// between extern and void
extern  void Foo();

// With this CL
extern void Foo();

Updates #43591.

Change-Id: Ic2c21f8d806fe35a7be7183dbfe35ac605b6e4f6
Reviewed-on: https://go-review.googlesource.com/c/go/+/283892
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Katie Hockman <katie@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/300694
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Emmanuel Odeke <emmanuel@orijtech.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants