-
-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
UEFI cross: don't overthink it #228374
UEFI cross: don't overthink it #228374
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking this on; this looks much better than our hacks in #226145! 🙂
The next part will be configuring Rust in Nixpkgs to build UEFI applications in a way that's compatible with our compiler wrappers. That work is well under way, but it requires discussing some patches with Rust upstream before it makes sense to integrate it into Nixpkgs.
Is the plan to get rustc
to (when using lld-link
) emit UNIX style command line args for Windows targets so we don't have to rework cc-wrapper
/ld-wrapper
?
The plan is to get rustc to not use |
This fixes building for x86_64-windows with no libc (for UEFI). Otherwise, it would try to include a malloc header.
It makes sense to allow platform definitions to opt out of having libc at all. One use case would be targetting some obscure new Linux target that doesn't have a libc implementation yet, and another is UEFI, which is basically libc-less Windows. Not having libc is not commonly specified in (GNU) triples (even Linux's build system will just target either -gnu or -musl depending on the platform), so instead, we use a separate attribute for it.
clangNoLibc always uses LLVM bintools, so it still has the useLLVM semantics.
3630579
to
6be7a58
Compare
|
If you look at a little bit more of my sentence:
you'll see that I was not saying "triples don't encode which libc is in use". I am making a more specific claim, which is that triples do not encode absence of libc. I looked at the same files you linked to verify I was correct about that. I think it's strange too, but that's how it is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for the triple choice.
lib/systems/examples.nix
Outdated
|
||
# See: https://github.com/rust-lang/rust/pull/56769/files#r241916113 | ||
i686-uefi = { | ||
system = "x86_64-windows"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Edit: This causes )platform.config
to be i686-unknown-uefi
which is rejected by config.sub
Please:
system = "x86_64-windows"; | |
config = "x86_64-w64-mingw32"; |
This is the gnu-config
triple for this platform. Please use it. See #226145 (review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
causes
platform.config
to bei686-unknown-uefi
which is rejected byconfig.sub
:$ /nix/store/ayj17jhbiqc1k043ld3nnb3xqs0p22sl-gnu-config-2023-01-21/config.sub i686-unknown-uefi Invalid configuration `i686-unknown-uefi': OS `uefi' not recognized
No it doesn't:
nix-repl> (lib.systems.elaborate "i686-windows").config
"i686-pc-windows-msvc"
config.sub won't recognise this either, but that's because GNU doesn't support the MSVC ABI, so there's no correct triple to use here. This is correct for LLVM, which is what we actually use with these system definitions.
Are you just copying these comments from the other PR without checking whether they apply to this one? It seems like you're arguing against things that this PR isn't trying to do…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config.sub won't recognise this either, but that's because GNU doesn't support the MSVC ABI, so there's no correct triple to use here.
config.sub
recognizes x86_64-w64-mingw32
just fine:
$ /nix/store/166jql18s3x9sbzwd6bls0k9v9dbra02-gnu-config-2021-01-25/config.sub x86_64-w64-mingw32
x86_64-w64-mingw32
It is the correct triple and has been used by EFI hackers since long before clang could target EFI.
lib/systems/examples.nix
Outdated
}; | ||
|
||
x86_64-uefi = { | ||
system = "x86_64-windows"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
system = "x86_64-windows"; | |
config = "x86_64-w64-mingw32"; |
That is a much clearer way of putting it, but I think it's moot: for binaries with no libc that aren't kernels, this field still encodes the ABI.
The distinctive part of UEFI is its calling convention, i.e. its ABI. Are you sure that UEFI's calling convention is exactly identical to Windows'? On ARM it is definitely totally different. |
I know that Rust just tells LLVM it's compiling for Windows, so I was working based on that. Do you think they're wrong to do that? |
Okay, you know what? The example are not necessary, because people can specify what system they want to use when they import Nixpkgs, and I really just wanted to do a few simple package fixes to get the ball rolling. So I'm going to drop the examples from this PR, and then hopefully we can agree that the package fixes make sense, and if somebody else wants to take up adding examples, they can do that without me. |
That link is talking specifically about |
Examples dropped. |
According to <https://gcc.gnu.org/legacy-ml/gcc-patches/2015-08/msg00836.html>, all code is position-independent on Windows. Some compilers apparently warn for -fPIC on Windows, and clang errors: > clang-15: error: unsupported option '-fPIC' for target 'x86_64-pc-windows-msvc' I'm guessing the check was hostPlatform instead of targetPlatform by mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked through commit by commit, and everything looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much :)
Hey @alyssais I'd like to apologize for over-trimming the part of your message I was replying to, and in doing so changing its meaning. That was not intentional. I'd also like to apologize for the terse wording in my review. There seem to be three versions of this work. I discovered each of them only after completing my review of the previous one, so by the time I got to this one (the third) it was getting a bit tedious, and late. No more 4:00am code reviews for me. |
Description of changes
#226145 was a good spike at getting UEFI cross compilation to work, but we got bogged down in questions about triples and wrappers. I've come to the conclusion that actually, we don't need a new triple, as the only place where we want to diverge from normal MSVC Windows is where we need to accomodate not having a libc, and that's not something that triples encode (at least not the GNU-style ones we generally use in Nixpkgs). (It's a bit surprising, but it's true! For example, Linux's build system just uses either -gnu or -musl for each architecture it supports.) After that realisation, I started implementing UEFI from scratch, and found it became much simpler. Here, I've built up to UEFI cross support by just making a bunch of small, common sense changes that improve handling of either Windows, or no-libc. This is all we need to be able to build the same lanzaboote example package from #226145.
The main goal here is to build Rust programs, but having a Rust compiler means building compiler-rt for the cross system, which demonstrates that with these changes Nixpkgs is also capable of building C/C++ for UEFI.
This is part 1 in a series. The next part will be configuring Rust in Nixpkgs to build UEFI applications in a way that's compatible with our compiler wrappers. That work is well under way, but it requires discussing some patches with Rust upstream before it makes sense to integrate it into Nixpkgs.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)