Skip to content
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.

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

[AArch64] Match GCC behaviour for zero-size structs #124760

Merged
merged 4 commits into from
Jan 29, 2025

Conversation

ostannard
Copy link
Collaborator

We had a test claiming that this empty struct type consumes a register slot when passing it to a function with GCC, but that does not appear to be the case, at least with GCC versions going back to 4.8.

This also caused a miscompilation when passing one of these structs to a variadic function, but it turned out that our implementation of va_arg matches GCC's ABI, so the one change fixes both bugs.

We had a test claiming that this empty struct type consumes a register
slot when passing it to a function with GCC, but that does not appear to
be the case, at least with GCC versions going back to 4.8.
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:AArch64 clang:codegen labels Jan 28, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 28, 2025

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Oliver Stannard (ostannard)

Changes

We had a test claiming that this empty struct type consumes a register slot when passing it to a function with GCC, but that does not appear to be the case, at least with GCC versions going back to 4.8.

This also caused a miscompilation when passing one of these structs to a variadic function, but it turned out that our implementation of va_arg matches GCC's ABI, so the one change fixes both bugs.


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

2 Files Affected:

  • (modified) clang/lib/CodeGen/Targets/AArch64.cpp (+8-5)
  • (modified) clang/test/CodeGen/AArch64/args.cpp (+65-20)
diff --git a/clang/lib/CodeGen/Targets/AArch64.cpp b/clang/lib/CodeGen/Targets/AArch64.cpp
index 7db67ecba07c8f..b9261dee54db99 100644
--- a/clang/lib/CodeGen/Targets/AArch64.cpp
+++ b/clang/lib/CodeGen/Targets/AArch64.cpp
@@ -418,18 +418,21 @@ ABIArgInfo AArch64ABIInfo::classifyArgumentType(QualType Ty, bool IsVariadicFn,
                                      CGCXXABI::RAA_DirectInMemory);
   }
 
-  // Empty records are always ignored on Darwin, but actually passed in C++ mode
-  // elsewhere for GNU compatibility.
+  // Empty records:
   uint64_t Size = getContext().getTypeSize(Ty);
   bool IsEmpty = isEmptyRecord(getContext(), Ty, true);
   if (!Ty->isSVESizelessBuiltinType() && (IsEmpty || Size == 0)) {
+    // Empty records are ignored in C mode, and in C++ on Darwin.
     if (!getContext().getLangOpts().CPlusPlus || isDarwinPCS())
       return ABIArgInfo::getIgnore();
 
-    // GNU C mode. The only argument that gets ignored is an empty one with size
-    // 0.
-    if (IsEmpty && Size == 0)
+    // In C++ mode, arguments which have sizeof() == 0 (which are non-standard
+    // C++) are ignored. This isn't defined by any standard, so we copy GCC's
+    // behaviour here.
+    if (Size == 0)
       return ABIArgInfo::getIgnore();
+
+    // Otherwise, they are passed as if they have a size of 1 byte.
     return ABIArgInfo::getDirect(llvm::Type::getInt8Ty(getVMContext()));
   }
 
diff --git a/clang/test/CodeGen/AArch64/args.cpp b/clang/test/CodeGen/AArch64/args.cpp
index fe1298cc683a40..6f1f3d5e2a062b 100644
--- a/clang/test/CodeGen/AArch64/args.cpp
+++ b/clang/test/CodeGen/AArch64/args.cpp
@@ -1,6 +1,6 @@
-// RUN: %clang_cc1 -triple arm64-apple-ios7.0 -target-abi darwinpcs -emit-llvm -o - %s | FileCheck %s
-// RUN: %clang_cc1 -triple aarch64-linux-gnu -emit-llvm -o - -x c %s | FileCheck %s --check-prefix=CHECK-GNU-C
-// RUN: %clang_cc1 -triple aarch64-linux-gnu -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-GNU-CXX
+// RUN: %clang_cc1 -triple arm64-apple-ios7.0 -target-abi darwinpcs -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,DARWIN
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -emit-llvm -o - -x c %s | FileCheck %s --check-prefixes=CHECK,C
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,CXX
 
 // Empty structs are ignored for PCS purposes on Darwin and in C mode elsewhere.
 // In C++ mode on ELF they consume a register slot though. Functions are
@@ -15,16 +15,16 @@
 
 struct Empty {};
 
-// CHECK: define{{.*}} i32 @empty_arg(i32 noundef %a)
-// CHECK-GNU-C: define{{.*}} i32 @empty_arg(i32 noundef %a)
-// CHECK-GNU-CXX: define{{.*}} i32 @empty_arg(i8 %e.coerce, i32 noundef %a)
+// DARWIN: define{{.*}} i32 @empty_arg(i32 noundef %a)
+// C: define{{.*}} i32 @empty_arg(i32 noundef %a)
+// CXX: define{{.*}} i32 @empty_arg(i8 %e.coerce, i32 noundef %a)
 EXTERNC int empty_arg(struct Empty e, int a) {
   return a;
 }
 
-// CHECK: define{{.*}} void @empty_ret()
-// CHECK-GNU-C: define{{.*}} void @empty_ret()
-// CHECK-GNU-CXX: define{{.*}} void @empty_ret()
+// DARWIN: define{{.*}} void @empty_ret()
+// C: define{{.*}} void @empty_ret()
+// CXX: define{{.*}} void @empty_ret()
 EXTERNC struct Empty empty_ret(void) {
   struct Empty e;
   return e;
@@ -38,30 +38,75 @@ struct SuperEmpty {
   int arr[0];
 };
 
-// CHECK: define{{.*}} i32 @super_empty_arg(i32 noundef %a)
-// CHECK-GNU-C: define{{.*}} i32 @super_empty_arg(i32 noundef %a)
-// CHECK-GNU-CXX: define{{.*}} i32 @super_empty_arg(i32 noundef %a)
+// DARWIN: define{{.*}} i32 @super_empty_arg(i32 noundef %a)
+// C: define{{.*}} i32 @super_empty_arg(i32 noundef %a)
+// CXX: define{{.*}} i32 @super_empty_arg(i32 noundef %a)
 EXTERNC int super_empty_arg(struct SuperEmpty e, int a) {
   return a;
 }
 
-// This is not empty. It has 0 size but consumes a register slot for GCC.
+// This is also not empty, and non-standard. We previously considered it to
+// consume a register slot, but GCC does not, so we match that.
 
 struct SortOfEmpty {
   struct SuperEmpty e;
 };
 
-// CHECK: define{{.*}} i32 @sort_of_empty_arg(i32 noundef %a)
-// CHECK-GNU-C: define{{.*}} i32 @sort_of_empty_arg(i32 noundef %a)
-// CHECK-GNU-CXX: define{{.*}} i32 @sort_of_empty_arg(i8 %e.coerce, i32 noundef %a)
-EXTERNC int sort_of_empty_arg(struct Empty e, int a) {
+// DARWIN: define{{.*}} i32 @sort_of_empty_arg(i32 noundef %a)
+// C: define{{.*}} i32 @sort_of_empty_arg(i32 noundef %a)
+// CXX: define{{.*}} i32 @sort_of_empty_arg(i32 noundef %a)
+EXTERNC int sort_of_empty_arg(struct SortOfEmpty e, int a) {
   return a;
 }
 
-// CHECK: define{{.*}} void @sort_of_empty_ret()
-// CHECK-GNU-C: define{{.*}} void @sort_of_empty_ret()
-// CHECK-GNU-CXX: define{{.*}} void @sort_of_empty_ret()
+// DARWIN: define{{.*}} void @sort_of_empty_ret()
+// C: define{{.*}} void @sort_of_empty_ret()
+// CXX: define{{.*}} void @sort_of_empty_ret()
 EXTERNC struct SortOfEmpty sort_of_empty_ret(void) {
   struct SortOfEmpty e;
   return e;
 }
+
+#include <stdarg.h>
+
+// va_arg matches the above rules, consuming an incoming argument in cases
+// where one would be passed, and not doing so when the argument should be
+// ignored.
+
+EXTERNC struct Empty empty_arg_variadic(int a, ...) {
+// CHECK-LABEL: @empty_arg_variadic(
+// DARWIN-NOT: {{ getelementptr }}
+// C-NOT: {{ getelementptr }}
+// CXX: %new_reg_offs = add i32 %gr_offs, 8
+// CXX: %new_stack = getelementptr inbounds i8, ptr %stack, i64 8
+  va_list vl;
+  va_start(vl, a);
+  struct Empty b = va_arg(vl, struct Empty);
+  va_end(vl);
+  return b;
+}
+
+EXTERNC struct SuperEmpty super_empty_arg_variadic(int a, ...) {
+// CHECK-LABEL: @super_empty_arg_variadic(
+// DARWIN-NOT: {{ getelementptr }}
+// C-NOT: {{ getelementptr }}
+// CXX-NOT: {{ getelementptr }}
+  va_list vl;
+  va_start(vl, a);
+  struct SuperEmpty b = va_arg(vl, struct SuperEmpty);
+  va_end(vl);
+  return b;
+}
+
+EXTERNC struct SortOfEmpty sort_of_empty_arg_variadic(int a, ...) {
+// CHECK-LABEL: @sort_of_empty_arg_variadic(
+// DARWIN: %argp.next = getelementptr inbounds i8, ptr %argp.cur, i64 0
+// C-NOT: {{ getelementptr }}
+// CXX-NOT: {{ getelementptr }}
+  va_list vl;
+  va_start(vl, a);
+  struct SortOfEmpty b = va_arg(vl, struct SortOfEmpty);
+  va_end(vl);
+  return b;
+}
+

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

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

LGTM

@davemgreen
Copy link
Collaborator

Is it worth adding a release note, if this is altering the ABI between versions?

@zygoloid
Copy link
Collaborator

Also, is it worth adding -fclang-abi-compat= support to control this change?

@efriedma-quic
Copy link
Collaborator

The likelihood of anyone actually tripping over this outside a synthetic testcase is very low; you need a combination of unusual constructs to hit this. So I'm not really worried about documentation/ABI compat flags.

@ostannard
Copy link
Collaborator Author

I agree with @efriedma-quic that this is a very artificial test case, and I wouldn't expect anyone to hit this in reality (I only noticed it while copying the test for #124762), so it's not worth release-noting or adding a compatibility option.

@ostannard ostannard merged commit e9c2e0a into llvm:main Jan 29, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AArch64 clang:codegen clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants