[MS] Follow up fix to pass aligned args to variadic x86_32 functions

[rnk]

MSVC allows users to pass structures with required alignments greater than 4 to variadic functions. It does not pass them indirectly to correctly align them. Instead, it passes them directly with the usual 4 byte stack alignment.

This change implements the same logic in clang on the passing side. The receiving side (va_arg) never implemented any of this indirect logic, so it doesn't need to be updated.

This issue pre-existed, but @aaron.ballman noticed it when we started passing structs containing aligned fields indirectly in D152752.

[AaronBallman]

I believe these changes are correct and they look to be reasonable, but adding the codegen code owners for final approval.

[RKSimon]

LGTM if the clang team are happy with us not raising the alignment error

[AaronBallman]

LGTM if the clang team are happy with us not raising the alignment error

Yup, that's what got us on this path in the first place. Our downstream at Intel started getting errors on code that was previously working, this restores the previous behavior in that case.

[AaronBallman]

Approving, but do wait a bit before landing in case a codegen code owner has comments.

[rjmccall]

Functionally LGTM; just a minor request.

[llvmbot]

@llvm/pr-subscribers-clang

Changes

MSVC allows users to pass structures with required alignments greater than 4 to variadic functions. It does not pass them indirectly to correctly align them. Instead, it passes them directly with the usual 4 byte stack alignment.

This change implements the same logic in clang on the passing side. The receiving side (va_arg) never implemented any of this indirect logic, so it doesn't need to be updated.

This issue pre-existed, but @aaron.ballman noticed it when we started passing structs containing aligned fields indirectly in D152752.

Full diff: https://github.com/llvm/llvm-project/pull/65692.diff

3 Files Affected:

• (modified) clang/include/clang/CodeGen/CGFunctionInfo.h (+5)
• (modified) clang/lib/CodeGen/Targets/X86.cpp (+15-11)
• (modified) clang/test/CodeGen/X86/x86_32-arguments-win32.c (+29)

diff --git a/clang/include/clang/CodeGen/CGFunctionInfo.h b/clang/include/clang/CodeGen/CGFunctionInfo.h
index b8971d5793f3618..e388901b8a504c5 100644
--- a/clang/include/clang/CodeGen/CGFunctionInfo.h
+++ b/clang/include/clang/CodeGen/CGFunctionInfo.h
@@ -527,6 +527,11 @@ class RequiredArgs {
     return NumRequired;
   }
 
+  /// Return true if the argument at a given index is required.
+  bool isRequiredArg(unsigned argIdx) const {
+    return argIdx == ~0U || argIdx < NumRequired;
+  }
+
   unsigned getOpaqueData() const { return NumRequired; }
   static RequiredArgs getFromOpaqueData(unsigned value) {
     if (value == ~0U) return All;
diff --git a/clang/lib/CodeGen/Targets/X86.cpp b/clang/lib/CodeGen/Targets/X86.cpp
index 2ea82397f11909e..b2e2f6789cce328 100644
--- a/clang/lib/CodeGen/Targets/X86.cpp
+++ b/clang/lib/CodeGen/Targets/X86.cpp
@@ -87,12 +87,15 @@ static ABIArgInfo getDirectX86Hva(llvm::Type* T = nullptr) {
 /// Similar to llvm::CCState, but for Clang.
 struct CCState {
   CCState(CGFunctionInfo &FI)
-      : IsPreassigned(FI.arg_size()), CC(FI.getCallingConvention()) {}
+      : IsPreassigned(FI.arg_size()), CC(FI.getCallingConvention()),
+	Required(FI.getRequiredArgs()), IsDelegateCall(FI.isDelegateCall()) {}
 
   llvm::SmallBitVector IsPreassigned;
   unsigned CC = CallingConv::CC_C;
   unsigned FreeRegs = 0;
   unsigned FreeSSERegs = 0;
+  RequiredArgs Required;
+  bool IsDelegateCall = false;
 };
 
 /// X86_32ABIInfo - The X86-32 ABI information.
@@ -141,7 +144,7 @@ class X86_32ABIInfo : public ABIInfo {
   Class classify(QualType Ty) const;
   ABIArgInfo classifyReturnType(QualType RetTy, CCState &State) const;
   ABIArgInfo classifyArgumentType(QualType RetTy, CCState &State,
-                                  bool isDelegateCall) const;
+                                  unsigned ArgIndex) const;
 
   /// Updates the number of available free registers, returns
   /// true if any registers were allocated.
@@ -739,7 +742,7 @@ void X86_32ABIInfo::runVectorCallFirstPass(CGFunctionInfo &FI, CCState &State) c
 }
 
 ABIArgInfo X86_32ABIInfo::classifyArgumentType(QualType Ty, CCState &State,
-                                               bool isDelegateCall) const {
+                                               unsigned ArgIndex) const {
   // FIXME: Set alignment on indirect arguments.
   bool IsFastCall = State.CC == llvm::CallingConv::X86_FastCall;
   bool IsRegCall = State.CC == llvm::CallingConv::X86_RegCall;
@@ -754,7 +757,7 @@ ABIArgInfo X86_32ABIInfo::classifyArgumentType(QualType Ty, CCState &State,
     CGCXXABI::RecordArgABI RAA = getRecordArgABI(RT, getCXXABI());
     if (RAA == CGCXXABI::RAA_Indirect) {
       return getIndirectResult(Ty, false, State);
-    } else if (isDelegateCall) {
+    } else if (State.IsDelegateCall) {
       // Avoid having different alignments on delegate call args by always
       // setting the alignment to 4, which is what we do for inallocas.
       ABIArgInfo Res = getIndirectResult(Ty, false, State);
@@ -812,11 +815,12 @@ ABIArgInfo X86_32ABIInfo::classifyArgumentType(QualType Ty, CCState &State,
     }
     llvm::IntegerType *PaddingType = NeedsPadding ? Int32 : nullptr;
 
-    // Pass over-aligned aggregates on Windows indirectly. This behavior was
-    // added in MSVC 2015. Use the required alignment from the record layout,
-    // since that may be less than the regular type alignment, and types with
-    // required alignment of less than 4 bytes are not passed indirectly.
-    if (IsWin32StructABI) {
+    // Pass over-aligned aggregates to non-variadic functions on Windows
+    // indirectly. This behavior was added in MSVC 2015. Use the required
+    // alignment from the record layout, since that may be less than the
+    // regular type alignment, and types with required alignment of less than 4
+    // bytes are not passed indirectly.
+    if (IsWin32StructABI && State.Required.isRequiredArg(ArgIndex)) {
       unsigned AlignInBits = 0;
       if (RT) {
         const ASTRecordLayout &Layout =
@@ -942,13 +946,13 @@ void X86_32ABIInfo::computeInfo(CGFunctionInfo &FI) const {
 
   bool UsedInAlloca = false;
   MutableArrayRef<CGFunctionInfoArgInfo> Args = FI.arguments();
-  for (int I = 0, E = Args.size(); I < E; ++I) {
+  for (unsigned I = 0, E = Args.size(); I < E; ++I) {
     // Skip arguments that have already been assigned.
     if (State.IsPreassigned.test(I))
       continue;
 
     Args[I].info =
-        classifyArgumentType(Args[I].type, State, FI.isDelegateCall());
+        classifyArgumentType(Args[I].type, State, I);
     UsedInAlloca |= (Args[I].info.getKind() == ABIArgInfo::InAlloca);
   }
 
diff --git a/clang/test/CodeGen/X86/x86_32-arguments-win32.c b/clang/test/CodeGen/X86/x86_32-arguments-win32.c
index 53f6b5b79642c37..5b81c43f4bbb878 100644
--- a/clang/test/CodeGen/X86/x86_32-arguments-win32.c
+++ b/clang/test/CodeGen/X86/x86_32-arguments-win32.c
@@ -128,3 +128,32 @@ void pass_underaligned_record_field() {
 // CHECK: call void @receive_falign1(i64 {{[^,)]*}})
 // CHECK: call void @receive_falign4(i64 {{[^,)]*}})
 // CHECK: call void @receive_falign8(ptr {{[^,)]*}})
+
+struct __declspec(align(8)) BigAligned {
+  int big[5];
+};
+
+void receive_aligned_variadic(int f, ...);
+void pass_aligned_variadic() {
+  struct Align8 a8 = {42};
+  struct FieldAlign8 f8 = {42};
+  struct BigAligned big;
+  receive_aligned_variadic(1, a8, f8, big);
+}
+// MSVC doesn't pass aligned objects to variadic functions indirectly.
+// CHECK-LABEL: define dso_local void @pass_aligned_variadic()
+// CHECK: call void (i32, ...) @receive_aligned_variadic(i32 noundef 1, i64 %{{[^,]*}}, i64 %{{[^,]*}}, ptr noundef byval(%struct.BigAligned) align 4 %{{[^)]*}})
+
+
+void receive_fixed_align_variadic(struct BigAligned big, ...);
+void pass_fixed_align_variadic() {
+  struct BigAligned big;
+  receive_fixed_align_variadic(big, 42);
+}
+// MSVC emits error C2719 and C3916 when receiving and passing arguments with
+// required alignment greater than 4 to the fixed part of a variadic function
+// prototype, but it's actually easier to just implement this functionality
+// correctly in Clang than it is to be bug for bug compatible, so we pass such
+// arguments indirectly.
+// CHECK-LABEL: define dso_local void @pass_fixed_align_variadic()
+// CHECK: call void (ptr, ...) @receive_fixed_align_variadic(ptr noundef %{{[^)]*}}, i32 noundef 42)

[AaronBallman]

Thank you for the fix, I appreciate it!