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

Jump to bottom

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
Closed

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

gopherbot opened this issue Jan 8, 2021 · 14 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.15.11

Comments

@gopherbot
Copy link

gopherbot commented Jan 8, 2021 •
edited by dmitshur

@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.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Jan 8, 2021
@gopherbot gopherbot mentioned this issue Jan 8, 2021
Closed
@gopherbot gopherbot added this to the Go1.15.7 milestone Jan 8, 2021
@networkimprov
Copy link

Filed in case fix for #40795 depends on this.

@dmitshur dmitshur modified the milestones: Go1.15.7, Go1.15.8 Jan 19, 2021
@cagedmantis cagedmantis modified the milestones: Go1.15.8, Go1.15.9 Feb 4, 2021
@dmitshur
Copy link
Contributor

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.

@networkimprov
Copy link

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

@qmuntal
Copy link
Contributor

qmuntal commented Feb 18, 2021 •
edited

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).

@qmuntal qmuntal mentioned this issue Feb 18, 2021
Closed
@dmitshur
Copy link
Contributor

dmitshur commented Mar 8, 2021 •
edited

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.

@dmitshur dmitshur added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Mar 8, 2021
@networkimprov
Copy link

FYI @alexbrainman @zx2c4

@toothrot toothrot modified the milestones: Go1.15.9, Go1.15.10 Mar 10, 2021
@gopherbot
Copy link
Author

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

@gopherbot
Copy link
Author

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

@cagedmantis cagedmantis modified the milestones: Go1.15.10, Go1.15.11 Mar 11, 2021
@networkimprov
Copy link

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

@dmitshur
Copy link
Contributor

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.

@networkimprov
Copy link

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

@dmitshur
Copy link
Contributor

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?

@gopherbot
Copy link
Author

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

gopherbot pushed a commit that referenced this issue Mar 31, 2021
@qmuntal @dmitshur
[release-branch.go1.15] cmd/cgo: avoid exporting all symbols on windo…
5055314 
…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.

Sign in to view
gopherbot pushed a commit that referenced this issue Mar 31, 2021
@qmuntal @dmitshur
[release-branch.go1.15] cmd/cgo: remove unnecessary space in cgo expo…
1373d92 
…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>
@golang golang locked and limited conversation to collaborators Mar 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.15.11
Development

No branches or pull requests
6 participants
@toothrot @cagedmantis @networkimprov @dmitshur @qmuntal @gopherbot