Author: Argyrios Kyrtzidis Date: 2022-10-11T13:39:26-07:00 New Revision: 0456acbfb942f127359a8defd1b4f1f44420df3e
URL: https://github.com/llvm/llvm-project/commit/0456acbfb942f127359a8defd1b4f1f44420df3e DIFF: https://github.com/llvm/llvm-project/commit/0456acbfb942f127359a8defd1b4f1f44420df3e.diff LOG: [clang/Sema] Follow-up for fix of non-deterministic order of `-Wunused-variable` diagnostic Commit `371883f46dc23f8464cbf578e2d12a4f92e61917` caused a noticeable compile-time regression (about 0.4% geomean at -O0): http://llvm-compile-time-tracker.com/compare.php?from=92233159035d1b50face95d886901cf99035bd99&to=371883f46dc23f8464cbf578e2d12a4f92e61917&stat=instructions To address this switch `Scope::DeclSetTy` back to a `SmallPtrSet` and change `Sema::ActOnPopScope` to explicitly order the diagnostics that this function emits. Differential Revision: https://reviews.llvm.org/D135490 Added: Modified: clang/include/clang/Sema/Scope.h clang/include/clang/Sema/Sema.h clang/lib/Sema/SemaDecl.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Sema/Scope.h b/clang/include/clang/Sema/Scope.h index 04ab416bf7352..d2acf253552dc 100644 --- a/clang/include/clang/Sema/Scope.h +++ b/clang/include/clang/Sema/Scope.h @@ -16,7 +16,6 @@ #include "clang/AST/Decl.h" #include "clang/Basic/Diagnostic.h" #include "llvm/ADT/PointerIntPair.h" -#include "llvm/ADT/SetVector.h" #include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/iterator_range.h" @@ -197,7 +196,7 @@ class Scope { /// popped, these declarations are removed from the IdentifierTable's notion /// of current declaration. It is up to the current Action implementation to /// implement these semantics. - using DeclSetTy = llvm::SmallSetVector<Decl *, 32>; + using DeclSetTy = llvm::SmallPtrSet<Decl *, 32>; DeclSetTy DeclsInScope; /// The DeclContext with which this scope is associated. For @@ -322,7 +321,7 @@ class Scope { DeclsInScope.insert(D); } - void RemoveDecl(Decl *D) { DeclsInScope.remove(D); } + void RemoveDecl(Decl *D) { DeclsInScope.erase(D); } void incrementMSManglingNumber() { if (Scope *MSLMP = getMSLastManglingParent()) { @@ -350,9 +349,7 @@ class Scope { /// isDeclScope - Return true if this is the scope that the specified decl is /// declared in. - bool isDeclScope(const Decl *D) const { - return DeclsInScope.contains(const_cast<Decl *>(D)); - } + bool isDeclScope(const Decl *D) const { return DeclsInScope.contains(D); } /// Get the entity corresponding to this scope. DeclContext *getEntity() const { diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index a6acdf6b49526..405f84f0b5854 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -5221,15 +5221,21 @@ class Sema final { /// of it. void MarkUnusedFileScopedDecl(const DeclaratorDecl *D); + typedef llvm::function_ref<void(SourceLocation Loc, PartialDiagnostic PD)> + DiagReceiverTy; + /// DiagnoseUnusedExprResult - If the statement passed in is an expression /// whose result is unused, warn. void DiagnoseUnusedExprResult(const Stmt *S, unsigned DiagID); void DiagnoseUnusedNestedTypedefs(const RecordDecl *D); + void DiagnoseUnusedNestedTypedefs(const RecordDecl *D, + DiagReceiverTy DiagReceiver); void DiagnoseUnusedDecl(const NamedDecl *ND); + void DiagnoseUnusedDecl(const NamedDecl *ND, DiagReceiverTy DiagReceiver); /// If VD is set but not otherwise used, diagnose, for a parameter or a /// variable. - void DiagnoseUnusedButSetDecl(const VarDecl *VD); + void DiagnoseUnusedButSetDecl(const VarDecl *VD, DiagReceiverTy DiagReceiver); /// Emit \p DiagID if statement located on \p StmtLoc has a suspicious null /// statement as a \p Body, and it is located on the same line. diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 1bf959a35178a..5cc3db63a1996 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -2091,20 +2091,31 @@ static void GenerateFixForUnusedDecl(const NamedDecl *D, ASTContext &Ctx, } void Sema::DiagnoseUnusedNestedTypedefs(const RecordDecl *D) { + DiagnoseUnusedNestedTypedefs( + D, [this](SourceLocation Loc, PartialDiagnostic PD) { Diag(Loc, PD); }); +} + +void Sema::DiagnoseUnusedNestedTypedefs(const RecordDecl *D, + DiagReceiverTy DiagReceiver) { if (D->getTypeForDecl()->isDependentType()) return; for (auto *TmpD : D->decls()) { if (const auto *T = dyn_cast<TypedefNameDecl>(TmpD)) - DiagnoseUnusedDecl(T); + DiagnoseUnusedDecl(T, DiagReceiver); else if(const auto *R = dyn_cast<RecordDecl>(TmpD)) - DiagnoseUnusedNestedTypedefs(R); + DiagnoseUnusedNestedTypedefs(R, DiagReceiver); } } +void Sema::DiagnoseUnusedDecl(const NamedDecl *D) { + DiagnoseUnusedDecl( + D, [this](SourceLocation Loc, PartialDiagnostic PD) { Diag(Loc, PD); }); +} + /// DiagnoseUnusedDecl - Emit warnings about declarations that are not used /// unless they are marked attr(unused). -void Sema::DiagnoseUnusedDecl(const NamedDecl *D) { +void Sema::DiagnoseUnusedDecl(const NamedDecl *D, DiagReceiverTy DiagReceiver) { if (!ShouldDiagnoseUnusedDecl(D)) return; @@ -2126,10 +2137,11 @@ void Sema::DiagnoseUnusedDecl(const NamedDecl *D) { else DiagID = diag::warn_unused_variable; - Diag(D->getLocation(), DiagID) << D << Hint; + DiagReceiver(D->getLocation(), PDiag(DiagID) << D << Hint); } -void Sema::DiagnoseUnusedButSetDecl(const VarDecl *VD) { +void Sema::DiagnoseUnusedButSetDecl(const VarDecl *VD, + DiagReceiverTy DiagReceiver) { // If it's not referenced, it can't be set. If it has the Cleanup attribute, // it's not really unused. if (!VD->isReferenced() || !VD->getDeclName() || VD->hasAttr<UnusedAttr>() || @@ -2175,10 +2187,11 @@ void Sema::DiagnoseUnusedButSetDecl(const VarDecl *VD) { return; unsigned DiagID = isa<ParmVarDecl>(VD) ? diag::warn_unused_but_set_parameter : diag::warn_unused_but_set_variable; - Diag(VD->getLocation(), DiagID) << VD; + DiagReceiver(VD->getLocation(), PDiag(DiagID) << VD); } -static void CheckPoppedLabel(LabelDecl *L, Sema &S) { +static void CheckPoppedLabel(LabelDecl *L, Sema &S, + Sema::DiagReceiverTy DiagReceiver) { // Verify that we have no forward references left. If so, there was a goto // or address of a label taken, but no definition of it. Label fwd // definitions are indicated with a null substmt which is also not a resolved @@ -2189,7 +2202,8 @@ static void CheckPoppedLabel(LabelDecl *L, Sema &S) { else Diagnose = L->getStmt() == nullptr; if (Diagnose) - S.Diag(L->getLocation(), diag::err_undeclared_label_use) << L; + DiagReceiver(L->getLocation(), S.PDiag(diag::err_undeclared_label_use) + << L); } void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) { @@ -2199,6 +2213,24 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) { assert((S->getFlags() & (Scope::DeclScope | Scope::TemplateParamScope)) && "Scope shouldn't contain decls!"); + /// We visit the decls in non-deterministic order, but we want diagnostics + /// emitted in deterministic order. Collect any diagnostic that may be emitted + /// and sort the diagnostics before emitting them, after we visited all decls. + struct LocAndDiag { + SourceLocation Loc; + Optional<SourceLocation> PreviousDeclLoc; + PartialDiagnostic PD; + }; + SmallVector<LocAndDiag, 16> DeclDiags; + auto addDiag = [&DeclDiags](SourceLocation Loc, PartialDiagnostic PD) { + DeclDiags.push_back(LocAndDiag{Loc, None, std::move(PD)}); + }; + auto addDiagWithPrev = [&DeclDiags](SourceLocation Loc, + SourceLocation PreviousDeclLoc, + PartialDiagnostic PD) { + DeclDiags.push_back(LocAndDiag{Loc, PreviousDeclLoc, std::move(PD)}); + }; + for (auto *TmpD : S->decls()) { assert(TmpD && "This decl didn't get pushed??"); @@ -2207,11 +2239,11 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) { // Diagnose unused variables in this scope. if (!S->hasUnrecoverableErrorOccurred()) { - DiagnoseUnusedDecl(D); + DiagnoseUnusedDecl(D, addDiag); if (const auto *RD = dyn_cast<RecordDecl>(D)) - DiagnoseUnusedNestedTypedefs(RD); + DiagnoseUnusedNestedTypedefs(RD, addDiag); if (VarDecl *VD = dyn_cast<VarDecl>(D)) { - DiagnoseUnusedButSetDecl(VD); + DiagnoseUnusedButSetDecl(VD, addDiag); RefsMinusAssignments.erase(VD); } } @@ -2220,7 +2252,7 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) { // If this was a forward reference to a label, verify it was defined. if (LabelDecl *LD = dyn_cast<LabelDecl>(D)) - CheckPoppedLabel(LD, *this); + CheckPoppedLabel(LD, *this, addDiag); // Remove this name from our lexical scope, and warn on it if we haven't // already. @@ -2228,13 +2260,27 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) { auto ShadowI = ShadowingDecls.find(D); if (ShadowI != ShadowingDecls.end()) { if (const auto *FD = dyn_cast<FieldDecl>(ShadowI->second)) { - Diag(D->getLocation(), diag::warn_ctor_parm_shadows_field) - << D << FD << FD->getParent(); - Diag(FD->getLocation(), diag::note_previous_declaration); + addDiagWithPrev(D->getLocation(), FD->getLocation(), + PDiag(diag::warn_ctor_parm_shadows_field) + << D << FD << FD->getParent()); } ShadowingDecls.erase(ShadowI); } } + + llvm::sort(DeclDiags, + [](const LocAndDiag &LHS, const LocAndDiag &RHS) -> bool { + // The particular order for diagnostics is not important, as long + // as the order is deterministic. Using the raw location is going + // to generally be in source order unless there are macro + // expansions involved. + return LHS.Loc.getRawEncoding() < RHS.Loc.getRawEncoding(); + }); + for (const LocAndDiag &D : DeclDiags) { + Diag(D.Loc, D.PD); + if (D.PreviousDeclLoc) + Diag(*D.PreviousDeclLoc, diag::note_previous_declaration); + } } /// Look for an Objective-C class in the translation unit. _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits