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

gcc: enable stripping for cross-compilers #182513

Merged
merged 4 commits into from
Jul 28, 2022

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Jul 22, 2022

Description of changes

With explicit support for distinction between Host and Target libraries
we can now safely strip ELF binaries with their according strip tools
without fear of damaging binaries due to architecture mismatch (most
frequent on arm).

Closure size change for pkgsCross.mingwW64.gcc12Stdenv.cc.cc:

# before:
$ nix path-info -Sh $(nix-build -A pkgsCross.mingwW64.gcc12Stdenv.cc.cc) | unnix
/<<NIX>>/x86_64-w64-mingw32-stage-final-gcc-debug-12.1.0           2.5G

# after:
$ nix path-info -Sh $(nix-build -A pkgsCross.mingwW64.gcc12Stdenv.cc.cc) | unnix
/<<NIX>>/x86_64-w64-mingw32-stage-final-gcc-12.1.0         1.5G
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Since 1ac5398 "*-wrapper; Switch from `infixSalt` to `suffixSalt`"
(2020) 'TARGET_' prefix (and infix) is no more. '_FOR_TARGET' suffix
is the only used suffix for target-specific tools and flags.

Use that in stip instead of always-empty variable.
@trofi
Copy link
Contributor Author

trofi commented Jul 23, 2022

We will also need to run ranlib on .a files, otherwise static libraries don't work (I think it's not specific to gcc, strip.sh never seemed to handle it):

$ nix build --no-link -f. pkgsCross.mingw32.re2c
>...-i686-w64-mingw32-binutils-2.38/bin/i686-w64-mingw32-ld: /nix/store/v21fgwkh4zqmbxvlsvw3qndwlaah05gm-i686-w64-mingw32-stage-final-gcc-13.0.0-lib/i686-w64-mingw32/lib/libstdc++.dll.a: error adding symbols: archive has no index; run ranlib to add one

Copy link
Contributor

@Mindavi Mindavi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some textual comments at least

doc/stdenv/stdenv.chapter.md Outdated Show resolved Hide resolved
doc/stdenv/stdenv.chapter.md Outdated Show resolved Hide resolved
@trofi trofi force-pushed the strip-for-host-and-target branch from 1347a53 to 9ed7f3a Compare July 23, 2022 09:36
@trofi
Copy link
Contributor Author

trofi commented Jul 23, 2022

We will also need to run ranlib on .a files, otherwise static libraries don't work (I think it's not specific to gcc, strip.sh never seemed to handle it):

$ nix build --no-link -f. pkgsCross.mingw32.re2c
>...-i686-w64-mingw32-binutils-2.38/bin/i686-w64-mingw32-ld: /nix/store/v21fgwkh4zqmbxvlsvw3qndwlaah05gm-i686-w64-mingw32-stage-final-gcc-13.0.0-lib/i686-w64-mingw32/lib/libstdc++.dll.a: error adding symbols: archive has no index; run ranlib to add one

Fixed with setup-hooks/strip.sh: run RANLIB on static archives after stripping

@veprbl veprbl added the 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on label Jul 23, 2022
@trofi trofi marked this pull request as draft July 25, 2022 07:54
@trofi
Copy link
Contributor Author

trofi commented Jul 25, 2022

I decided to do a few more things:

  • drop *Host special variables and reused existing unsuffixed for host, make *Target variables empty by default.
  • extend the change to all gcc versions. That way the change would be less confusing for gcc_debug attribute and would benefit (or break) all targets instantly.

@trofi trofi force-pushed the strip-for-host-and-target branch from 9ed7f3a to 9146f12 Compare July 25, 2022 08:30
@trofi trofi changed the title gcc12: enable stripping for cross-compilers gcc: enable stripping for cross-compilers Jul 25, 2022
@trofi trofi marked this pull request as ready for review July 25, 2022 09:25
@trofi
Copy link
Contributor Author

trofi commented Jul 25, 2022

Tested on gcc, pkgsCross.mingw32.stdenv.cc, pkgsCross.powernv.stdenv.cc. Ready for review.
UPDATE: also tested pkgsLLVM, pkgsStatic, pkgsMusl.

pkgs/top-level/all-packages.nix Show resolved Hide resolved

preFixup = ''
# Populate most delicated lib/ part of stripDebugList{,Target}
updateDebugListPaths() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you put this into a function so that it can be re-used later in some other phase? If so I think we should use a hook instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function should be used only here. I used function only for local oldOpts="$(shopt -p nullglob)" variable scoping.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a subshell also scope it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function's main effect is to extend variables used by a pkgs/build-support/setup-hooks/strip.sh:

      stripDebugList="$stripDebugList ''${hostFiles[*]}"
      stripDebugListTarget="$stripDebugListTarget ''${targetFiles[*]}"

Thus subshell would need to be applied selectively on a subset of the function's code.

doc/stdenv/stdenv.chapter.md Outdated Show resolved Hide resolved
doc/stdenv/stdenv.chapter.md Outdated Show resolved Hide resolved
trofi and others added 2 commits July 25, 2022 11:06
This change mimics existing strip{All,Debug}List variables to
allow special stripping directories just for Target.

The primary use case in mind is gcc where package has to install
both host and target ELFs. They have to be stripped by their own
strip tools accordingly.

Co-authored-by: Rick van Schijndel <Mindavi@users.noreply.github.com>
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
'strip' does not normally preserve archive index in .a files.
This usually causes linking failures against static libs like:

    $ nix build --no-link -f. pkgsCross.mingw32.re2c
    > ...-i686-w64-mingw32-binutils-2.38/bin/i686-w64-mingw32-ld:
      /nix/store/...-i686-w64-mingw32-stage-final-gcc-13.0.0-lib/i686-w64-mingw32/lib/libstdc++.dll.a:
        error adding symbols: archive has no index; run ranlib to add one

We restore the index by running ranlib explicitly.
With explicit support for distinction between Host and Target strip paths
we can now safely strip ELF binaries with their according strip tools
without fear of damaging binaries due to architecture mismatch.

Closure size change for `pkgsCross.mingwW64.gcc12Stdenv.cc.cc`:

    # before:
    $ nix path-info -Sh $(nix-build -A pkgsCross.mingwW64.gcc12Stdenv.cc.cc) | unnix
    /<<NIX>>/x86_64-w64-mingw32-stage-final-gcc-debug-12.1.0           2.5G

    # after:
    $ nix path-info -Sh $(nix-build -A pkgsCross.mingwW64.gcc12Stdenv.cc.cc) | unnix
    /<<NIX>>/x86_64-w64-mingw32-stage-final-gcc-12.1.0         1.5G
@trofi trofi force-pushed the strip-for-host-and-target branch from a0a86b1 to eece5d0 Compare July 25, 2022 10:07
@lovesegfault lovesegfault merged commit 88c63ca into NixOS:staging Jul 28, 2022
@trofi trofi deleted the strip-for-host-and-target branch July 28, 2022 17:01
@fricklerhandwerk fricklerhandwerk removed their request for review August 5, 2022 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants