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: pass --with-build-sysroot=/ #181943

Merged
merged 1 commit into from
Jul 24, 2022
Merged

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Jul 18, 2022

Without this change cross-built gcc fails to detect stack protector style:

$ nix log -f pkgs/stdenv/linux/make-bootstrap-tools-cross.nix powerpc64le.bootGCC | fgrep __stack_chk_fail
checking __stack_chk_fail in target C library... no
checking __stack_chk_fail in target C library... no

It happens because gcc treats search paths differently:

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/configure.ac;h=446747311a6aec3c810ad6aa4190f7bd383b94f7;hb=HEAD#l2458

 if test x$host != x$target || test "x$TARGET_SYSTEM_ROOT" != x ||
    test x$build != x$host || test "x$with_build_sysroot" != x; then
   ...
   if test "x$with_build_sysroot" != "x"; then
     target_header_dir="${with_build_sysroot}${native_system_header_dir}"
   elif test "x$with_sysroot" = x; then
     target_header_dir="${test_exec_prefix}/${target_noncanonical}/sys-include"
   elif test "x$with_sysroot" = xyes; then
     target_header_dir="${test_exec_prefix}/${target_noncanonical}/sys-root${native_system_header_dir}"
   else
     target_header_dir="${with_sysroot}${native_system_header_dir}"
   fi
 else
   target_header_dir=${native_system_header_dir}
 fi

By passing --with-sysroot=/ we trick cross-case to use
target_header_dir="${with_sysroot}${native_system_header_dir}"
which makes it equivalent to non-cross
target_header_dir=${native_system_header_dir}

Tested the following setups:

  • cross-compiler without libc headers (powerpc64le-static)
  • cross-compiler with libc headers (powerpc64le-debug)
  • cross-build compiler with libc headers (powerpc64le bootstrapTools)

Before the change only 2 of 3 compilers detected libc headers.
After the change all 3 compilers detected libc headers.

Description of changes
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.

@trofi trofi requested a review from matthewbauer as a code owner July 18, 2022 12:19
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jul 18, 2022
@ghost
Copy link

ghost commented Jul 19, 2022

To clarify:

Without this change cross-built gcc

... meaning build!=host.

Just want to make sure nobody reading along is thinking host!=target. What we're talking about here is ${stdenvBootstrapTools}/bin/gcc, not the compiler that builds it.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thank you for sorting this out, @trofi.

This is such a "spooky action at a distance" that we really ought to force gcc's build to fail if it cannot find the glibc headers. Do you know if there is a configure option we can pass to gcc to prevent it from silently disabling features instead of reporting an error?

@ghost
Copy link

ghost commented Jul 19, 2022

Okay, I think this is starting to make sense now. I need to run a bunch of builds overnight; assuming those all go well, and we can explain why this was a problem for powerpc64le but not for mips64el, I'll close #181802 in favor of this.

Unfortunately that means we will need to wait for Hydra to rebuild with it and then post a new bootstrap-files to tarballs.nixos.org ☹️

@ghost
Copy link

ghost commented Jul 19, 2022

I've verified that this PR fixes the stack-protector issue in stdenv on powerpc64le, so I've closed #181802.

I've also rebuilt using a version of this PR which removes the &&hostPlatform==targetPlatform and passes --with-build-sysroot as suggested here, and have started a new bootstrap with that. No problems so far; if the build finishes I do think we should use that flag instead so we can simplify the conditional and so this bug won't resurface in canadian cross compiles.

I'm also re-running the bootstrap on mips64el just to make sure nothing broke. That may take a day or two unfortunately.

@ghost
Copy link

ghost commented Jul 19, 2022

I've also rebuilt using a version of this PR which removes the &&hostPlatform==targetPlatform and passes --with-build-sysroot as suggested here, and have started a new bootstrap with that.

This bootstrap completed successfully.

@trofi trofi changed the base branch from master to staging July 19, 2022 21:54
@ofborg ofborg bot added 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jul 19, 2022
@trofi trofi changed the title gcc: pass --with-sysroot=/ for cross-built gcc gcc: pass --with-build-sysroot=/ for cross-built gcc Jul 21, 2022
@trofi
Copy link
Contributor Author

trofi commented Jul 23, 2022

Darwin did not like it:

impure path `//' used in link
collect2: error: ld returned 1 exit status

Will need to check why,

Don't immediately see how // sneaks into ld argument. Suspect linker script. Turned the change back to only cross-compilers and cross-built compilers.

Found -syslibroot // in darwin-specific gcc specs. We can safely skip it as not introducing impurity. Re-enabled --with-build-sysroot=/ on non-cross as well.

@trofi
Copy link
Contributor Author

trofi commented Jul 23, 2022

One more change:

Dropped mingw special case for no-libc build. Before the change we passed both --without-headers --with-native-system-headers-dir for no-libc gcc-static builds. This tricked darwin builds to find sys/sdt.h and fail inhibid_libc builds. Now all targets avoid passing native headers for gcc-static builds.

Without this change cross-built gcc fails to detect stack protector style:

    $ nix log -f pkgs/stdenv/linux/make-bootstrap-tools-cross.nix powerpc64le.bootGCC | fgrep __stack_chk_fail
    checking __stack_chk_fail in target C library... no
    checking __stack_chk_fail in target C library... no

It happens because gcc treats search paths differently:

    https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/configure.ac;h=446747311a6aec3c810ad6aa4190f7bd383b94f7;hb=HEAD#l2458

     if test x$host != x$target || test "x$TARGET_SYSTEM_ROOT" != x ||
        test x$build != x$host || test "x$with_build_sysroot" != x; then
       ...
       if test "x$with_build_sysroot" != "x"; then
         target_header_dir="${with_build_sysroot}${native_system_header_dir}"
       elif test "x$with_sysroot" = x; then
         target_header_dir="${test_exec_prefix}/${target_noncanonical}/sys-include"
       elif test "x$with_sysroot" = xyes; then
         target_header_dir="${test_exec_prefix}/${target_noncanonical}/sys-root${native_system_header_dir}"
       else
         target_header_dir="${with_sysroot}${native_system_header_dir}"
       fi
     else
       target_header_dir=${native_system_header_dir}
     fi

By passing --with-build-sysroot=/ we trick cross-case to use
`target_header_dir="${with_sysroot}${native_system_header_dir}"`
which makes it equivalent to non-cross
`target_header_dir="${with_build_sysroot}${native_system_header_dir}"`

Tested the following setups:
- cross-compiler without libc headers (powerpc64le-static)
- cross-compiler with libc headers (powerpc64le-debug)
- cross-build compiler with libc headers (powerpc64le bootstrapTools)

Before the change only 2 of 3 compilers detected libc headers.
After the change all 3 compilers detected libc headers.

For darwin we silently ignore '-syslibroot //' argument as it does not
introduce impurities.

While at it dropped mingw special case for no-libc build. Before the change
we passed both '--without-headers --with-native-system-headers-dir' for
no-libc gcc-static builds. This tricked darwin builds to find sys/sdt.h
and fail inhibid_libc builds. Now all targets avoid passing native headers
for gcc-static builds.

While at it fixed correct headers passing to
--with-native-system-headers-dir= in host != target case: we were passing
host's headers where intention was to pass target's headers.
Noticed the mismatch as a build failure on pkgsCross.powernv.stdenv.cc
on darwin where `sys/sdt.h` is present in host's headers (libSystem)
but not target's headers (`glibc`).

Co-authored-by: Adam Joseph <[email protected]>
@trofi
Copy link
Contributor Author

trofi commented Jul 23, 2022

While at it fixed correct headers passing to --with-native-system-headers-dir= in host != target case: we were passing host's headers where intention was to pass target's headers. Noticed the mismatch as a build failure on pkgsCross.powernv.stdenv.cc on darwin where sys/sdt.h is present in host's headers (libSystem) but not target's headers (glibc).

@trofi
Copy link
Contributor Author

trofi commented Jul 23, 2022

Should be ready now.

I tested a few things:

  • on x86_64-linux:
    • gcc (native)
    • pkgsMusl.hello, pkgsStatic.hello (less frequent toolchains)
    • pkgsCross.powernv.stdenv.cc, pkgsCross.mingw32.stdenv.cc (cross-compiler toolchains)
    • pkgsCross.powernv.gcc (cross-building compilers)
  • on x86_64-darwin:
    • gcc (native)
    • pkgsMusl.hello, pkgsStatic.hello (less frequent toolchains)
    • pkgsCross.powernv.stdenv.cc, pkgsCross.mingw32.stdenv.cc, pkgsCross.aarch64-darwin.stdenv.cc (cross-compiler toolchains)
    • pkgsCross.powernv.gcc (cross-building compilers)

All cases seem to work and detect target libc features. darwin test was the best as it's host libc is very different from target linux libc. It made pre-existing headers mismatches very clear.

@Ericson2314
Copy link
Member

This is a very nicely done fix. I love the attention to not just making sure things work, but making sure things work for the same reasons across both cross and native. Thanks!!

@Ericson2314 Ericson2314 merged commit 21966e1 into NixOS:staging Jul 24, 2022
@trofi trofi deleted the fix-cross-built-gcc branch July 24, 2022 07:33
@trofi
Copy link
Contributor Author

trofi commented Jul 24, 2022

Thank you! There is one minor fallout: not-quite-cross case of pkgsLLVM breakage. Proposed the fix as #182666.

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.

4 participants