Skip to content

[clang] fix uefi target for aarch64 & x86_64 #120632

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[clang] fix uefi target for aarch64 & x86_64 #120632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e0d525f
[clang] fix uefi data layout on x86 & aarch64
RossComputerGuy Dec 19, 2024
6d242b0
[clang] add __uefi__ test for aarch64 & x86_64
RossComputerGuy Dec 19, 2024
dfcd456
[clang] add uefi to target os macros
RossComputerGuy Dec 19, 2024
3177fc8
[clang] use mixin for aarch64 and x86_64 uefi
RossComputerGuy Dec 19, 2024
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
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/TargetOSMacros.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,7 @@ TARGET_OS(TARGET_OS_NANO, Triple.isWatchOS())
TARGET_OS(TARGET_IPHONE_SIMULATOR, Triple.isSimulatorEnvironment())
TARGET_OS(TARGET_OS_UIKITFORMAC, Triple.isMacCatalystEnvironment())

// UEFI target.
TARGET_OS(TARGET_OS_UEFI, Triple.isUEFI())

#undef TARGET_OS
6 changes: 5 additions & 1 deletion clang/lib/Basic/Targets.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,10 @@ std::unique_ptr<TargetInfo> AllocateTarget(const llvm::Triple &Triple,
case llvm::Triple::OpenBSD:
return std::make_unique<OpenBSDTargetInfo<AArch64leTargetInfo>>(Triple,
Opts);
case llvm::Triple::UEFI:
return std::make_unique<UEFITargetInfo<AArch64leTargetInfo>>(Triple,
Opts);

case llvm::Triple::Win32:
switch (Triple.getEnvironment()) {
case llvm::Triple::GNU:
Expand Down Expand Up @@ -631,7 +635,7 @@ std::unique_ptr<TargetInfo> AllocateTarget(const llvm::Triple &Triple,
return std::make_unique<SolarisTargetInfo<X86_64TargetInfo>>(Triple,
Opts);
case llvm::Triple::UEFI:
return std::make_unique<UEFIX86_64TargetInfo>(Triple, Opts);
return std::make_unique<UEFITargetInfo<X86_64TargetInfo>>(Triple, Opts);

case llvm::Triple::Win32: {
switch (Triple.getEnvironment()) {
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/Basic/Targets/AArch64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1626,6 +1626,9 @@ void AArch64leTargetInfo::setDataLayout() {
resetDataLayout("e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-"
"n32:64-S128-Fn32",
"_");
} else if (getTriple().isUEFI() && getTriple().isOSBinFormatCOFF()) {
resetDataLayout("e-m:w-p270:32:32-p271:32:32-p272:64:64-p:64:64-i32:32-"
"i64:64-i128:128-n32:64-S128-Fn32");
} else
resetDataLayout("e-m:e-p270:32:32-p271:32:32-p272:64:64-i8:8:32-i16:16:32-"
"i64:64-i128:128-n32:64-S128-Fn32");
Expand Down
13 changes: 12 additions & 1 deletion clang/lib/Basic/Targets/OSTargets.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#define LLVM_CLANG_LIB_BASIC_TARGETS_OSTARGETS_H

#include "Targets.h"
#include "clang/Basic/TargetInfo.h"

namespace clang {
namespace targets {
Expand Down Expand Up @@ -806,6 +807,8 @@ class LLVM_LIBRARY_VISIBILITY ZOSTargetInfo : public OSTargetInfo<Target> {
// UEFI target
template <typename Target>
class LLVM_LIBRARY_VISIBILITY UEFITargetInfo : public OSTargetInfo<Target> {
llvm::Triple Triple;

protected:
void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
MacroBuilder &Builder) const override {
Expand All @@ -814,11 +817,19 @@ class LLVM_LIBRARY_VISIBILITY UEFITargetInfo : public OSTargetInfo<Target> {

public:
UEFITargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
: OSTargetInfo<Target>(Triple, Opts) {
: OSTargetInfo<Target>(Triple, Opts), Triple(Triple) {
this->WCharType = TargetInfo::UnsignedShort;
this->WIntType = TargetInfo::UnsignedShort;
this->UseMicrosoftManglingForC = true;
}

TargetInfo::CallingConvKind
getCallingConvKind(bool ClangABICompat4) const override {
if (Triple.getArch() == llvm::Triple::x86_64)
return TargetInfo::CallingConvKind::CCK_MicrosoftWin64;

return Target::getCallingConvKind(ClangABICompat4);
}
};

void addWindowsDefines(const llvm::Triple &Triple, const LangOptions &Opts,
Expand Down
36 changes: 2 additions & 34 deletions clang/lib/Basic/Targets/X86.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,8 +722,8 @@ class LLVM_LIBRARY_VISIBILITY X86_64TargetInfo : public X86TargetInfo {
X86_64TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
: X86TargetInfo(Triple, Opts) {
const bool IsX32 = getTriple().isX32();
bool IsWinCOFF =
getTriple().isOSWindows() && getTriple().isOSBinFormatCOFF();
bool IsWinCOFF = (getTriple().isOSWindows() || getTriple().isUEFI()) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This could simply be

getTriple().isUEFI() || (getTriple().isOSWindows() && getTriple().isOSBinFormatCOFF()) as isOSBinFormatCOFF will always be true for UEFI.

getTriple().isOSBinFormatCOFF();
LongWidth = LongAlign = PointerWidth = PointerAlign = IsX32 ? 32 : 64;
Copy link
Contributor

Choose a reason for hiding this comment

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

While debugging some of our EFI workloads noticed that the integer type definitions set here are not correct for UEFI X86_64. I have a local patch which overrides the values set here to fix the problem. I am simplifying the code and pasting it here:

if(getTriple().isUEFI() && !IsX32) {
        LongWidth = LongAlign = 32;
        DoubleAlign = LongLongAlign = 64;
        IntMaxType = SignedLongLong;
        Int64Type = SignedLongLong;
        SizeType = UnsignedLongLong;
        PtrDiffType = SignedLongLong;
        IntPtrType = SignedLongLong;
        LongDoubleWidth = LongDoubleAlign = 64;
        LongDoubleFormat = &llvm::APFloat::IEEEdouble();
      }

I am starting lean towards retaining the UEFIX86_64TargetInfo to not "pollute" the X86_64TargetInfo with too many conditional overrides for UEFI. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, doesn't sound right that the various types and stuff aren't right for x86.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that some of these are things we'll eventually want command-line switch flexibility for. Matching Windows all around is the right default. But structurally, keep in mind that these are no things intrinsic to the UEFI target per se but to a particular set of C/C++ API expectations that future users (i.e. me) will want to have the option of configuring to be consistent with other norms rather than Windows norms.

Copy link
Member Author

Choose a reason for hiding this comment

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

@frobtech Yeah, that's kinda why I am unsure of the changes suggested.

Copy link
Contributor

Choose a reason for hiding this comment

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

@frobtech Yeah, that's kinda why I am unsure of the changes suggested.

Here AFAICT IsWinCOFF is being used only to choose the datalayout string. The only difference I see is m:w vs m:e, which is about assembler symbol naming details. That's about the rules and conventions for PE-COFF compilers, assemblers, and linkers. This one case I don't think is something where UEFI users would ever need an option to differ from Windows users. There's probably only one right way to interface with the linker.

It seems to be called IsWinCOFF mainly because the way it's been checked for is "is COFF and is Windows". I suspect that "is COFF" is true for other forms of COFF that aren't PE-COFF, but there isn't an isOSBinFormatPECOFF() that distinguishes, so this has been used as a proxy without explanation. For this case, IsPECOFF would be better name locally, anyway, to express what it is that matters to the check.

IMHO it would be wisest not to go around making lots of things write out ...isOsWindows() || ...isOsUEFI(). Instead there should be an .IsOSBinFormatPECOFF() or the like (probably written as just that same OR anyway), where it's clearly expressed in the name of the call what property it is that is the determinant for each use case.

Just in this same file there are other local variables called IsWinCOFF, presumably likewise named because they are set the same way rather than clearly expressing what they are really checking for. But that other use is do set MaxVectorAlign, which seems a lot like something that's really about runtime ABI subtleties, perhaps such as the expectations about stack and heap alignment. Something like that very well may more properly be for Windows (and I don't know why it's for Windows&&COFF instead of just for Windows, maybe there's a distinction), and not for "all PE-COFF" and thus not for UEFI.

LongDoubleWidth = 128;
LongDoubleAlign = 128;
Expand Down Expand Up @@ -829,38 +829,6 @@ class LLVM_LIBRARY_VISIBILITY X86_64TargetInfo : public X86TargetInfo {
}
};

// x86-64 UEFI target
class LLVM_LIBRARY_VISIBILITY UEFIX86_64TargetInfo
: public UEFITargetInfo<X86_64TargetInfo> {
public:
UEFIX86_64TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
: UEFITargetInfo<X86_64TargetInfo>(Triple, Opts) {
this->TheCXXABI.set(TargetCXXABI::Microsoft);
Copy link
Contributor

Choose a reason for hiding this comment

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

Guessing we don't need this or the CC check code below?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, verify that the behavior before and after this is the same to be on the safe side.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what the behavior even is. Looking at the UEFI spec, it doesn't mention C++ ABI stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Though I couldn't find it in the actual UEFI spec, Microsoft CXX ABI seems to be the norm for EFI. EDK2 toolchain if I am not mistaken assumes Microsoft ABI so is the existing target triples in Clang (e.g. X86_64-windows-msvc) also set the ABI to Microsoft.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll add a this->TheCXXABI.set(TargetCXXABI::Microsoft) to the UEFITargetInfo template class.

Copy link
Contributor

Choose a reason for hiding this comment

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

UEFI has nothing to say about C++. It is an ABI spec that is described in terms of C. It says nothing about what code not directly interacting with UEFI ABIs might do.

That said, heretofore the only way to compile code directly for the UEFI ABI is to use the Windows target. Thus, the expectations for existing source code that targets UEFI directly via Clang is to get all the ABI and API details of whatever language as the Windows target does them (except of course for the actual OS-specific aspects per se). So for all "ABI choice" things, the sensible default for UEFI target on $ARCH is what the Windows target for $ARCH does. This goes for what type long is, what C++ ABI to use, alignment quirks, etc. It also goes for language API details/extensions that are not directly related to Windows per se, such as the various pragmas for PECOFF-specific features or Windows spellings of generic extensions (e.g. -fms-extensions stuff).

Once we have the UEFI targets on their feet, we'll want to look into offering flexibility to make non-default choices (i.e. choices different from Windows) for various ABI and API things. But we can get to that later.

this->MaxTLSAlign = 8192u * this->getCharWidth();
this->resetDataLayout("e-m:w-p270:32:32-p271:32:32-p272:64:64-"
"i64:64-i128:128-f80:128-n8:16:32:64-S128");
}

BuiltinVaListKind getBuiltinVaListKind() const override {
return TargetInfo::CharPtrBuiltinVaList;
}

CallingConvCheckResult checkCallingConvention(CallingConv CC) const override {
switch (CC) {
case CC_C:
case CC_Win64:
return CCCR_OK;
default:
return CCCR_Warning;
}
}

TargetInfo::CallingConvKind
getCallingConvKind(bool ClangABICompat4) const override {
return CCK_MicrosoftWin64;
}
};

// x86-64 Windows target
class LLVM_LIBRARY_VISIBILITY WindowsX86_64TargetInfo
: public WindowsTargetInfo<X86_64TargetInfo> {
Expand Down
2 changes: 2 additions & 0 deletions clang/test/Preprocessor/init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2835,6 +2835,8 @@
// RISCV64-LINUX: #define unix 1

// RUN: %clang_cc1 -dM -triple=x86_64-uefi -E /dev/null | FileCheck -match-full-lines -check-prefix UEFI %s
// RUN: %clang_cc1 -dM -triple=aarch64-unknown-uefi -E < /dev/null | FileCheck -match-full-lines -check-prefix UEFI %s
// RUN: %clang_cc1 -dM -triple=x86_64-unknown-uefi -E /dev/null | FileCheck -match-full-lines -check-prefix UEFI %s

// UEFI: #define __UEFI__ 1

Expand Down