Skip to content

[clang] UEFI ABI fixes for X86_64 #124992

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

Closed
wants to merge 1 commit into from
Closed

[clang] UEFI ABI fixes for X86_64 #124992

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 clang/lib/AST/Mangle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ static CCMangling getCallingConvMangling(const ASTContext &Context,
if (FD->isMain() && FD->getNumParams() == 2)
return CCM_WasmMainArgcArgv;

if (!Triple.isOSWindows() || !Triple.isX86())
if (!Triple.isOSWindowsOrUEFI() || !Triple.isX86())
Copy link
Contributor

@frobtech frobtech Jan 30, 2025

Choose a reason for hiding this comment

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

As we discussed offline, I'd really rather see all sites like this use a predicate that's specific to what matters to the call site in precise terms. This predicate is really an answer to the question rather than the question that should be asked. It presumes the answer is "Windows or UEFI targets", when the question it should be asking is something more like, "What C++ ABI name mangling regime is in use by this target?"

Copy link
Collaborator

@rnk rnk Feb 4, 2025

Choose a reason for hiding this comment

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

To make things more complicated, consider mingw. I am totally unfamiliar to UEFI, but it seems like folks want it to inherit lots of MSVC ABI features: the mangling scheme, the vftable ABI rules, the struct layout, etc. These rules are usually conditional on isWindowsMSVCEnvironment(), so you'd need to audit those conditions.

While explicit triple checks are convenient and necessary in many cases, we generally try to abstract these details with the various CXXABI interfaces, and if you can find a way to do that, it may help with readability.

return CCM_Other;

if (Context.getLangOpts().CPlusPlus && !isExternC(ND) &&
Expand Down
11 changes: 11 additions & 0 deletions clang/lib/Basic/Targets/X86.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,17 @@ class LLVM_LIBRARY_VISIBILITY UEFIX86_64TargetInfo
public:
UEFIX86_64TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
: UEFITargetInfo<X86_64TargetInfo>(Triple, Opts) {
LongWidth = LongAlign = 32;
DoubleAlign = LongLongAlign = 64;
IntMaxType = SignedLongLong;
Int64Type = SignedLongLong;
SizeType = UnsignedLongLong;
PtrDiffType = SignedLongLong;
IntPtrType = SignedLongLong;
LongDoubleWidth = LongDoubleAlign = 64;
LongDoubleFormat = &llvm::APFloat::IEEEdouble();
WCharType = UnsignedShort;
WIntType = UnsignedShort;
this->TheCXXABI.set(TargetCXXABI::Microsoft);
this->MaxTLSAlign = 8192u * this->getCharWidth();
this->resetDataLayout("e-m:w-p270:32:32-p271:32:32-p272:64:64-"
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Driver/ToolChains/Arch/X86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ void x86::getX86TargetFeatures(const Driver &D, const llvm::Triple &Triple,
// Claim and report unsupported -mabi=. Note: we don't support "sysv_abi" or
// "ms_abi" as default function attributes.
if (const Arg *A = Args.getLastArg(clang::driver::options::OPT_mabi_EQ)) {
StringRef DefaultAbi = Triple.isOSWindows() ? "ms" : "sysv";
StringRef DefaultAbi = Triple.isOSWindowsOrUEFI() ? "ms" : "sysv";
if (A->getValue() != DefaultAbi)
D.Diag(diag::err_drv_unsupported_opt_for_target)
<< A->getSpelling() << Triple.getTriple();
Expand Down
10 changes: 5 additions & 5 deletions clang/lib/Sema/SemaChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4751,27 +4751,27 @@ static bool checkVAStartABI(Sema &S, unsigned BuiltinID, Expr *Fn) {
bool IsX64 = TT.getArch() == llvm::Triple::x86_64;
bool IsAArch64 = (TT.getArch() == llvm::Triple::aarch64 ||
TT.getArch() == llvm::Triple::aarch64_32);
bool IsWindows = TT.isOSWindows();
bool IsWindowsOrUEFI = TT.isOSWindowsOrUEFI();
bool IsMSVAStart = BuiltinID == Builtin::BI__builtin_ms_va_start;
if (IsX64 || IsAArch64) {
CallingConv CC = CC_C;
if (const FunctionDecl *FD = S.getCurFunctionDecl())
CC = FD->getType()->castAs<FunctionType>()->getCallConv();
if (IsMSVAStart) {
// Don't allow this in System V ABI functions.
if (CC == CC_X86_64SysV || (!IsWindows && CC != CC_Win64))
if (CC == CC_X86_64SysV || (!IsWindowsOrUEFI && CC != CC_Win64))
return S.Diag(Fn->getBeginLoc(),
diag::err_ms_va_start_used_in_sysv_function);
} else {
// On x86-64/AArch64 Unix, don't allow this in Win64 ABI functions.
// On x64 Windows, don't allow this in System V ABI functions.
// (Yes, that means there's no corresponding way to support variadic
// System V ABI functions on Windows.)
if ((IsWindows && CC == CC_X86_64SysV) ||
(!IsWindows && CC == CC_Win64))
if ((IsWindowsOrUEFI && CC == CC_X86_64SysV) ||
(!IsWindowsOrUEFI && CC == CC_Win64))
return S.Diag(Fn->getBeginLoc(),
diag::err_va_start_used_in_wrong_abi_function)
<< !IsWindows;
<< !IsWindowsOrUEFI;
}
return false;
}
Expand Down
8 changes: 4 additions & 4 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5193,12 +5193,12 @@ bool Sema::CheckCallingConvAttr(const ParsedAttr &Attrs, CallingConv &CC,
CC = CC_X86RegCall;
break;
case ParsedAttr::AT_MSABI:
CC = Context.getTargetInfo().getTriple().isOSWindows() ? CC_C :
CC_Win64;
CC = Context.getTargetInfo().getTriple().isOSWindowsOrUEFI() ? CC_C
Copy link
Contributor

@frobtech frobtech Jan 30, 2025

Choose a reason for hiding this comment

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

This really highlights how getTargetInfo().getCallingConv() is what we ought to have. For now it can be defined just this way, but in future it might be controlled by switches within a given target like UEFI. But right now, it makes code like this and checkVAStartABI much clearer.

Copy link
Collaborator

@rnk rnk Feb 4, 2025

Choose a reason for hiding this comment

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

I'm not sure your comment makes sense in the context of the __attribute__((msabi)) implementation. I would interpret your suggestion instead as something like getTargetInfo().getCcForMsAbiAttribute(), and the target would return the appropriate convention.

I think in general this will only matter on Win / non-Win x64 platforms where we have divergence between the Win/Posix worlds, while the are aligned in the ARM world.

: CC_Win64;
break;
case ParsedAttr::AT_SysVABI:
CC = Context.getTargetInfo().getTriple().isOSWindows() ? CC_X86_64SysV :
CC_C;
CC = Context.getTargetInfo().getTriple().isOSWindowsOrUEFI() ? CC_X86_64SysV
: CC_C;
break;
case ParsedAttr::AT_Pcs: {
StringRef StrRef;
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/SemaExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17980,7 +17980,7 @@ static bool funcHasParameterSizeMangling(Sema &S, FunctionDecl *FD) {
// These manglings don't do anything on non-Windows or non-x86 platforms, so
// we don't need parameter type sizes.
const llvm::Triple &TT = S.Context.getTargetInfo().getTriple();
if (!TT.isOSWindows() || !TT.isX86())
if (!TT.isOSWindowsOrUEFI() || !TT.isX86())
return false;

// If this is C++ and this isn't an extern "C" function, parameters do not
Expand Down