Revert "[CLANG-CL] ignores wpadded"

[asb]

Reverts #130182

This is causing failures on RISC-V and ppc builders as mentioned on #130182 (comment)

Posting PR to check it reverts cleanly and pre-commit CI is happy. I'm happy with alternate fixes that don't require a revert, but getting this in the pipeline as one option.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Alex Bradbury (asb)

Changes

Reverts llvm/llvm-project#130182

This is causing failures on RISC-V and ppc builders as mentioned on #130182 (comment)

Posting PR to check it reverts cleanly and pre-commit CI is happy. I'm happy with alternate fixes that don't require a revert, but getting this in the pipeline as one option.

CC @therealcoochieman

---

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

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (-2)
• (modified) clang/lib/AST/RecordLayoutBuilder.cpp (+11-54)
• (removed) clang/test/SemaCXX/windows-Wpadded-bitfield.cpp (-32)
• (removed) clang/test/SemaCXX/windows-Wpadded.cpp (-40)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 47f9c3caa0e47..fdf9a246d6373 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -190,8 +190,6 @@ Modified Compiler Flags
 
 - The compiler flag `-fbracket-depth` default value is increased from 256 to 2048. (#GH94728)
 
-- `-Wpadded` option implemented for the `x86_64-windows-msvc` target. Fixes #61702
-
 Removed Compiler Flags
 -------------------------
 
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index 41e7198cb7581..3e756ab9b9bfe 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2274,9 +2274,9 @@ static unsigned getPaddingDiagFromTagKind(TagTypeKind Tag) {
   }
 }
 
-static void CheckFieldPadding(const ASTContext &Context, bool IsUnion,
-                              uint64_t Offset, uint64_t UnpaddedOffset,
-                              const FieldDecl *D) {
+void ItaniumRecordLayoutBuilder::CheckFieldPadding(
+    uint64_t Offset, uint64_t UnpaddedOffset, uint64_t UnpackedOffset,
+    unsigned UnpackedAlign, bool isPacked, const FieldDecl *D) {
   // We let objc ivars without warning, objc interfaces generally are not used
   // for padding tricks.
   if (isa<ObjCIvarDecl>(D))
@@ -2300,8 +2300,7 @@ static void CheckFieldPadding(const ASTContext &Context, bool IsUnion,
     if (D->getIdentifier()) {
       auto Diagnostic = D->isBitField() ? diag::warn_padded_struct_bitfield
                                         : diag::warn_padded_struct_field;
-      Context.getDiagnostics().Report(D->getLocation(),
-                                      Diagnostic)
+      Diag(D->getLocation(), Diagnostic)
           << getPaddingDiagFromTagKind(D->getParent()->getTagKind())
           << Context.getTypeDeclType(D->getParent()) << PadSize
           << (InBits ? 1 : 0) // (byte|bit)
@@ -2309,22 +2308,15 @@ static void CheckFieldPadding(const ASTContext &Context, bool IsUnion,
     } else {
       auto Diagnostic = D->isBitField() ? diag::warn_padded_struct_anon_bitfield
                                         : diag::warn_padded_struct_anon_field;
-      Context.getDiagnostics().Report(D->getLocation(),
-                                      Diagnostic)
+      Diag(D->getLocation(), Diagnostic)
           << getPaddingDiagFromTagKind(D->getParent()->getTagKind())
           << Context.getTypeDeclType(D->getParent()) << PadSize
           << (InBits ? 1 : 0); // (byte|bit)
     }
-  }
-}
-
-void ItaniumRecordLayoutBuilder::CheckFieldPadding(
-    uint64_t Offset, uint64_t UnpaddedOffset, uint64_t UnpackedOffset,
-    unsigned UnpackedAlign, bool isPacked, const FieldDecl *D) {
-  ::CheckFieldPadding(Context, IsUnion, Offset, UnpaddedOffset, D);
-  if (isPacked && Offset != UnpackedOffset) {
-    HasPackedField = true;
-  }
+ }
+ if (isPacked && Offset != UnpackedOffset) {
+   HasPackedField = true;
+ }
 }
 
 static const CXXMethodDecl *computeKeyFunction(ASTContext &Context,
@@ -2650,6 +2642,8 @@ struct MicrosoftRecordLayoutBuilder {
   /// virtual base classes and their offsets in the record.
   ASTRecordLayout::VBaseOffsetsMapTy VBases;
   /// The number of remaining bits in our last bitfield allocation.
+  /// This value isn't meaningful unless LastFieldIsNonZeroWidthBitfield is
+  /// true.
   unsigned RemainingBitsInField;
   bool IsUnion : 1;
   /// True if the last field laid out was a bitfield and was not 0
@@ -3010,15 +3004,6 @@ void MicrosoftRecordLayoutBuilder::layoutField(const FieldDecl *FD) {
   } else {
     FieldOffset = Size.alignTo(Info.Alignment);
   }
-
-  uint64_t UnpaddedFielddOffsetInBits =
-      Context.toBits(DataSize) - RemainingBitsInField;
-
-  ::CheckFieldPadding(Context, IsUnion, Context.toBits(FieldOffset),
-                      UnpaddedFielddOffsetInBits, FD);
-
-  RemainingBitsInField = 0;
-
   placeFieldAtOffset(FieldOffset);
 
   if (!IsOverlappingEmptyField)
@@ -3064,14 +3049,10 @@ void MicrosoftRecordLayoutBuilder::layoutBitField(const FieldDecl *FD) {
   } else {
     // Allocate a new block of memory and place the bitfield in it.
     CharUnits FieldOffset = Size.alignTo(Info.Alignment);
-    uint64_t UnpaddedFieldOffsetInBits =
-        Context.toBits(DataSize) - RemainingBitsInField;
     placeFieldAtOffset(FieldOffset);
     Size = FieldOffset + Info.Size;
     Alignment = std::max(Alignment, Info.Alignment);
     RemainingBitsInField = Context.toBits(Info.Size) - Width;
-    ::CheckFieldPadding(Context, IsUnion, Context.toBits(FieldOffset),
-                        UnpaddedFieldOffsetInBits, FD);
   }
   DataSize = Size;
 }
@@ -3095,14 +3076,9 @@ MicrosoftRecordLayoutBuilder::layoutZeroWidthBitField(const FieldDecl *FD) {
   } else {
     // Round up the current record size to the field's alignment boundary.
     CharUnits FieldOffset = Size.alignTo(Info.Alignment);
-    uint64_t UnpaddedFieldOffsetInBits =
-        Context.toBits(DataSize) - RemainingBitsInField;
     placeFieldAtOffset(FieldOffset);
-    RemainingBitsInField = 0;
     Size = FieldOffset;
     Alignment = std::max(Alignment, Info.Alignment);
-    ::CheckFieldPadding(Context, IsUnion, Context.toBits(FieldOffset),
-                        UnpaddedFieldOffsetInBits, FD);
   }
   DataSize = Size;
 }
@@ -3227,9 +3203,6 @@ void MicrosoftRecordLayoutBuilder::layoutVirtualBases(const CXXRecordDecl *RD) {
 }
 
 void MicrosoftRecordLayoutBuilder::finalizeLayout(const RecordDecl *RD) {
-  uint64_t UnpaddedSizeInBits = Context.toBits(DataSize);
-  UnpaddedSizeInBits -= RemainingBitsInField;
-
   // Respect required alignment.  Note that in 32-bit mode Required alignment
   // may be 0 and cause size not to be updated.
   DataSize = Size;
@@ -3258,22 +3231,6 @@ void MicrosoftRecordLayoutBuilder::finalizeLayout(const RecordDecl *RD) {
     Size = Context.toCharUnitsFromBits(External.Size);
     if (External.Align)
       Alignment = Context.toCharUnitsFromBits(External.Align);
-    return;
-  }
-  unsigned CharBitNum = Context.getTargetInfo().getCharWidth();
-  uint64_t SizeInBits = Context.toBits(Size);
-  if (SizeInBits > UnpaddedSizeInBits) {
-    unsigned int PadSize = SizeInBits - UnpaddedSizeInBits;
-    bool InBits = true;
-    if (PadSize % CharBitNum == 0) {
-      PadSize = PadSize / CharBitNum;
-      InBits = false;
-    }
-
-    Context.getDiagnostics().Report(RD->getLocation(),
-                                    diag::warn_padded_struct_size)
-        << Context.getTypeDeclType(RD) << PadSize
-        << (InBits ? 1 : 0); // (byte|bit)
   }
 }
 
diff --git a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp b/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
deleted file mode 100644
index ee5a57124eca5..0000000000000
--- a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
+++ /dev/null
@@ -1,32 +0,0 @@
-// RUN: %clang_cc1 -triple x86_64-windows-msvc -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;
-  int : 0; // expected-warning {{padding struct 'BitfieldStruct' with 31 bits to align anonymous bit-field}}
-  char i;
-};
-
-struct __attribute__((ms_struct)) SevenBitfieldStruct { // expected-warning {{padding size of 'SevenBitfieldStruct' with 3 bytes to alignment boundary}}
-  char c : 7;
-  int : 0; // expected-warning {{padding struct 'SevenBitfieldStruct' with 25 bits to align anonymous bit-field}}
-  char i;
-};
-
-struct __attribute__((ms_struct)) SameUnitSizeBitfield {
-  char c : 7;
-  char : 1; // Same unit size attributes fall in the same unit + they fill the unit -> no padding
-  char i;
-};
-
-struct __attribute__((ms_struct)) DifferentUnitSizeBitfield { // expected-warning {{padding size of 'DifferentUnitSizeBitfield' with 3 bytes to alignment boundary}}
-  char c : 7;
-  int : 1; // expected-warning {{padding struct 'DifferentUnitSizeBitfield' with 25 bits to align anonymous bit-field}}
-  char i; // expected-warning {{padding struct 'DifferentUnitSizeBitfield' with 31 bits to align 'i'}}
-};
-
-int main() {
-  BitfieldStruct b;
-  SevenBitfieldStruct s;
-  SameUnitSizeBitfield su;
-  DifferentUnitSizeBitfield du;
-}
diff --git a/clang/test/SemaCXX/windows-Wpadded.cpp b/clang/test/SemaCXX/windows-Wpadded.cpp
deleted file mode 100644
index da3f2bf08c6b8..0000000000000
--- a/clang/test/SemaCXX/windows-Wpadded.cpp
+++ /dev/null
@@ -1,40 +0,0 @@
-// 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)) Foo { // expected-warning {{padding size of 'Foo' with 3 bytes to alignment boundary}}
-  int b : 1;
-  char a; // expected-warning {{padding struct 'Foo' with 31 bits to align 'a'}}
-};
-
-struct __attribute__((ms_struct)) AlignedStruct { // expected-warning {{padding size of 'AlignedStruct' with 4 bytes to alignment boundary}}
-  char c;
-  alignas(8) int i; // expected-warning {{padding struct 'AlignedStruct' with 7 bytes to align 'i'}}
-};
-
-
-struct Base {
-  int b;
-};
-
-struct Derived : public Base { // expected-warning {{padding size of 'Derived' with 3 bytes to alignment boundary}}
-  char c;
-};
-
-union __attribute__((ms_struct)) Union {
-  char c;
-  long long u;
-};
-
-struct __attribute__((ms_struct)) StructWithUnion { // expected-warning {{padding size of 'StructWithUnion' with 6 bytes to alignment boundary}}
-  char c;
-  int : 0;
-  Union t; // expected-warning {{padding struct 'StructWithUnion' with 7 bytes to align 't'}}
-  short i;
-};
-
-int main() {
-  Foo f;
-  AlignedStruct a;
-  Derived d;
-  StructWithUnion swu;
-}

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions cpp -- clang/lib/AST/RecordLayoutBuilder.cpp

View the diff from clang-format here.

diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index 3e756ab9b..d00cc2ec2 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2313,10 +2313,10 @@ void ItaniumRecordLayoutBuilder::CheckFieldPadding(
           << Context.getTypeDeclType(D->getParent()) << PadSize
           << (InBits ? 1 : 0); // (byte|bit)
     }
- }
- if (isPacked && Offset != UnpackedOffset) {
-   HasPackedField = true;
- }
+  }
+  if (isPacked && Offset != UnpackedOffset) {
+    HasPackedField = true;
+  }
 }
 
 static const CXXMethodDecl *computeKeyFunction(ASTContext &Context,

[asb]

Pre-commit tests show this reverts cleanly. I'll commit directly in line with our patch reversion policy https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy