[CLANG-CL] ignores wpadded #130182
Merged
[CLANG-CL] ignores wpadded #130182
+128
−11
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Theo de Magalhaes (therealcoochieman) ChangesAims 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: struct __attribute__((ms_struct)) Foo {
long long x;
char y : 1;
long long yy : 1;
};My understanding is we would expect 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:
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]
|
|
Please make sure to keep the formatting of the file as it was (you can format the changes you made to the file with Thanks! |
Hello, I misread the guidelines and thought I had to make a first clang-format commit before editing the file. I reverted the commit. |
llvmbot
added
the
clang:driver
|
Hello @cor3ntin @AaronBallman! I think this PR is ready for review. I did not know who to ping exactly, sorry if you were not the correct people for this. I added a test in Thank you for your time! |
AaronBallman
requested review from
cor3ntin,
AaronBallman,
erichkeane and
Sirraide
erichkeane
reviewed
Mar 14, 2025
There was a problem hiding this comment.
This seems reasonable to me, but frankly, the record layout builder stuff is far from my expertise. @rjmccall and @efriedma-quic both touched it in the last... 15 years, as did @majnemer at one point. Perhaps they can confirm here too?
|
Yeah, I’m also not the right person to review anything ABI-related... |
|
I'd like to see significantly more test coverage. Combinations of stuff like base classes, unions, zero-length bitfields, fields explicitly aligned with Note a test can have multiple RUN lines, so you can check both Itanium and Microsoft ABI with the same test with different target triples. Most tests are going to produce the same warnings, so sharing the test file might make the intent easier to understand. Code itself seems mostly fine. Maybe we can make "CheckFieldPadding" a static helper function instead of copy-pasting it.
I think what's happening is just that it isn't keep track of the padding it inserts after "y": there's a bit of code near the beginning of ItaniumRecordLayoutBuilder::LayoutBitField which handles ms_struct rules for bitfield layout. Please file a bug. (Please edit this question out of the pull description, since we don't need this to be in the commit log.) |
theomagellan
force-pushed
the
clang-cl-ignores-Wpadded
branch
from
6401e68 to
ccca2f8
Compare
|
Hello @efriedma-quic, thank you for your input.
Yes, thank you. Please note the test |
| @@ -0,0 +1,12 @@ | |||
| // RUN: %clang_cl -Wpadded -Wno-msvc-not-found -fsyntax-only -- %s 2>&1 | FileCheck -check-prefix=WARN %s | |||
There was a problem hiding this comment.
Put tests in clang/test/SemaCXX . Use a RUN line like RUN: %clang_cc1 -triple=x86_64-windows-msvc -fsyntax-only -verify -Wpadded %s. Use "expected-warning" functionality from -verify instead of FileCheck'ing the output. Combine the tests into one file if you can.
There was a problem hiding this comment.
I have merged every test inside the same file (clang/test/SemaCXX/windows-Wpadded.cpp) except for the one dealing with Bitfields as it cannot be tested with both target triples. This test is in a separate file.
Thank you for your patience.
There was a problem hiding this comment.
Hello @efriedma-quic, I hope you don’t mind the ping.
While waiting for your response, I’ve added more bitfield tests.
I’ve also fixed #131647 in a different branch. Once these tests are validated, I’ll rebase this PR onto that branch and merge the two test files.
Would you like me to notify you directly when the next PR is ready for review?
Thank you.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Oh, sorry, one more thing: needs a release note (modify clang/docs/ReleaseNotes.rst). |
…der.cpp" This reverts commit 3a9bc90.
theomagellan
force-pushed
the
clang-cl-ignores-Wpadded
branch
from
549b8ce to
721da29
Compare
|
Hello @efriedma-quic, I hope you don't mind the ping. I have also updated this PR to the latest version of the main branch to date. Thank you. |
|
@therealcoochieman Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
If I compile clang with clang (18.1.8) the new windows-Wpadded.cpp testcase fails for me like However, if I compile clang with gcc (13.3.0) and run the testcase it does not fail. Am I the only one seeing this? |
No idea if this is relevant or if it's just false positives but if I run valgrind on the clang command for the windows-Wpadded.cpp testcase it complains with |
|
I'm not sure if the automation that reports failures from buildbots back to the PR got broken, but I can report that this seems to fail for the RISC-V RVA23 config (vector enabled) https://lab.llvm.org/buildbot/#/builders/132/builds/597 but not the RVA20 build done at the same time https://lab.llvm.org/buildbot/#/builders/87/builds/750 The error on the RVA23 builder is: |
mikaelholmen
reviewed
Apr 3, 2025
| @@ -3004,6 +3010,15 @@ void MicrosoftRecordLayoutBuilder::layoutField(const FieldDecl *FD) { | |||
| } else { | |||
| FieldOffset = Size.alignTo(Info.Alignment); | |||
| } | |||
|
|
|||
| uint64_t UnpaddedFielddOffsetInBits = | |||
| Context.toBits(DataSize) - RemainingBitsInField; | |||
There was a problem hiding this comment.
Could it be that RemainingBitsInField is not always initialized here?
When I add some printouts and compare a failing and passing build I see that when it fails RemainingBitsInField is 2767481344 which looks a bit odd.
There was a problem hiding this comment.
Hello, this is also my main suspicion. I will fix this later today.
Thank you for your thoroughness, I did not think to check with other compilers or do a valgrind check. Sorry for that.
There was a problem hiding this comment.
Thank you both! In line with our patch reversion policy https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy I've gone ahead and reverted so that the issue can be fixed without undue time pressure, and so bots can go green again in the meanwhile.
There was a problem hiding this comment.
Do we know why the failed bots didn't show up as comments in this PR or is that something someone/somewhere should look into?
There was a problem hiding this comment.
I can't see an obvious reason why that didn't happen here - I've filed a bug on llvm-zorg llvm/llvm-zorg#427
There was a problem hiding this comment.
Ah, there was an obvious reason. The GitHub PR reporting only kicks in if a builder built with this as the only commit that changed since the last build. The failing builders had more PRs than just this one in their changelist, so no comment on GitHub.
|
Failing on ppc64 too https://lab.llvm.org/buildbot/#/builders/95/builds/11539 |
asb
added a commit
that referenced
this pull request
Apr 3, 2025
Reverts #130182 This is causing failures on RISC-V and ppc builders as mentioned on #130182 (comment) Reverting so the issue can be fixed by the path author without time pressure (as noted in that PR, it seems a value is uninitialised).
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Apr 3, 2025
Reverts llvm/llvm-project#130182 This is causing failures on RISC-V and ppc builders as mentioned on llvm/llvm-project#130182 (comment) Reverting so the issue can be fixed by the path author without time pressure (as noted in that PR, it seems a value is uninitialised).
|
I didn't receive any emails about the buildbot failures. Not sure if that's intentional, or if something broke. |
|
This seems to be failing on the msan Buildbot: https://lab.llvm.org/buildbot/#/builders/169/builds/10068 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Aims to fix #61702.
Implements the
-Wpaddedwarning for clang-cl.