[clang] [CLANG-CL] ignores wpadded (PR #130182)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 6 13:59:35 PST 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Theo de Magalhaes (therealcoochieman)
<details>
<summary>Changes</summary>
Aims to fix #<!-- -->61702.
I encountered an interesting behavior during my testing. It seems the Itanium Record Layout Builder can give out Wpadded-bitfield warnings with the wrong amount of padding when mimicking the Microsoft struct layout.
For the following struct:
```cpp
struct __attribute__((ms_struct)) Foo {
long long x;
char y : 1;
long long yy : 1;
};
```
My understanding is we would expect `yy` to be padded by 63 bits, but the warning says there are 7 bytes of padding.
Which one is correct? I am not sure this question is within the scope of this PR but I am still shooting my shot here. I can always create an issue if needed.
---
Patch is 29.89 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/130182.diff
1 Files Affected:
- (modified) clang/lib/AST/RecordLayoutBuilder.cpp (+178-107)
``````````diff
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index b8600e6a344a4..fe8e29df9e271 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -43,7 +43,7 @@ struct BaseSubobjectInfo {
bool IsVirtual;
/// Bases - Information about the base subobjects.
- SmallVector<BaseSubobjectInfo*, 4> Bases;
+ SmallVector<BaseSubobjectInfo *, 4> Bases;
/// PrimaryVirtualBaseInfo - Holds the base info for the primary virtual base
/// of this base info (if one exists).
@@ -77,8 +77,7 @@ struct ExternalLayout {
/// Get the offset of the given field. The external source must provide
/// entries for all fields in the record.
uint64_t getExternalFieldOffset(const FieldDecl *FD) {
- assert(FieldOffsets.count(FD) &&
- "Field does not have an external offset");
+ assert(FieldOffsets.count(FD) && "Field does not have an external offset");
return FieldOffsets[FD];
}
@@ -167,16 +166,15 @@ class EmptySubobjectMap {
CharUnits SizeOfLargestEmptySubobject;
EmptySubobjectMap(const ASTContext &Context, const CXXRecordDecl *Class)
- : Context(Context), CharWidth(Context.getCharWidth()), Class(Class) {
- ComputeEmptySubobjectSizes();
+ : Context(Context), CharWidth(Context.getCharWidth()), Class(Class) {
+ ComputeEmptySubobjectSizes();
}
/// CanPlaceBaseAtOffset - Return whether the given base class can be placed
/// at the given offset.
/// Returns false if placing the record will result in two components
/// (direct or indirect) of the same type having the same offset.
- bool CanPlaceBaseAtOffset(const BaseSubobjectInfo *Info,
- CharUnits Offset);
+ bool CanPlaceBaseAtOffset(const BaseSubobjectInfo *Info, CharUnits Offset);
/// CanPlaceFieldAtOffset - Return whether a field can be placed at the given
/// offset.
@@ -227,9 +225,8 @@ void EmptySubobjectMap::ComputeEmptySubobjectSizes() {
}
}
-bool
-EmptySubobjectMap::CanPlaceSubobjectAtOffset(const CXXRecordDecl *RD,
- CharUnits Offset) const {
+bool EmptySubobjectMap::CanPlaceSubobjectAtOffset(const CXXRecordDecl *RD,
+ CharUnits Offset) const {
// We only need to check empty bases.
if (!RD->isEmpty())
return true;
@@ -265,9 +262,8 @@ void EmptySubobjectMap::AddSubobjectAtOffset(const CXXRecordDecl *RD,
MaxEmptyClassOffset = Offset;
}
-bool
-EmptySubobjectMap::CanPlaceBaseSubobjectAtOffset(const BaseSubobjectInfo *Info,
- CharUnits Offset) {
+bool EmptySubobjectMap::CanPlaceBaseSubobjectAtOffset(
+ const BaseSubobjectInfo *Info, CharUnits Offset) {
// We don't have to keep looking past the maximum offset that's known to
// contain an empty class.
if (!AnyEmptySubobjectsBeyondOffset(Offset))
@@ -368,10 +364,9 @@ bool EmptySubobjectMap::CanPlaceBaseAtOffset(const BaseSubobjectInfo *Info,
return true;
}
-bool
-EmptySubobjectMap::CanPlaceFieldSubobjectAtOffset(const CXXRecordDecl *RD,
- const CXXRecordDecl *Class,
- CharUnits Offset) const {
+bool EmptySubobjectMap::CanPlaceFieldSubobjectAtOffset(
+ const CXXRecordDecl *RD, const CXXRecordDecl *Class,
+ CharUnits Offset) const {
// We don't have to keep looking past the maximum offset that's known to
// contain an empty class.
if (!AnyEmptySubobjectsBeyondOffset(Offset))
@@ -418,9 +413,8 @@ EmptySubobjectMap::CanPlaceFieldSubobjectAtOffset(const CXXRecordDecl *RD,
return true;
}
-bool
-EmptySubobjectMap::CanPlaceFieldSubobjectAtOffset(const FieldDecl *FD,
- CharUnits Offset) const {
+bool EmptySubobjectMap::CanPlaceFieldSubobjectAtOffset(const FieldDecl *FD,
+ CharUnits Offset) const {
// We don't have to keep looking past the maximum offset that's known to
// contain an empty class.
if (!AnyEmptySubobjectsBeyondOffset(Offset))
@@ -560,7 +554,7 @@ void EmptySubobjectMap::UpdateEmptyFieldSubobjects(
}
}
-typedef llvm::SmallPtrSet<const CXXRecordDecl*, 4> ClassSetTy;
+typedef llvm::SmallPtrSet<const CXXRecordDecl *, 4> ClassSetTy;
class ItaniumRecordLayoutBuilder {
protected:
@@ -712,15 +706,13 @@ class ItaniumRecordLayoutBuilder {
bool FieldPacked, const FieldDecl *D);
void LayoutBitField(const FieldDecl *D);
- TargetCXXABI getCXXABI() const {
- return Context.getTargetInfo().getCXXABI();
- }
+ TargetCXXABI getCXXABI() const { return Context.getTargetInfo().getCXXABI(); }
/// BaseSubobjectInfoAllocator - Allocator for BaseSubobjectInfo objects.
llvm::SpecificBumpPtrAllocator<BaseSubobjectInfo> BaseSubobjectInfoAllocator;
typedef llvm::DenseMap<const CXXRecordDecl *, BaseSubobjectInfo *>
- BaseSubobjectInfoMapTy;
+ BaseSubobjectInfoMapTy;
/// VirtualBaseInfo - Map from all the (direct or indirect) virtual bases
/// of the class we're laying out to their base subobject info.
@@ -793,8 +785,8 @@ class ItaniumRecordLayoutBuilder {
uint64_t ComputedOffset);
void CheckFieldPadding(uint64_t Offset, uint64_t UnpaddedOffset,
- uint64_t UnpackedOffset, unsigned UnpackedAlign,
- bool isPacked, const FieldDecl *D);
+ uint64_t UnpackedOffset, unsigned UnpackedAlign,
+ bool isPacked, const FieldDecl *D);
DiagnosticBuilder Diag(SourceLocation Loc, unsigned DiagID);
@@ -965,8 +957,7 @@ BaseSubobjectInfo *ItaniumRecordLayoutBuilder::ComputeBaseSubobjectInfo(
// Traversing the bases must have created the base info for our primary
// virtual base.
PrimaryVirtualBaseInfo = VirtualBaseInfo.lookup(PrimaryVirtualBase);
- assert(PrimaryVirtualBaseInfo &&
- "Did not create a primary virtual base!");
+ assert(PrimaryVirtualBaseInfo && "Did not create a primary virtual base!");
// Claim the primary virtual base as our primary virtual base.
Info->PrimaryVirtualBaseInfo = PrimaryVirtualBaseInfo;
@@ -984,13 +975,12 @@ void ItaniumRecordLayoutBuilder::ComputeBaseSubobjectInfo(
const CXXRecordDecl *BaseDecl = I.getType()->getAsCXXRecordDecl();
// Compute the base subobject info for this base.
- BaseSubobjectInfo *Info = ComputeBaseSubobjectInfo(BaseDecl, IsVirtual,
- nullptr);
+ BaseSubobjectInfo *Info =
+ ComputeBaseSubobjectInfo(BaseDecl, IsVirtual, nullptr);
if (IsVirtual) {
// ComputeBaseInfo has already added this base for us.
- assert(VirtualBaseInfo.count(BaseDecl) &&
- "Did not add virtual base!");
+ assert(VirtualBaseInfo.count(BaseDecl) && "Did not add virtual base!");
} else {
// Add the base info to the map of non-virtual bases.
assert(!NonVirtualBaseInfo.count(BaseDecl) &&
@@ -1043,15 +1033,15 @@ void ItaniumRecordLayoutBuilder::LayoutNonVirtualBases(
LayoutVirtualBase(PrimaryBaseInfo);
} else {
BaseSubobjectInfo *PrimaryBaseInfo =
- NonVirtualBaseInfo.lookup(PrimaryBase);
+ NonVirtualBaseInfo.lookup(PrimaryBase);
assert(PrimaryBaseInfo &&
"Did not find base info for non-virtual primary base!");
LayoutNonVirtualBase(PrimaryBaseInfo);
}
- // If this class needs a vtable/vf-table and didn't get one from a
- // primary base, add it in now.
+ // If this class needs a vtable/vf-table and didn't get one from a
+ // primary base, add it in now.
} else if (RD->isDynamicClass()) {
assert(DataSize == 0 && "Vtable pointer must be at offset zero!");
CharUnits PtrWidth = Context.toCharUnitsFromBits(
@@ -1191,8 +1181,8 @@ void ItaniumRecordLayoutBuilder::LayoutVirtualBase(
// Add its base class offset.
assert(!VBases.count(Base->Class) && "vbase offset already exists!");
- VBases.insert(std::make_pair(Base->Class,
- ASTRecordLayout::VBaseInfo(Offset, false)));
+ VBases.insert(
+ std::make_pair(Base->Class, ASTRecordLayout::VBaseInfo(Offset, false)));
AddPrimaryVirtualBaseOffsets(Base, Offset);
}
@@ -1450,9 +1440,8 @@ void ItaniumRecordLayoutBuilder::LayoutFields(const RecordDecl *D) {
}
// Rounds the specified size to have it a multiple of the char size.
-static uint64_t
-roundUpSizeToCharAlignment(uint64_t Size,
- const ASTContext &Context) {
+static uint64_t roundUpSizeToCharAlignment(uint64_t Size,
+ const ASTContext &Context) {
uint64_t CharAlignment = Context.getTargetInfo().getCharAlign();
return llvm::alignTo(Size, CharAlignment);
}
@@ -1497,8 +1486,7 @@ void ItaniumRecordLayoutBuilder::LayoutWideBitField(uint64_t FieldSize,
uint64_t UnpaddedFieldOffset = getDataSizeInBits() - UnfilledBitsInLastUnit;
if (IsUnion) {
- uint64_t RoundedFieldSize = roundUpSizeToCharAlignment(FieldSize,
- Context);
+ uint64_t RoundedFieldSize = roundUpSizeToCharAlignment(FieldSize, Context);
setDataSize(std::max(getDataSizeInBits(), RoundedFieldSize));
FieldOffset = 0;
} else {
@@ -1648,7 +1636,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const FieldDecl *D) {
// Compute the next available bit offset.
uint64_t FieldOffset =
- IsUnion ? 0 : (getDataSizeInBits() - UnfilledBitsInLastUnit);
+ IsUnion ? 0 : (getDataSizeInBits() - UnfilledBitsInLastUnit);
// Handle targets that don't honor bitfield type alignment.
if (!IsMsStruct && !Context.getTargetInfo().useBitFieldTypeAlignment()) {
@@ -1666,7 +1654,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const FieldDecl *D) {
Context.getTargetInfo().getZeroLengthBitfieldBoundary();
FieldAlign = std::max(FieldAlign, ZeroLengthBitfieldBoundary);
}
- // If that doesn't apply, just ignore the field alignment.
+ // If that doesn't apply, just ignore the field alignment.
} else {
FieldAlign = 1;
}
@@ -1810,8 +1798,8 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const FieldDecl *D) {
}
setDataSize(std::max(getDataSizeInBits(), RoundedFieldSize));
- // For non-zero-width bitfields in ms_struct structs, allocate a new
- // storage unit if necessary.
+ // For non-zero-width bitfields in ms_struct structs, allocate a new
+ // storage unit if necessary.
} else if (IsMsStruct && FieldSize) {
// We should have cleared UnfilledBitsInLastUnit in every case
// where we changed storage units.
@@ -1960,13 +1948,13 @@ void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
}
}
- bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
- FieldClass->hasAttr<PackedAttr>() ||
- Context.getLangOpts().getClangABICompat() <=
- LangOptions::ClangABI::Ver15 ||
- Target.isPS() || Target.isOSDarwin() ||
- Target.isOSAIX())) ||
- D->hasAttr<PackedAttr>();
+ bool FieldPacked =
+ (Packed && (!FieldClass || FieldClass->isPOD() ||
+ FieldClass->hasAttr<PackedAttr>() ||
+ Context.getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver15 ||
+ Target.isPS() || Target.isOSDarwin() || Target.isOSAIX())) ||
+ D->hasAttr<PackedAttr>();
// When used as part of a typedef, or together with a 'packed' attribute, the
// 'aligned' attribute can be used to decrease alignment. In that case, it
@@ -2036,7 +2024,6 @@ void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
UnpackedFieldAlign = std::min(UnpackedFieldAlign, MaxFieldAlignment);
}
-
if (!FieldPacked)
FieldAlign = UnpackedFieldAlign;
if (DefaultsToAIXPowerAlignment)
@@ -2142,8 +2129,7 @@ void ItaniumRecordLayoutBuilder::FinishLayout(const NamedDecl *D) {
// array of zero-length, remains of Size 0
if (RD->isEmpty())
setSize(CharUnits::One());
- }
- else
+ } else
setSize(CharUnits::One());
}
@@ -2190,8 +2176,7 @@ void ItaniumRecordLayoutBuilder::FinishLayout(const NamedDecl *D) {
InBits = false;
}
Diag(RD->getLocation(), diag::warn_padded_struct_size)
- << Context.getTypeDeclType(RD)
- << PadSize
+ << Context.getTypeDeclType(RD) << PadSize
<< (InBits ? 1 : 0); // (byte|bit)
}
@@ -2270,7 +2255,8 @@ static unsigned getPaddingDiagFromTagKind(TagTypeKind Tag) {
return 1;
case TagTypeKind::Class:
return 2;
- default: llvm_unreachable("Invalid tag kind for field padding diagnostic!");
+ default:
+ llvm_unreachable("Invalid tag kind for field padding diagnostic!");
}
}
@@ -2313,10 +2299,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,
@@ -2340,7 +2326,7 @@ static const CXXMethodDecl *computeKeyFunction(ASTContext &Context,
return nullptr;
bool allowInlineFunctions =
- Context.getTargetInfo().getCXXABI().canKeyFunctionBeInline();
+ Context.getTargetInfo().getCXXABI().canKeyFunctionBeInline();
for (const CXXMethodDecl *MD : RD->methods()) {
if (!MD->isVirtual())
@@ -2560,6 +2546,7 @@ struct MicrosoftRecordLayoutBuilder {
private:
MicrosoftRecordLayoutBuilder(const MicrosoftRecordLayoutBuilder &) = delete;
void operator=(const MicrosoftRecordLayoutBuilder &) = delete;
+
public:
void layout(const RecordDecl *RD);
void cxxLayout(const CXXRecordDecl *RD);
@@ -2605,6 +2592,8 @@ struct MicrosoftRecordLayoutBuilder {
void computeVtorDispSet(
llvm::SmallPtrSetImpl<const CXXRecordDecl *> &HasVtorDispSet,
const CXXRecordDecl *RD) const;
+ void CheckFieldPadding(uint64_t Offset, uint64_t UnpaddedOffset,
+ const FieldDecl *D);
const ASTContext &Context;
EmptySubobjectMap *EmptySubobjects;
@@ -2642,8 +2631,6 @@ 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
@@ -2684,15 +2671,15 @@ MicrosoftRecordLayoutBuilder::getAdjustedElementInfo(
// the alignment in the case of pragma pack. Note that the required alignment
// doesn't actually apply to the struct alignment at this point.
Alignment = std::max(Alignment, Info.Alignment);
- RequiredAlignment = std::max(RequiredAlignment, Layout.getRequiredAlignment());
+ RequiredAlignment =
+ std::max(RequiredAlignment, Layout.getRequiredAlignment());
Info.Alignment = std::max(Info.Alignment, Layout.getRequiredAlignment());
Info.Size = Layout.getNonVirtualSize();
return Info;
}
MicrosoftRecordLayoutBuilder::ElementInfo
-MicrosoftRecordLayoutBuilder::getAdjustedElementInfo(
- const FieldDecl *FD) {
+MicrosoftRecordLayoutBuilder::getAdjustedElementInfo(const FieldDecl *FD) {
// Get the alignment of the field type's natural alignment, ignore any
// alignment attributes.
auto TInfo =
@@ -2715,8 +2702,8 @@ MicrosoftRecordLayoutBuilder::getAdjustedElementInfo(
FD->getType()->getBaseElementTypeUnsafe()->getAs<RecordType>()) {
auto const &Layout = Context.getASTRecordLayout(RT->getDecl());
EndsWithZeroSizedObject = Layout.endsWithZeroSizedObject();
- FieldRequiredAlignment = std::max(FieldRequiredAlignment,
- Layout.getRequiredAlignment());
+ FieldRequiredAlignment =
+ std::max(FieldRequiredAlignment, Layout.getRequiredAlignment());
}
// Capture required alignment as a side-effect.
RequiredAlignment = std::max(RequiredAlignment, FieldRequiredAlignment);
@@ -2778,10 +2765,11 @@ void MicrosoftRecordLayoutBuilder::initializeLayout(const RecordDecl *RD) {
MaxFieldAlignment = CharUnits::Zero();
// Honor the default struct packing maximum alignment flag.
if (unsigned DefaultMaxFieldAlignment = Context.getLangOpts().PackStruct)
- MaxFieldAlignment = CharUnits::fromQuantity(DefaultMaxFieldAlignment);
+ MaxFieldAlignment = CharUnits::fromQuantity(DefaultMaxFieldAlignment);
// Honor the packing attribute. The MS-ABI ignores pragma pack if its larger
// than the pointer size.
- if (const MaxFieldAlignmentAttr *MFAA = RD->getAttr<MaxFieldAlignmentAttr>()){
+ if (const MaxFieldAlignmentAttr *MFAA =
+ RD->getAttr<MaxFieldAlignmentAttr>()) {
unsigned PackedAlignment = MFAA->getAlignment();
if (PackedAlignment <=
Context.getTargetInfo().getPointerWidth(LangAS::Default))
@@ -2799,8 +2787,8 @@ void MicrosoftRecordLayoutBuilder::initializeLayout(const RecordDecl *RD) {
External.BaseOffsets, External.VirtualBaseOffsets);
}
-void
-MicrosoftRecordLayoutBuilder::initializeCXXLayout(const CXXRecordDecl *RD) {
+void MicrosoftRecordLayoutBuilder::initializeCXXLayout(
+ const CXXRecordDecl *RD) {
EndsWithZeroSizedObject = false;
LeadsWithZeroSizedBase = false;
HasOwnVFPtr = false;
@@ -2818,8 +2806,8 @@ MicrosoftRecordLayoutBuilder::initializeCXXLayout(const CXXRecordDecl *RD) {
PointerInfo.Alignment = std::min(PointerInfo.Alignment, MaxFieldAlignment);
}
-void
-MicrosoftRecordLayoutBuilder::layoutNonVirtualBases(const CXXRecordDecl *RD) {
+void MicrosoftRecordLayoutBuilder::layoutNonVirtualBases(
+ const CXXRecordDecl *RD) {
// The MS-ABI lays out all bases that contain leading vfptrs before it lays
// out any bases that do not contain vfptrs. We implement this as two passes
// over the bases. This approach guarantees that the primary base is laid out
@@ -3004,6 +2992,15 @@ void MicrosoftRecordLayoutBuilder::layoutField(const FieldDecl *FD) {
} else {
FieldOffset = Size.alignTo(Info.Alignment);
}
+
+ uint64_t UnpaddedFielddOffsetInBits =
+ Context.toBits(DataSize) - RemainingBitsInField;
+
+ CheckFieldPadding(Context.toBits(FieldOffset), UnpaddedFielddOffsetInBits,
+ FD);
+
+ RemainingBitsInField = 0;
+
placeFieldAtOffset(FieldOffset);
if (!IsOverlappingEmptyField)
@@ -3049,16 +3046,20 @@ 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.toBits(FieldOffset), UnpaddedFieldOffsetInBits,
+ FD);
}
DataSize = Size;
}
-void
-MicrosoftRecordLayoutBuilder::layoutZeroWidthBitField(const FieldDecl *FD) {
+void MicrosoftRecordLayoutBuilder::layoutZeroWidthBitField(
+ const FieldDecl *FD) {
// Zero-width bitfields are ignored unless they follow a non-zero-width
// bitfield.
if (!LastFieldIsNonZeroWidthBitfield) {
@@ -3076,9 +3077,14 @@ 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...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/130182
More information about the cfe-commits
mailing list