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

Add response file support when compiling with clang #112449

Merged
merged 2 commits into from
Mar 6, 2021
Merged

Add response file support when compiling with clang #112449

merged 2 commits into from
Mar 6, 2021

Conversation

angerman
Copy link
Contributor

@angerman angerman commented Feb 9, 2021

Motivation for this change

nixpkgs easily produces lots of arguments to the c compiler. This can in extreme cases lead to the arguments exceeding the space for arguments to be passed. A common fix for this is to write the arguments into a file and pass that file (called response file) to the compiler.

This issues comes up often when building large haskell applications on macOS due to the large amount of library search paths that are being passed as well as other c flags.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@angerman angerman changed the base branch from master to staging February 9, 2021 02:05
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux-stdenv This PR causes stdenv to rebuild labels Feb 9, 2021
@FRidh FRidh added this to the 21.05 milestone Feb 20, 2021
@angerman @matthewbauer
Update pkgs/build-support/cc-wrapper/cc-wrapper.sh
60c5cf9 
Co-authored-by: Matthew Bauer <mjbauer95@gmail.com>
@Mic92
Copy link
Member

Mic92 commented Feb 26, 2021 •
edited
Loading

Does macOS reserves a smaller stack for arguments compared to Linux? Is the motivation to only do this for clang that we use clang on macOS?

@matthewbauer
Copy link
Member

Does macOS reserves a smaller stack for arguments compared to Linux? Is the motivation to only do this for clang that we use clang on macOS?

I think the main issue is ARG_MAX is a lot smaller on macOS. You can hit this on Linux too in some cases with GHC which can create a really long argument list.

@Ericson2314
Copy link
Member

Yeah i would rather the condition be macOS or Darwin than Clang too. There's no need to change clang on Linux, and there is a need to change non-clang on Darwin.

@angerman
Copy link
Contributor Author

angerman commented Mar 6, 2021 •
edited
Loading

Yeah i would rather the condition be macOS or Darwin than Clang too. There's no need to change clang on Linux, and there is a need to change non-clang on Darwin.

As @matthewbauer pointed out, this is sadly not limited to darwin. You can even exceed linux rather generous argument list.

need to change non-clang on Darwin

non-clang on Darwin? GCC? That ones is almost impossible to fix, because GCC internally expands response files and feeds them expanded to collect2, which in turn will blow up. That's the reason why this is clang guarded.

@Ericson2314
Copy link
Member

Ah OK. That's a good point.

@Ericson2314 Ericson2314 merged commit 6979a72 into NixOS:staging Mar 6, 2021
thefloweringash added a commit to thefloweringash/nixpkgs that referenced this pull request Mar 9, 2021
@thefloweringash
cc-wrapper: remove quoting of response file
86e962a 
Fixes build failures with clang:

    clang-7: error: unknown argument: '-fPIC                -target'
    clang-7: error: no such file or directory: '@<(printf %qn        -O2'
    clang-7: error: no such file or directory: 'x86_64-apple-darwin'

Introduced by 60c5cf9 in NixOS#112449
@thefloweringash thefloweringash mentioned this pull request Mar 9, 2021
Merged
10 tasks
@Kha Kha mentioned this pull request Apr 18, 2021
Closed
Gabriella439 added a commit to MercuryTechnologies/cctools-port that referenced this pull request Jan 31, 2023
@Gabriella439
Fix ld's response files support for special files
4a73407 
Currently `ld` uses `mmap` to read in the contents of the
response file (presumably for performance reasons), but `mmap`
sometimes doesn't work on special files like pipes.  For example,
if you do something like:

```
$ ld @<(echo --help)
```

… that will currently fail with a segmentation fault.

You might wonder why one would want to generate a response file
from process substitution.  The rationale behind this is that
I'm currently in the process of fixing a longstanding issue
with the linker failing in Nixpkgs on macOS due to hitting
command-line length limits and the fix entails the use of
process substitution to generate the process file.

Specifically, what I was doing was building upon the work from
this PR:

NixOS/nixpkgs#112449

… which modified the `cc-wrapper` in Nixpkgs to use a
response file generate from process substitution.  I was
going to do essentially the same for the `ld-wrapper` in
Nixpkgs, but that failed with a segmentation fault (for the
reasons outlined above).

There are other possible ways to work around that, but using
process substitution is the "leanest" way of generating the
response file for `ld` in Nixpkgs, so I wanted to push on
getting that working here instead of working around the problem
downstream.

So the way I fixed it was to fall back to using `read` instead
of `mmap` if the `mmap` failed.  After this change, the above
sample command now works correctly.

This also fixes another small issue along the way: this now
correctly detects when the `mmap` fails.  Previously, the `mmap`
logic was detecting failure by looking for a `NULL`/`0` return
value, but that is not the correct error-handling behavior.
`mmap` returns `MAP_FAILED` on failure, which is `-1` in practice,
and not `0`.  That's the reason why the code was failing with
a segmentation fault before because it wasn't detecting the failure
and proceeding to read from the invalid buffer anyway.
@Gabriella439 Gabriella439 mentioned this pull request Jan 31, 2023
Merged
@Gabriella439 Gabriella439 mentioned this pull request Jan 31, 2023
Merged
3 tasks
Gabriella439 added a commit to MercuryTechnologies/nixpkgs that referenced this pull request Feb 3, 2023
@Gabriella439
bintools: Add response file support to ld-wrapper
4ffd094 
The motivation behind this is to alleviate the problem
described in NixOS#41340.
I'm not sure if this completely fixes the problem, but it
eliminates one more area where we can exceed command line
length limits.

This is essentially the same change as in NixOS#112449,
except for `ld-wrapper.sh` instead of `cc-wrapper.sh`.

However, that change alone was not enough; on macOS the
`ld` provided by `darwin.cctools` fails if you use process
substitution to generate the response file, so I put up a
PR to fix that:

tpoechtrager/cctools-port#131

… and I included a patch referencing that fix so that the
new `ld-wrapper` still works on macOS.
Gabriella439 added a commit to MercuryTechnologies/nixpkgs that referenced this pull request Feb 22, 2023
@Gabriella439
bintools: Add response file support to ld-wrapper
17f8f90 
The motivation behind this is to alleviate the problem
described in NixOS#41340.
I'm not sure if this completely fixes the problem, but it
eliminates one more area where we can exceed command line
length limits.

This is essentially the same change as in NixOS#112449,
except for `ld-wrapper.sh` instead of `cc-wrapper.sh`.

However, that change alone was not enough; on macOS the
`ld` provided by `darwin.cctools` fails if you use process
substitution to generate the response file, so I put up two
PRs to fix that:

tpoechtrager/cctools-port#131
tpoechtrager/cctools-port#132

… and I included a patch referencing that fix so that the
new `ld-wrapper` still works on macOS.
@Gabriella439 Gabriella439 mentioned this pull request Feb 22, 2023
Merged
Gabriella439 added a commit to MercuryTechnologies/nixpkgs that referenced this pull request Feb 22, 2023
@Gabriella439
bintools: Add response file support to ld-wrapper (#2)
412e9cb 
The motivation behind this is to alleviate the problem
described in NixOS#41340.
I'm not sure if this completely fixes the problem, but it
eliminates one more area where we can exceed command line
length limits.

This is essentially the same change as in NixOS#112449,
except for `ld-wrapper.sh` instead of `cc-wrapper.sh`.

However, that change alone was not enough; on macOS the
`ld` provided by `darwin.cctools` fails if you use process
substitution to generate the response file, so I put up two
PRs to fix that:

tpoechtrager/cctools-port#131
tpoechtrager/cctools-port#132

… and I included a patch referencing that fix so that the
new `ld-wrapper` still works on macOS.
Gabriella439 added a commit that referenced this pull request Feb 24, 2023
@Gabriella439
bintools: Add response file support to ld-wrapper (#213831)
79484b1 
The motivation behind this is to alleviate the problem
described in #41340.
I'm not sure if this completely fixes the problem, but it
eliminates one more area where we can exceed command line
length limits.

This is essentially the same change as in #112449,
except for `ld-wrapper.sh` instead of `cc-wrapper.sh`.

However, that change alone was not enough; on macOS the
`ld` provided by `darwin.cctools` fails if you use process
substitution to generate the response file, so I put up a
PR to fix that:

tpoechtrager/cctools-port#131

… and I included a patch referencing that fix so that the
new `ld-wrapper` still works on macOS.
@rrbutani rrbutani mentioned this pull request Apr 13, 2023
Open
@babbaj babbaj mentioned this pull request Apr 14, 2023
Open
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer left review comments

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@Lucus16 Lucus16 Awaiting requested review from Lucus16

@peti peti Awaiting requested review from peti

@Synthetica9 Synthetica9 Awaiting requested review from Synthetica9

@veprbl veprbl Awaiting requested review from veprbl

@davidtwco davidtwco Awaiting requested review from davidtwco

@edolstra edolstra Awaiting requested review from edolstra

@Mic92 Mic92 Awaiting requested review from Mic92

@LnL7 LnL7 Awaiting requested review from LnL7

Assignees
No one assigned
Labels
10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild
Projects
None yet
Milestone
21.05
Development

Successfully merging this pull request may close these issues.
5 participants
@angerman @Mic92 @matthewbauer @Ericson2314 @FRidh