diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp index 4c75b42270114..200bb87a5ac3c 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -380,6 +380,7 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic( ++Context.Stats.ErrorsIgnoredNOLINT; // Ignored a warning, should ignore related notes as well LastErrorWasIgnored = true; + Context.DiagEngine->Clear(); for (const auto &Error : SuppressionErrors) Context.diag(Error); return; @@ -456,6 +457,7 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic( if (Info.hasSourceManager()) checkFilters(Info.getLocation(), Info.getSourceManager()); + Context.DiagEngine->Clear(); for (const auto &Error : SuppressionErrors) Context.diag(Error); } diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h index e17ed8f98afa9..54b69e9854023 100644 --- a/clang/include/clang/Basic/Diagnostic.h +++ b/clang/include/clang/Basic/Diagnostic.h @@ -183,41 +183,6 @@ struct DiagnosticStorage { DiagnosticStorage() = default; }; -/// An allocator for DiagnosticStorage objects, which uses a small cache to -/// objects, used to reduce malloc()/free() traffic for partial diagnostics. -class DiagStorageAllocator { - static const unsigned NumCached = 16; - DiagnosticStorage Cached[NumCached]; - DiagnosticStorage *FreeList[NumCached]; - unsigned NumFreeListEntries; - -public: - DiagStorageAllocator(); - ~DiagStorageAllocator(); - - /// Allocate new storage. - DiagnosticStorage *Allocate() { - if (NumFreeListEntries == 0) - return new DiagnosticStorage; - - DiagnosticStorage *Result = FreeList[--NumFreeListEntries]; - Result->NumDiagArgs = 0; - Result->DiagRanges.clear(); - Result->FixItHints.clear(); - return Result; - } - - /// Free the given storage object. - void Deallocate(DiagnosticStorage *S) { - if (S >= Cached && S <= Cached + NumCached) { - FreeList[NumFreeListEntries++] = S; - return; - } - - delete S; - } -}; - /// Concrete class used by the front-end to report problems and issues. /// /// This massages the diagnostics (e.g. handling things like "report warnings @@ -557,6 +522,27 @@ class DiagnosticsEngine : public RefCountedBase { void *ArgToStringCookie = nullptr; ArgToStringFnTy ArgToStringFn; + /// ID of the "delayed" diagnostic, which is a (typically + /// fatal) diagnostic that had to be delayed because it was found + /// while emitting another diagnostic. + unsigned DelayedDiagID; + + /// First string argument for the delayed diagnostic. + std::string DelayedDiagArg1; + + /// Second string argument for the delayed diagnostic. + std::string DelayedDiagArg2; + + /// Third string argument for the delayed diagnostic. + std::string DelayedDiagArg3; + + /// Optional flag value. + /// + /// Some flags accept values, for instance: -Wframe-larger-than= and + /// -Rpass=. The content of this string is emitted after the flag name + /// and '='. + std::string FlagValue; + public: explicit DiagnosticsEngine(IntrusiveRefCntPtr Diags, IntrusiveRefCntPtr DiagOpts, @@ -963,18 +949,70 @@ class DiagnosticsEngine : public RefCountedBase { void Report(const StoredDiagnostic &storedDiag); + /// Determine whethere there is already a diagnostic in flight. + bool isDiagnosticInFlight() const { + return CurDiagID != std::numeric_limits::max(); + } + + /// Set the "delayed" diagnostic that will be emitted once + /// the current diagnostic completes. + /// + /// If a diagnostic is already in-flight but the front end must + /// report a problem (e.g., with an inconsistent file system + /// state), this routine sets a "delayed" diagnostic that will be + /// emitted after the current diagnostic completes. This should + /// only be used for fatal errors detected at inconvenient + /// times. If emitting a delayed diagnostic causes a second delayed + /// diagnostic to be introduced, that second delayed diagnostic + /// will be ignored. + /// + /// \param DiagID The ID of the diagnostic being delayed. + /// + /// \param Arg1 A string argument that will be provided to the + /// diagnostic. A copy of this string will be stored in the + /// DiagnosticsEngine object itself. + /// + /// \param Arg2 A string argument that will be provided to the + /// diagnostic. A copy of this string will be stored in the + /// DiagnosticsEngine object itself. + /// + /// \param Arg3 A string argument that will be provided to the + /// diagnostic. A copy of this string will be stored in the + /// DiagnosticsEngine object itself. + void SetDelayedDiagnostic(unsigned DiagID, StringRef Arg1 = "", + StringRef Arg2 = "", StringRef Arg3 = ""); + + /// Clear out the current diagnostic. + void Clear() { CurDiagID = std::numeric_limits::max(); } + + /// Return the value associated with this diagnostic flag. + StringRef getFlagValue() const { return FlagValue; } + private: // This is private state used by DiagnosticBuilder. We put it here instead of // in DiagnosticBuilder in order to keep DiagnosticBuilder a small lightweight - // object. This implementation choice means that we can only have a few - // diagnostics "in flight" at a time, but this seems to be a reasonable - // tradeoff to keep these objects small. + // object. This implementation choice means that we can only have one + // diagnostic "in flight" at a time, but this seems to be a reasonable + // tradeoff to keep these objects small. Assertions verify that only one + // diagnostic is in flight at a time. friend class Diagnostic; friend class DiagnosticBuilder; friend class DiagnosticErrorTrap; friend class DiagnosticIDs; friend class PartialDiagnostic; + /// Report the delayed diagnostic. + void ReportDelayed(); + + /// The location of the current diagnostic that is in flight. + SourceLocation CurDiagLoc; + + /// The ID of the current diagnostic that is in flight. + /// + /// This is set to std::numeric_limits::max() when there is no + /// diagnostic in flight. + unsigned CurDiagID; + enum { /// The maximum number of arguments we can hold. /// @@ -984,7 +1022,7 @@ class DiagnosticsEngine : public RefCountedBase { MaxArguments = DiagnosticStorage::MaxArguments, }; - DiagStorageAllocator DiagAllocator; + DiagnosticStorage DiagStorage; DiagnosticMapping makeUserMapping(diag::Severity Map, SourceLocation L) { bool isPragma = L.isValid(); @@ -1004,8 +1042,8 @@ class DiagnosticsEngine : public RefCountedBase { /// Used to report a diagnostic that is finally fully formed. /// /// \returns true if the diagnostic was emitted, false if it was suppressed. - bool ProcessDiag(const DiagnosticBuilder &DiagBuilder) { - return Diags->ProcessDiag(*this, DiagBuilder); + bool ProcessDiag() { + return Diags->ProcessDiag(*this); } /// @name Diagnostic Emission @@ -1020,10 +1058,14 @@ class DiagnosticsEngine : public RefCountedBase { // Sema::Diag() patterns. friend class Sema; - /// Emit the diagnostic + /// Emit the current diagnostic and clear the diagnostic state. /// /// \param Force Emit the diagnostic regardless of suppression settings. - bool EmitDiagnostic(const DiagnosticBuilder &DB, bool Force = false); + bool EmitCurrentDiagnostic(bool Force = false); + + unsigned getCurrentDiagID() const { return CurDiagID; } + + SourceLocation getCurrentDiagLoc() const { return CurDiagLoc; } /// @} }; @@ -1076,7 +1118,40 @@ class DiagnosticErrorTrap { /// class StreamingDiagnostic { public: - using DiagStorageAllocator = clang::DiagStorageAllocator; + /// An allocator for DiagnosticStorage objects, which uses a small cache to + /// objects, used to reduce malloc()/free() traffic for partial diagnostics. + class DiagStorageAllocator { + static const unsigned NumCached = 16; + DiagnosticStorage Cached[NumCached]; + DiagnosticStorage *FreeList[NumCached]; + unsigned NumFreeListEntries; + + public: + DiagStorageAllocator(); + ~DiagStorageAllocator(); + + /// Allocate new storage. + DiagnosticStorage *Allocate() { + if (NumFreeListEntries == 0) + return new DiagnosticStorage; + + DiagnosticStorage *Result = FreeList[--NumFreeListEntries]; + Result->NumDiagArgs = 0; + Result->DiagRanges.clear(); + Result->FixItHints.clear(); + return Result; + } + + /// Free the given storage object. + void Deallocate(DiagnosticStorage *S) { + if (S >= Cached && S <= Cached + NumCached) { + FreeList[NumFreeListEntries++] = S; + return; + } + + delete S; + } + }; protected: mutable DiagnosticStorage *DiagStorage = nullptr; @@ -1165,6 +1240,11 @@ class StreamingDiagnostic { protected: StreamingDiagnostic() = default; + /// Construct with an external storage not owned by itself. The allocator + /// is a null pointer in this case. + explicit StreamingDiagnostic(DiagnosticStorage *Storage) + : DiagStorage(Storage) {} + /// Construct with a storage allocator which will manage the storage. The /// allocator is not a null pointer in this case. explicit StreamingDiagnostic(DiagStorageAllocator &Alloc) @@ -1195,20 +1275,9 @@ class StreamingDiagnostic { class DiagnosticBuilder : public StreamingDiagnostic { friend class DiagnosticsEngine; friend class PartialDiagnostic; - friend class Diagnostic; mutable DiagnosticsEngine *DiagObj = nullptr; - SourceLocation DiagLoc; - unsigned DiagID; - - /// Optional flag value. - /// - /// Some flags accept values, for instance: -Wframe-larger-than= and - /// -Rpass=. The content of this string is emitted after the flag name - /// and '='. - mutable std::string FlagValue; - /// Status variable indicating if this diagnostic is still active. /// // NOTE: This field is redundant with DiagObj (IsActive iff (DiagObj == 0)), @@ -1222,8 +1291,16 @@ class DiagnosticBuilder : public StreamingDiagnostic { DiagnosticBuilder() = default; - DiagnosticBuilder(DiagnosticsEngine *DiagObj, SourceLocation DiagLoc, - unsigned DiagID); + explicit DiagnosticBuilder(DiagnosticsEngine *diagObj) + : StreamingDiagnostic(&diagObj->DiagStorage), DiagObj(diagObj), + IsActive(true) { + assert(diagObj && "DiagnosticBuilder requires a valid DiagnosticsEngine!"); + assert(DiagStorage && + "DiagnosticBuilder requires a valid DiagnosticStorage!"); + DiagStorage->NumDiagArgs = 0; + DiagStorage->DiagRanges.clear(); + DiagStorage->FixItHints.clear(); + } protected: /// Clear out the current diagnostic. @@ -1249,7 +1326,7 @@ class DiagnosticBuilder : public StreamingDiagnostic { if (!isActive()) return false; // Process the diagnostic. - bool Result = DiagObj->EmitDiagnostic(*this, IsForceEmit); + bool Result = DiagObj->EmitCurrentDiagnostic(IsForceEmit); // This diagnostic is dead. Clear(); @@ -1260,7 +1337,13 @@ class DiagnosticBuilder : public StreamingDiagnostic { public: /// Copy constructor. When copied, this "takes" the diagnostic info from the /// input and neuters it. - DiagnosticBuilder(const DiagnosticBuilder &D); + DiagnosticBuilder(const DiagnosticBuilder &D) : StreamingDiagnostic() { + DiagObj = D.DiagObj; + DiagStorage = D.DiagStorage; + IsActive = D.IsActive; + IsForceEmit = D.IsForceEmit; + D.Clear(); + } template const DiagnosticBuilder &operator<<(const T &V) const { assert(isActive() && "Clients must not add to cleared diagnostic!"); @@ -1292,7 +1375,7 @@ class DiagnosticBuilder : public StreamingDiagnostic { return *this; } - void addFlagValue(StringRef V) const { FlagValue = std::string(V); } + void addFlagValue(StringRef V) const { DiagObj->FlagValue = std::string(V); } }; struct AddFlagValue { @@ -1467,7 +1550,12 @@ const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB, inline DiagnosticBuilder DiagnosticsEngine::Report(SourceLocation Loc, unsigned DiagID) { - return DiagnosticBuilder(this, Loc, DiagID); + assert(CurDiagID == std::numeric_limits::max() && + "Multiple diagnostics in flight at once!"); + CurDiagLoc = Loc; + CurDiagID = DiagID; + FlagValue.clear(); + return DiagnosticBuilder(this); } const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB, @@ -1482,29 +1570,24 @@ inline DiagnosticBuilder DiagnosticsEngine::Report(unsigned DiagID) { //===----------------------------------------------------------------------===// /// A little helper class (which is basically a smart pointer that forwards -/// info from DiagnosticsEngine and DiagnosticStorage) that allows clients to -/// enquire about the diagnostic. +/// info from DiagnosticsEngine) that allows clients to enquire about the +/// currently in-flight diagnostic. class Diagnostic { const DiagnosticsEngine *DiagObj; - SourceLocation DiagLoc; - unsigned DiagID; - std::string FlagValue; - const DiagnosticStorage &DiagStorage; std::optional StoredDiagMessage; public: - Diagnostic(const DiagnosticsEngine *DO, const DiagnosticBuilder &DiagBuilder); - Diagnostic(const DiagnosticsEngine *DO, SourceLocation DiagLoc, - unsigned DiagID, const DiagnosticStorage &DiagStorage, - StringRef StoredDiagMessage); + explicit Diagnostic(const DiagnosticsEngine *DO) : DiagObj(DO) {} + Diagnostic(const DiagnosticsEngine *DO, StringRef storedDiagMessage) + : DiagObj(DO), StoredDiagMessage(storedDiagMessage) {} const DiagnosticsEngine *getDiags() const { return DiagObj; } - unsigned getID() const { return DiagID; } - const SourceLocation &getLocation() const { return DiagLoc; } + unsigned getID() const { return DiagObj->CurDiagID; } + const SourceLocation &getLocation() const { return DiagObj->CurDiagLoc; } bool hasSourceManager() const { return DiagObj->hasSourceManager(); } SourceManager &getSourceManager() const { return DiagObj->getSourceManager();} - unsigned getNumArgs() const { return DiagStorage.NumDiagArgs; } + unsigned getNumArgs() const { return DiagObj->DiagStorage.NumDiagArgs; } /// Return the kind of the specified index. /// @@ -1514,7 +1597,8 @@ class Diagnostic { /// \pre Idx < getNumArgs() DiagnosticsEngine::ArgumentKind getArgKind(unsigned Idx) const { assert(Idx < getNumArgs() && "Argument index out of range!"); - return (DiagnosticsEngine::ArgumentKind)DiagStorage.DiagArgumentsKind[Idx]; + return (DiagnosticsEngine::ArgumentKind) + DiagObj->DiagStorage.DiagArgumentsKind[Idx]; } /// Return the provided argument string specified by \p Idx. @@ -1522,7 +1606,7 @@ class Diagnostic { const std::string &getArgStdStr(unsigned Idx) const { assert(getArgKind(Idx) == DiagnosticsEngine::ak_std_string && "invalid argument accessor!"); - return DiagStorage.DiagArgumentsStr[Idx]; + return DiagObj->DiagStorage.DiagArgumentsStr[Idx]; } /// Return the specified C string argument. @@ -1530,7 +1614,8 @@ class Diagnostic { const char *getArgCStr(unsigned Idx) const { assert(getArgKind(Idx) == DiagnosticsEngine::ak_c_string && "invalid argument accessor!"); - return reinterpret_cast(DiagStorage.DiagArgumentsVal[Idx]); + return reinterpret_cast( + DiagObj->DiagStorage.DiagArgumentsVal[Idx]); } /// Return the specified signed integer argument. @@ -1538,7 +1623,7 @@ class Diagnostic { int64_t getArgSInt(unsigned Idx) const { assert(getArgKind(Idx) == DiagnosticsEngine::ak_sint && "invalid argument accessor!"); - return (int64_t)DiagStorage.DiagArgumentsVal[Idx]; + return (int64_t)DiagObj->DiagStorage.DiagArgumentsVal[Idx]; } /// Return the specified unsigned integer argument. @@ -1546,7 +1631,7 @@ class Diagnostic { uint64_t getArgUInt(unsigned Idx) const { assert(getArgKind(Idx) == DiagnosticsEngine::ak_uint && "invalid argument accessor!"); - return DiagStorage.DiagArgumentsVal[Idx]; + return DiagObj->DiagStorage.DiagArgumentsVal[Idx]; } /// Return the specified IdentifierInfo argument. @@ -1555,7 +1640,7 @@ class Diagnostic { assert(getArgKind(Idx) == DiagnosticsEngine::ak_identifierinfo && "invalid argument accessor!"); return reinterpret_cast( - DiagStorage.DiagArgumentsVal[Idx]); + DiagObj->DiagStorage.DiagArgumentsVal[Idx]); } /// Return the specified non-string argument in an opaque form. @@ -1563,32 +1648,37 @@ class Diagnostic { uint64_t getRawArg(unsigned Idx) const { assert(getArgKind(Idx) != DiagnosticsEngine::ak_std_string && "invalid argument accessor!"); - return DiagStorage.DiagArgumentsVal[Idx]; + return DiagObj->DiagStorage.DiagArgumentsVal[Idx]; } /// Return the number of source ranges associated with this diagnostic. - unsigned getNumRanges() const { return DiagStorage.DiagRanges.size(); } + unsigned getNumRanges() const { + return DiagObj->DiagStorage.DiagRanges.size(); + } /// \pre Idx < getNumRanges() const CharSourceRange &getRange(unsigned Idx) const { assert(Idx < getNumRanges() && "Invalid diagnostic range index!"); - return DiagStorage.DiagRanges[Idx]; + return DiagObj->DiagStorage.DiagRanges[Idx]; } /// Return an array reference for this diagnostic's ranges. - ArrayRef getRanges() const { return DiagStorage.DiagRanges; } + ArrayRef getRanges() const { + return DiagObj->DiagStorage.DiagRanges; + } - unsigned getNumFixItHints() const { return DiagStorage.FixItHints.size(); } + unsigned getNumFixItHints() const { + return DiagObj->DiagStorage.FixItHints.size(); + } const FixItHint &getFixItHint(unsigned Idx) const { assert(Idx < getNumFixItHints() && "Invalid index!"); - return DiagStorage.FixItHints[Idx]; + return DiagObj->DiagStorage.FixItHints[Idx]; } - ArrayRef getFixItHints() const { return DiagStorage.FixItHints; } - - /// Return the value associated with this diagnostic flag. - StringRef getFlagValue() const { return FlagValue; } + ArrayRef getFixItHints() const { + return DiagObj->DiagStorage.FixItHints; + } /// Format this diagnostic into a string, substituting the /// formal arguments into the %0 slots. diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h index 1fa38ed6066e2..daad66f499538 100644 --- a/clang/include/clang/Basic/DiagnosticIDs.h +++ b/clang/include/clang/Basic/DiagnosticIDs.h @@ -24,7 +24,6 @@ namespace clang { class DiagnosticsEngine; - class DiagnosticBuilder; class SourceLocation; // Import the diagnostic enums themselves. @@ -487,13 +486,11 @@ class DiagnosticIDs : public RefCountedBase { /// /// \returns \c true if the diagnostic was emitted, \c false if it was /// suppressed. - bool ProcessDiag(DiagnosticsEngine &Diag, - const DiagnosticBuilder &DiagBuilder) const; + bool ProcessDiag(DiagnosticsEngine &Diag) const; /// Used to emit a diagnostic that is finally fully formed, /// ignoring suppression. - void EmitDiag(DiagnosticsEngine &Diag, const DiagnosticBuilder &DiagBuilder, - Level DiagLevel) const; + void EmitDiag(DiagnosticsEngine &Diag, Level DiagLevel) const; /// Whether the diagnostic may leave the AST in a state where some /// invariants can break. diff --git a/clang/include/clang/Basic/PartialDiagnostic.h b/clang/include/clang/Basic/PartialDiagnostic.h index 4bf6049d08fdb..507d789c54ff9 100644 --- a/clang/include/clang/Basic/PartialDiagnostic.h +++ b/clang/include/clang/Basic/PartialDiagnostic.h @@ -166,10 +166,13 @@ class PartialDiagnostic : public StreamingDiagnostic { void EmitToString(DiagnosticsEngine &Diags, SmallVectorImpl &Buf) const { + // FIXME: It should be possible to render a diagnostic to a string without + // messing with the state of the diagnostics engine. DiagnosticBuilder DB(Diags.Report(getDiagID())); Emit(DB); - Diagnostic(&Diags, DB).FormatDiagnostic(Buf); + Diagnostic(&Diags).FormatDiagnostic(Buf); DB.Clear(); + Diags.Clear(); } /// Clear out this partial diagnostic, giving it a new diagnostic ID diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 887d4beea7cfb..99eef472223a0 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -626,10 +626,10 @@ class Sema final : public SemaBase { const llvm::MapVector & getMismatchingDeleteExpressions() const; - /// Cause the built diagnostic to be emitted on the DiagosticsEngine. - /// This is closely coupled to the SemaDiagnosticBuilder class and + /// Cause the active diagnostic on the DiagosticsEngine to be + /// emitted. This is closely coupled to the SemaDiagnosticBuilder class and /// should not be used elsewhere. - void EmitDiagnostic(unsigned DiagID, const DiagnosticBuilder &DB); + void EmitCurrentDiagnostic(unsigned DiagID); void addImplicitTypedef(StringRef Name, QualType T); diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp index ffb3cd4270770..ecff80a509063 100644 --- a/clang/lib/Basic/Diagnostic.cpp +++ b/clang/lib/Basic/Diagnostic.cpp @@ -126,7 +126,9 @@ void DiagnosticsEngine::Reset(bool soft /*=false*/) { TrapNumErrorsOccurred = 0; TrapNumUnrecoverableErrorsOccurred = 0; + CurDiagID = std::numeric_limits::max(); LastDiagLevel = DiagnosticIDs::Ignored; + DelayedDiagID = 0; if (!soft) { // Clear state related to #pragma diagnostic. @@ -141,6 +143,23 @@ void DiagnosticsEngine::Reset(bool soft /*=false*/) { } } +void DiagnosticsEngine::SetDelayedDiagnostic(unsigned DiagID, StringRef Arg1, + StringRef Arg2, StringRef Arg3) { + if (DelayedDiagID) + return; + + DelayedDiagID = DiagID; + DelayedDiagArg1 = Arg1.str(); + DelayedDiagArg2 = Arg2.str(); + DelayedDiagArg3 = Arg3.str(); +} + +void DiagnosticsEngine::ReportDelayed() { + unsigned ID = DelayedDiagID; + DelayedDiagID = 0; + Report(ID) << DelayedDiagArg1 << DelayedDiagArg2 << DelayedDiagArg3; +} + DiagnosticMapping & DiagnosticsEngine::DiagState::getOrAddMapping(diag::kind Diag) { std::pair Result = @@ -484,31 +503,39 @@ void DiagnosticsEngine::setSeverityForAll(diag::Flavor Flavor, } void DiagnosticsEngine::Report(const StoredDiagnostic &storedDiag) { - DiagnosticStorage DiagStorage; + assert(CurDiagID == std::numeric_limits::max() && + "Multiple diagnostics in flight at once!"); + + CurDiagLoc = storedDiag.getLocation(); + CurDiagID = storedDiag.getID(); + DiagStorage.NumDiagArgs = 0; + + DiagStorage.DiagRanges.clear(); DiagStorage.DiagRanges.append(storedDiag.range_begin(), storedDiag.range_end()); + DiagStorage.FixItHints.clear(); DiagStorage.FixItHints.append(storedDiag.fixit_begin(), storedDiag.fixit_end()); assert(Client && "DiagnosticConsumer not set!"); Level DiagLevel = storedDiag.getLevel(); - Diagnostic Info(this, storedDiag.getLocation(), storedDiag.getID(), - DiagStorage, storedDiag.getMessage()); + Diagnostic Info(this, storedDiag.getMessage()); Client->HandleDiagnostic(DiagLevel, Info); if (Client->IncludeInDiagnosticCounts()) { if (DiagLevel == DiagnosticsEngine::Warning) ++NumWarnings; } + + CurDiagID = std::numeric_limits::max(); } -bool DiagnosticsEngine::EmitDiagnostic(const DiagnosticBuilder &DB, - bool Force) { +bool DiagnosticsEngine::EmitCurrentDiagnostic(bool Force) { assert(getClient() && "DiagnosticClient not set!"); bool Emitted; if (Force) { - Diagnostic Info(this, DB); + Diagnostic Info(this); // Figure out the diagnostic level of this message. DiagnosticIDs::Level DiagLevel @@ -517,50 +544,24 @@ bool DiagnosticsEngine::EmitDiagnostic(const DiagnosticBuilder &DB, Emitted = (DiagLevel != DiagnosticIDs::Ignored); if (Emitted) { // Emit the diagnostic regardless of suppression level. - Diags->EmitDiag(*this, DB, DiagLevel); + Diags->EmitDiag(*this, DiagLevel); } } else { // Process the diagnostic, sending the accumulated information to the // DiagnosticConsumer. - Emitted = ProcessDiag(DB); + Emitted = ProcessDiag(); } - return Emitted; -} + // Clear out the current diagnostic object. + Clear(); -DiagnosticBuilder::DiagnosticBuilder(DiagnosticsEngine *DiagObj, - SourceLocation DiagLoc, unsigned DiagID) - : StreamingDiagnostic(DiagObj->DiagAllocator), DiagObj(DiagObj), - DiagLoc(DiagLoc), DiagID(DiagID), IsActive(true) { - assert(DiagObj && "DiagnosticBuilder requires a valid DiagnosticsEngine!"); -} + // If there was a delayed diagnostic, emit it now. + if (!Force && DelayedDiagID) + ReportDelayed(); -DiagnosticBuilder::DiagnosticBuilder(const DiagnosticBuilder &D) - : StreamingDiagnostic() { - DiagLoc = D.DiagLoc; - DiagID = D.DiagID; - FlagValue = D.FlagValue; - DiagObj = D.DiagObj; - DiagStorage = D.DiagStorage; - D.DiagStorage = nullptr; - Allocator = D.Allocator; - IsActive = D.IsActive; - IsForceEmit = D.IsForceEmit; - D.Clear(); + return Emitted; } -Diagnostic::Diagnostic(const DiagnosticsEngine *DO, - const DiagnosticBuilder &DiagBuilder) - : DiagObj(DO), DiagLoc(DiagBuilder.DiagLoc), - DiagID(DiagBuilder.DiagID), FlagValue(DiagBuilder.FlagValue), - DiagStorage(*DiagBuilder.getStorage()) {} - -Diagnostic::Diagnostic(const DiagnosticsEngine *DO, SourceLocation DiagLoc, - unsigned DiagID, const DiagnosticStorage &DiagStorage, - StringRef StoredDiagMessage) - : DiagObj(DO), DiagLoc(DiagLoc), DiagID(DiagID), - DiagStorage(DiagStorage), StoredDiagMessage(StoredDiagMessage) {} - DiagnosticConsumer::~DiagnosticConsumer() = default; void DiagnosticConsumer::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, @@ -1215,13 +1216,13 @@ bool ForwardingDiagnosticConsumer::IncludeInDiagnosticCounts() const { return Target.IncludeInDiagnosticCounts(); } -DiagStorageAllocator::DiagStorageAllocator() { +PartialDiagnostic::DiagStorageAllocator::DiagStorageAllocator() { for (unsigned I = 0; I != NumCached; ++I) FreeList[I] = Cached + I; NumFreeListEntries = NumCached; } -DiagStorageAllocator::~DiagStorageAllocator() { +PartialDiagnostic::DiagStorageAllocator::~DiagStorageAllocator() { // Don't assert if we are in a CrashRecovery context, as this invariant may // be invalidated during a crash. assert((NumFreeListEntries == NumCached || diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp index 031d9d7817d1f..cae6642bd4bd3 100644 --- a/clang/lib/Basic/DiagnosticIDs.cpp +++ b/clang/lib/Basic/DiagnosticIDs.cpp @@ -566,7 +566,7 @@ DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc, // If explicitly requested, map fatal errors to errors. if (Result == diag::Severity::Fatal && - DiagID != diag::fatal_too_many_errors && Diag.FatalsAsError) + Diag.CurDiagID != diag::fatal_too_many_errors && Diag.FatalsAsError) Result = diag::Severity::Error; bool ShowInSystemHeader = @@ -800,9 +800,8 @@ StringRef DiagnosticIDs::getNearestOption(diag::Flavor Flavor, /// ProcessDiag - This is the method used to report a diagnostic that is /// finally fully formed. -bool DiagnosticIDs::ProcessDiag(DiagnosticsEngine &Diag, - const DiagnosticBuilder &DiagBuilder) const { - Diagnostic Info(&Diag, DiagBuilder); +bool DiagnosticIDs::ProcessDiag(DiagnosticsEngine &Diag) const { + Diagnostic Info(&Diag); assert(Diag.getClient() && "DiagnosticClient not set!"); @@ -868,24 +867,22 @@ bool DiagnosticIDs::ProcessDiag(DiagnosticsEngine &Diag, // stop a flood of bogus errors. if (Diag.ErrorLimit && Diag.NumErrors > Diag.ErrorLimit && DiagLevel == DiagnosticIDs::Error) { - Diag.Report(diag::fatal_too_many_errors); + Diag.SetDelayedDiagnostic(diag::fatal_too_many_errors); return false; } } // Make sure we set FatalErrorOccurred to ensure that the notes from the // diagnostic that caused `fatal_too_many_errors` won't be emitted. - if (Info.getID() == diag::fatal_too_many_errors) + if (Diag.CurDiagID == diag::fatal_too_many_errors) Diag.FatalErrorOccurred = true; // Finally, report it. - EmitDiag(Diag, DiagBuilder, DiagLevel); + EmitDiag(Diag, DiagLevel); return true; } -void DiagnosticIDs::EmitDiag(DiagnosticsEngine &Diag, - const DiagnosticBuilder &DiagBuilder, - Level DiagLevel) const { - Diagnostic Info(&Diag, DiagBuilder); +void DiagnosticIDs::EmitDiag(DiagnosticsEngine &Diag, Level DiagLevel) const { + Diagnostic Info(&Diag); assert(DiagLevel != DiagnosticIDs::Ignored && "Cannot emit ignored diagnostics!"); Diag.Client->HandleDiagnostic((DiagnosticsEngine::Level)DiagLevel, Info); @@ -893,6 +890,8 @@ void DiagnosticIDs::EmitDiag(DiagnosticsEngine &Diag, if (DiagLevel == DiagnosticIDs::Warning) ++Diag.NumWarnings; } + + Diag.CurDiagID = ~0U; } bool DiagnosticIDs::isUnrecoverable(unsigned DiagID) const { diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp index 65a8a7253e054..d6ec26af80aad 100644 --- a/clang/lib/Basic/SourceManager.cpp +++ b/clang/lib/Basic/SourceManager.cpp @@ -130,8 +130,13 @@ ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM, // the file could also have been removed during processing. Since we can't // really deal with this situation, just create an empty buffer. if (!BufferOrError) { - Diag.Report(Loc, diag::err_cannot_open_file) - << ContentsEntry->getName() << BufferOrError.getError().message(); + if (Diag.isDiagnosticInFlight()) + Diag.SetDelayedDiagnostic(diag::err_cannot_open_file, + ContentsEntry->getName(), + BufferOrError.getError().message()); + else + Diag.Report(Loc, diag::err_cannot_open_file) + << ContentsEntry->getName() << BufferOrError.getError().message(); return std::nullopt; } @@ -148,7 +153,12 @@ ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM, // ContentsEntry::getSize() could have the wrong size. Use // MemoryBuffer::getBufferSize() instead. if (Buffer->getBufferSize() >= std::numeric_limits::max()) { - Diag.Report(Loc, diag::err_file_too_large) << ContentsEntry->getName(); + if (Diag.isDiagnosticInFlight()) + Diag.SetDelayedDiagnostic(diag::err_file_too_large, + ContentsEntry->getName()); + else + Diag.Report(Loc, diag::err_file_too_large) + << ContentsEntry->getName(); return std::nullopt; } @@ -158,7 +168,12 @@ ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM, // have come from a stat cache). if (!ContentsEntry->isNamedPipe() && Buffer->getBufferSize() != (size_t)ContentsEntry->getSize()) { - Diag.Report(Loc, diag::err_file_modified) << ContentsEntry->getName(); + if (Diag.isDiagnosticInFlight()) + Diag.SetDelayedDiagnostic(diag::err_file_modified, + ContentsEntry->getName()); + else + Diag.Report(Loc, diag::err_file_modified) + << ContentsEntry->getName(); return std::nullopt; } diff --git a/clang/lib/Frontend/Rewrite/FixItRewriter.cpp b/clang/lib/Frontend/Rewrite/FixItRewriter.cpp index 7309553e3bc0b..44dfaf20eae73 100644 --- a/clang/lib/Frontend/Rewrite/FixItRewriter.cpp +++ b/clang/lib/Frontend/Rewrite/FixItRewriter.cpp @@ -200,8 +200,10 @@ void FixItRewriter::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, /// Emit a diagnostic via the adapted diagnostic client. void FixItRewriter::Diag(SourceLocation Loc, unsigned DiagID) { // When producing this diagnostic, we temporarily bypass ourselves, - // and let the downstream client format the diagnostic. + // clear out any current diagnostic, and let the downstream client + // format the diagnostic. Diags.setClient(Client, false); + Diags.Clear(); Diags.Report(Loc, DiagID); Diags.setClient(this, false); } diff --git a/clang/lib/Frontend/TextDiagnosticPrinter.cpp b/clang/lib/Frontend/TextDiagnosticPrinter.cpp index 28f7218dc23f5..c2fea3d03f0c0 100644 --- a/clang/lib/Frontend/TextDiagnosticPrinter.cpp +++ b/clang/lib/Frontend/TextDiagnosticPrinter.cpp @@ -84,7 +84,7 @@ static void printDiagnosticOptions(raw_ostream &OS, if (!Opt.empty()) { OS << (Started ? "," : " [") << (Level == DiagnosticsEngine::Remark ? "-R" : "-W") << Opt; - StringRef OptValue = Info.getFlagValue(); + StringRef OptValue = Info.getDiags()->getFlagValue(); if (!OptValue.empty()) OS << "=" << OptValue; Started = true; diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp index e950ad675072a..d567de703903c 100644 --- a/clang/lib/Sema/Sema.cpp +++ b/clang/lib/Sema/Sema.cpp @@ -1589,7 +1589,7 @@ LangAS Sema::getDefaultCXXMethodAddrSpace() const { return LangAS::Default; } -void Sema::EmitDiagnostic(unsigned DiagID, const DiagnosticBuilder &DB) { +void Sema::EmitCurrentDiagnostic(unsigned DiagID) { // FIXME: It doesn't make sense to me that DiagID is an incoming argument here // and yet we also use the current diag ID on the DiagnosticsEngine. This has // been made more painfully obvious by the refactor that introduced this @@ -1597,9 +1597,9 @@ void Sema::EmitDiagnostic(unsigned DiagID, const DiagnosticBuilder &DB) { // eliminated. If it truly cannot be (for example, there is some reentrancy // issue I am not seeing yet), then there should at least be a clarifying // comment somewhere. - Diagnostic DiagInfo(&Diags, DB); if (std::optional Info = isSFINAEContext()) { - switch (DiagnosticIDs::getDiagnosticSFINAEResponse(DiagInfo.getID())) { + switch (DiagnosticIDs::getDiagnosticSFINAEResponse( + Diags.getCurrentDiagID())) { case DiagnosticIDs::SFINAE_Report: // We'll report the diagnostic below. break; @@ -1612,11 +1612,13 @@ void Sema::EmitDiagnostic(unsigned DiagID, const DiagnosticBuilder &DB) { // Make a copy of this suppressed diagnostic and store it with the // template-deduction information. if (*Info && !(*Info)->hasSFINAEDiagnostic()) { + Diagnostic DiagInfo(&Diags); (*Info)->addSFINAEDiagnostic(DiagInfo.getLocation(), PartialDiagnostic(DiagInfo, Context.getDiagAllocator())); } Diags.setLastDiagnosticIgnored(true); + Diags.Clear(); return; case DiagnosticIDs::SFINAE_AccessControl: { @@ -1627,7 +1629,7 @@ void Sema::EmitDiagnostic(unsigned DiagID, const DiagnosticBuilder &DB) { if (!AccessCheckingSFINAE && !getLangOpts().CPlusPlus11) break; - SourceLocation Loc = DiagInfo.getLocation(); + SourceLocation Loc = Diags.getCurrentDiagLoc(); // Suppress this diagnostic. ++NumSFINAEErrors; @@ -1635,13 +1637,16 @@ void Sema::EmitDiagnostic(unsigned DiagID, const DiagnosticBuilder &DB) { // Make a copy of this suppressed diagnostic and store it with the // template-deduction information. if (*Info && !(*Info)->hasSFINAEDiagnostic()) { + Diagnostic DiagInfo(&Diags); (*Info)->addSFINAEDiagnostic(DiagInfo.getLocation(), PartialDiagnostic(DiagInfo, Context.getDiagAllocator())); } Diags.setLastDiagnosticIgnored(true); + Diags.Clear(); - // Now produce a C++98 compatibility warning. + // Now the diagnostic state is clear, produce a C++98 compatibility + // warning. Diag(Loc, diag::warn_cxx98_compat_sfinae_access_control); // The last diagnostic which Sema produced was ignored. Suppress any @@ -1654,12 +1659,14 @@ void Sema::EmitDiagnostic(unsigned DiagID, const DiagnosticBuilder &DB) { // Make a copy of this suppressed diagnostic and store it with the // template-deduction information; if (*Info) { + Diagnostic DiagInfo(&Diags); (*Info)->addSuppressedDiagnostic(DiagInfo.getLocation(), PartialDiagnostic(DiagInfo, Context.getDiagAllocator())); } // Suppress this diagnostic. Diags.setLastDiagnosticIgnored(true); + Diags.Clear(); return; } } @@ -1669,7 +1676,7 @@ void Sema::EmitDiagnostic(unsigned DiagID, const DiagnosticBuilder &DB) { Context.setPrintingPolicy(getPrintingPolicy()); // Emit the diagnostic. - if (!Diags.EmitDiagnostic(DB)) + if (!Diags.EmitCurrentDiagnostic()) return; // If this is not a note, and we're in a template instantiation diff --git a/clang/lib/Sema/SemaBase.cpp b/clang/lib/Sema/SemaBase.cpp index 5c24f21b469b0..a2f12d622e8cc 100644 --- a/clang/lib/Sema/SemaBase.cpp +++ b/clang/lib/Sema/SemaBase.cpp @@ -26,7 +26,7 @@ SemaBase::ImmediateDiagBuilder::~ImmediateDiagBuilder() { Clear(); // Dispatch to Sema to emit the diagnostic. - SemaRef.EmitDiagnostic(DiagID, *this); + SemaRef.EmitCurrentDiagnostic(DiagID); } PartialDiagnostic SemaBase::PDiag(unsigned DiagID) { diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 7efcc81e194d9..4fae6ff02ea9a 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -1382,7 +1382,7 @@ bool ASTReader::ReadVisibleDeclContextStorage(ModuleFile &M, void ASTReader::Error(StringRef Msg) const { Error(diag::err_fe_pch_malformed, Msg); - if (PP.getLangOpts().Modules && + if (PP.getLangOpts().Modules && !Diags.isDiagnosticInFlight() && !PP.getHeaderSearchInfo().getModuleCachePath().empty()) { Diag(diag::note_module_cache_path) << PP.getHeaderSearchInfo().getModuleCachePath(); @@ -1391,7 +1391,10 @@ void ASTReader::Error(StringRef Msg) const { void ASTReader::Error(unsigned DiagID, StringRef Arg1, StringRef Arg2, StringRef Arg3) const { - Diag(DiagID) << Arg1 << Arg2 << Arg3; + if (Diags.isDiagnosticInFlight()) + Diags.SetDelayedDiagnostic(DiagID, Arg1, Arg2, Arg3); + else + Diag(DiagID) << Arg1 << Arg2 << Arg3; } void ASTReader::Error(llvm::Error &&Err) const { @@ -2710,7 +2713,7 @@ InputFile ASTReader::getInputFile(ModuleFile &F, unsigned ID, bool Complain) { // For an overridden file, there is nothing to validate. if (!Overridden && FileChange.Kind != Change::None) { - if (Complain) { + if (Complain && !Diags.isDiagnosticInFlight()) { // Build a list of the PCH imports that got us here (in reverse). SmallVector ImportStack(1, &F); while (!ImportStack.back()->ImportedBy.empty()) @@ -3686,8 +3689,10 @@ llvm::Error ASTReader::ReadASTBlock(ModuleFile &F, SourceMgr.AllocateLoadedSLocEntries(F.LocalNumSLocEntries, SLocSpaceSize); if (!F.SLocEntryBaseID) { - Diags.Report(SourceLocation(), diag::remark_sloc_usage); - SourceMgr.noteSLocAddressSpaceUsage(Diags); + if (!Diags.isDiagnosticInFlight()) { + Diags.Report(SourceLocation(), diag::remark_sloc_usage); + SourceMgr.noteSLocAddressSpaceUsage(Diags); + } return llvm::createStringError(std::errc::invalid_argument, "ran out of source locations"); } diff --git a/clang/test/PCH/race-condition.cpp b/clang/test/PCH/race-condition.cpp deleted file mode 100644 index 752b0cc3ff628..0000000000000 --- a/clang/test/PCH/race-condition.cpp +++ /dev/null @@ -1,41 +0,0 @@ -// RUN: %clang_cc1 -fallow-pch-with-compiler-errors -std=c++20 -x c++-header -emit-pch %s -o %t -verify -// RUN: %clang_cc1 -fallow-pch-with-compiler-errors -std=c++20 -include-pch %t %s -verify -#ifndef HEADER_H -#define HEADER_H - -#include "bad_include.h" -// expected-error@6{{'bad_include.h' file not found}} - -template struct enable_if {}; -template struct enable_if { typedef T type; }; -template using enable_if_t = typename enable_if::type; - -template struct meta { static constexpr int value = 0; }; -template <> struct meta { static constexpr int value = 1; }; -template <> struct meta { static constexpr int value = 2; }; - -namespace N { -inline namespace inner { - -template -constexpr enable_if_t::value == 0, void> midpoint(T) {} - -template -constexpr enable_if_t::value == 1, void> midpoint(U) {} - -template -constexpr enable_if_t::value == 2, void> midpoint(F) {} - -} // namespace inner -} // namespace N - -#else - -// expected-error@27{{'N::midpoint' has different definitions in different modules; defined here first difference is 1st parameter with type 'F'}} -// expected-error@24{{'N::midpoint' has different definitions in different modules; defined here first difference is 1st parameter with type 'U'}} -// expected-note@21{{but in '' found 1st parameter with type 'T'}} -int x = N::something; -// expected-error@37{{no member named 'something' in namespace 'N'}} -// expected-note@21{{but in '' found 1st parameter with type 'T'}} - -#endif diff --git a/clang/unittests/Basic/DiagnosticTest.cpp b/clang/unittests/Basic/DiagnosticTest.cpp index 691d74f697f27..7469019391716 100644 --- a/clang/unittests/Basic/DiagnosticTest.cpp +++ b/clang/unittests/Basic/DiagnosticTest.cpp @@ -17,6 +17,9 @@ using namespace llvm; using namespace clang; void clang::DiagnosticsTestHelper(DiagnosticsEngine &diag) { + unsigned delayedDiagID = 0U; + + EXPECT_EQ(diag.DelayedDiagID, delayedDiagID); EXPECT_FALSE(diag.DiagStates.empty()); EXPECT_TRUE(diag.DiagStatesByLoc.empty()); EXPECT_TRUE(diag.DiagStateOnPushStack.empty()); @@ -80,21 +83,6 @@ TEST(DiagnosticTest, fatalsAsError) { } } -TEST(DiagnosticTest, tooManyErrorsIsAlwaysFatal) { - DiagnosticsEngine Diags(new DiagnosticIDs(), new DiagnosticOptions, - new IgnoringDiagConsumer()); - Diags.setFatalsAsError(true); - - // Report a fatal_too_many_errors diagnostic to ensure that still - // acts as a fatal error despite downgrading fatal errors to errors. - Diags.Report(diag::fatal_too_many_errors); - EXPECT_TRUE(Diags.hasFatalErrorOccurred()); - - // Ensure that the severity of that diagnostic is really "fatal". - EXPECT_EQ(Diags.getDiagnosticLevel(diag::fatal_too_many_errors, {}), - DiagnosticsEngine::Level::Fatal); -} - // Check that soft RESET works as intended TEST(DiagnosticTest, softReset) { DiagnosticsEngine Diags(new DiagnosticIDs(), new DiagnosticOptions, @@ -116,6 +104,7 @@ TEST(DiagnosticTest, softReset) { // Check for private variables of DiagnosticsEngine differentiating soft reset DiagnosticsTestHelper(Diags); + EXPECT_FALSE(Diags.isDiagnosticInFlight()); EXPECT_TRUE(Diags.isLastDiagnosticIgnored()); } diff --git a/clang/unittests/Driver/DXCModeTest.cpp b/clang/unittests/Driver/DXCModeTest.cpp index 2a079a62f1bc1..41ab30bc81d5f 100644 --- a/clang/unittests/Driver/DXCModeTest.cpp +++ b/clang/unittests/Driver/DXCModeTest.cpp @@ -51,6 +51,7 @@ static void validateTargetProfile( EXPECT_TRUE(C); EXPECT_EQ(Diags.getNumErrors(), NumOfErrors); EXPECT_STREQ(DiagConsumer->Errors.back().c_str(), ExpectError.data()); + Diags.Clear(); DiagConsumer->clear(); } @@ -159,6 +160,7 @@ TEST(DxcModeTest, ValidatorVersionValidation) { DiagConsumer->Errors.back().c_str(), "invalid validator version : 0.1; if validator major version is 0, " "minor version must also be 0"); + Diags.Clear(); DiagConsumer->clear(); Args = TheDriver.ParseArgStrings({"-validator-version", "1"}, false, @@ -174,6 +176,7 @@ TEST(DxcModeTest, ValidatorVersionValidation) { EXPECT_STREQ(DiagConsumer->Errors.back().c_str(), "invalid validator version : 1; format of validator version is " "\".\" (ex:\"1.4\")"); + Diags.Clear(); DiagConsumer->clear(); Args = TheDriver.ParseArgStrings({"-validator-version", "-Tlib_6_7"}, false, @@ -190,6 +193,7 @@ TEST(DxcModeTest, ValidatorVersionValidation) { DiagConsumer->Errors.back().c_str(), "invalid validator version : -Tlib_6_7; format of validator version is " "\".\" (ex:\"1.4\")"); + Diags.Clear(); DiagConsumer->clear(); Args = TheDriver.ParseArgStrings({"-validator-version", "foo"}, false, @@ -206,6 +210,7 @@ TEST(DxcModeTest, ValidatorVersionValidation) { DiagConsumer->Errors.back().c_str(), "invalid validator version : foo; format of validator version is " "\".\" (ex:\"1.4\")"); + Diags.Clear(); DiagConsumer->clear(); }