Use LLVM's UEFI targets #132570
Use LLVM's UEFI targets #132570
Conversation
|
r? @chenyukang rustbot has assigned @chenyukang. Use |
rustbot
added
S-waiting-on-review
|
These commits modify compiler targets. |
|
cc @Ayush1325 |
This comment has been minimized.
This comment has been minimized.
The UEFI targets previously passed Windows targets to LLVM, because that's what they're the most similar to. That's not ideal though (part of this problem was analyzed in 21e062d), but it seems like LLVM has since the introduction of these targets gained support for UEFI in-tree, so we should just use that: https://discourse.llvm.org/t/rfc-uefi-driver-support-uefi-target/73261
| data_layout: "e-m:x-p:32:32-p270:32:32-p271:32:32-p272:64:64-\ | ||
| i64:64-i128:128-f80:32-n8:16:32-a:0:32-S32" | ||
| i128:128-f64:32:64-f80:32-n8:16:32-S128" |
There was a problem hiding this comment.
Running ./x test tests/assembly/targets/targets-pe.rs shows that the data layout for our i686-unknown-uefi, for it to match Clang's i686-unknown-uefi, should have the following modifications:
- Remove
i64:64: Decreases the alignment ofi64to 32 bits. - Add
f64:32:64: Decreases the alignment off64to 32 bits. - Remove
a:0:32: Decreases the alignment of aggregates (structs?) to 32 bits. - Replace
S32withS128: Increases the natural alignment of the stack to 128 bits.
I have no idea if this is the desired behaviour?
There was a problem hiding this comment.
Remove i64:64: Decreases the alignment of i64 to 32 bits.
Add f64:32:64: Decreases the alignment of f64 to 32 bits.
These I think are not desirable. Edk2 sets -malign-double for ia32 in https://github.com/tianocore/edk2/blob/HEAD/BaseTools/Conf/tools_def.template. So while the UEFI spec isn't super clear on what the alignment for double and long long should be on ia32, the de facto alignment is 8 bytes.
See rhboot/shim#516 for a real-world example where this makes a difference.
There was a problem hiding this comment.
can we file a bug against LLVM, then?
There was a problem hiding this comment.
I would prefer if @nicholasbishop did it, you seem more knowledgeable about it - but I can try to do it if you don't have the time?
There was a problem hiding this comment.
I just did a quick sanity check of compiling C code with these triples, and I suspect this isn't fully implemented in clang yet (tested 19.1.0). Getting errors like: error: backend data layout 'e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128' does not match expected target description 'e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128'.
I think the implementation work in LLVM is ongoing, so not quite ready for us to use yet. @Prabhuk can you confirm that?
There was a problem hiding this comment.
Breaking from edk2 is definitely not desirable. From what I understand, even most of the proprietary UEFI implementations use edk2 as the base.
There was a problem hiding this comment.
Apologies for the slow response! I am on vacation till mid November. I have a WIP backend patch (llvm/llvm-project#109320) to support X86_64 first. I am debugging a assertion failure I ran into while compiling a downstream EFI example. I will prioritize upstreaming fixing it and sending out a PR as soon as I return. aarch64 will be next. w.r.t to data layout, I need to look at a closer look once I return before commenting.
workingjubilee
added
S-blocked
The UEFI targets previously passed Windows targets to LLVM, because that's what they're the most similar to.
That's not ideal though (part of this problem was analyzed in #64334), but it seems like LLVM has gained support for UEFI in-tree, so we should probably just use that?
Unresolved is whether the linker flags
/entry:efi_main /subsystem:efi_applicationare still needed, or if such details are handled by LLVM/LLD nowadays automatically?Note that I am only opening this because it causes issues for
ccwhen trying to pass that target name to LLVM, I have no idea if this is actually correct, and it's completely untested!CC target maintainers @dvdhrm @nicholasbishop
@rustbot label O-uefi