Skip to content

Revert "Fix OOM in FormatDiagnostic" #108838

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

AaronBallman
Copy link
Collaborator

Reverts #108187

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
This reverts commit e5d2556.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang-tools-extra clang-tidy clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Sep 16, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 16, 2024

@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-modules

Author: Aaron Ballman (AaronBallman)

Changes

Reverts llvm/llvm-project#108187


Patch is 44.93 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/108838.diff

16 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp (+2)
  • (modified) clang/include/clang/Basic/Diagnostic.h (+180-90)
  • (modified) clang/include/clang/Basic/DiagnosticIDs.h (+2-5)
  • (modified) clang/include/clang/Basic/PartialDiagnostic.h (+4-1)
  • (modified) clang/include/clang/Sema/Sema.h (+3-3)
  • (modified) clang/lib/Basic/Diagnostic.cpp (+43-42)
  • (modified) clang/lib/Basic/DiagnosticIDs.cpp (+10-11)
  • (modified) clang/lib/Basic/SourceManager.cpp (+19-4)
  • (modified) clang/lib/Frontend/Rewrite/FixItRewriter.cpp (+3-1)
  • (modified) clang/lib/Frontend/TextDiagnosticPrinter.cpp (+1-1)
  • (modified) clang/lib/Sema/Sema.cpp (+13-6)
  • (modified) clang/lib/Sema/SemaBase.cpp (+1-1)
  • (modified) clang/lib/Serialization/ASTReader.cpp (+10-5)
  • (removed) clang/test/PCH/race-condition.cpp (-41)
  • (modified) clang/unittests/Basic/DiagnosticTest.cpp (+4-15)
  • (modified) clang/unittests/Driver/DXCModeTest.cpp (+5)
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 4c75b422701148..200bb87a5ac3cb 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 e17ed8f98afa9a..54b69e98540239 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<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,
@@ -963,18 +949,70 @@ 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 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<unsigned>::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<DiagnosticsEngine> {
     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<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(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<DiagnosticsEngine> {
   // 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=<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)),
@@ -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 <typename T> 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<unsigned>::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<StringRef> 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<const char *>(DiagStorage.DiagArgumentsVal[Idx]);
+    return reinterpret_cast<const char *>(
+        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<IdentifierInfo *>(
-        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<CharSourceRange> getRanges() const { return DiagStorage.DiagRanges; }
+  ArrayRef<CharSourceRange> 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<FixItHint> getFixItHints() const { return DiagStorage.FixItHints; }
-
-  /// Return the value associated with this diagnostic flag.
-  StringRef getFlagValue() const { return FlagValue; }
+  ArrayRef<FixItHint> 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 1fa38ed6066e26..daad66f499538f 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<DiagnosticIDs> {
   ///
   /// \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 4bf6049d08fdb4..507d789c54ff9b 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,
    ...
[truncated]

@AaronBallman AaronBallman merged commit 5cead0c into main Sep 16, 2024
11 of 14 checks passed
@AaronBallman AaronBallman deleted the revert-108187-fix--race-condition-caused--infinity-cycle--memory-leak branch September 16, 2024 14:49
AaronBallman pushed a commit that referenced this pull request Sep 18, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Resolves: #70930 (and probably latest comments from clangd/clangd#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 DiagnosticBuilders having independent states.
Also, encapsulating the rest of state (e.g. ID and Loc) into DiagnosticBuilder.

The last attempt failed -
#108187 (comment)
so was reverted - #108838
tmsri pushed a commit to tmsri/llvm-project that referenced this pull request Sep 19, 2024
Resolves: llvm#70930 (and probably latest comments from clangd/clangd#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 DiagnosticBuilders having independent states.
Also, encapsulating the rest of state (e.g. ID and Loc) into DiagnosticBuilder.

The last attempt failed -
llvm#108187 (comment)
so was reverted - llvm#108838
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category clang-tidy clang-tools-extra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants