https://github.com/erichkeane updated https://github.com/llvm/llvm-project/pull/206820
>From dffed03f6c4d46f4163a3794a3514da4ab39e3bc Mon Sep 17 00:00:00 2001 From: erichkeane <[email protected]> Date: Tue, 30 Jun 2026 13:05:24 -0700 Subject: [PATCH 1/4] Make -Wpadded cause us to check the layout immediately, always checking This matches the GCC behavior, which checks every time with -Wpadded or one of its associated diagnostics. This patch makes sure we check these immediately after we complete a record. Since we are also trying to combine the logic with some existing work with -fdump-record-layouts-complete, this also ends up fixing a bug where the layout would be generated wrong since the attributes weren't applied yet. Fixes: #15711 --- clang/docs/ReleaseNotes.md | 5 +++++ clang/include/clang/Basic/Diagnostic.h | 11 +++++++++++ clang/lib/AST/Decl.cpp | 13 ------------- clang/lib/Sema/SemaDecl.cpp | 24 ++++++++++++++++++++++-- clang/test/SemaCXX/windows-Wpadded.cpp | 19 +++++++++++++++++++ 5 files changed, 57 insertions(+), 15 deletions(-) diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index 26d5d08979391..fa3e47c21f785 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -661,6 +661,10 @@ latest release, please see the [Clang Web Site](https://clang.llvm.org) or the by the kernel, so setting them is almost always a typo (matching the bionic libc `diagnose_if` check). +- `-Wpadded` now works even when a type is not referenced (that is, it is + triggered on completion rather than first reference) and in `-fsyntax-only` + mode (#GH15711). + ### Improvements to Clang's time-trace ### Improvements to Coverage Mapping @@ -725,6 +729,7 @@ latest release, please see the [Clang Web Site](https://clang.llvm.org) or the - Fixed an assertion where we improperly handled implicit conversions to integral types from an atomic-type with a conversion function. (#GH201770) - Fixed assertion failures involving code completion with delayed default arguments and exception specifications. (#GH200879) - Fixed a regression where calling a function that takes a class-type parameter by value inside `decltype` of a concept could be incorrectly rejected when used as a non-type template argument. (#GH175831) +- Fixed a bug where `-fdump-record-layouts-complete` would apply the layout checking before it had its trailing attributes applied, which would make combining it with a `packed` attribute cause the wrong layout (#GH15711) #### Bug Fixes to Compiler Builtins diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h index 826b747f2c751..961560fedd16a 100644 --- a/clang/include/clang/Basic/Diagnostic.h +++ b/clang/include/clang/Basic/Diagnostic.h @@ -962,6 +962,17 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> { diag::Severity::Ignored; } + bool areAllIgnored(StringRef Group, SourceLocation Loc) const { + llvm::SmallVector<diag::kind> diagsInGroup; + bool Failed = Diags->getDiagnosticsInGroup(diag::Flavor::WarningOrError, + Group, diagsInGroup); + assert(!Failed && "Incorrect group name?"); + (void)Failed; + return llvm::all_of(diagsInGroup, [&](diag::kind DiagID) { + return isIgnored(DiagID, Loc); + }); + } + /// Based on the way the client configured the DiagnosticsEngine /// object, classify the specified diagnostic ID into a Level, consumable by /// the DiagnosticConsumer. diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp index b23bf73ae803c..974eb74d9e190 100644 --- a/clang/lib/AST/Decl.cpp +++ b/clang/lib/AST/Decl.cpp @@ -5288,19 +5288,6 @@ RecordDecl::field_iterator RecordDecl::noload_field_begin() const { void RecordDecl::completeDefinition() { assert(!isCompleteDefinition() && "Cannot redefine record!"); TagDecl::completeDefinition(); - - ASTContext &Ctx = getASTContext(); - - // Layouts are dumped when computed, so if we are dumping for all complete - // types, we need to force usage to get types that wouldn't be used elsewhere. - // - // If the type is dependent, then we can't compute its layout because there - // is no way for us to know the size or alignment of a dependent type. Also - // ignore declarations marked as invalid since 'getASTRecordLayout()' asserts - // on that. - if (Ctx.getLangOpts().DumpRecordLayoutsComplete && !isDependentType() && - !isInvalidDecl()) - (void)Ctx.getASTRecordLayout(this); } /// isMsStruct - Get whether or not this record uses ms_struct layout. diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index d45c3eb35094f..fc257735992cc 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -20279,8 +20279,28 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl, // Handle attributes before checking the layout. ProcessDeclAttributeList(S, Record, Attrs); + // Layouts are dumped when computed, so if we are dumping for all complete + // types, we need to force usage to get types that wouldn't be used + // elsewhere. + // + // If the type is dependent, then we can't compute its layout because there + // is no way for us to know the size or alignment of a dependent type. Also + // ignore declarations marked as invalid since 'getASTRecordLayout()' + // asserts on that. + // + // We also want to trigger -Wpadded and associated warnings even at + // -fsyntax-only time, and when they aren't referenced. So we need to + // trigger this if any of those diagnostics are enabled for the start of + // this. + if (!Record->isDependentType() && !Record->isInvalidDecl()) { + if (getLangOpts().DumpRecordLayoutsComplete || + !getDiagnostics().areAllIgnored("padded", Record->getBeginLoc())) + (void)getASTContext().getASTRecordLayout(Record); + } + // Maybe randomize the record's decls. We automatically randomize a record - // of function pointers, unless it has the "no_randomize_layout" attribute. + // of function pointers, unless it has the "no_randomize_layout" + // attribute. if (!getLangOpts().CPlusPlus && !getLangOpts().RandstructSeed.empty() && !Record->isRandomized() && !Record->isUnion() && (Record->hasAttr<RandomizeLayoutAttr>() || @@ -20290,7 +20310,7 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl, if (randstruct::randomizeStructureLayout(Context, Record, NewDeclOrdering)) Record->reorderDecls(NewDeclOrdering); - } + } // We may have deferred checking for a deleted destructor. Check now. if (CXXRecord) { diff --git a/clang/test/SemaCXX/windows-Wpadded.cpp b/clang/test/SemaCXX/windows-Wpadded.cpp index fc170b9220cdc..b235aae37b1d2 100644 --- a/clang/test/SemaCXX/windows-Wpadded.cpp +++ b/clang/test/SemaCXX/windows-Wpadded.cpp @@ -1,5 +1,7 @@ // 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 +// RUN: %clang_cc1 -triple x86_64-windows-msvc -fsyntax-only -verify -Wpadded %s -DUSED +// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify -Wpadded %s -DUSED struct __attribute__((ms_struct)) Foo { // expected-warning {{padding size of 'Foo' with 3 bytes to alignment boundary}} int b : 1; @@ -45,7 +47,22 @@ struct alignas(32) __attribute__((ms_struct)) AlignedNonEmptyStruct { // expecte struct alignas(16) __attribute__((ms_struct)) AlignedEmptyStruct {}; // expected-warning {{padding size of 'AlignedEmptyStruct' with 15 bytes to alignment boundary}} +#define ETH_ALEN 14 +struct ethhdr { + unsigned char h_dest[ETH_ALEN]; + unsigned char h_source[ETH_ALEN]; + unsigned short h_proto; +} __attribute__((packed)); // Should suppress the diagnostic. + + +struct arc_eth_encap { + unsigned char proto; + struct ethhdr eth; + unsigned char payload[]; +}; + int main() { +#ifdef USED Foo f; AlignedStruct a; Derived d; @@ -54,4 +71,6 @@ int main() { AlignedNonEmptyStruct anes; AlignedMemberStruct ams; AlignedEmptyStruct aes; + arc_eth_encap aee; +#endif } >From 8e7662e022a3a339e22b053667257491635ad29b Mon Sep 17 00:00:00 2001 From: erichkeane <[email protected]> Date: Tue, 30 Jun 2026 13:11:24 -0700 Subject: [PATCH 2/4] Run clang-format --- clang/lib/Sema/SemaDecl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index fc257735992cc..ab5bf187eaca9 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -20310,7 +20310,7 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl, if (randstruct::randomizeStructureLayout(Context, Record, NewDeclOrdering)) Record->reorderDecls(NewDeclOrdering); - } + } // We may have deferred checking for a deleted destructor. Check now. if (CXXRecord) { >From db802a23374b5f7b745f3153683748415863f3cc Mon Sep 17 00:00:00 2001 From: erichkeane <[email protected]> Date: Wed, 1 Jul 2026 13:45:38 -0700 Subject: [PATCH 3/4] Split up the getDiagnosticSeverity to a function that can re-use the GetDiagStateForLoc when checking multiples at once as requested --- clang/include/clang/Basic/Diagnostic.h | 5 +- clang/include/clang/Basic/DiagnosticIDs.h | 6 + clang/lib/Basic/DiagnosticIDs.cpp | 196 ++++++++++++---------- 3 files changed, 120 insertions(+), 87 deletions(-) diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h index 961560fedd16a..3a685f9033d8f 100644 --- a/clang/include/clang/Basic/Diagnostic.h +++ b/clang/include/clang/Basic/Diagnostic.h @@ -968,9 +968,8 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> { Group, diagsInGroup); assert(!Failed && "Incorrect group name?"); (void)Failed; - return llvm::all_of(diagsInGroup, [&](diag::kind DiagID) { - return isIgnored(DiagID, Loc); - }); + return Diags->getDiagnosticListHighestSeverity(diagsInGroup, Loc, *this) == + diag::Severity::Ignored; } /// Based on the way the client configured the DiagnosticsEngine diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h index 63b5e6a28aac0..6f0fda8d5608a 100644 --- a/clang/include/clang/Basic/DiagnosticIDs.h +++ b/clang/include/clang/Basic/DiagnosticIDs.h @@ -514,6 +514,12 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> { getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc, const DiagnosticsEngine &Diag) const LLVM_READONLY; + /// Given a collection of diagnostic IDs, get the 'highest' severity of them + /// at the provided location for this DiagnosticsEngine. + diag::Severity getDiagnosticListHighestSeverity( + llvm::ArrayRef<diag::kind> DiagIDs, SourceLocation Loc, + const DiagnosticsEngine &Diag) const LLVM_READONLY; + Class getDiagClass(unsigned DiagID) const; /// Whether the diagnostic may leave the AST in a state where some diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp index d9c5e4082c1a7..2b2448f6d1177 100644 --- a/clang/lib/Basic/DiagnosticIDs.cpp +++ b/clang/lib/Basic/DiagnosticIDs.cpp @@ -538,103 +538,131 @@ DiagnosticIDs::getDiagnosticLevel(unsigned DiagID, SourceLocation Loc, diag::Severity DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc, const DiagnosticsEngine &Diag) const { - bool IsCustomDiag = DiagnosticIDs::IsCustomDiag(DiagID); - assert(getDiagClass(DiagID) != CLASS_NOTE); - - // Specific non-error diagnostics may be mapped to various levels from ignored - // to error. Errors can only be mapped to fatal. - diag::Severity Result = diag::Severity::Fatal; + return getDiagnosticListHighestSeverity({DiagID}, Loc, Diag); +} - // Get the mapping information, or compute it lazily. +diag::Severity DiagnosticIDs::getDiagnosticListHighestSeverity( + llvm::ArrayRef<diag::kind> DiagIDs, SourceLocation Loc, + const DiagnosticsEngine &Diag) const { DiagnosticsEngine::DiagState *State = Diag.GetDiagStateForLoc(Loc); - DiagnosticMapping Mapping = State->getOrAddMapping((diag::kind)DiagID); - - // TODO: Can a null severity really get here? - if (Mapping.getSeverity() != diag::Severity()) - Result = Mapping.getSeverity(); - - // Upgrade ignored diagnostics if -Weverything is enabled. - if (State->EnableAllWarnings && Result == diag::Severity::Ignored && - !Mapping.isUser() && - (IsCustomDiag || getDiagClass(DiagID) != CLASS_REMARK)) - Result = diag::Severity::Warning; - - // Ignore -pedantic diagnostics inside __extension__ blocks. - // (The diagnostics controlled by -pedantic are the extension diagnostics - // that are not enabled by default.) - bool EnabledByDefault = false; - bool IsExtensionDiag = isExtensionDiag(DiagID, EnabledByDefault); - if (Diag.AllExtensionsSilenced && IsExtensionDiag && !EnabledByDefault) - return diag::Severity::Ignored; - - // For extension diagnostics that haven't been explicitly mapped, check if we - // should upgrade the diagnostic. Skip if the user explicitly suppressed it - // (e.g. -Wno-foo). - if (IsExtensionDiag && - !(Mapping.isUser() && Result == diag::Severity::Ignored)) { - if (Mapping.hasNoWarningAsError()) - Result = std::max(Result, - std::min(State->ExtBehavior, diag::Severity::Warning)); - else - Result = std::max(Result, State->ExtBehavior); - } - - // At this point, ignored errors can no longer be upgraded. - if (Result == diag::Severity::Ignored) - return Result; - // Honor -w: this disables all messages which are not Error/Fatal by - // default (disregarding attempts to upgrade severity from Warning to Error), - // as well as disabling all messages which are currently mapped to Warning - // (whether by default or downgraded from Error via e.g. -Wno-error or #pragma - // diagnostic.) - // FIXME: Should -w be ignored for custom warnings without a group? - if (State->IgnoreAllWarnings) { - if ((!IsCustomDiag || CustomDiagInfo->getDescription(DiagID).GetGroup()) && - (Result == diag::Severity::Warning || - (Result >= diag::Severity::Error && - !isDefaultMappingAsError((diag::kind)DiagID)))) + auto checkSingleDiag = [&](diag::kind DiagID) -> diag::Severity { + bool IsCustomDiag = DiagnosticIDs::IsCustomDiag(DiagID); + assert(getDiagClass(DiagID) != CLASS_NOTE); + + // Specific non-error diagnostics may be mapped to various levels from + // ignored to error. Errors can only be mapped to fatal. + diag::Severity Result = diag::Severity::Fatal; + + // Get the mapping information, or compute it lazily. + DiagnosticMapping Mapping = State->getOrAddMapping((diag::kind)DiagID); + + // TODO: Can a null severity really get here? + if (Mapping.getSeverity() != diag::Severity()) + Result = Mapping.getSeverity(); + + // Upgrade ignored diagnostics if -Weverything is enabled. + if (State->EnableAllWarnings && Result == diag::Severity::Ignored && + !Mapping.isUser() && + (IsCustomDiag || getDiagClass(DiagID) != CLASS_REMARK)) + Result = diag::Severity::Warning; + + // Ignore -pedantic diagnostics inside __extension__ blocks. + // (The diagnostics controlled by -pedantic are the extension diagnostics + // that are not enabled by default.) + bool EnabledByDefault = false; + bool IsExtensionDiag = isExtensionDiag(DiagID, EnabledByDefault); + if (Diag.AllExtensionsSilenced && IsExtensionDiag && !EnabledByDefault) return diag::Severity::Ignored; - } - // If -Werror is enabled, map warnings to errors unless explicitly disabled. - if (Result == diag::Severity::Warning) { - if (State->WarningsAsErrors && !Mapping.hasNoWarningAsError()) + // For extension diagnostics that haven't been explicitly mapped, check if + // we should upgrade the diagnostic. Skip if the user explicitly + // suppressed it (e.g. -Wno-foo). + if (IsExtensionDiag && + !(Mapping.isUser() && Result == diag::Severity::Ignored)) { + if (Mapping.hasNoWarningAsError()) + Result = std::max( + Result, std::min(State->ExtBehavior, diag::Severity::Warning)); + else + Result = std::max(Result, State->ExtBehavior); + } + + // At this point, ignored errors can no longer be upgraded. + if (Result == diag::Severity::Ignored) + return Result; + + // Honor -w: this disables all messages which are not Error/Fatal by + // default (disregarding attempts to upgrade severity from Warning to + // Error), as well as disabling all messages which are currently mapped to + // Warning (whether by default or downgraded from Error via e.g. + // -Wno-error or #pragma diagnostic.) + // FIXME: Should -w be ignored for custom warnings without a group? + if (State->IgnoreAllWarnings) { + if ((!IsCustomDiag || + CustomDiagInfo->getDescription(DiagID).GetGroup()) && + (Result == diag::Severity::Warning || + (Result >= diag::Severity::Error && + !isDefaultMappingAsError((diag::kind)DiagID)))) + return diag::Severity::Ignored; + } + + // If -Werror is enabled, map warnings to errors unless explicitly + // disabled. + if (Result == diag::Severity::Warning) { + if (State->WarningsAsErrors && !Mapping.hasNoWarningAsError()) + Result = diag::Severity::Error; + } + + // If -Wfatal-errors is enabled, map errors to fatal unless explicitly + // disabled. + if (Result == diag::Severity::Error) { + if (State->ErrorsAsFatal && !Mapping.hasNoErrorAsFatal()) + Result = diag::Severity::Fatal; + } + + // If explicitly requested, map fatal errors to errors. + if (Result == diag::Severity::Fatal && + DiagID != diag::fatal_too_many_errors && Diag.FatalsAsError) Result = diag::Severity::Error; - } - // If -Wfatal-errors is enabled, map errors to fatal unless explicitly - // disabled. - if (Result == diag::Severity::Error) { - if (State->ErrorsAsFatal && !Mapping.hasNoErrorAsFatal()) - Result = diag::Severity::Fatal; - } + // Rest of the mappings are only applicable for diagnostics associated + // with a SourceLocation, bail out early for others. + if (!Diag.hasSourceManager()) + return Result; + + // We check both the location-specific state and the ForceSystemWarnings + // override. In some cases (like template instantiations from system + // modules), the location-specific state might have suppression enabled, + // but the engine might have an override (e.g. + // AllowWarningInSystemHeaders) to show the warning. + if (State->SuppressSystemWarnings && !Diag.getForceSystemWarnings() && + shouldSuppressAsSystemWarning(DiagID, Loc, Diag)) { + return diag::Severity::Ignored; + } - // If explicitly requested, map fatal errors to errors. - if (Result == diag::Severity::Fatal && - DiagID != diag::fatal_too_many_errors && Diag.FatalsAsError) - Result = diag::Severity::Error; + // Clang-diagnostics pragmas always take precedence over suppression + // mapping. + if (!Mapping.isPragma() && Diag.isSuppressedViaMapping(DiagID, Loc)) + return diag::Severity::Ignored; - // Rest of the mappings are only applicable for diagnostics associated with a - // SourceLocation, bail out early for others. - if (!Diag.hasSourceManager()) return Result; + }; - // We check both the location-specific state and the ForceSystemWarnings - // override. In some cases (like template instantiations from system modules), - // the location-specific state might have suppression enabled, but the - // engine might have an override (e.g. AllowWarningInSystemHeaders) to show - // the warning. - if (State->SuppressSystemWarnings && !Diag.getForceSystemWarnings() && - shouldSuppressAsSystemWarning(DiagID, Loc, Diag)) { - return diag::Severity::Ignored; + diag::Severity CompositeResult = diag::Severity::Ignored; + for (diag::kind DiagID : DiagIDs) { + CompositeResult = std::max(CompositeResult, checkSingleDiag(DiagID)); + + // If we already hit 'fatal', we can't get any higher! So just return that. + // We could potentially short-cut this by taking a parameter for "return + // first greater than", but since our uses of this are fairly small, and + // that only optimizes for the "we are about to do something expensive + // anyway" variant (that is, when everything is NOT ignored), it doesn't + // seem particularly valuable. + if (CompositeResult == diag::Severity::Fatal) + break; } - // Clang-diagnostics pragmas always take precedence over suppression mapping. - if (!Mapping.isPragma() && Diag.isSuppressedViaMapping(DiagID, Loc)) - return diag::Severity::Ignored; - - return Result; + return CompositeResult; } bool DiagnosticIDs::shouldSuppressAsSystemWarning( >From 45d9e17538f70869649a244a598a7b13049a0400 Mon Sep 17 00:00:00 2001 From: erichkeane <[email protected]> Date: Wed, 1 Jul 2026 13:49:05 -0700 Subject: [PATCH 4/4] Clang-format --- clang/include/clang/Basic/DiagnosticIDs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h index 6f0fda8d5608a..aa85805bcc8c8 100644 --- a/clang/include/clang/Basic/DiagnosticIDs.h +++ b/clang/include/clang/Basic/DiagnosticIDs.h @@ -515,7 +515,7 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> { const DiagnosticsEngine &Diag) const LLVM_READONLY; /// Given a collection of diagnostic IDs, get the 'highest' severity of them - /// at the provided location for this DiagnosticsEngine. + /// at the provided location for this DiagnosticsEngine. diag::Severity getDiagnosticListHighestSeverity( llvm::ArrayRef<diag::kind> DiagIDs, SourceLocation Loc, const DiagnosticsEngine &Diag) const LLVM_READONLY; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
