stdenv: make stage3.{gmp,mpfr,mpc,isl} do what the comment says #169378

ghost · 2022-04-20T00:18:22Z

This PR consists of three commits, which should be reviewed independently (but sequentially). You can merge any prefix of the three-commit series without breaking anything.

Motivation

The comments in stdenv/linux/default.nix disagree with what the code actually does. In particular, the comments claim that the special static builds of libgmp/libmpfr/libmpc/libisl in stage3 are "only used by GCC". This not quite correct: the "special" stage3.gmp ends up linked into the ephemeral coreutils which is built in stage4 (stage4.coreutils).

This PR fixes the code to agree with the comment and intent.

This also closes #168983 which was caused by a conflict between statically-linked libraries (libgmp.a) and dynamically-linked libraries (everything else) which were both compiled with -fstack-protector. Mixing static and shared libraries in the presence of hardening flags is not generally well-supported, and is certainly not well-tested. In particular, on some platforms -fstack-protector induces a link-time dependency on libssp, which is implemented differently for statically (libssp_nonshared.a) vs dynamically (libssp.so) linked objects; this causes link-time failures in the ephemeral stage4.coreutils.

Description of changes

The first commit c527af1 forces gmp to rebuild in stage4, using the stage4 stdenv, rather than recycling stage3.gmp.

The second commit cad68ef causes the implementation of stage3 to agree with the intent and descriptive comment, by restricting the visibility of the specially-built libraries so they are visible only to gcc-unwrapped. This will cause stage4.coreutils to use the ordinary stage4.gmp rather than the customized staticified stage3.gcc-unwrapped.gmp; because of this cad68ef requires c527af1.

The final commit 9dd7908 is strictly ergonomic; it appends "-stage4" to the name of the ephemeral coreutils that is used only for stdenv bootstrapping, so it can be distinguished easily from the final coreutils. I found this to be extremely helpful when untangling what was going on here.

Things done

Built on platform(s)
- x86_64-linux
- aarch64-linux
- powerpc64le-linux
- mips64el-linux
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/)
Fits CONTRIBUTING.md.

sternenseemann

Overall sounds sensible.

pkgs/stdenv/linux/default.nix

ghost · 2022-04-23T22:09:59Z

@ofborg eval

ghost · 2022-04-27T04:49:11Z

@ofborg eval

ghost · 2022-04-27T07:38:31Z

The aarch64-darwin builder timeout appears to not be functioning correctly.

ghost · 2022-04-27T07:42:04Z

Last commit extends the -stageX marker to the other packages that get built built twice: libmpc, libmpfr, gmp, and isl.

Mindavi

Yeah, I think this makes sense. I personally like the -stageX indicators to easily spot any errors where these packages leak in.

Needs rebase.

As explained in the comment, this ensures that stage4-coreutils does not leak a reference to the bootstrap-files by way of libgmp. This will allow the next patch in this series to build stage4-coreutils using a dynamically-linked (rather than statically-linked) libgmp.

The usage of `makeStaticLibraries` in stdenv/linux/default.nix is prefaced by this comment: # Link GCC statically against GMP etc. This makes sense because # these builds of the libraries are only used by GCC, so it # reduces the size of the stdenv closure. However "these builds of the libraries are only used by GCC" is not actually true. As currently written, the stage4 coreutils links against these customized, static-ified libraries. Beside the fact that the code doesn't actually do what it says, this causes other problems as well. One example is #168983, which arises because have a dynamically-linked binary (coreutils) which is built from statically-linked libraries (libgmp.a); doing this causes mayhem on platforms where `-fstack-protector` needs an auxiliary `libssp.{so,a}` library; we end up with link failures because some parts of the resulting binary want `libssp.so` and other parts want `libssp_nonshared.a`. Let's make the code actually do what the comment says, by moving these definitions into the `gcc-unwrapped` override. This will cause the stage4-coreutils to link against libgmp dynamically, rather than statically. For this reason this commit depends on the previous commit, which allows that to be done without creating a forbidden reference from stdenv-final to the bootstrap-files.

During stdenv bootstrapping, coreutils is built twice. This makes troubleshooting very difficult, because both packages have name="coreutils", so it is a hassle to figure out "which coreutils am I using / is not building"? The first of these builds is used only in stage4, and is not part of the final stdenv. Let's label that one with a different `name` attribute to make it obvious which is which.

Co-authored-by: sternenseemann <[email protected]>

ghost · 2022-06-28T22:50:30Z

Ping...

