Skip to content

[ARM,AArch64] Fix ABI bugs with over-sized bitfields #126774

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

Merged
merged 4 commits into from
Feb 20, 2025

Conversation

ostannard
Copy link
Collaborator

This fixes two bugs in the ABI for over-sized bitfields for ARM and AArch64:

The container type picked for an over-sized bitfield already contributes to the alignment of the structure, but it should also contribute to the "unadjusted alignment" which is used by the ARM and AArch64 PCS.

AAPCS64 defines the bitfield layout algorithm for over-sized bitfields as picking a container which is the fundamental integer data type with the largest size less than or equal to the bit-field width. Since AAPCS64 has a 128-bit integer fundamental data type, we need to consider Int128 as a container type for AArch64.

AAPCS64 defines the bitfield layout algorithm for over-sized bitfields
as picking a container which is the fundamental integer data type with
the largest size less than or equal to the bit-field width. Since
AAPCS64 has a 128-bit integer fundamental data type, we need to consider
Int128 as a container type for AArch64.
The container type picked for an over-sized bitfield already contributes
to the alignment of the structure, but it should also contribute to the
"unadjusted alignment" which is used by the ARM and AArch64 PCS.
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:AArch64 clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 11, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 11, 2025

@llvm/pr-subscribers-backend-aarch64

Author: Oliver Stannard (ostannard)

Changes

This fixes two bugs in the ABI for over-sized bitfields for ARM and AArch64:

The container type picked for an over-sized bitfield already contributes to the alignment of the structure, but it should also contribute to the "unadjusted alignment" which is used by the ARM and AArch64 PCS.

AAPCS64 defines the bitfield layout algorithm for over-sized bitfields as picking a container which is the fundamental integer data type with the largest size less than or equal to the bit-field width. Since AAPCS64 has a 128-bit integer fundamental data type, we need to consider Int128 as a container type for AArch64.


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

7 Files Affected:

  • (modified) clang/include/clang/Basic/TargetInfo.h (+8)
  • (modified) clang/lib/AST/RecordLayoutBuilder.cpp (+7-3)
  • (modified) clang/lib/Basic/TargetInfo.cpp (+1)
  • (modified) clang/lib/Basic/Targets/AArch64.cpp (+4)
  • (modified) clang/test/CodeGen/aapcs-align.cpp (+43)
  • (modified) clang/test/CodeGen/aapcs64-align.cpp (+64)
  • (modified) clang/test/CodeGenCXX/debug-info-structured-binding-bitfield.cpp (+2-2)
diff --git a/clang/include/clang/Basic/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index 070cc792ca7db..db23afa6d6f0b 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -199,6 +199,10 @@ struct TransferrableTargetInfo {
   /// zero length bitfield, regardless of the zero length bitfield type.
   unsigned ZeroLengthBitfieldBoundary;
 
+  /// The largest container size which should be used for an over-sized
+  /// bitfield, in bits.
+  unsigned LargestOverSizedBitfieldContainer;
+
   /// If non-zero, specifies a maximum alignment to truncate alignment
   /// specified in the aligned attribute of a static variable to this value.
   unsigned MaxAlignedAttribute;
@@ -954,6 +958,10 @@ class TargetInfo : public TransferrableTargetInfo,
     return ZeroLengthBitfieldBoundary;
   }
 
+  unsigned getLargestOverSizedBitfieldContainer() const {
+    return LargestOverSizedBitfieldContainer;
+  }
+
   /// Get the maximum alignment in bits for a static variable with
   /// aligned attribute.
   unsigned getMaxAlignedAttribute() const { return MaxAlignedAttribute; }
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index ae6d299024c6d..d9380c4b6ae96 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1469,15 +1469,18 @@ void ItaniumRecordLayoutBuilder::LayoutWideBitField(uint64_t FieldSize,
   //   sizeof(T')*8 <= n.
 
   QualType IntegralPODTypes[] = {
-    Context.UnsignedCharTy, Context.UnsignedShortTy, Context.UnsignedIntTy,
-    Context.UnsignedLongTy, Context.UnsignedLongLongTy
+      Context.UnsignedCharTy,     Context.UnsignedShortTy,
+      Context.UnsignedIntTy,      Context.UnsignedLongTy,
+      Context.UnsignedLongLongTy, Context.UnsignedInt128Ty,
   };
 
   QualType Type;
+  uint64_t MaxSize =
+      Context.getTargetInfo().getLargestOverSizedBitfieldContainer();
   for (const QualType &QT : IntegralPODTypes) {
     uint64_t Size = Context.getTypeSize(QT);
 
-    if (Size > FieldSize)
+    if (Size > FieldSize || Size > MaxSize)
       break;
 
     Type = QT;
@@ -1520,6 +1523,7 @@ void ItaniumRecordLayoutBuilder::LayoutWideBitField(uint64_t FieldSize,
   setSize(std::max(getSizeInBits(), getDataSizeInBits()));
 
   // Remember max struct/class alignment.
+  UnadjustedAlignment = std::max(UnadjustedAlignment, TypeAlign);
   UpdateAlignment(TypeAlign);
 }
 
diff --git a/clang/lib/Basic/TargetInfo.cpp b/clang/lib/Basic/TargetInfo.cpp
index c0bf4e686cf03..0699ec686e4e6 100644
--- a/clang/lib/Basic/TargetInfo.cpp
+++ b/clang/lib/Basic/TargetInfo.cpp
@@ -141,6 +141,7 @@ TargetInfo::TargetInfo(const llvm::Triple &T) : Triple(T) {
   UseLeadingZeroLengthBitfield = true;
   UseExplicitBitFieldAlignment = true;
   ZeroLengthBitfieldBoundary = 0;
+  LargestOverSizedBitfieldContainer = 64;
   MaxAlignedAttribute = 0;
   HalfFormat = &llvm::APFloat::IEEEhalf();
   FloatFormat = &llvm::APFloat::IEEEsingle();
diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index fad8d773bfc52..3633bab6e0df9 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -261,6 +261,10 @@ AArch64TargetInfo::AArch64TargetInfo(const llvm::Triple &Triple,
   assert(UseBitFieldTypeAlignment && "bitfields affect type alignment");
   UseZeroLengthBitfieldAlignment = true;
 
+  // AAPCS64 allows any "fundamental integer data type" to be used for
+  // over-sized bitfields, which includes 128-bit integers.
+  LargestOverSizedBitfieldContainer = 128;
+
   HasUnalignedAccess = true;
 
   // AArch64 targets default to using the ARM C++ ABI.
diff --git a/clang/test/CodeGen/aapcs-align.cpp b/clang/test/CodeGen/aapcs-align.cpp
index 4f393d9e6b7f3..c7bc5ba0bbfef 100644
--- a/clang/test/CodeGen/aapcs-align.cpp
+++ b/clang/test/CodeGen/aapcs-align.cpp
@@ -6,6 +6,11 @@
 
 extern "C" {
 
+// CHECK: @sizeof_OverSizedBitfield ={{.*}} global i32 8
+// CHECK: @alignof_OverSizedBitfield ={{.*}} global i32 8
+// CHECK: @sizeof_VeryOverSizedBitfield ={{.*}} global i32 16
+// CHECK: @alignof_VeryOverSizedBitfield ={{.*}} global i32 8
+
 // Base case, nothing interesting.
 struct S {
   int x, y;
@@ -138,4 +143,42 @@ void g6() {
 // CHECK: call void @f6m(i32 noundef 1, i32 noundef 2, i32 noundef 3, i32 noundef 4, i32 noundef 5, [4 x i32] [i32 6, i32 7, i32 0, i32 undef])
 // CHECK: declare void @f6(i32 noundef, [4 x i32])
 // CHECK: declare void @f6m(i32 noundef, i32 noundef, i32 noundef, i32 noundef, i32 noundef, [4 x i32])
+
+// Over-sized bitfield, which results in a 64-bit container type, so 64-bit
+// alignment.
+struct OverSizedBitfield {
+  int x : 64;
+};
+
+unsigned sizeof_OverSizedBitfield = sizeof(OverSizedBitfield);
+unsigned alignof_OverSizedBitfield = alignof(OverSizedBitfield);
+
+// CHECK: define{{.*}} void @g7
+// CHECK: call void @f7(i32 noundef 1, [1 x i64] [i64 42])
+// CHECK: declare void @f7(i32 noundef, [1 x i64])
+void f7(int a, OverSizedBitfield b);
+void g7() {
+  OverSizedBitfield s = {42};
+  f7(1, s);
+}
+
+// There are no 128-bit fundamental data types defined by AAPCS32, so this gets
+// a 64-bit container plus 64 bits of padding, giving it a size of 16 bytes and
+// alignment of 8 bytes.
+struct VeryOverSizedBitfield {
+  int x : 128;
+};
+
+unsigned sizeof_VeryOverSizedBitfield = sizeof(VeryOverSizedBitfield);
+unsigned alignof_VeryOverSizedBitfield = alignof(VeryOverSizedBitfield);
+
+// CHECK: define{{.*}} void @g8
+// CHECK: call void @f8(i32 noundef 1, [2 x i64] [i64 42, i64 0])
+// CHECK: declare void @f8(i32 noundef, [2 x i64])
+void f8(int a, VeryOverSizedBitfield b);
+void g8() {
+  VeryOverSizedBitfield s = {42};
+  f8(1, s);
+}
+
 }
diff --git a/clang/test/CodeGen/aapcs64-align.cpp b/clang/test/CodeGen/aapcs64-align.cpp
index 7a8151022852e..9578772dace75 100644
--- a/clang/test/CodeGen/aapcs64-align.cpp
+++ b/clang/test/CodeGen/aapcs64-align.cpp
@@ -5,6 +5,13 @@
 
 extern "C" {
 
+// CHECK: @sizeof_OverSizedBitfield ={{.*}} global i32 8
+// CHECK: @alignof_OverSizedBitfield ={{.*}} global i32 8
+// CHECK: @sizeof_VeryOverSizedBitfield ={{.*}} global i32 16
+// CHECK: @alignof_VeryOverSizedBitfield ={{.*}} global i32 16
+// CHECK: @sizeof_RidiculouslyOverSizedBitfield ={{.*}} global i32 32
+// CHECK: @alignof_RidiculouslyOverSizedBitfield ={{.*}} global i32 16
+
 // Base case, nothing interesting.
 struct S {
   long x, y;
@@ -161,5 +168,62 @@ int test_bitint8(){
 }
 // CHECK:  ret i32 1
 
+// Over-sized bitfield, which results in a 64-bit container type, so 64-bit
+// alignment.
+struct OverSizedBitfield {
+  int x : 64;
+};
+
+unsigned sizeof_OverSizedBitfield = sizeof(OverSizedBitfield);
+unsigned alignof_OverSizedBitfield = alignof(OverSizedBitfield);
+
+// CHECK: define{{.*}} void @g7
+// CHECK: call void @f7(i32 noundef 1, i64 42)
+// CHECK: declare void @f7(i32 noundef, i64)
+void f7(int a, OverSizedBitfield b);
+void g7() {
+  OverSizedBitfield s = {42};
+  f7(1, s);
+}
+
+// There are no 128-bit fundamental data types defined by AAPCS32, so this gets
+// a 64-bit container plus 64 bits of padding, giving it a size of 16 bytes and
+// alignment of 8 bytes.
+struct VeryOverSizedBitfield {
+  int x : 128;
+};
+
+unsigned sizeof_VeryOverSizedBitfield = sizeof(VeryOverSizedBitfield);
+unsigned alignof_VeryOverSizedBitfield = alignof(VeryOverSizedBitfield);
+
+// CHECK: define{{.*}} void @g8
+// CHECK: call void @f8(i32 noundef 1, i128 42)
+// CHECK: declare void @f8(i32 noundef, i128)
+void f8(int a, VeryOverSizedBitfield b);
+void g8() {
+  VeryOverSizedBitfield s = {42};
+  f8(1, s);
+}
+
+// There are no bigger fundamental data types, so this gets a 128-bit container
+// and 128 bits of padding, giving the struct a size of 32 bytes, and an
+// alignment of 16 bytes. This is over the PCS size limit of 16 bytes, so it
+// will be passed indirectly.
+struct RidiculouslyOverSizedBitfield {
+  int x : 256;
+};
+
+unsigned sizeof_RidiculouslyOverSizedBitfield = sizeof(RidiculouslyOverSizedBitfield);
+unsigned alignof_RidiculouslyOverSizedBitfield = alignof(RidiculouslyOverSizedBitfield);
+
+// CHECK: define{{.*}} void @g9
+// CHECK: call void @f9(i32 noundef 1, ptr noundef nonnull %agg.tmp)
+// CHECK: declare void @f9(i32 noundef, ptr noundef)
+void f9(int a, RidiculouslyOverSizedBitfield b);
+void g9() {
+  RidiculouslyOverSizedBitfield s = {42};
+  f9(1, s);
+}
+
 }
 
diff --git a/clang/test/CodeGenCXX/debug-info-structured-binding-bitfield.cpp b/clang/test/CodeGenCXX/debug-info-structured-binding-bitfield.cpp
index e475f032f5ce3..b7aad6a5bcd21 100644
--- a/clang/test/CodeGenCXX/debug-info-structured-binding-bitfield.cpp
+++ b/clang/test/CodeGenCXX/debug-info-structured-binding-bitfield.cpp
@@ -248,8 +248,8 @@ struct S15 {
 };
 
 // CHECK-LABEL: define dso_local void @_Z4fS15v
-// CHECK:                        alloca %struct.S15, align 8
-// CHECK-NEXT:    [[TMP0:%.*]] = alloca %struct.S15, align 8
+// CHECK:                        alloca %struct.S15, align 16
+// CHECK-NEXT:    [[TMP0:%.*]] = alloca %struct.S15, align 16
 // CHECK:         #dbg_declare(ptr [[TMP0]], [[S15_A:![0-9]+]], !DIExpression(DW_OP_LLVM_extract_bits_sext, 0, 32),
 // CHECK-NEXT:    #dbg_declare(ptr [[TMP0]], [[S15_B:![0-9]+]], !DIExpression(DW_OP_plus_uconst, 16, DW_OP_LLVM_extract_bits_zext, 0, 32),
 //

@llvmbot
Copy link
Member

llvmbot commented Feb 11, 2025

@llvm/pr-subscribers-clang

Author: Oliver Stannard (ostannard)

Changes

This fixes two bugs in the ABI for over-sized bitfields for ARM and AArch64:

The container type picked for an over-sized bitfield already contributes to the alignment of the structure, but it should also contribute to the "unadjusted alignment" which is used by the ARM and AArch64 PCS.

AAPCS64 defines the bitfield layout algorithm for over-sized bitfields as picking a container which is the fundamental integer data type with the largest size less than or equal to the bit-field width. Since AAPCS64 has a 128-bit integer fundamental data type, we need to consider Int128 as a container type for AArch64.


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

7 Files Affected:

  • (modified) clang/include/clang/Basic/TargetInfo.h (+8)
  • (modified) clang/lib/AST/RecordLayoutBuilder.cpp (+7-3)
  • (modified) clang/lib/Basic/TargetInfo.cpp (+1)
  • (modified) clang/lib/Basic/Targets/AArch64.cpp (+4)
  • (modified) clang/test/CodeGen/aapcs-align.cpp (+43)
  • (modified) clang/test/CodeGen/aapcs64-align.cpp (+64)
  • (modified) clang/test/CodeGenCXX/debug-info-structured-binding-bitfield.cpp (+2-2)
diff --git a/clang/include/clang/Basic/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index 070cc792ca7db..db23afa6d6f0b 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -199,6 +199,10 @@ struct TransferrableTargetInfo {
   /// zero length bitfield, regardless of the zero length bitfield type.
   unsigned ZeroLengthBitfieldBoundary;
 
+  /// The largest container size which should be used for an over-sized
+  /// bitfield, in bits.
+  unsigned LargestOverSizedBitfieldContainer;
+
   /// If non-zero, specifies a maximum alignment to truncate alignment
   /// specified in the aligned attribute of a static variable to this value.
   unsigned MaxAlignedAttribute;
@@ -954,6 +958,10 @@ class TargetInfo : public TransferrableTargetInfo,
     return ZeroLengthBitfieldBoundary;
   }
 
+  unsigned getLargestOverSizedBitfieldContainer() const {
+    return LargestOverSizedBitfieldContainer;
+  }
+
   /// Get the maximum alignment in bits for a static variable with
   /// aligned attribute.
   unsigned getMaxAlignedAttribute() const { return MaxAlignedAttribute; }
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index ae6d299024c6d..d9380c4b6ae96 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1469,15 +1469,18 @@ void ItaniumRecordLayoutBuilder::LayoutWideBitField(uint64_t FieldSize,
   //   sizeof(T')*8 <= n.
 
   QualType IntegralPODTypes[] = {
-    Context.UnsignedCharTy, Context.UnsignedShortTy, Context.UnsignedIntTy,
-    Context.UnsignedLongTy, Context.UnsignedLongLongTy
+      Context.UnsignedCharTy,     Context.UnsignedShortTy,
+      Context.UnsignedIntTy,      Context.UnsignedLongTy,
+      Context.UnsignedLongLongTy, Context.UnsignedInt128Ty,
   };
 
   QualType Type;
+  uint64_t MaxSize =
+      Context.getTargetInfo().getLargestOverSizedBitfieldContainer();
   for (const QualType &QT : IntegralPODTypes) {
     uint64_t Size = Context.getTypeSize(QT);
 
-    if (Size > FieldSize)
+    if (Size > FieldSize || Size > MaxSize)
       break;
 
     Type = QT;
@@ -1520,6 +1523,7 @@ void ItaniumRecordLayoutBuilder::LayoutWideBitField(uint64_t FieldSize,
   setSize(std::max(getSizeInBits(), getDataSizeInBits()));
 
   // Remember max struct/class alignment.
+  UnadjustedAlignment = std::max(UnadjustedAlignment, TypeAlign);
   UpdateAlignment(TypeAlign);
 }
 
diff --git a/clang/lib/Basic/TargetInfo.cpp b/clang/lib/Basic/TargetInfo.cpp
index c0bf4e686cf03..0699ec686e4e6 100644
--- a/clang/lib/Basic/TargetInfo.cpp
+++ b/clang/lib/Basic/TargetInfo.cpp
@@ -141,6 +141,7 @@ TargetInfo::TargetInfo(const llvm::Triple &T) : Triple(T) {
   UseLeadingZeroLengthBitfield = true;
   UseExplicitBitFieldAlignment = true;
   ZeroLengthBitfieldBoundary = 0;
+  LargestOverSizedBitfieldContainer = 64;
   MaxAlignedAttribute = 0;
   HalfFormat = &llvm::APFloat::IEEEhalf();
   FloatFormat = &llvm::APFloat::IEEEsingle();
diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index fad8d773bfc52..3633bab6e0df9 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -261,6 +261,10 @@ AArch64TargetInfo::AArch64TargetInfo(const llvm::Triple &Triple,
   assert(UseBitFieldTypeAlignment && "bitfields affect type alignment");
   UseZeroLengthBitfieldAlignment = true;
 
+  // AAPCS64 allows any "fundamental integer data type" to be used for
+  // over-sized bitfields, which includes 128-bit integers.
+  LargestOverSizedBitfieldContainer = 128;
+
   HasUnalignedAccess = true;
 
   // AArch64 targets default to using the ARM C++ ABI.
diff --git a/clang/test/CodeGen/aapcs-align.cpp b/clang/test/CodeGen/aapcs-align.cpp
index 4f393d9e6b7f3..c7bc5ba0bbfef 100644
--- a/clang/test/CodeGen/aapcs-align.cpp
+++ b/clang/test/CodeGen/aapcs-align.cpp
@@ -6,6 +6,11 @@
 
 extern "C" {
 
+// CHECK: @sizeof_OverSizedBitfield ={{.*}} global i32 8
+// CHECK: @alignof_OverSizedBitfield ={{.*}} global i32 8
+// CHECK: @sizeof_VeryOverSizedBitfield ={{.*}} global i32 16
+// CHECK: @alignof_VeryOverSizedBitfield ={{.*}} global i32 8
+
 // Base case, nothing interesting.
 struct S {
   int x, y;
@@ -138,4 +143,42 @@ void g6() {
 // CHECK: call void @f6m(i32 noundef 1, i32 noundef 2, i32 noundef 3, i32 noundef 4, i32 noundef 5, [4 x i32] [i32 6, i32 7, i32 0, i32 undef])
 // CHECK: declare void @f6(i32 noundef, [4 x i32])
 // CHECK: declare void @f6m(i32 noundef, i32 noundef, i32 noundef, i32 noundef, i32 noundef, [4 x i32])
+
+// Over-sized bitfield, which results in a 64-bit container type, so 64-bit
+// alignment.
+struct OverSizedBitfield {
+  int x : 64;
+};
+
+unsigned sizeof_OverSizedBitfield = sizeof(OverSizedBitfield);
+unsigned alignof_OverSizedBitfield = alignof(OverSizedBitfield);
+
+// CHECK: define{{.*}} void @g7
+// CHECK: call void @f7(i32 noundef 1, [1 x i64] [i64 42])
+// CHECK: declare void @f7(i32 noundef, [1 x i64])
+void f7(int a, OverSizedBitfield b);
+void g7() {
+  OverSizedBitfield s = {42};
+  f7(1, s);
+}
+
+// There are no 128-bit fundamental data types defined by AAPCS32, so this gets
+// a 64-bit container plus 64 bits of padding, giving it a size of 16 bytes and
+// alignment of 8 bytes.
+struct VeryOverSizedBitfield {
+  int x : 128;
+};
+
+unsigned sizeof_VeryOverSizedBitfield = sizeof(VeryOverSizedBitfield);
+unsigned alignof_VeryOverSizedBitfield = alignof(VeryOverSizedBitfield);
+
+// CHECK: define{{.*}} void @g8
+// CHECK: call void @f8(i32 noundef 1, [2 x i64] [i64 42, i64 0])
+// CHECK: declare void @f8(i32 noundef, [2 x i64])
+void f8(int a, VeryOverSizedBitfield b);
+void g8() {
+  VeryOverSizedBitfield s = {42};
+  f8(1, s);
+}
+
 }
diff --git a/clang/test/CodeGen/aapcs64-align.cpp b/clang/test/CodeGen/aapcs64-align.cpp
index 7a8151022852e..9578772dace75 100644
--- a/clang/test/CodeGen/aapcs64-align.cpp
+++ b/clang/test/CodeGen/aapcs64-align.cpp
@@ -5,6 +5,13 @@
 
 extern "C" {
 
+// CHECK: @sizeof_OverSizedBitfield ={{.*}} global i32 8
+// CHECK: @alignof_OverSizedBitfield ={{.*}} global i32 8
+// CHECK: @sizeof_VeryOverSizedBitfield ={{.*}} global i32 16
+// CHECK: @alignof_VeryOverSizedBitfield ={{.*}} global i32 16
+// CHECK: @sizeof_RidiculouslyOverSizedBitfield ={{.*}} global i32 32
+// CHECK: @alignof_RidiculouslyOverSizedBitfield ={{.*}} global i32 16
+
 // Base case, nothing interesting.
 struct S {
   long x, y;
@@ -161,5 +168,62 @@ int test_bitint8(){
 }
 // CHECK:  ret i32 1
 
+// Over-sized bitfield, which results in a 64-bit container type, so 64-bit
+// alignment.
+struct OverSizedBitfield {
+  int x : 64;
+};
+
+unsigned sizeof_OverSizedBitfield = sizeof(OverSizedBitfield);
+unsigned alignof_OverSizedBitfield = alignof(OverSizedBitfield);
+
+// CHECK: define{{.*}} void @g7
+// CHECK: call void @f7(i32 noundef 1, i64 42)
+// CHECK: declare void @f7(i32 noundef, i64)
+void f7(int a, OverSizedBitfield b);
+void g7() {
+  OverSizedBitfield s = {42};
+  f7(1, s);
+}
+
+// There are no 128-bit fundamental data types defined by AAPCS32, so this gets
+// a 64-bit container plus 64 bits of padding, giving it a size of 16 bytes and
+// alignment of 8 bytes.
+struct VeryOverSizedBitfield {
+  int x : 128;
+};
+
+unsigned sizeof_VeryOverSizedBitfield = sizeof(VeryOverSizedBitfield);
+unsigned alignof_VeryOverSizedBitfield = alignof(VeryOverSizedBitfield);
+
+// CHECK: define{{.*}} void @g8
+// CHECK: call void @f8(i32 noundef 1, i128 42)
+// CHECK: declare void @f8(i32 noundef, i128)
+void f8(int a, VeryOverSizedBitfield b);
+void g8() {
+  VeryOverSizedBitfield s = {42};
+  f8(1, s);
+}
+
+// There are no bigger fundamental data types, so this gets a 128-bit container
+// and 128 bits of padding, giving the struct a size of 32 bytes, and an
+// alignment of 16 bytes. This is over the PCS size limit of 16 bytes, so it
+// will be passed indirectly.
+struct RidiculouslyOverSizedBitfield {
+  int x : 256;
+};
+
+unsigned sizeof_RidiculouslyOverSizedBitfield = sizeof(RidiculouslyOverSizedBitfield);
+unsigned alignof_RidiculouslyOverSizedBitfield = alignof(RidiculouslyOverSizedBitfield);
+
+// CHECK: define{{.*}} void @g9
+// CHECK: call void @f9(i32 noundef 1, ptr noundef nonnull %agg.tmp)
+// CHECK: declare void @f9(i32 noundef, ptr noundef)
+void f9(int a, RidiculouslyOverSizedBitfield b);
+void g9() {
+  RidiculouslyOverSizedBitfield s = {42};
+  f9(1, s);
+}
+
 }
 
diff --git a/clang/test/CodeGenCXX/debug-info-structured-binding-bitfield.cpp b/clang/test/CodeGenCXX/debug-info-structured-binding-bitfield.cpp
index e475f032f5ce3..b7aad6a5bcd21 100644
--- a/clang/test/CodeGenCXX/debug-info-structured-binding-bitfield.cpp
+++ b/clang/test/CodeGenCXX/debug-info-structured-binding-bitfield.cpp
@@ -248,8 +248,8 @@ struct S15 {
 };
 
 // CHECK-LABEL: define dso_local void @_Z4fS15v
-// CHECK:                        alloca %struct.S15, align 8
-// CHECK-NEXT:    [[TMP0:%.*]] = alloca %struct.S15, align 8
+// CHECK:                        alloca %struct.S15, align 16
+// CHECK-NEXT:    [[TMP0:%.*]] = alloca %struct.S15, align 16
 // CHECK:         #dbg_declare(ptr [[TMP0]], [[S15_A:![0-9]+]], !DIExpression(DW_OP_LLVM_extract_bits_sext, 0, 32),
 // CHECK-NEXT:    #dbg_declare(ptr [[TMP0]], [[S15_B:![0-9]+]], !DIExpression(DW_OP_plus_uconst, 16, DW_OP_LLVM_extract_bits_zext, 0, 32),
 //

@ostannard
Copy link
Collaborator Author

Ping

Copy link
Collaborator

@john-brawn-arm john-brawn-arm left a comment

Choose a reason for hiding this comment

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

LGTM.

@ostannard ostannard merged commit 02e8fd7 into llvm:main Feb 20, 2025
8 checks passed
@fmayer
Copy link
Contributor

fmayer commented Apr 29, 2025

This changed the size of the following struct:

#include <stdint.h>

struct A {
     bool value : 64 = false;
        uint64_t : 448;
};

static_assert(sizeof(A) == 64);

Used to be 64 but became 80. I bisected to this change.

$ llvm-project/build/bin/clang++ -target aarch64 -c foo.c
clang++: warning: treating 'c' input as 'c++' when in C++ mode, this behavior is deprecated [-Wdeprecated]
foo.c:4:22: warning: default member initializer for bit-field is a C++20 extension [-Wc++20-extensions]
    4 |      bool value : 64 = false;
      |                      ^
foo.c:8:15: error: static assertion failed due to requirement 'sizeof(A) == 64'
    8 | static_assert(sizeof(A) == 64);
      |               ^~~~~~~~~~~~~~~
foo.c:8:25: note: expression evaluates to '80 == 64'
    8 | static_assert(sizeof(A) == 64);
      |               ~~~~~~~~~~^~~~~
1 warning and 1 error generated.
% clang++-19 -target aarch64 -c foo.c
clang++-19: warning: treating 'c' input as 'c++' when in C++ mode, this behavior is deprecated [-Wdeprecated]
foo.c:4:22: warning: default member initializer for bit-field is a C++20 extension [-Wc++20-extensions]
    4 |      bool value : 64 = false;
      |                      ^
1 warning generated.

@ostannard
Copy link
Collaborator Author

That looks like the expected consequence of this change:

  • Previously, the anonymous bitfield was erroneously given a 64-bit container, so it occupied 7 copies of the container type, starting at byte 8 of the struct and ending at 64.
  • Now, it is given a 128-bit container, as required by AAPCS64, so it needs 4 copies of the container type, starting at byte 16 and ending at 80.

GCC also gives this struct a size of 80 bytes.

@fmayer
Copy link
Contributor

fmayer commented May 1, 2025

Could we make sure this gets called out in the Release Notes? Even if it's a fix, this is a change of ABI which could break stuff.

@ostannard
Copy link
Collaborator Author

I'd assumed that this was a sufficiently uncommon edge-case that it wasn't worth a release note, but I'll keep that in mind in future and add release notes for things like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AArch64 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants