Skip to content

[clang][NFC] Remove class layout scissor #89055

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 3 commits into from
May 12, 2024
Merged

Conversation

urnathan
Copy link
Contributor

Now that accumulateBitfields does the correct clipping, we don't need the scissor any more -- checkBitfieldClipping can compute its location directly.

@urnathan urnathan marked this pull request as ready for review April 17, 2024 21:16
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Apr 17, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 17, 2024

@llvm/pr-subscribers-clang

Author: Nathan Sidwell (urnathan)

Changes

Now that accumulateBitfields does the correct clipping, we don't need the scissor any more -- checkBitfieldClipping can compute its location directly.


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

1 Files Affected:

  • (modified) clang/lib/CodeGen/CGRecordLayoutBuilder.cpp (+11-11)
diff --git a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
index 868b1ab98e048a..38167903cda508 100644
--- a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
+++ b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
@@ -75,7 +75,7 @@ struct CGRecordLowering {
   // sentinel member type that ensures correct rounding.
   struct MemberInfo {
     CharUnits Offset;
-    enum InfoKind { VFPtr, VBPtr, Field, Base, VBase, Scissor } Kind;
+    enum InfoKind { VFPtr, VBPtr, Field, Base, VBase } Kind;
     llvm::Type *Data;
     union {
       const FieldDecl *FD;
@@ -197,7 +197,7 @@ struct CGRecordLowering {
                      const CXXRecordDecl *Query) const;
   void calculateZeroInit();
   CharUnits calculateTailClippingOffset(bool isNonVirtualBaseType) const;
-  void checkBitfieldClipping() const;
+  void checkBitfieldClipping(bool isNonVirtualBaseType) const;
   /// Determines if we need a packed llvm struct.
   void determinePacked(bool NVBaseType);
   /// Inserts padding everywhere it's needed.
@@ -299,8 +299,8 @@ void CGRecordLowering::lower(bool NVBaseType) {
       accumulateVBases();
   }
   llvm::stable_sort(Members);
+  checkBitfieldClipping(NVBaseType);
   Members.push_back(StorageInfo(Size, getIntNType(8)));
-  checkBitfieldClipping();
   determinePacked(NVBaseType);
   insertPadding();
   Members.pop_back();
@@ -894,8 +894,6 @@ CGRecordLowering::calculateTailClippingOffset(bool isNonVirtualBaseType) const {
 }
 
 void CGRecordLowering::accumulateVBases() {
-  Members.push_back(MemberInfo(calculateTailClippingOffset(false),
-                               MemberInfo::Scissor, nullptr, RD));
   for (const auto &Base : RD->vbases()) {
     const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
     if (BaseDecl->isEmpty())
@@ -950,18 +948,20 @@ void CGRecordLowering::calculateZeroInit() {
 }
 
 // Verify accumulateBitfields computed the correct storage representations.
-void CGRecordLowering::checkBitfieldClipping() const {
+void CGRecordLowering::checkBitfieldClipping(
+    bool IsNonVirtualBaseType LLVM_ATTRIBUTE_UNUSED) const {
 #ifndef NDEBUG
+  auto ScissorOffset = calculateTailClippingOffset(IsNonVirtualBaseType);
   auto Tail = CharUnits::Zero();
   for (const auto &M : Members) {
-    // Only members with data and the scissor can cut into tail padding.
-    if (!M.Data && M.Kind != MemberInfo::Scissor)
+    // Only members with data could possibly overlap.
+    if (!M.Data)
       continue;
 
     assert(M.Offset >= Tail && "Bitfield access unit is not clipped");
-    Tail = M.Offset;
-    if (M.Data)
-      Tail += getSize(M.Data);
+    Tail = M.Offset + getSize(M.Data);
+    assert((Tail <= ScissorOffset || M.Offset >= ScissorOffset) &&
+           "Bitfield straddles scissor offset");
   }
 #endif
 }

@llvmbot
Copy link
Member

llvmbot commented Apr 17, 2024

@llvm/pr-subscribers-clang-codegen

Author: Nathan Sidwell (urnathan)

Changes

Now that accumulateBitfields does the correct clipping, we don't need the scissor any more -- checkBitfieldClipping can compute its location directly.


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

1 Files Affected:

  • (modified) clang/lib/CodeGen/CGRecordLayoutBuilder.cpp (+11-11)
diff --git a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
index 868b1ab98e048a..38167903cda508 100644
--- a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
+++ b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
@@ -75,7 +75,7 @@ struct CGRecordLowering {
   // sentinel member type that ensures correct rounding.
   struct MemberInfo {
     CharUnits Offset;
-    enum InfoKind { VFPtr, VBPtr, Field, Base, VBase, Scissor } Kind;
+    enum InfoKind { VFPtr, VBPtr, Field, Base, VBase } Kind;
     llvm::Type *Data;
     union {
       const FieldDecl *FD;
@@ -197,7 +197,7 @@ struct CGRecordLowering {
                      const CXXRecordDecl *Query) const;
   void calculateZeroInit();
   CharUnits calculateTailClippingOffset(bool isNonVirtualBaseType) const;
-  void checkBitfieldClipping() const;
+  void checkBitfieldClipping(bool isNonVirtualBaseType) const;
   /// Determines if we need a packed llvm struct.
   void determinePacked(bool NVBaseType);
   /// Inserts padding everywhere it's needed.
@@ -299,8 +299,8 @@ void CGRecordLowering::lower(bool NVBaseType) {
       accumulateVBases();
   }
   llvm::stable_sort(Members);
+  checkBitfieldClipping(NVBaseType);
   Members.push_back(StorageInfo(Size, getIntNType(8)));
-  checkBitfieldClipping();
   determinePacked(NVBaseType);
   insertPadding();
   Members.pop_back();
@@ -894,8 +894,6 @@ CGRecordLowering::calculateTailClippingOffset(bool isNonVirtualBaseType) const {
 }
 
 void CGRecordLowering::accumulateVBases() {
-  Members.push_back(MemberInfo(calculateTailClippingOffset(false),
-                               MemberInfo::Scissor, nullptr, RD));
   for (const auto &Base : RD->vbases()) {
     const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
     if (BaseDecl->isEmpty())
@@ -950,18 +948,20 @@ void CGRecordLowering::calculateZeroInit() {
 }
 
 // Verify accumulateBitfields computed the correct storage representations.
-void CGRecordLowering::checkBitfieldClipping() const {
+void CGRecordLowering::checkBitfieldClipping(
+    bool IsNonVirtualBaseType LLVM_ATTRIBUTE_UNUSED) const {
 #ifndef NDEBUG
+  auto ScissorOffset = calculateTailClippingOffset(IsNonVirtualBaseType);
   auto Tail = CharUnits::Zero();
   for (const auto &M : Members) {
-    // Only members with data and the scissor can cut into tail padding.
-    if (!M.Data && M.Kind != MemberInfo::Scissor)
+    // Only members with data could possibly overlap.
+    if (!M.Data)
       continue;
 
     assert(M.Offset >= Tail && "Bitfield access unit is not clipped");
-    Tail = M.Offset;
-    if (M.Data)
-      Tail += getSize(M.Data);
+    Tail = M.Offset + getSize(M.Data);
+    assert((Tail <= ScissorOffset || M.Offset >= ScissorOffset) &&
+           "Bitfield straddles scissor offset");
   }
 #endif
 }

@urnathan
Copy link
Contributor Author

urnathan commented May 3, 2024

ping?

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

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

Might be useful to mention #87090 in the commit message (first comment).

@urnathan urnathan force-pushed the push/scissor-kill branch from 2d2dcde to 4b93593 Compare May 10, 2024 17:54
@urnathan urnathan merged commit 1d6bf0c into llvm:main May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants