llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Vakhurin Sergei (igelbox) <details> <summary>Changes</summary> Resolves: #<!-- -->70930 (and probably latest comments from https://github.com/clangd/clangd/issues/251) by fixing racing for the shared `DiagStorage` value which caused messing with args inside the storage and then formatting the following message with `getArgSInt(1)` == 2: ``` def err_module_odr_violation_function : Error< "%q0 has different definitions in different modules; " "%select{definition in module '%2'|defined here}1 " "first difference is " ``` which causes `HandleSelectModifier` to go beyond the `ArgumentLen` so the recursive call to `FormatDiagnostic` was made with `DiagStr` > `DiagEnd` that leads to infinite `while (DiagStr != DiagEnd)`. **The Main Idea:** Reuse the existing `DiagStorageAllocator` logic to make all `DiagnosticBuilder`s having independent states. Also, encapsulating the rest of state (e.g. ID and Loc) into `DiagnosticBuilder`. **TODO (if it will be requested by reviewer):** - [ ] add a test (I have no idea how to turn a whole bunch of my proprietary code which leads `clangd` to OOM into a small public example.. probably I must try using [this](https://github.com/llvm/llvm-project/issues/70930#issuecomment-2209872975) instead) - [ ] [`Diag.CurDiagID != diag::fatal_too_many_errors`](https://github.com/llvm/llvm-project/pull/108187#pullrequestreview-2296395489) - [ ] ? get rid of `DiagStorageAllocator` at all and make `DiagnosticBuilder` having they own `DiagnosticStorage` coz it seems pretty small so should fit the stack for short-living `DiagnosticBuilder` instances --- Patch is 42.21 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/108187.diff 15 Files Affected: - (modified) clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp (-2) - (modified) clang/include/clang/Basic/Diagnostic.h (+93-176) - (modified) clang/include/clang/Basic/DiagnosticIDs.h (+5-2) - (modified) clang/include/clang/Basic/PartialDiagnostic.h (+1-4) - (modified) clang/include/clang/Sema/Sema.h (+3-3) - (modified) clang/lib/Basic/Diagnostic.cpp (+43-43) - (modified) clang/lib/Basic/DiagnosticIDs.cpp (+11-11) - (modified) clang/lib/Basic/SourceManager.cpp (+4-19) - (modified) clang/lib/Frontend/Rewrite/FixItRewriter.cpp (+1-3) - (modified) clang/lib/Frontend/TextDiagnosticPrinter.cpp (+1-1) - (modified) clang/lib/Sema/Sema.cpp (+6-13) - (modified) clang/lib/Sema/SemaBase.cpp (+1-1) - (modified) clang/lib/Serialization/ASTReader.cpp (+5-10) - (modified) clang/unittests/Basic/DiagnosticTest.cpp (-4) - (modified) clang/unittests/Driver/DXCModeTest.cpp (-5) ``````````diff diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp index 200bb87a5ac3cb..4c75b422701148 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -380,7 +380,6 @@ 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; @@ -457,7 +456,6 @@ 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 0c7836c2ea569c..1eecab4f6e49a2 100644 --- a/clang/include/clang/Basic/Diagnostic.h +++ b/clang/include/clang/Basic/Diagnostic.h @@ -183,6 +183,41 @@ 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 @@ -520,27 +555,6 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> { 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=<value> and - /// -Rpass=<value>. The content of this string is emitted after the flag name - /// and '='. - std::string FlagValue; - public: explicit DiagnosticsEngine(IntrusiveRefCntPtr<DiagnosticIDs> Diags, IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts, @@ -945,50 +959,11 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> { void Report(const StoredDiagnostic &storedDiag); - /// Determine whethere there is already a diagnostic in flight. - bool isDiagnosticInFlight() const { - return CurDiagID != std::numeric_limits<unsigned>::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<unsigned>::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 one - // diagnostic "in flight" at a time, but this seems to be a reasonable + // 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. Assertions verify that only one // diagnostic is in flight at a time. friend class Diagnostic; @@ -997,18 +972,6 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> { 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<unsigned>::max() when there is no - /// diagnostic in flight. - unsigned CurDiagID; - enum { /// The maximum number of arguments we can hold. /// @@ -1018,7 +981,7 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> { MaxArguments = DiagnosticStorage::MaxArguments, }; - DiagnosticStorage DiagStorage; + DiagStorageAllocator DiagAllocator; DiagnosticMapping makeUserMapping(diag::Severity Map, SourceLocation L) { bool isPragma = L.isValid(); @@ -1038,8 +1001,8 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> { /// Used to report a diagnostic that is finally fully formed. /// /// \returns true if the diagnostic was emitted, false if it was suppressed. - bool ProcessDiag() { - return Diags->ProcessDiag(*this); + bool ProcessDiag(const DiagnosticBuilder &DiagBuilder) { + return Diags->ProcessDiag(*this, DiagBuilder); } /// @name Diagnostic Emission @@ -1054,14 +1017,10 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> { // Sema::Diag() patterns. friend class Sema; - /// Emit the current diagnostic and clear the diagnostic state. + /// Emit the diagnostic /// /// \param Force Emit the diagnostic regardless of suppression settings. - bool EmitCurrentDiagnostic(bool Force = false); - - unsigned getCurrentDiagID() const { return CurDiagID; } - - SourceLocation getCurrentDiagLoc() const { return CurDiagLoc; } + bool EmitDiagnostic(const DiagnosticBuilder &DB, bool Force = false); /// @} }; @@ -1114,40 +1073,7 @@ class DiagnosticErrorTrap { /// class StreamingDiagnostic { public: - /// 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; - } - }; + using DiagStorageAllocator = clang::DiagStorageAllocator; protected: mutable DiagnosticStorage *DiagStorage = nullptr; @@ -1236,11 +1162,6 @@ 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) @@ -1271,9 +1192,26 @@ class StreamingDiagnostic { class DiagnosticBuilder : public StreamingDiagnostic { friend class DiagnosticsEngine; friend class PartialDiagnostic; + friend class Diagnostic; mutable DiagnosticsEngine *DiagObj = nullptr; + /// 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<unsigned>::max() when there is no + /// diagnostic in flight. + unsigned CurDiagID; + + /// Optional flag value. + /// + /// Some flags accept values, for instance: -Wframe-larger-than=<value> and + /// -Rpass=<value>. 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)), @@ -1287,16 +1225,8 @@ class DiagnosticBuilder : public StreamingDiagnostic { DiagnosticBuilder() = default; - 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(); - } + DiagnosticBuilder(DiagnosticsEngine *diagObj, SourceLocation CurDiagLoc, + unsigned CurDiagID); protected: /// Clear out the current diagnostic. @@ -1322,7 +1252,7 @@ class DiagnosticBuilder : public StreamingDiagnostic { if (!isActive()) return false; // Process the diagnostic. - bool Result = DiagObj->EmitCurrentDiagnostic(IsForceEmit); + bool Result = DiagObj->EmitDiagnostic(*this, IsForceEmit); // This diagnostic is dead. Clear(); @@ -1333,13 +1263,7 @@ class DiagnosticBuilder : public StreamingDiagnostic { public: /// Copy constructor. When copied, this "takes" the diagnostic info from the /// input and neuters it. - DiagnosticBuilder(const DiagnosticBuilder &D) : StreamingDiagnostic() { - DiagObj = D.DiagObj; - DiagStorage = D.DiagStorage; - IsActive = D.IsActive; - IsForceEmit = D.IsForceEmit; - D.Clear(); - } + DiagnosticBuilder(const DiagnosticBuilder &D); template <typename T> const DiagnosticBuilder &operator<<(const T &V) const { assert(isActive() && "Clients must not add to cleared diagnostic!"); @@ -1371,7 +1295,7 @@ class DiagnosticBuilder : public StreamingDiagnostic { return *this; } - void addFlagValue(StringRef V) const { DiagObj->FlagValue = std::string(V); } + void addFlagValue(StringRef V) const { FlagValue = std::string(V); } }; struct AddFlagValue { @@ -1546,12 +1470,7 @@ const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB, inline DiagnosticBuilder DiagnosticsEngine::Report(SourceLocation Loc, unsigned DiagID) { - assert(CurDiagID == std::numeric_limits<unsigned>::max() && - "Multiple diagnostics in flight at once!"); - CurDiagLoc = Loc; - CurDiagID = DiagID; - FlagValue.clear(); - return DiagnosticBuilder(this); + return DiagnosticBuilder(this, Loc, DiagID); } const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB, @@ -1570,20 +1489,25 @@ inline DiagnosticBuilder DiagnosticsEngine::Report(unsigned DiagID) { /// currently in-flight diagnostic. class Diagnostic { const DiagnosticsEngine *DiagObj; + SourceLocation CurDiagLoc; + unsigned CurDiagID; + std::string FlagValue; + const DiagnosticStorage &DiagStorage; std::optional<StringRef> StoredDiagMessage; public: - explicit Diagnostic(const DiagnosticsEngine *DO) : DiagObj(DO) {} - Diagnostic(const DiagnosticsEngine *DO, StringRef storedDiagMessage) - : DiagObj(DO), StoredDiagMessage(storedDiagMessage) {} + Diagnostic(const DiagnosticsEngine *DO, const DiagnosticBuilder &DiagBuilder); + Diagnostic(const DiagnosticsEngine *DO, SourceLocation CurDiagLoc, + unsigned CurDiagID, const DiagnosticStorage &DiagStorage, + StringRef storedDiagMessage); const DiagnosticsEngine *getDiags() const { return DiagObj; } - unsigned getID() const { return DiagObj->CurDiagID; } - const SourceLocation &getLocation() const { return DiagObj->CurDiagLoc; } + unsigned getID() const { return CurDiagID; } + const SourceLocation &getLocation() const { return CurDiagLoc; } bool hasSourceManager() const { return DiagObj->hasSourceManager(); } SourceManager &getSourceManager() const { return DiagObj->getSourceManager();} - unsigned getNumArgs() const { return DiagObj->DiagStorage.NumDiagArgs; } + unsigned getNumArgs() const { return DiagStorage.NumDiagArgs; } /// Return the kind of the specified index. /// @@ -1593,8 +1517,7 @@ class Diagnostic { /// \pre Idx < getNumArgs() DiagnosticsEngine::ArgumentKind getArgKind(unsigned Idx) const { assert(Idx < getNumArgs() && "Argument index out of range!"); - return (DiagnosticsEngine::ArgumentKind) - DiagObj->DiagStorage.DiagArgumentsKind[Idx]; + return (DiagnosticsEngine::ArgumentKind)DiagStorage.DiagArgumentsKind[Idx]; } /// Return the provided argument string specified by \p Idx. @@ -1602,7 +1525,7 @@ class Diagnostic { const std::string &getArgStdStr(unsigned Idx) const { assert(getArgKind(Idx) == DiagnosticsEngine::ak_std_string && "invalid argument accessor!"); - return DiagObj->DiagStorage.DiagArgumentsStr[Idx]; + return DiagStorage.DiagArgumentsStr[Idx]; } /// Return the specified C string argument. @@ -1610,8 +1533,7 @@ class Diagnostic { const char *getArgCStr(unsigned Idx) const { assert(getArgKind(Idx) == DiagnosticsEngine::ak_c_string && "invalid argument accessor!"); - return reinterpret_cast<const char *>( - DiagObj->DiagStorage.DiagArgumentsVal[Idx]); + return reinterpret_cast<const char *>(DiagStorage.DiagArgumentsVal[Idx]); } /// Return the specified signed integer argument. @@ -1619,7 +1541,7 @@ class Diagnostic { int64_t getArgSInt(unsigned Idx) const { assert(getArgKind(Idx) == DiagnosticsEngine::ak_sint && "invalid argument accessor!"); - return (int64_t)DiagObj->DiagStorage.DiagArgumentsVal[Idx]; + return (int64_t)DiagStorage.DiagArgumentsVal[Idx]; } /// Return the specified unsigned integer argument. @@ -1627,7 +1549,7 @@ class Diagnostic { uint64_t getArgUInt(unsigned Idx) const { assert(getArgKind(Idx) == DiagnosticsEngine::ak_uint && "invalid argument accessor!"); - return DiagObj->DiagStorage.DiagArgumentsVal[Idx]; + return DiagStorage.DiagArgumentsVal[Idx]; } /// Return the specified IdentifierInfo argument. @@ -1636,7 +1558,7 @@ class Diagnostic { assert(getArgKind(Idx) == DiagnosticsEngine::ak_identifierinfo && "invalid argument accessor!"); return reinterpret_cast<IdentifierInfo *>( - DiagObj->DiagStorage.DiagArgumentsVal[Idx]); + DiagStorage.DiagArgumentsVal[Idx]); } /// Return the specified non-string argument in an opaque form. @@ -1644,37 +1566,32 @@ class Diagnostic { uint64_t getRawArg(unsigned Idx) const { assert(getArgKind(Idx) != DiagnosticsEngine::ak_std_string && "invalid argument accessor!"); - return DiagObj->DiagStorage.DiagArgumentsVal[Idx]; + return DiagStorage.DiagArgumentsVal[Idx]; } /// Return the number of source ranges associated with this diagnostic. - unsigned getNumRanges() const { - return DiagObj->DiagStorage.DiagRanges.size(); - } + unsigned getNumRanges() const { return DiagStorage.DiagRanges.size(); } /// \pre Idx < getNumRanges() const CharSourceRange &getRange(unsigned Idx) const { assert(Idx < getNumRanges() && "Invalid diagnostic range index!"); - return DiagObj->DiagStorage.DiagRanges[Idx]; + return DiagStorage.DiagRanges[Idx]; } /// Return an array reference for this diagnostic's ranges. - ArrayRef<CharSourceRange> getRanges() const { - return DiagObj->DiagStorage.DiagRanges; - } + ArrayRef<CharSourceRange> getRanges() const { return DiagStorage.DiagRanges; } - unsigned getNumFixItHints() const { - return DiagObj->DiagStorage.FixItHints.size(); - } + unsigned getNumFixItHints() const { return DiagStorage.FixItHints.size(); } const FixItHint &getFixItHint(unsigned Idx) const { assert(Idx < getNumFixItHints() && "Invalid index!"); - return DiagObj->DiagStorage.FixItHints[Idx]; + return DiagStorage.FixItHints[Idx]; } - ArrayRef<FixItHint> getFixItHints() const { - return DiagObj->DiagStorage.FixItHints; - } + ArrayRef<FixItHint> getFixItHints() const { return DiagStorage.FixItHints; } + + /// Return the value associated with this diagnostic flag. + StringRef getFlagValue() const { return FlagValue; } /// 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 8b976bdac6dc51..f16fea6e798a95 100644 --- a/clang/include/clang/Basic/DiagnosticIDs.h +++ b/clang/include/clang/Basic/DiagnosticIDs.h @@ -22,6 +22,7 @@ namespace clang { class DiagnosticsEngine; + class DiagnosticBuilder; class SourceLocation; // Import the diagnostic enums themselves. @@ -367,11 +368,13 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> { /// /// \returns \c true if the diagnostic was emitted, \c false if it was /// suppressed. - bool ProcessDiag(DiagnosticsEngine &Diag) const; + bool ProcessDiag(DiagnosticsEngine &Diag, + const DiagnosticBuilder &DiagBuilder) const; /// Used to emit a diagnostic that is finally fully formed, /// ignoring suppression. - void EmitDiag(DiagnosticsEngine &Diag, Level DiagLevel) const; + void EmitDiag(DiagnosticsEngine &Diag, const DiagnosticBuilder &DiagBuilder, + 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 507d789c54ff9b..4bf6049d08fdb4 100644 --- a/clang/include/clang/Basic/PartialDiagnostic.h +++ b/clang/include/clang/Basic/PartialDiagnostic.h @@ -166,13 +166,10 @@ class PartialDiagnostic : public StreamingDiagnostic { void EmitToString(DiagnosticsEngine &Diags, SmallVectorImpl<char> &Buf) const { - // FIXME: It should be possible to render a diagno... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/108187 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