This is the last PR needed in order for stdenv to build on powerpc64le-linux.

Mindavi · 2022-06-29T22:32:18Z

Tried building libwebsockets curl gmp isl boost without any issues. Now also rebuilding without this patchset, with ca-derivations to see if it's bit-by-bit the same (it may not be for unrelated reasons, but I'm curious). In principle good to go in my opinion.

ghost · 2022-06-29T23:58:40Z

Now also rebuilding without this patchset, with ca-derivations to see if it's bit-by-bit the same (it may not be for unrelated reasons, but I'm curious).

All the RPATHs will change due to the outpath of stdenv changing. You'll definitely need to run nuke-refs on both sides before doing the comparison.

ghost · 2022-06-30T01:36:24Z

I'll try the nuke-refs approach once my build gets caught up, might not be until tomorrow.

I don't think ca-refs helps here because this PR will not leave ${stdenv.cc} bit-for-bit exactly the same as before the PR. So the outpaths of ${stdenv.cc}, and therefore of stdenv, and anything with a reference to it, will change even with ca-refs.

A nuke-refs that operates on a nar piped to standard input would be the right tool here. I might write one. I can see how being able to prove that a PR changes nothing but outpaths would improve mergeability of PRs that change things deep within stdenv.

Mindavi · 2022-06-30T05:33:49Z

The outputs created before and after applying this

before

  "old hashes (f529aef3dd8daff2a8d342736974d97e1b9adde8)",
  [
    {
      "drvPath": "/nix/store/6z3cjric4i9l7rdjyn1k9c69sh8phwzn-boost-1.77.0.drv",
      "outputs": {
        "dev": "/nix/store/89azyhlgn9qc5qgdq3fijvqfwnkky0k4-boost-1.77.0-dev",
        "out": "/nix/store/is2w1z69jn37cyn5dk0dwsx7vmi5fgkd-boost-1.77.0"
      }
    },
    {
      "drvPath": "/nix/store/iqsr3dxvv3851wwnfcj57d7kqnj2bj3x-curl-7.83.1.drv",
      "outputs": {
        "bin": "/nix/store/34syrjdr1971hgv32anz6xwkwrsandry-curl-7.83.1-bin",
        "man": "/nix/store/a8qc805dn1q520mxxh4bq3iwqd4ybk0d-curl-7.83.1-man"
      }
    },
    {
      "drvPath": "/nix/store/ijr6902bvqzdkddwb8sm572mizk73msn-gmp-with-cxx-6.2.1.drv",
      "outputs": {
        "out": "/nix/store/04amblxwih41nzslcscs6jaqzr2gjdn4-gmp-with-cxx-6.2.1"
      }
    },
    {
      "drvPath": "/nix/store/fic9vh40xcikp1s75fa76l75bxzw7b59-isl-0.20.drv",
      "outputs": {
        "out": "/nix/store/2xlhbbsh8q58xmldqzsad8cr8xwl2n1j-isl-0.20"
      }
    },
    {
      "drvPath": "/nix/store/1dxmddd93df80qdd6ljkhhv2vjympafx-libwebsockets-4.3.1.drv",
      "outputs": {
        "out": "/nix/store/w610n13mv6aplf4906wqg9vkrqaq6aan-libwebsockets-4.3.1"
      }
    }
  ]

after:

  "new hashes (02630180fad510ee877fa51112b7c7b230ef2f13)",
  [
    {
      "drvPath": "/nix/store/v4dbh7p18pka11cvd7b14w75dqm9md63-boost-1.77.0.drv",
      "outputs": {
        "out": "/nix/store/is2w1z69jn37cyn5dk0dwsx7vmi5fgkd-boost-1.77.0"
      }
    },
    {
      "drvPath": "/nix/store/pq28fqdld2chxsf2cmv9nq3jara0cbbi-curl-7.83.1.drv",
      "outputs": {
        "bin": "/nix/store/52vflqgbqqris07cscqifr920llvk80k-curl-7.83.1-bin",
        "man": "/nix/store/a8qc805dn1q520mxxh4bq3iwqd4ybk0d-curl-7.83.1-man"
      }
    },
    {
      "drvPath": "/nix/store/bc1i51b70srkcavc3gmlwydk0rcl1ja3-gmp-with-cxx-6.2.1.drv",
      "outputs": {
        "out": "/nix/store/04amblxwih41nzslcscs6jaqzr2gjdn4-gmp-with-cxx-6.2.1"
      }
    },
    {
      "drvPath": "/nix/store/78d5aqk0vm5ic6yij6ynygijqfymjhcd-isl-0.20.drv",
      "outputs": {
        "out": "/nix/store/2xlhbbsh8q58xmldqzsad8cr8xwl2n1j-isl-0.20"
      }
    },
    {
      "drvPath": "/nix/store/qig24v5jsjc9mf57a523cm66hvhaddqc-libwebsockets-4.3.1.drv",
      "outputs": {
        "out": "/nix/store/dcsk0imymx88lsqrkx0mz9hdc8zdpv0n-libwebsockets-4.3.1"
      }
    }
  ],

I did a diffoscope of curl and I think this is a case of #151475, since the only difference I see is the Build-ID that's injected. Libwebsockets differs because openssl differs, which also has separateDebugInfo and also has a different Build-ID.

Not sure about the extra boost output that's listed... But otherwise, boost, gmp, isl all give the same output, so that's reassuring.

Note that I indeed did not diff stdenv.cc, maybe that could be interesting to see too.

Mindavi · 2022-06-30T05:43:52Z

I don't see a big risk anymore that this will cause regressions, so let's go for it. Both your testing and mine is sufficient to give me confidence that this is ok and an improvement. Thanks for pushing through!

Mindavi · 2022-07-02T07:08:33Z

I see this in perl:

old

····#·pwd·in·their·path·but·it·is·not·/nix/store/hm0rii2ps75absqv8nlb3lcl8gn2a8xl-coreutils-9.1/bin/pwd·or·/usr/nix/store/hm0rii2ps75absqv8nlb3lcl8gn2a8xl-coreutils-9.1/bin/pwd?

new:

····#·pwd·in·their·path·but·it·is·not·/nix/store/bb76llghh056xyf0jzg2nvalyaaiy5c0-coreutils-stage4-9.1/bin/pwd·or·/usr/nix/store/bb76llghh056xyf0jzg2nvalyaaiy5c0-coreutils-stage4-9.1/bin/pwd?

Seems like stage4 is leaking into stdenv? Those annotations are surely useful to find these kinds of issues :)

Path is perl-5.34.1/lib/perl5/5.34.1/x86_64-linux-thread-multi/Cwd.pm

Edit: Oh! I think that's because of the inherit in the last stage... oops?

Mindavi · 2022-07-02T07:33:55Z

pkgs/stdenv/linux/default.nix

+      #   stage5.gcc -> stage4.coreutils -> stage3.glibc -> bootstrap
+      gmp = lib.makeOverridable (super.gmp.override { stdenv = self.stdenv; }).overrideAttrs (a: { pname = "${a.pname}-stage4"; });
+
+      # coreutils gets rebuilt both here and also in the final stage; we rename this one to avoid confusion


I think it's not true that it gets rebuilt in the final stage.

You are correct.

I'm unable to reproduce the situation I was experiencing two months ago, where coreutils was getting built twice.

Unfortunately I don't have much more time today, so I think the best course of action is to simply revert the commit that added the "-stage4" marker. If it is still necessary (which seems unlikely) I will resubmit it as a standalone PR. I'll have more time to look at this tomorrow morning.

I'm starting to suspect that the problem may have been caused by my own site-specific overlays, which have changed a lot in the last two months. I maintain them as extra commits on top of nixpkgs/master with frequent rebasing, so I don't have a version history for them. I need to start keeping one.

I figured out the reason. We should still revert the commit (i.e. we should merge #179961).

I will explain in more detail tomorrow.

ghost · 2022-07-02T22:10:27Z

Investigating.

ghost · 2022-07-02T22:46:03Z

#179961

ghost · 2022-07-03T17:22:31Z

#179961

Postmortem in case anybody is curious why this happened.

github-actions bot added the 6.topic: stdenv Standard environment label Apr 20, 2022

This was referenced Apr 20, 2022

stdenv/adapters.nix: disable stackprotector hardening on 64-bit powerpc during static-only stages of stdenv bootstrap #168983

Closed

haskell.compiler.ghc865Binary: add powerpc64le bootstrap #168213

Merged

ghost changed the title ~~stdenv: make it do what it says~~ stdenv: make stage3.{gmp,mpfr,mpc,isl} it do what the comment says Apr 20, 2022

ghost changed the title ~~stdenv: make stage3.{gmp,mpfr,mpc,isl} it do what the comment says~~ stdenv: make stage3.{gmp,mpfr,mpc,isl} do what the comment says Apr 20, 2022

Artturin requested review from Ericson2314, Mindavi and vcunat April 20, 2022 02:46

This comment was marked as resolved.

Sign in to view

ofborg bot added 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Apr 20, 2022

sternenseemann reviewed Apr 20, 2022

View reviewed changes

pkgs/stdenv/linux/default.nix Outdated Show resolved Hide resolved

This comment was marked as duplicate.

Sign in to view

ofborg bot requested review from edolstra and dasJ April 24, 2022 00:27

This was referenced Apr 29, 2022

gcc: if hostPlatform.isPower64, configure with "--with-long-double-64" #170402

Closed

gcc: if isPower64, add --with-long-double-64 --without-long-double-128 #170857

Closed

Mindavi approved these changes Jun 2, 2022

View reviewed changes

Adam Joseph and others added 4 commits June 5, 2022 00:35

Update pkgs/stdenv/linux/default.nix

a9e0d86

Co-authored-by: sternenseemann <[email protected]>

stdenv: add -stageX markers to gmp, mpfr, libmpc, and isl

0263018

ghost marked this pull request as ready for review June 5, 2022 07:47

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Jun 5, 2022

ghost requested a review from sternenseemann June 12, 2022 20:26

Mindavi merged commit 2fcdf54 into NixOS:staging Jun 30, 2022

Mindavi reviewed Jul 2, 2022

View reviewed changes

ghost mentioned this pull request Jul 2, 2022

Revert "stdenv: label the ephemeral coreutils-stage4 package" #179961

Merged

13 tasks

ncfavier mentioned this pull request Jul 19, 2022

Fix tests maybe? nix-community/home-manager#3099

Closed

ghost deleted the stdenv-makeStaticLibraries-do-what-comment-says branch January 23, 2024 06:52

Artturin mentioned this pull request Jan 29, 2024

List of @amjoseph-nixpkgs Pull requests before account removal #284796

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stdenv: make stage3.{gmp,mpfr,mpc,isl} do what the comment says #169378

stdenv: make stage3.{gmp,mpfr,mpc,isl} do what the comment says #169378

ghost commented Apr 20, 2022 •

edited by ghost

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

sternenseemann left a comment

This comment was marked as duplicate.

ghost commented Apr 23, 2022

ghost commented Apr 27, 2022

ghost commented Apr 27, 2022

ghost commented Apr 27, 2022

Mindavi left a comment •

edited

Loading

ghost commented Jun 28, 2022

Mindavi commented Jun 29, 2022

ghost commented Jun 29, 2022

ghost commented Jun 30, 2022 •

edited by ghost

Loading

Mindavi commented Jun 30, 2022 •

edited

Loading

Mindavi commented Jun 30, 2022

Mindavi commented Jul 2, 2022 •

edited

Loading

Mindavi Jul 2, 2022

ghost Jul 2, 2022 •

edited by ghost

Loading

ghost Jul 3, 2022 •

edited by ghost

Loading

ghost commented Jul 2, 2022

ghost commented Jul 2, 2022

ghost commented Jul 3, 2022

stdenv: make stage3.{gmp,mpfr,mpc,isl} do what the comment says #169378

stdenv: make stage3.{gmp,mpfr,mpc,isl} do what the comment says #169378

Conversation

ghost commented Apr 20, 2022 • edited by ghost Loading

Motivation

Description of changes

Things done

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

sternenseemann left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

ghost commented Apr 23, 2022

ghost commented Apr 27, 2022

ghost commented Apr 27, 2022

ghost commented Apr 27, 2022

Mindavi left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Jun 28, 2022

Mindavi commented Jun 29, 2022

ghost commented Jun 29, 2022

ghost commented Jun 30, 2022 • edited by ghost Loading

Mindavi commented Jun 30, 2022 • edited Loading

Mindavi commented Jun 30, 2022

Mindavi commented Jul 2, 2022 • edited Loading

Mindavi Jul 2, 2022

Choose a reason for hiding this comment

ghost Jul 2, 2022 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jul 3, 2022 • edited by ghost Loading

Choose a reason for hiding this comment

ghost commented Jul 2, 2022

ghost commented Jul 2, 2022

ghost commented Jul 3, 2022

ghost commented Apr 20, 2022 •

edited by ghost

Loading

Mindavi left a comment •

edited

Loading

ghost commented Jun 30, 2022 •

edited by ghost

Loading

Mindavi commented Jun 30, 2022 •

edited

Loading

Mindavi commented Jul 2, 2022 •

edited

Loading

ghost Jul 2, 2022 •

edited by ghost

Loading

ghost Jul 3, 2022 •

edited by ghost

Loading