llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-codegen Author: Reid Kleckner (rnk) <details> <summary>Changes</summary> Fixes #<!-- -->220712 Scoped enum values don't undergo default integer promotions, so they don't naturally get extended to 32-bit integers. However, MSVC appears to extend scoped enum values, *specifically* when they appear as variadic arguments, but *not* when the are passed as fixed arguments: https://godbolt.org/z/oGozjMnEn I think we should do the same. I think the main upshot here is that users will be able to `printf("%d\n", my_u8_enum);` without casting to `int` explicitly. Even if the `va_arg(ap, int)` on the printf side is *technically* UB, this seems like a sharp corner we really ought to sand off. I dug up some references for how we handle this on the SysV side, and it seems that we do extend there: a71cc1536167f44f542da2857685f01aa29c0e55 Codex decided it was worth plumbing the `IsNamedArg` boolean through the ARM64EC classification side channel for good measure. Assisted-by: Codex --- Full diff: https://github.com/llvm/llvm-project/pull/221356.diff 5 Files Affected: - (modified) clang/lib/CodeGen/ABIInfo.cpp (+2-1) - (modified) clang/lib/CodeGen/ABIInfo.h (+2-1) - (modified) clang/lib/CodeGen/Targets/AArch64.cpp (+2-1) - (modified) clang/lib/CodeGen/Targets/X86.cpp (+34-13) - (added) clang/test/CodeGenCXX/windows-x86_64-varargs.cpp (+36) ``````````diff diff --git a/clang/lib/CodeGen/ABIInfo.cpp b/clang/lib/CodeGen/ABIInfo.cpp index eede3b54d7b4a..7ce11575ff1e6 100644 --- a/clang/lib/CodeGen/ABIInfo.cpp +++ b/clang/lib/CodeGen/ABIInfo.cpp @@ -257,7 +257,8 @@ void ABIInfo::createCoercedStore(llvm::Value *Val, Address DstAddr, const ABIArgInfo &AI, bool DestIsVolatile, CodeGenFunction &CGF) const {} -ABIArgInfo ABIInfo::classifyArgForArm64ECVarArg(QualType Ty) const { +ABIArgInfo ABIInfo::classifyArgForArm64ECVarArg(QualType Ty, + bool IsNamedArg) const { llvm_unreachable("Only implemented for x86"); } diff --git a/clang/lib/CodeGen/ABIInfo.h b/clang/lib/CodeGen/ABIInfo.h index b9a970919740f..58c502dc9fac2 100644 --- a/clang/lib/CodeGen/ABIInfo.h +++ b/clang/lib/CodeGen/ABIInfo.h @@ -156,7 +156,8 @@ class ABIInfo { /// Used by Arm64EC calling convention code to call into x86 calling /// convention code for varargs function. - virtual ABIArgInfo classifyArgForArm64ECVarArg(QualType Ty) const; + virtual ABIArgInfo classifyArgForArm64ECVarArg(QualType Ty, + bool IsNamedArg) const; }; /// Target specific hooks for defining how a type should be passed or returned diff --git a/clang/lib/CodeGen/Targets/AArch64.cpp b/clang/lib/CodeGen/Targets/AArch64.cpp index 2d73dd8cc2916..23a4e5c0ce9ec 100644 --- a/clang/lib/CodeGen/Targets/AArch64.cpp +++ b/clang/lib/CodeGen/Targets/AArch64.cpp @@ -378,7 +378,8 @@ ABIArgInfo AArch64ABIInfo::classifyArgumentType(QualType Ty, bool IsVariadicFn, if (IsVariadicFn && getTarget().getTriple().isWindowsArm64EC()) { // Arm64EC varargs functions use the x86_64 classification rules, // not the AArch64 ABI rules. - return WinX86_64CodegenInfo->getABIInfo().classifyArgForArm64ECVarArg(Ty); + return WinX86_64CodegenInfo->getABIInfo().classifyArgForArm64ECVarArg( + Ty, IsNamedArg); } // Handle illegal vector types here. diff --git a/clang/lib/CodeGen/Targets/X86.cpp b/clang/lib/CodeGen/Targets/X86.cpp index d60d71775a9d1..90028b64f8efa 100644 --- a/clang/lib/CodeGen/Targets/X86.cpp +++ b/clang/lib/CodeGen/Targets/X86.cpp @@ -1426,14 +1426,18 @@ class WinX86_64ABIInfo : public ABIInfo { return isX86VectorCallAggregateSmallEnough(NumMembers); } - ABIArgInfo classifyArgForArm64ECVarArg(QualType Ty) const override { + ABIArgInfo classifyArgForArm64ECVarArg(QualType Ty, + bool IsNamedArg) const override { unsigned FreeSSERegs = 0; - return classify(Ty, FreeSSERegs, /*IsReturnType=*/false, - llvm::CallingConv::C); + ClassifyKind Kind = + IsNamedArg ? ClassifyKind::FixedArgument : ClassifyKind::VarArg; + return classify(Ty, FreeSSERegs, Kind, llvm::CallingConv::C); } private: - ABIArgInfo classify(QualType Ty, unsigned &FreeSSERegs, bool IsReturnType, + enum class ClassifyKind { Return, FixedArgument, VarArg }; + + ABIArgInfo classify(QualType Ty, unsigned &FreeSSERegs, ClassifyKind Kind, unsigned CC) const; ABIArgInfo reclassifyHvaArgForVectorCall(QualType Ty, unsigned &FreeSSERegs, const ABIArgInfo ¤t) const; @@ -3452,15 +3456,26 @@ ABIArgInfo WinX86_64ABIInfo::reclassifyHvaArgForVectorCall( } ABIArgInfo WinX86_64ABIInfo::classify(QualType Ty, unsigned &FreeSSERegs, - bool IsReturnType, unsigned CC) const { + ClassifyKind Kind, unsigned CC) const { bool IsVectorCall = CC == llvm::CallingConv::X86_VectorCall; bool IsRegCall = CC == llvm::CallingConv::X86_RegCall; if (Ty->isVoidType()) return ABIArgInfo::getIgnore(); - if (const auto *ED = Ty->getAsEnumDecl()) + bool PromoteScopedEnum = false; + if (const auto *ED = Ty->getAsEnumDecl()) { Ty = ED->getIntegerType(); + PromoteScopedEnum = Kind == ClassifyKind::VarArg && ED->isScoped() && + getContext().isPromotableIntegerType(Ty); + } + + // MSVC extends scoped enums with a sub-int underlying type when they are + // passed through an ellipsis. Unlike unscoped enums, scoped enums are not + // subject to the language's default argument promotions, so handle the + // extension as part of the ABI classification. + if (PromoteScopedEnum) + return ABIArgInfo::getExtend(Ty); TypeInfo Info = getContext().getTypeInfo(Ty); uint64_t Width = Info.Width; @@ -3468,7 +3483,7 @@ ABIArgInfo WinX86_64ABIInfo::classify(QualType Ty, unsigned &FreeSSERegs, const RecordType *RT = Ty->getAsCanonical<RecordType>(); if (RT) { - if (!IsReturnType) { + if (Kind != ClassifyKind::Return) { if (CGCXXABI::RecordArgABI RAA = getRecordArgABI(RT, getCXXABI())) return getNaturalAlignIndirect(Ty, getDataLayout().getAllocaAddrSpace(), RAA == CGCXXABI::RAA_DirectInMemory); @@ -3488,7 +3503,8 @@ ABIArgInfo WinX86_64ABIInfo::classify(QualType Ty, unsigned &FreeSSERegs, if (IsRegCall) { if (FreeSSERegs >= NumElts) { FreeSSERegs -= NumElts; - if (IsReturnType || Ty->isBuiltinType() || Ty->isVectorType()) + if (Kind == ClassifyKind::Return || Ty->isBuiltinType() || + Ty->isVectorType()) return ABIArgInfo::getDirect(); return ABIArgInfo::getExpand(); } @@ -3497,10 +3513,11 @@ ABIArgInfo WinX86_64ABIInfo::classify(QualType Ty, unsigned &FreeSSERegs, /*ByVal=*/false); } else if (IsVectorCall) { if (FreeSSERegs >= NumElts && - (IsReturnType || Ty->isBuiltinType() || Ty->isVectorType())) { + (Kind == ClassifyKind::Return || Ty->isBuiltinType() || + Ty->isVectorType())) { FreeSSERegs -= NumElts; return ABIArgInfo::getDirect(); - } else if (IsReturnType) { + } else if (Kind == ClassifyKind::Return) { return ABIArgInfo::getExpand(); } else if (!Ty->isBuiltinType() && !Ty->isVectorType()) { // HVAs are delayed and reclassified in the 2nd step. @@ -3555,7 +3572,7 @@ ABIArgInfo WinX86_64ABIInfo::classify(QualType Ty, unsigned &FreeSSERegs, // If it's a parameter type, the normal ABI rule is that arguments larger // than 8 bytes are passed indirectly. GCC follows it. We follow it too, // even though it isn't particularly efficient. - if (!IsReturnType) + if (Kind != ClassifyKind::Return) return ABIArgInfo::getIndirect( Align, /*AddrSpace=*/getDataLayout().getAllocaAddrSpace(), /*ByVal=*/false); @@ -3641,7 +3658,8 @@ void WinX86_64ABIInfo::computeInfo(CGFunctionInfo &FI) const { } if (!getCXXABI().classifyReturnType(FI)) - FI.getReturnInfo() = classify(FI.getReturnType(), FreeSSERegs, true, CC); + FI.getReturnInfo() = + classify(FI.getReturnType(), FreeSSERegs, ClassifyKind::Return, CC); if (IsVectorCall) { // We can use up to 6 SSE register parameters with vectorcall. @@ -3659,7 +3677,10 @@ void WinX86_64ABIInfo::computeInfo(CGFunctionInfo &FI) const { // registers are left. unsigned *MaybeFreeSSERegs = (IsVectorCall && ArgNum >= 6) ? &ZeroSSERegs : &FreeSSERegs; - I.info = classify(I.type, *MaybeFreeSSERegs, false, CC); + ClassifyKind Kind = ArgNum >= FI.getNumRequiredArgs() + ? ClassifyKind::VarArg + : ClassifyKind::FixedArgument; + I.info = classify(I.type, *MaybeFreeSSERegs, Kind, CC); ++ArgNum; } diff --git a/clang/test/CodeGenCXX/windows-x86_64-varargs.cpp b/clang/test/CodeGenCXX/windows-x86_64-varargs.cpp new file mode 100644 index 0000000000000..21637aac873b9 --- /dev/null +++ b/clang/test/CodeGenCXX/windows-x86_64-varargs.cpp @@ -0,0 +1,36 @@ +// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -std=c++11 -emit-llvm \ +// RUN: -o - %s | FileCheck %s +// RUN: %clang_cc1 -triple arm64ec-pc-windows-msvc -std=c++11 -emit-llvm \ +// RUN: -o - %s | FileCheck %s + +enum class U8 : unsigned char {}; +enum class S8 : signed char {}; +enum class U16 : unsigned short {}; +enum class S16 : short {}; +enum class U32 : unsigned int {}; +enum ClassicU8 : unsigned char {}; + +extern "C" void variadic(int, ...); + +// Fixed parameters should continue to be passed without extension. +// CHECK-LABEL: define dso_local void @fixed(i8 noundef %{{[^)]+}}) +extern "C" void fixed(U8) {} + +// Named parameters of variadic functions are still fixed arguments. +// CHECK-LABEL: define dso_local void @named_variadic(i8 noundef %{{[^,]+}}, ...) +extern "C" void named_variadic(U8, ...) {} + +// CHECK-LABEL: define dso_local void @test( +// CHECK: call void (i32, ...) @variadic( +// CHECK-SAME: i32 noundef 0, +// Scoped enums do not undergo the default argument integer promotions, so they +// retain their i8/i16 IR types and use extension attributes for ABI widening. +// CHECK-SAME: i8 noundef zeroext %{{[^,]+}}, i8 noundef signext %{{[^,]+}}, +// CHECK-SAME: i16 noundef zeroext %{{[^,]+}}, i16 noundef signext %{{[^,]+}}, +// Regular integer types and unscoped enums do undergo integer promotion (for +// example, unsigned char to int), so those arguments are passed as i32. +// CHECK-SAME: i32 noundef %{{[^,]+}}, i32 noundef %{{[^,]+}}, i32 noundef %{{[^)]+}}) +extern "C" void test(U8 u8, S8 s8, U16 u16, S16 s16, U32 u32, + ClassicU8 classic_u8, unsigned char plain_u8) { + variadic(0, u8, s8, u16, s16, u32, classic_u8, plain_u8); +} `````````` </details> https://github.com/llvm/llvm-project/pull/221356 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
