[X86_32] Teach X86_32 va_arg to ignore empty structs.

[CoTinker]

Empty structs are ignored for parameter passing purposes, but va_arg was incrementing the pointer anyway for that the size of empty struct in c++ is 1 byte, which could lead to va_list getting out of sync. Fix #86057.

[llvmbot]

@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Longsheng Mou (CoTinker)

Changes

Empty structs are ignored for parameter passing purposes, but va_arg was incrementing the pointer anyway for that the size of empty struct in c++ is 1 byte, which could lead to va_list getting out of sync. Fix #86057.

---

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

2 Files Affected:

• (modified) clang/lib/CodeGen/Targets/X86.cpp (+4)
• (added) clang/test/CodeGenCXX/x86_32-vaarg.cpp (+20)

diff --git a/clang/lib/CodeGen/Targets/X86.cpp b/clang/lib/CodeGen/Targets/X86.cpp
index 1ec0f159ebcb8a..7931f56ad6835f 100644
--- a/clang/lib/CodeGen/Targets/X86.cpp
+++ b/clang/lib/CodeGen/Targets/X86.cpp
@@ -1069,6 +1069,10 @@ Address X86_32ABIInfo::EmitVAArg(CodeGenFunction &CGF,
 
   auto TypeInfo = getContext().getTypeInfoInChars(Ty);
 
+  // Empty records are ignored for parameter passing purposes on non-Windows.
+  if (!IsWin32StructABI && isEmptyRecord(getContext(), Ty, true))
+    return CGF.CreateMemTemp(Ty);
+
   // x86-32 changes the alignment of certain arguments on the stack.
   //
   // Just messing with TypeInfo like this works because we never pass
diff --git a/clang/test/CodeGenCXX/x86_32-vaarg.cpp b/clang/test/CodeGenCXX/x86_32-vaarg.cpp
new file mode 100644
index 00000000000000..23eac1164118c6
--- /dev/null
+++ b/clang/test/CodeGenCXX/x86_32-vaarg.cpp
@@ -0,0 +1,20 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -triple i386-linux-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple i386-linux-gnu -emit-llvm -x c -o - %s | FileCheck %s
+
+typedef struct {} empty;
+
+// CHECK-LABEL: @{{.*}}empty_record_test
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[RESULT_PTR:%.*]] = alloca ptr, align 4
+// CHECK-NEXT:    [[Z_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    [[LIST:%.*]] = alloca ptr, align 4
+// CHECK-NEXT:    [[TMP:%.*]] = alloca [[STRUCT_EMPTY:%.*]], align 1
+// CHECK-NEXT:    store ptr [[AGG_RESULT:%.*]], ptr [[RESULT_PTR]], align 4
+// CHECK-NEXT:    store i32 [[Z:%.*]], ptr [[Z_ADDR]], align 4
+// CHECK-NEXT:    call void @llvm.va_start(ptr [[LIST]])
+empty empty_record_test(int z, ...) {
+  __builtin_va_list list;
+  __builtin_va_start(list, z);
+  return __builtin_va_arg(list, empty);
+}

[efriedma-quic]

I'd prefer to use classifyArgumentType, like we do for the 64-bit case, instead of trying to duplicate the logic.

[CoTinker]

classifyArgumentType in X86_32 need CCState parameter, but we can not get it in this scope, because the CCState is temporarily calculated in X86_32ABIInfo::computeInfo.

[efriedma-quic]

If it doesn't work with the code as-is, please refactor to the code to make it work. Scattered checks like this are hard to maintain.

The simplest thing here would be to just construct a CCState here, I think? Or if that doesn't work for some reason, make the CCState parameter to classifyArgumentType optional.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[efriedma-quic]

LGTM

[efriedma-quic]

(Please fix the formatting issue before merging)

[CoTinker]

Thank you very much. The format has been modified.