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: add -ldflags to augment CGO_LDFLAGS #66456

Closed
grrtrr opened this issue Mar 21, 2024 · 20 comments
Closed

cmd/cgo: add -ldflags to augment CGO_LDFLAGS #66456

grrtrr opened this issue Mar 21, 2024 · 20 comments
Labels
Proposal Proposal-Accepted
Milestone
Go1.23

Comments

@grrtrr
Copy link

grrtrr commented Mar 21, 2024

Proposal Details

cgo proposal to avoid "argument list too long" (E2BIG) issues when CGO_LDFLAGS outgrows system limits.

Problem Description

The cgo command writes the contents of the CGO_LDFLAGS environment variable into the archive.
C/C++ linker flags and dependencies are passed via this environment variable.

When there is a large list of C/C++ dependencies, it is easy to hit the system size limits, causing compilation to fail with cgo: argument list too long.

Related issues

Overly large lists dependencies can be written into a response file, using the @file syntax, passing CGO_LDFLAGS=@<path-to-file> instead of failing the compilation.

This is a convention supported by many compilers, such as gcc or clang, and mirrors the passLongArgsInResponseFiles fix for long argument lists in #18468.
Multiple go commands also support response file arguments via objabi.Flagparse.

Proposal: Since build systems such as bazel may execute compile and link stages in different locations, cmd/cgo should

  • expand the contents of an @response_file,
  • the existing expandArgs function can be used to do this.
@gopherbot gopherbot added this to the Proposal milestone Mar 21, 2024
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Mar 22, 2024
@rsc
Copy link
Contributor

rsc commented Apr 3, 2024

What is the specific change? Cgo already understands response files for command line arguments; the problem here is that the info is being passed to cgo in $CGO_LDFLAGS, and that's exacerbated by the go command and Bazel rules_go both not deduplicating the CGO_LDFLAGS info they gather from every cgo-using package in the build. (If 10 packages all say the same CGO_LDFLAGS, no need to say that part 10 times, just say it once.) Perhaps they should both deduplicate and then the length limit is not a problem anymore.

We may need to deduplicate some linker arguments to appease the Mac linker anyway.

@grrtrr
Copy link
Author

grrtrr commented Apr 3, 2024

Our CGO_LDFLAGS got huge, using a response file avoided the overflow. Can post the PR we used, this fixed the problem.

I had tried de-duplication, but there were some ordering dependencies in the linker command.

@rsc
Copy link
Contributor

rsc commented Apr 3, 2024

How did you use a response file at all? Response files are for command-line arguments, not environment variables.

@rsc
Copy link
Contributor

rsc commented Apr 3, 2024

If we need to do something, we should add a -ldflags argument to the cgo command, and then that can be put in a response file. Interpreting @ signs in environment variables is a big scary door to open.

@rsc
Copy link
Contributor

rsc commented Apr 3, 2024

The linked bazel-contrib/rules_go#2654 has an actual CGO_LDFLAGS line in the top comment. It is one line but it is very very long and GitHub has added a scroll bar, so you don't notice how long it is.

That CGO_LDFLAGS has:

  • 843 .lo files, each wrapped in -Wl,--whole-archive ... -Wl,--no-whole-archive.
  • 207 .a files, without that wrapping.
  • 75 -pthread
  • 4 -lpthread
  • 3 -lm
  • 2 -ldl
  • 1 -static-libstdc++
  • 1 -static-libgcc
  • 1 -lrt
  • 1 -l:libstdc++.a
  • 1 -fuse-ld=/usr/bin/ld.gold
  • 1 -Wl,-z,relro,-z,now
  • 1 -Wl,-no-as-needed
  • 1 -B/usr/bin

So deduplicating is not going to cut it. My suggestion is we add -ldflags to cgo, and then the go command and Bazel can use the support cgo already has for response files on its command line.

@grrtrr
Copy link
Author

grrtrr commented Apr 3, 2024

How did you use a response file at all? Response files are for command-line arguments, not environment variables.

In short, CGO_LDFLAGS accepts @file in the same way as an executable would , expanding the arguments from the given file.

