Skip to content

separate-debug-info setup hook: strip debug symbols from static libs#164520

Closed
risicle wants to merge 3 commits intoNixOS:stagingfrom
risicle:ris-separate-debug-static
Closed

separate-debug-info setup hook: strip debug symbols from static libs#164520
risicle wants to merge 3 commits intoNixOS:stagingfrom
risicle:ris-separate-debug-static

Conversation

@risicle
Copy link
Contributor

@risicle risicle commented Mar 17, 2022 •
edited
Loading

Description of changes

Currently, separateDebugInfo = true will cause binaries to be built with debug symbols, before "separating" them in a setup hook. The problem is, this setup hook doesn't take static libraries into account and so they end up retaining their debug info. This often makes them huge and is what caused the reversion of the first attempt at adding separateDebugInfo to cpython in #93083.

This adds the ability to detect AR archives to the generic setup script, then makes the separate-debug-info.sh attempt to strip any likely-static-libraries it finds.

Separate debug info for static libraries reportedly doesn't really work and outputting the unstripped version of the library to the debug output is something I don't see as being significantly more useful than making the user do a dontStrip build. Though it would significantly increase the size of debug outputs.

This PR results in some space savings for packages that output static libraries while using separateDebugInfo. postgresql's lib/libpgcommon.a goes from ~800KB to ~200KB on x86_64 linux. A static build of openssl goes from 24MB to 6.5MB.

My hope with this is that people will be less hesitant to enable separateDebugInfo on packages as it is much less likely to cause unexpected bloat.

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.05 Release Notes (or backporting 21.11 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.

risicle added 3 commits March 16, 2022 22:52
@risicle
stdenv generic setup: add isAR function
3e07053 
to allow us to identify AR archives (possibly static libraries)
@risicle
separate-debug-info setup hook: strip debug symbols from static libs
be2b7ec 
this should make it less risky to enable separateDebugInfo as
otherwise it may result in massive static libraries
@risicle
cpython: remove explicit stripping of static library
dfb450c 
this should now be automatically done by separateDebugInfo
@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment labels Mar 17, 2022
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Mar 17, 2022
symphorien
symphorien reviewed Apr 10, 2022
View reviewed changes
pkgs/build-support/setup-hooks/separate-debug-info.sh
local id="$($READELF -n "$i" | sed 's/.*Build ID: \([0-9a-f]*\).*/\1/; t; d')"
if [ "${#id}" != 40 ]; then
echo "could not find build ID of $i, skipping" >&2
continue
Copy link
Member

@symphorien symphorien Apr 10, 2022

Choose a reason for hiding this comment

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

maybe we should strip .so without Build IDs as well, they could be unexpected bloat.

Copy link
Contributor Author

@risicle risicle Apr 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 Yes, interesting. The more I think about this, the more it feels what we should actually be doing is falling through to allow the regular strip setup hook to do its job afterwards instead of disabling it with dontStrip. Because surely after this hook's run all the outputs other than debug are safe to strip. Should this setup hook perhaps just concern itself with pulling the debug info out to debug and leave the existing stripping mechanisms to continue operating as normal?

Though that would be a bigger change and I'm not confident I'd understand all the ramifications of that.

@Mindavi Mindavi added the 6.topic: static Static builds (e.g. pkgsStatic) label Apr 10, 2022
@risicle risicle mentioned this pull request Aug 7, 2022
Merged
13 tasks
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 2, 2022
@RaitoBezarius
Copy link
Member

RaitoBezarius commented Dec 22, 2022

Can I help get this merged?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 22, 2022
@risicle
Copy link
Contributor Author

risicle commented Dec 23, 2022

I think this may have been superseded by #185537

@risicle risicle closed this Dec 23, 2022
@RaitoBezarius
Copy link
Member

RaitoBezarius commented Dec 24, 2022

Reproducer:

{ pkgs ? import <nixpkgs> {} }:
pkgs.pkgsStatic.openssl.overrideAttrs (old: {
  separateDebugInfo = true;
})

Resulting drv:

  • /nix/store/pv05dxdm7mfi67q16qzppb4vnmcixa5q-openssl-static-x86_64-unknown-linux-musl-3.0.7-bin
  • /nix/store/ark20hbj0kpyd34b77flcf59ia9anky5-openssl-static-x86_64-unknown-linux-musl-3.0.7-debug

Size of openssl binary (apparently unstripped given file output on it):

❯ du -hs openssl-unstripped                                                                                                       
5,6M	openssl-unstripped

After manual stripping using strip --strip-debug:

❯ du -hs openssl                                                                                                   
5,6M	openssl

It seems to be working as intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@symphorien symphorien symphorien left review comments

@FRidh FRidh Awaiting requested review from FRidh

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

Assignees

No one assigned

Labels

6.topic: python Python is a high-level, general-purpose programming language. 6.topic: static Static builds (e.g. pkgsStatic) 6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@risicle @RaitoBezarius @symphorien @Mindavi