[CLANG][MS-STRUCT] bitfield padding warning presents padding to exact bit count

[theomagellan]

Aims to fix #131647.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Theo de Magalhaes (theomagellan)

Changes

Aims to fix #131647.

---

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

2 Files Affected:

• (modified) clang/lib/AST/RecordLayoutBuilder.cpp (+3-1)
• (modified) clang/test/SemaCXX/windows-Wpadded-bitfield.cpp (+15)

diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index ea353f88a8aec..ca08e186f4ff2 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1538,6 +1538,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const FieldDecl *D) {
   uint64_t StorageUnitSize = FieldInfo.Width;
   unsigned FieldAlign = FieldInfo.Align;
   bool AlignIsRequired = FieldInfo.isAlignRequired();
+  unsigned char PaddingInLastUnit = 0;
 
   // UnfilledBitsInLastUnit is the difference between the end of the
   // last allocated bitfield (i.e. the first bit offset available for
@@ -1610,6 +1611,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const FieldDecl *D) {
       if (!LastBitfieldStorageUnitSize && !FieldSize)
         FieldAlign = 1;
 
+      PaddingInLastUnit = UnfilledBitsInLastUnit;
       UnfilledBitsInLastUnit = 0;
       LastBitfieldStorageUnitSize = 0;
     }
@@ -1706,7 +1708,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const FieldDecl *D) {
   // For purposes of diagnostics, we're going to simultaneously
   // compute the field offsets that we would have used if we weren't
   // adding any alignment padding or if the field weren't packed.
-  uint64_t UnpaddedFieldOffset = FieldOffset;
+  uint64_t UnpaddedFieldOffset = FieldOffset - PaddingInLastUnit;
   uint64_t UnpackedFieldOffset = FieldOffset;
 
   // Check if we need to add padding to fit the bitfield within an
diff --git a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp b/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
index ee5a57124eca5..0b88b4c170617 100644
--- a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
+++ b/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -triple x86_64-windows-msvc -fsyntax-only -verify -Wpadded %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify -Wpadded %s
 
 struct __attribute__((ms_struct)) BitfieldStruct { // expected-warning {{padding size of 'BitfieldStruct' with 3 bytes to alignment boundary}}
   char c : 1;
@@ -24,9 +25,23 @@ struct __attribute__((ms_struct)) DifferentUnitSizeBitfield { // expected-warnin
   char i; // expected-warning {{padding struct 'DifferentUnitSizeBitfield' with 31 bits to align 'i'}}
 };
 
+struct __attribute__((ms_struct)) Foo { // expected-warning {{padding size of 'Foo' with 63 bits to alignment boundary}}
+  long long x;
+  char a : 1;
+  long long b : 1; // expected-warning {{padding struct 'Foo' with 63 bits to align 'b'}}
+};
+
+struct __attribute__((ms_struct)) SameUnitSizeMultiple { // expected-warning {{padding size of 'SameUnitSizeMultiple' with 2 bits to alignment boundary}}
+  char c : 1;
+  char cc : 2;
+  char ccc : 3;
+};
+
 int main() {
   BitfieldStruct b;
   SevenBitfieldStruct s;
   SameUnitSizeBitfield su;
   DifferentUnitSizeBitfield du;
+  Foo f;
+  SameUnitSizeMultiple susm;
 }

[theomagellan]

Hello @efriedma-quic, hope you don’t mind the ping.

This patch addresses the padded bitfield warning, which was reporting the wrong number of padded bits when using ms_struct and two consecutive bit-fields under the Itanium ABI.

I originally planned to merge SemaCXX/windows-Wpadded.cc and SemaCXX/windows-Wpadded-bitfield.cc into a single test file, but after some consideration, I think keeping them separate makes the coverage clearer. What do you think?

Thank you!

[efriedma-quic]

If I'm following correctly, this also affects non-ms_struct definitions? Can you also add test coverage for that?

I'm not sure people will be happy to see "padding struct 'Foo' with 1 bit to align 'xx'", but I guess we can see how it goes. Looks like gcc's -Wpadded does warn.

[theomagellan]

It doesn't affect non-ms_struct definitions! The new PaddingInLastUnit variable only gets a non-zero value when the following condition is met:
IsMsStruct && (LastBitfieldStorageUnitSize != StorageUnitSize || UnfilledBitsInLastUnit < FieldSize)
Otherwise, it remains zero and doesn't impact any calculations.

I could still add tests for that but I am not sure what they would look like as so far the tests only make sure both ABIs (MSVC and Itanium under ms_struct) produce the correct padding warnings.
I'm not entirely sure what new cases you'd like to see covered.
Do you have something specific in mind?

I gave the Foo structure a more descriptive name, thank you.

[efriedma-quic]

Oh, I see, I missed the nesting.

LGTM