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

cc-wrapper: pass always -B and -L together to avoid mixing parts of libc #158047

Merged
merged 1 commit into from
Feb 20, 2022

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Feb 3, 2022

Motivation for this change

Before this change cc-wrapper was mixing crt1.o and libc.so.6
from different libc during bootstrap:

expand-response-params>   -B/...-bootstrap-tools/lib
expand-response-params>   -B/...-glibc-2.35/lib/
...
expand-response-params>   -L/...-glibc-2.35/lib
expand-response-params>   -L/...-bootstrap-tools/lib

Normally it's not a problem as crt1.o did not change for
quite a few releases. But glibc-2.35 dropped __libc_csu_fini symbol:

expand-response-params> ld: ...-bootstrap-tools/lib/crt1.o: in function `_start':
expand-response-params> ...glibc-2.27/csu/../sysdeps/x86_64/start.S:101: undefined reference to `__libc_csu_fini'
expand-response-params> ld: /build/glibc-2.27/csu/../sysdeps/x86_64/start.S:102: undefined reference to `__libc_csu_init'
expand-response-params> collect2: error: ld returned 1 exit status

The change always passes -B/-L together to have a higher chance
to override both parts of libc. It allows bootstrapping toolchain
on glibc-2.35.

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 basic functionality of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@trofi
Copy link
Contributor Author

trofi commented Feb 3, 2022

Was too impatient. After a few packages it still fails. I probably need to find the opposite place where only -L is injected. Will debug a bit more.

@trofi trofi force-pushed the fix-cc-wrapper-lookup-paths branch from cc9d238 to 2610377 Compare February 3, 2022 22:12
@trofi
Copy link
Contributor Author

trofi commented Feb 3, 2022

Added -L in a second missing place. And now -rpath for both libcs is being injected and breaking bianries.

@trofi trofi force-pushed the fix-cc-wrapper-lookup-paths branch from 2610377 to ed9b9c1 Compare February 11, 2022 19:06
@trofi
Copy link
Contributor Author

trofi commented Feb 11, 2022

This version should be better now. Ready for review.

@trofi trofi marked this pull request as ready for review February 11, 2022 21:09
@trofi trofi mentioned this pull request Feb 13, 2022
14 tasks
@Ericson2314
Copy link
Member

Ericson2314 commented Feb 17, 2022

@trofi @lovesegfault If you have the cycles to spare, I would really, deeply appreciate instead making the bootstrap tools not throw everything together, in combined bin / inc / lib dirs. If we organized it like a little mini Nix store, with a separate dirs per each item, we would flat out avoid sketchy ordering problems altogether, and it would better line up with the other bootstrapping we do.

This would also pave the way for both of

especially the former, which failed precisely because of these sorts of issues!

I have been meaning to do this for years, but never found the time. It would be a game changer to see this first step done!

@trofi
Copy link
Contributor Author

trofi commented Feb 17, 2022

@trofi @lovesegfault If you have the cycles to spare, I would really, deeply appreciate instead making the bootstrap tools not throw everything together, in combined bin / inc / lib dirs. If we organized it like a little mini Nix store, with a separate dirs per each item, we would flat out avoid sketchy ordering problems altogether, and it would better line up with the other bootstrapping we do.

This would also pave the way for both of

* [Stdenv setup: Process dependencies before dependents #31414](https://github.com/NixOS/nixpkgs/pull/31414)

* [WIP: GCC with separated runtime libraries #132343](https://github.com/NixOS/nixpkgs/pull/132343)

* More generally making the native bootstrapping follow the cross patterns and be more "automatic"

especially the former, which failed precisely because of these sorts of issues!

I have been meaning to do this for years, but never found the time. It would be a game changer to see this first step done!

I agree it would help. I'd like to look at it longer term as well. I have a few questions:

  1. Is this change blocking the other efforts or or has a potential of causing subtle problems? I'd like to get it merged :)
  2. Is bootstrap from FHS system somewhat supported by nixpkgs? I think we would need to deal with the -B/-L ordering for that case as well.

@Ericson2314
Copy link
Member

Bootstrap from native FHS is at most barely supported and a separate bootstrap. I wouldn't worry about it.

Could we just cut down the comment to say something like "warning, changing the order could break certain bootstraps until libraries are better separated to more does"? I don't want to falsely imply this is a better order when I think the rearranging is just a stop-gap fix and the order ought not to matter.

In NixOS#158042 I noticed order
mismatch as a bootstrap build failure when building x86_64-linux
against glibc-2.35 in nixpkgs (bootstrap libs has glibc-2.27):

    expand-response-params> ld: /nix/store/p4s4jf7aq6v6z9iazll1aiqwb34aqxq9-bootstrap-tools/lib/crt1.o: in function `_start':
    expand-response-params> /build/glibc-2.27/csu/../sysdeps/x86_64/start.S:101: undefined reference to `__libc_csu_fini'
    expand-response-params> ld: /build/glibc-2.27/csu/../sysdeps/x86_64/start.S:102: undefined reference to `__libc_csu_init'
    expand-response-params> collect2: error: ld returned 1 exit status

Here crt1.o from glibc-2.27 links against libc.so.6 from glibc-2.35.

This happens because ordering of `-L` (influences `libc.so` lookup) and
`-B` (influences `crt1.o` lookup) flags differs:

    expand-response-params>   -B/...-bootstrap-tools/lib
    expand-response-params>   -B/...-glibc-2.35/lib/
    ...
    expand-response-params>   -L/...-glibc-2.35/lib
    expand-response-params>   -L/...-bootstrap-tools/lib

The change makes consistent ordering of `-L`/`-B` and allows getting to
stage4 for `glibc-2.35` target.
@trofi trofi force-pushed the fix-cc-wrapper-lookup-paths branch from ed9b9c1 to 649ebfb Compare February 20, 2022 10:46
@trofi
Copy link
Contributor Author

trofi commented Feb 20, 2022

Shrunk the comment, explicitly mentioned that ordering is a workaround.

@lovesegfault lovesegfault merged commit 86be433 into NixOS:staging Feb 20, 2022
@trofi trofi deleted the fix-cc-wrapper-lookup-paths branch February 20, 2022 19:15
@Ericson2314
Copy link
Member

Thanks, and sorry to be a nit-pick!

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.

3 participants