Support for this in cgo is needed to fix the corresponding open problem in @rules_go (bazel-contrib/rules_go#2654):

  • @rules_go collects all the linker flags into CGO_LDFLAGS and then
  • calls cgo with this environment in go/tools/builders/cgo2.go.

The fixed environment-variable length limit on Linux/Windows makes it currently impossible to compile larger projects.

Thinking about whether -ldflags would help - there is also a maximum commandline length.

@rsc
Copy link
Contributor

rsc commented Apr 3, 2024

Thinking about whether -ldflags would help - there is also a maximum commandline length.

It would help because cgo accepts response files on the command line already.

@grrtrr
Copy link
Author

grrtrr commented Apr 3, 2024

Sorry for taking so long to understand - if adding -ldflags is an equivalent solution to passing via CGO_LDFLAGS in @rules_go, then this is clearly preferable.

@rsc rsc changed the title proposal: cmd/cgo support response files to avoid "argument too long" issues with CGO_LDFLAGS proposal: cmd/cgo: avoid over-long CGO_LDFLAGS Apr 4, 2024
@rsc rsc changed the title proposal: cmd/cgo: avoid over-long CGO_LDFLAGS proposal: cmd/cgo: add -ldflags argument to supplement CGO_LDFLAGS Apr 4, 2024
@rsc rsc changed the title proposal: cmd/cgo: add -ldflags argument to supplement CGO_LDFLAGS proposal: cmd/cgo: add -ldflags argument to augment CGO_LDFLAGS Apr 4, 2024
@rsc rsc changed the title proposal: cmd/cgo: add -ldflags argument to augment CGO_LDFLAGS proposal: cmd/cgo: add -ldflags to augment CGO_LDFLAGS Apr 4, 2024
@rsc rsc moved this from Incoming to Active in Proposals Apr 4, 2024
@rsc
Copy link
Contributor

rsc commented Apr 4, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@grrtrr
Copy link
Author

grrtrr commented Apr 4, 2024

Thank you.

@rsc rsc moved this from Active to Likely Accept in Proposals Apr 10, 2024
@rsc
Copy link
Contributor

rsc commented Apr 10, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to add a -ldflags argument to cmd/cgo and then use that argument inside the go command. When the command line gets too long, the go command will automatically switch to response files, avoiding the possibility of $CGO_LDFLAGS getting too long.

Bazel could then do the same (adopt -ldflags and use the existing response file functionality for command line arguments) to avoid the same problem.

@rsc
Copy link
Contributor

rsc commented Apr 24, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to add a -ldflags argument to cmd/cgo and then use that argument inside the go command. When the command line gets too long, the go command will automatically switch to response files, avoiding the possibility of $CGO_LDFLAGS getting too long.

Bazel could then do the same (adopt -ldflags and use the existing response file functionality for command line arguments) to avoid the same problem.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Apr 24, 2024
@rsc rsc changed the title proposal: cmd/cgo: add -ldflags to augment CGO_LDFLAGS cmd/cgo: add -ldflags to augment CGO_LDFLAGS Apr 24, 2024
@rsc rsc modified the milestones: Proposal, Backlog Apr 24, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/584655 mentions this issue: cmd/cgo, cmd/go: add cgo -ldflags option, use it in cmd/go

@dmitshur dmitshur modified the milestones: Backlog, Go1.23 May 23, 2024
@fmeum
Copy link

fmeum commented Jul 3, 2024

The PR that closed this issue ended up breaking rules_go (and other external users of cgo) since CGO_LDFLAGS is now silently ignored in 1.23, even if the new -ldflags is not set.

Could this be fixed in a patch release? If that isn't possible, what would you recommend as a workaround?

Cc @ianlancetaylor

@fmeum fmeum mentioned this issue Jul 3, 2024
Closed
@grrtrr
Copy link
Author

grrtrr commented Jul 3, 2024

@fmeum - the problem is in rules_go, #2654, quoting @jayconrod:

this can't be fixed in rules_go without a corresponding fix in the Go toolchain. Consider filing a bug in golang/go for that.

The "fix in the Go toolchain" was this proposal and the PR that folllowed.

The next step is updating @rules_go to take advantage of the -ldflags.

This should not be difficult to do, happy to collaborate.

@fmeum
Copy link

fmeum commented Jul 3, 2024

The next step is updating @rules_go to take advantage of the -ldflags.

This part of the PR looks good to me, thanks! We can adopt it.

The problem is that existing versions of rules_go no longer work with Go 1.23 due to cgo no longer honoring the env variable. See bazel-contrib/rules_go#3979 for the reported breakage this causes.

@ianlancetaylor
Copy link
Member

I will send a patch to cgo to use both the CGO_LDFLAGS environment variable and the -ldflags command line option.

We should presumably be able to remove that when we no longer need to support older versions of Bazel.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/596615 mentions this issue: cmd/cgo: read CGO_LDFLAGS environment variable

gopherbot pushed a commit that referenced this issue Jul 3, 2024
@ianlancetaylor @gopherbot
cmd/cgo: read CGO_LDFLAGS environment variable
7d19d50 
For #66456 we changed from the CGO_LDFLAGS environment variable to
the -ldflags option. This broke Bazel, which uses CGO_LDFLAGS.
So restore reading CGO_LDFLAGS for now.

For #66456

Change-Id: Iebdd8bde1c7c18da09c6370e284c7ac7fa08fc54
Reviewed-on: https://go-review.googlesource.com/c/go/+/596615
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Commit-Queue: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@ianlancetaylor
Copy link
Member

Thanks, this should be fixed in the next release candidate. If you are able to, please test Go repo HEAD.

@fmeum
Copy link

fmeum commented Jul 4, 2024

@ianlancetaylor Thanks for the quick fix!

@grrtrr Happy to review a change to rules_go to make use of the new flag.

Mchnan pushed a commit to Mchnan/go-sylixos that referenced this issue Jul 9, 2024
@ianlancetaylor
cmd/cgo: read CGO_LDFLAGS environment variable
9c50fdf 
For golang#66456 we changed from the CGO_LDFLAGS environment variable to
the -ldflags option. This broke Bazel, which uses CGO_LDFLAGS.
So restore reading CGO_LDFLAGS for now.

For golang#66456

Change-Id: Iebdd8bde1c7c18da09c6370e284c7ac7fa08fc54
Reviewed-on: https://go-review.googlesource.com/c/go/+/596615
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Commit-Queue: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal Proposal-Accepted
Projects
Proposals
Status: Accepted
Milestone
Go1.23
Development

No branches or pull requests
6 participants
@rsc @dmitshur @ianlancetaylor @fmeum @grrtrr @gopherbot