[clang] fix uefi target for aarch64 & x86_64 #120632
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Tristan Ross (RossComputerGuy) ChangesThis PR introduces some fixes where the data layout wasn't propagated correctly. To test this, feed clang any C file (I used an empty main for testing) and pass Before this PR, you would get an error similar to this: This PR also adds a new Full diff: https://github.com/llvm/llvm-project/pull/120632.diff 3 Files Affected:
diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index 53e102bbe44687..d8aef4837f9267 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -1537,6 +1537,10 @@ 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");
diff --git a/clang/lib/Basic/Targets/OSTargets.h b/clang/lib/Basic/Targets/OSTargets.h
index cd9b3760ca5874..53dd23c3129636 100644
--- a/clang/lib/Basic/Targets/OSTargets.h
+++ b/clang/lib/Basic/Targets/OSTargets.h
@@ -790,7 +790,9 @@ template <typename Target>
class LLVM_LIBRARY_VISIBILITY UEFITargetInfo : public OSTargetInfo<Target> {
protected:
void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
- MacroBuilder &Builder) const override {}
+ MacroBuilder &Builder) const override {
+ Builder.defineMacro("__UEFI__");
+ }
public:
UEFITargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
diff --git a/clang/lib/Basic/Targets/X86.h b/clang/lib/Basic/Targets/X86.h
index 3ed36c8fa724b5..bb72a92b1cf5b7 100644
--- a/clang/lib/Basic/Targets/X86.h
+++ b/clang/lib/Basic/Targets/X86.h
@@ -714,7 +714,7 @@ class LLVM_LIBRARY_VISIBILITY X86_64TargetInfo : public X86TargetInfo {
: X86TargetInfo(Triple, Opts) {
const bool IsX32 = getTriple().isX32();
bool IsWinCOFF =
- getTriple().isOSWindows() && getTriple().isOSBinFormatCOFF();
+ (getTriple().isOSWindows() || getTriple().isUEFI()) && getTriple().isOSBinFormatCOFF();
LongWidth = LongAlign = PointerWidth = PointerAlign = IsX32 ? 32 : 64;
LongDoubleWidth = 128;
LongDoubleAlign = 128;
|
|
@llvm/pr-subscribers-backend-aarch64 Author: Tristan Ross (RossComputerGuy) ChangesThis PR introduces some fixes where the data layout wasn't propagated correctly. To test this, feed clang any C file (I used an empty main for testing) and pass Before this PR, you would get an error similar to this: This PR also adds a new Full diff: https://github.com/llvm/llvm-project/pull/120632.diff 3 Files Affected:
diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index 53e102bbe44687..d8aef4837f9267 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -1537,6 +1537,10 @@ 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");
diff --git a/clang/lib/Basic/Targets/OSTargets.h b/clang/lib/Basic/Targets/OSTargets.h
index cd9b3760ca5874..53dd23c3129636 100644
--- a/clang/lib/Basic/Targets/OSTargets.h
+++ b/clang/lib/Basic/Targets/OSTargets.h
@@ -790,7 +790,9 @@ template <typename Target>
class LLVM_LIBRARY_VISIBILITY UEFITargetInfo : public OSTargetInfo<Target> {
protected:
void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
- MacroBuilder &Builder) const override {}
+ MacroBuilder &Builder) const override {
+ Builder.defineMacro("__UEFI__");
+ }
public:
UEFITargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
diff --git a/clang/lib/Basic/Targets/X86.h b/clang/lib/Basic/Targets/X86.h
index 3ed36c8fa724b5..bb72a92b1cf5b7 100644
--- a/clang/lib/Basic/Targets/X86.h
+++ b/clang/lib/Basic/Targets/X86.h
@@ -714,7 +714,7 @@ class LLVM_LIBRARY_VISIBILITY X86_64TargetInfo : public X86TargetInfo {
: X86TargetInfo(Triple, Opts) {
const bool IsX32 = getTriple().isX32();
bool IsWinCOFF =
- getTriple().isOSWindows() && getTriple().isOSBinFormatCOFF();
+ (getTriple().isOSWindows() || getTriple().isUEFI()) && getTriple().isOSBinFormatCOFF();
LongWidth = LongAlign = PointerWidth = PointerAlign = IsX32 ? 32 : 64;
LongDoubleWidth = 128;
LongDoubleAlign = 128;
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
RossComputerGuy
force-pushed
the
feat/clang-compiler-rt-uefi
branch
from
8f5ef58 to
7b210e9
Compare
clang/lib/Basic/Targets/OSTargets.h
Outdated
| @@ -790,7 +790,9 @@ template <typename Target> | |||
| class LLVM_LIBRARY_VISIBILITY UEFITargetInfo : public OSTargetInfo<Target> { | |||
| protected: | |||
| void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple, | |||
| MacroBuilder &Builder) const override {} | |||
| MacroBuilder &Builder) const override { | |||
| Builder.defineMacro("__UEFI__"); | |||
There was a problem hiding this comment.
I wonder if this should be consistent with the Unix flavors, I don't know what the UEFI target does or if GCC or another other compiler already has a macro for this.
| Builder.defineMacro("__UEFI__"); | |
| DefineStd(Builder, "uefi", Opts); |
There was a problem hiding this comment.
Probably, although UEFI spec-wise is similar to Windows afaict. I'm not sure which style we'd want. GCC does not support UEFI targets and prefer to do a Windows style toolchain.
There was a problem hiding this comment.
Personally I prefer DefineStd for this area where possible or we know we're not adding conflicting macros. What do other toolchains produce here?
There was a problem hiding this comment.
I'm not familiar with any other toolchains supporting UEFI targets. LLVM would be the first I think. I know zig has some UEFI support but it's based on Windows stuff and they have their own stdlib for the language. But as far as C goes, LLVM would be the first.
There was a problem hiding this comment.
I've switched over to DefineStd.
There was a problem hiding this comment.
DefineStd will define uefi (in the GNU mode), __uefi and __uefi__. I don't think we should be defining any new symbols in the user's namespace, even in the GNU mode. I also don't think we should have more then one define, we do it in other targets for backwards compatibility, but there is no backwards compatibility for UEFI. Regarding upper case vs lower case, I have mild preference for the upper case since it's more consistent with Windows and UEFI is essentially a flavor of Windows, not UNIX or GNU. TL;DR I'd prefer using Builder.defineMacro("__UEFI__"); as you had originally.
|
Also needs a test |
I think so, it wouldn't be a bad idea.
I might need some guidance on adding a test since I'm not fully familiar with the clang side of LLVM. |
Check |
RossComputerGuy
force-pushed
the
feat/clang-compiler-rt-uefi
branch
from
7b210e9 to
71eb5ea
Compare
clang/lib/Basic/Targets.cpp
Outdated
| @@ -165,6 +165,9 @@ 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<UEFIAArch64TargetInfo>(Triple, Opts); | |||
There was a problem hiding this comment.
I'm going to just guess that the original patch made a dedicated X64 UEFI target because there weren't any other users. That's not true anymore so we should probably do what the other targets do and take a mixin for the underlying architecture.
There was a problem hiding this comment.
I'm not sure how we should make a mixin. Also, from what I can find, the call convention and some of the data layouts are different between x86 and other UEFI capable ISA's. We could add in checks, the solution I came up with is simple enough lol. But I do agree a mixin would probably be better.
There was a problem hiding this comment.
I mean you should just be able to do like if (Target.Triple.isX86()) right? I think that's how the others do it.
There was a problem hiding this comment.
I think so or it might be better to leave the data layout to the architecture side. The call convention is simple enough that it could be in the mixin.
There was a problem hiding this comment.
I've moved it over to a mixin. I'm not sure how to test if the call convention and stuff is working as expected.
There was a problem hiding this comment.
If it doesn't break any existing tests it either wasn't tested enough or it's fine.
RossComputerGuy
force-pushed
the
feat/clang-compiler-rt-uefi
branch
2 times, most recently
from
af39133 to
2e3b764
Compare
clang/lib/Basic/Targets/X86.h
Outdated
|
|
||
| void getTargetDefines(const LangOptions &Opts, | ||
| MacroBuilder &Builder) const override { | ||
| getOSDefines(Opts, X86TargetInfo::getTriple(), Builder); |
There was a problem hiding this comment.
I forgot about that.
There was a problem hiding this comment.
Hmm, it looks like it might be using the default for OSTargetInfo which should do fine?
RossComputerGuy
force-pushed
the
feat/clang-compiler-rt-uefi
branch
from
2e3b764 to
881813d
Compare
|
Tests have been adjusted, they should pass now. |
RossComputerGuy
force-pushed
the
feat/clang-compiler-rt-uefi
branch
from
881813d to
06becc1
Compare
|
Clang driver unit tests have been fixed. |
clang/lib/Basic/Targets/OSTargets.h
Outdated
| if (Triple.getArch() == llvm::Triple::x86_64) { | ||
| return TargetInfo::CallingConvKind::CCK_MicrosoftWin64; | ||
| } |
There was a problem hiding this comment.
nit. style
| if (Triple.getArch() == llvm::Triple::x86_64) { | |
| return TargetInfo::CallingConvKind::CCK_MicrosoftWin64; | |
| } | |
| if (Triple.getArch() == llvm::Triple::x86_64) | |
| return TargetInfo::CallingConvKind::CCK_MicrosoftWin64; |
There was a problem hiding this comment.
Fixed + clang-format check should be happy again.
| public: | ||
| UEFIX86_64TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts) | ||
| : UEFITargetInfo<X86_64TargetInfo>(Triple, Opts) { | ||
| this->TheCXXABI.set(TargetCXXABI::Microsoft); |
There was a problem hiding this comment.
Guessing we don't need this or the CC check code below?
There was a problem hiding this comment.
Well, verify that the behavior before and after this is the same to be on the safe side.
There was a problem hiding this comment.
I'm not sure what the behavior even is. Looking at the UEFI spec, it doesn't mention C++ ABI stuff.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, I'll add a this->TheCXXABI.set(TargetCXXABI::Microsoft) to the UEFITargetInfo template class.
There was a problem hiding this comment.
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.
RossComputerGuy
force-pushed
the
feat/clang-compiler-rt-uefi
branch
from
06becc1 to
81cd003
Compare
clang/lib/Basic/Targets/OSTargets.h
Outdated
| protected: | ||
| void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple, | ||
| MacroBuilder &Builder) const override {} | ||
| MacroBuilder &Builder) const override { | ||
| DefineStd(Builder, "uefi", Opts); |
There was a problem hiding this comment.
I prefer having small PRs. I have a PR out to add a UEFI predefine here: #111719
For some reason I couldn't add you as a reviewer to the PR.
There was a problem hiding this comment.
I prefer how it's done here, with DefineStd. We could conceivably split these into separate patches, one to merge the x64 UEFI target back into a single one like all the others, one to add this defineStd and TargetOS defines, then one to fix the data layout. I don't really care that much and would prefer just to land this however.
There was a problem hiding this comment.
Yeah, I like making medium sized PR's typically with few small commits. It's much easier to review or look at specific components.
There was a problem hiding this comment.
I have approvals to land #111719 and #111473 which breaks out some aspects of this large PR. I am happy to either land them first or wait for all those changes to be part of this PR. @RossComputerGuy I am happy to go with whichever way makes landing this patch faster. Let me know.
There was a problem hiding this comment.
We can probably get the cleanup PR first then this one. Kinda on the fence with the define one.
There was a problem hiding this comment.
Merged the clean up PR. @petrhosek's thoughts on DefineMacro over DefineStd is sound in my opinion. Going to merge the predefine PR as well #111719 which we can revert/reland if we have significant push back and deal with it in a way that it doesn't block this PR.
RossComputerGuy
force-pushed
the
feat/clang-compiler-rt-uefi
branch
from
81cd003 to
b41e7a8
Compare
|
Will rebase tonight since #111719 was just merged. If we can get this merged soon then I can try more testing. |
RossComputerGuy
force-pushed
the
feat/clang-compiler-rt-uefi
branch
from
b41e7a8 to
1a29a74
Compare
|
This has been rebased now, conflicts are gone. |
| @@ -713,8 +713,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()) && | |||
There was a problem hiding this comment.
Nit: This could simply be
getTriple().isUEFI() || (getTriple().isOSWindows() && getTriple().isOSBinFormatCOFF()) as isOSBinFormatCOFF will always be true for UEFI.
| bool IsWinCOFF = | ||
| getTriple().isOSWindows() && getTriple().isOSBinFormatCOFF(); | ||
| bool IsWinCOFF = (getTriple().isOSWindows() || getTriple().isUEFI()) && | ||
| getTriple().isOSBinFormatCOFF(); | ||
| LongWidth = LongAlign = PointerWidth = PointerAlign = IsX32 ? 32 : 64; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Maybe, doesn't sound right that the various types and stuff aren't right for x86.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@frobtech Yeah, that's kinda why I am unsure of the changes suggested.
There was a problem hiding this comment.
@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.
RossComputerGuy
force-pushed
the
feat/clang-compiler-rt-uefi
branch
from
49e8cec to
3177fc8
Compare
This PR introduces some fixes where the data layout wasn't propagated correctly. To test this, feed clang any C file (I used an empty main for testing) and pass
-target aarch64-uefior-target x86_64-uefi.Before this PR, you would get an error similar to this:
This PR also adds a new
__uefi__macro which will hopefully let us work towards getting compiler-rt and libc working with UEFI.