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.

Sign up for GitHub

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

Use LLVM's UEFI targets #132570

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+5 −60
Open

Use LLVM's UEFI targets #132570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_target/src/spec/targets/aarch64_unknown_uefi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub(crate) fn target() -> Target {
base.features = "+v8a".into();

Target {
llvm_target: "aarch64-unknown-windows".into(),
llvm_target: "aarch64-unknown-uefi".into(),
metadata: crate::spec::TargetMetadata {
description: Some("ARM64 UEFI".into()),
tier: Some(2),
Expand Down
58 changes: 2 additions & 56 deletions compiler/rustc_target/src/spec/targets/i686_unknown_uefi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// UEFI systems always run in protected-mode, have the interrupt-controller pre-configured and
// force a single-CPU execution.
// The cdecl ABI is used. It differs from the stdcall or fastcall ABI.
// "i686-unknown-windows" is used to get the minimal subset of windows-specific features.

use crate::spec::{Target, base};

Expand All @@ -23,61 +22,8 @@ pub(crate) fn target() -> Target {
// arguments, thus giving you access to full MMX/SSE acceleration.
base.features = "-mmx,-sse,+soft-float".into();

// Use -GNU here, because of the reason below:
// Background and Problem:
// If we use i686-unknown-windows, the LLVM IA32 MSVC generates compiler intrinsic
// _alldiv, _aulldiv, _allrem, _aullrem, _allmul, which will cause undefined symbol.
// A real issue is __aulldiv() is referred by __udivdi3() - udivmod_inner!(), from
// https://github.com/rust-lang-nursery/compiler-builtins.
// As result, rust-lld generates link error finally.
// Root-cause:
// In rust\src\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp,
// we have below code to use MSVC intrinsics. It assumes MSVC target
// will link MSVC library. But that is NOT true in UEFI environment.
// UEFI does not link any MSVC or GCC standard library.
// if (Subtarget.isTargetKnownWindowsMSVC() ||
// Subtarget.isTargetWindowsItanium()) {
// // Setup Windows compiler runtime calls.
// setLibcallName(RTLIB::SDIV_I64, "_alldiv");
// setLibcallName(RTLIB::UDIV_I64, "_aulldiv");
// setLibcallName(RTLIB::SREM_I64, "_allrem");
// setLibcallName(RTLIB::UREM_I64, "_aullrem");
// setLibcallName(RTLIB::MUL_I64, "_allmul");
// setLibcallCallingConv(RTLIB::SDIV_I64, CallingConv::X86_StdCall);
// setLibcallCallingConv(RTLIB::UDIV_I64, CallingConv::X86_StdCall);
// setLibcallCallingConv(RTLIB::SREM_I64, CallingConv::X86_StdCall);
// setLibcallCallingConv(RTLIB::UREM_I64, CallingConv::X86_StdCall);
// setLibcallCallingConv(RTLIB::MUL_I64, CallingConv::X86_StdCall);
// }
// The compiler intrinsics should be implemented by compiler-builtins.
// Unfortunately, compiler-builtins has not provided those intrinsics yet. Such as:
// i386/divdi3.S
// i386/lshrdi3.S
// i386/moddi3.S
// i386/muldi3.S
// i386/udivdi3.S
// i386/umoddi3.S
// Possible solution:
// 1. Eliminate Intrinsics generation.
// 1.1 Choose different target to bypass isTargetKnownWindowsMSVC().
// 1.2 Remove the "Setup Windows compiler runtime calls" in LLVM
// 2. Implement Intrinsics.
// We evaluated all options.
// #2 is hard because we need implement the intrinsics (_aulldiv) generated
// from the other intrinsics (__udivdi3) implementation with the same
// functionality (udivmod_inner). If we let _aulldiv() call udivmod_inner!(),
// then we are in loop. We may have to find another way to implement udivmod_inner!().
// #1.2 may break the existing usage.
// #1.1 seems the simplest solution today.
// The IA32 -gnu calling convention is same as the one defined in UEFI specification.
// It uses cdecl, EAX/ECX/EDX as volatile register, and EAX/EDX as return value.
// We also checked the LLVM X86TargetLowering, the differences between -gnu and -msvc
// is fmodf(f32), longjmp() and TLS. None of them impacts the UEFI code.
// As a result, we choose -gnu for i686 version before those intrinsics are implemented in
// compiler-builtins. After compiler-builtins implements all required intrinsics, we may
// remove -gnu and use the default one.
Target {
llvm_target: "i686-unknown-windows-gnu".into(),
llvm_target: "i686-unknown-uefi".into(),
metadata: crate::spec::TargetMetadata {
description: Some("32-bit UEFI".into()),
tier: Some(2),
Expand All @@ -86,7 +32,7 @@ pub(crate) fn target() -> Target {
},
pointer_width: 32,
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"
Comment on lines 34 to +35
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 of i64 to 32 bits.
  • Add f64:32:64: Decreases the alignment of f64 to 32 bits.
  • Remove a:0:32: Decreases the alignment of aggregates (structs?) to 32 bits.
  • Replace S32 with S128: Increases the natural alignment of the stack to 128 bits.

I have no idea if this is the desired behaviour?

Copy link
Contributor

@nicholasbishop nicholasbishop Nov 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

can we file a bug against LLVM, then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking from edk2 is definitely not desirable. From what I understand, even most of the proprietary UEFI implementations use edk2 as the base.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

CC: @frobtech @petrhosek

.into(),
arch: "x86".into(),

Expand Down
5 changes: 2 additions & 3 deletions compiler/rustc_target/src/spec/targets/x86_64_unknown_uefi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
// uefi-base module for generic UEFI options. On x86_64 systems (mostly called "x64" in the spec)
// UEFI systems always run in long-mode, have the interrupt-controller pre-configured and force a
// single-CPU execution.
// The win64 ABI is used. It differs from the sysv64 ABI, so we must use a windows target with
// LLVM. "x86_64-unknown-windows" is used to get the minimal subset of windows-specific features.
// The win64 ABI is used. It differs from the sysv64 ABI.

use crate::abi::call::Conv;
use crate::spec::{Target, base};
Expand All @@ -28,7 +27,7 @@ pub(crate) fn target() -> Target {
base.features = "-mmx,-sse,+soft-float".into();

Target {
llvm_target: "x86_64-unknown-windows".into(),
llvm_target: "x86_64-unknown-uefi".into(),
metadata: crate::spec::TargetMetadata {
description: Some("64-bit UEFI".into()),
tier: Some(2),
Expand Down
Loading