Skip to content

Fix OOM in FormatDiagnostic (2nd attempt) #108866

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

igelbox
Copy link
Contributor

@igelbox igelbox commented Sep 16, 2024

The last attempt failed - #108187 (comment)
so was reverted - #108838

Not quite sure how to build targets available, so I run the same cmake and the ninja w/o arguments:

(resolves #70930)

Copy link

github-actions bot commented Sep 16, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@igelbox igelbox force-pushed the fix--race-condition-caused--infinity-cycle--memory-leak branch from 57b1dab to acfabf3 Compare September 16, 2024 18:23
@igelbox
Copy link
Contributor Author

igelbox commented Sep 16, 2024

Errors were fixed. If there's some other make target that could catch even more errors - please lemme know.
cc: @AaronBallman

I'm gonna remove Draft from this PR after check-all target finish with OK-state.

UPD: if my approach of merging main into the old branch and opening a new PR isn't OK - please lemme know and maybe advice some more appropriate one.

@igelbox
Copy link
Contributor Author

igelbox commented Sep 16, 2024

Testing Time: 301.48s
Total Discovered Tests: 105926
Skipped : 23 (0.02%)
Unsupported : 2682 (2.53%)
Passed : 103036 (97.27%)
Expectedly Failed: 185 (0.17%)
1 warning(s) in tests

@igelbox igelbox marked this pull request as ready for review September 16, 2024 18:59
@igelbox igelbox requested a review from Endilll as a code owner September 16, 2024 18:59
@llvmbot llvmbot added clang Clang issues not falling into any other category clang-tools-extra clangd 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 flang:driver flang Flang issues not falling into any other category labels Sep 16, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 16, 2024

@llvm/pr-subscribers-flang-driver
@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-clang-modules
@llvm/pr-subscribers-clang-tidy
@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangd

Author: Vakhurin Sergei (igelbox)

Changes

The last attempt failed - #108187 (comment)
so was reverted - #108838

Not quite sure how to build targets available, so I run the same cmake and the ninja w/o arguments:


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

18 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp (-2)
  • (modified) clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp (+6-6)
  • (modified) clang/include/clang/Basic/Diagnostic.h (+90-180)
  • (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 (+42-43)
  • (modified) clang/lib/Basic/DiagnosticIDs.cpp (+11-10)
  • (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)
  • (added) clang/test/PCH/race-condition.cpp (+41)
  • (modified) clang/unittests/Basic/DiagnosticTest.cpp (+15-4)
  • (modified) clang/unittests/Driver/DXCModeTest.cpp (-5)
  • (modified) flang/lib/Frontend/TextDiagnosticPrinter.cpp (+1-1)
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-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
index 021d731f8f1768..cf9b42828568da 100644
--- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -305,33 +305,33 @@ TEST_F(ConfigCompileTests, DiagnosticSuppression) {
   {
     auto D = DiagEngine.Report(diag::warn_unreachable);
     EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
+        Diag{&DiagEngine, D}, Conf.Diagnostics.Suppress, LangOptions()));
   }
   // Subcategory not respected/suppressed.
   {
     auto D = DiagEngine.Report(diag::warn_unreachable_break);
     EXPECT_FALSE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
+        Diag{&DiagEngine, D}, Conf.Diagnostics.Suppress, LangOptions()));
   }
   {
     auto D = DiagEngine.Report(diag::warn_unused_variable);
     EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
+        Diag{&DiagEngine, D}, Conf.Diagnostics.Suppress, LangOptions()));
   }
   {
     auto D = DiagEngine.Report(diag::err_typecheck_bool_condition);
     EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
+        Diag{&DiagEngine, D}, Conf.Diagnostics.Suppress, LangOptions()));
   }
   {
     auto D = DiagEngine.Report(diag::err_unexpected_friend);
     EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
+        Diag{&DiagEngine, D}, Conf.Diagnostics.Suppress, LangOptions()));
   }
   {
     auto D = DiagEngine.Report(diag::warn_alloca);
     EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
+        Diag{&DiagEngine, D}, Conf.Diagnostics.Suppress, LangOptions()));
   }
 
   Frag.Diagnostics.Suppress.emplace_back("*");
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index 54b69e98540239..e17ed8f98afa9a 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
@@ -522,27 +557,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,
@@ -949,70 +963,18 @@ 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
-  // tradeoff to keep these objects small.  Assertions verify that only one
-  // diagnostic is in flight at a time.
+  // 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.
   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.
     ///
@@ -1022,7 +984,7 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
     MaxArguments = DiagnosticStorage::MaxArguments,
   };
 
-  DiagnosticStorage DiagStorage;
+  DiagStorageAllocator DiagAllocator;
 
   DiagnosticMapping makeUserMapping(diag::Severity Map, SourceLocation L) {
     bool isPragma = L.isValid();
@@ -1042,8 +1004,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
@@ -1058,14 +1020,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);
 
   /// @}
 };
@@ -1118,40 +1076,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;
@@ -1240,11 +1165,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)
@@ -1275,9 +1195,20 @@ 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)),
@@ -1291,16 +1222,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 DiagLoc,
+                    unsigned DiagID);
 
 protected:
   /// Clear out the current diagnostic.
@@ -1326,7 +1249,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();
@@ -1337,13 +1260,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!");
@@ -1375,7 +1292,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 {
@@ -1550,12 +1467,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,24 +1482,29 @@ inline DiagnosticBuilder DiagnosticsEngine::Report(unsigned DiagID) {
 //===----------------------------------------------------------------------===//
 
 /// A little helper class (which is basically a smart pointer that forwards
-/// info from DiagnosticsEngine) that allows clients to enquire about the
-/// currently in-flight diagnostic.
+/// info from DiagnosticsEngine and DiagnosticStorage) that allows clients to
+/// enquire about the diagnostic.
 class Diagnostic {
   const DiagnosticsEngine *DiagObj;
+  SourceLocation DiagLoc;
+  unsigned DiagID;
+  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 DiagLoc,
+             unsigned DiagID, 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 DiagID; }
+  const SourceLocation &getLocation() const { return DiagLoc; }
   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.
   ///
@@ -1597,8 +1514,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.
@@ -1606,7 +1522,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.
@@ -1614,8 +1530,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.
@@ -1623,7 +1538,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.
@@ -1631,7 +1546,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.
@@ -1640,7 +1555,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.
@@ -1648,37 +1563,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!"...
[truncated]

@igelbox
Copy link
Contributor Author

igelbox commented Sep 18, 2024

Well, the more it waits the bigger a probability it will have logical conflicts after merging into main.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I think this fixes everything caught by post-commit, so let's re-land.

@AaronBallman AaronBallman merged commit eda72fa into llvm:main Sep 18, 2024
19 checks passed
@igelbox
Copy link
Contributor Author

igelbox commented Sep 18, 2024

30 of 31, this time is much closer)
image

it seems like those builds were broken a couple hours ago:
image

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
@kosarev
Copy link
Collaborator

kosarev commented Oct 18, 2024

@igelbox This seems to cause Clang-Unit :: Rename/./ClangRenameTests/* failures on my machine. Still reproduces on the current top of branch, dbe47c2. Anyone else can see the failures?

The Clang-Unit :: AST/Interp/./InterpTests/* failures are from before.

$ ninja check-clang-unit
...
Failed Tests (38):
  Clang-Unit :: AST/Interp/./InterpTests/1/5
  Clang-Unit :: AST/Interp/./InterpTests/2/5
  Clang-Unit :: AST/Interp/./InterpTests/3/5
  Clang-Unit :: AST/Interp/./InterpTests/4/5
  Clang-Unit :: AST/Interp/./InterpTests/Descriptor/Primitives
  Clang-Unit :: Rename/./ClangRenameTests/0/33
  Clang-Unit :: Rename/./ClangRenameTests/1/33
  Clang-Unit :: Rename/./ClangRenameTests/10/33
  Clang-Unit :: Rename/./ClangRenameTests/11/33
  Clang-Unit :: Rename/./ClangRenameTests/12/33
  Clang-Unit :: Rename/./ClangRenameTests/13/33
  Clang-Unit :: Rename/./ClangRenameTests/14/33
  Clang-Unit :: Rename/./ClangRenameTests/15/33
  Clang-Unit :: Rename/./ClangRenameTests/16/33
  Clang-Unit :: Rename/./ClangRenameTests/17/33
  Clang-Unit :: Rename/./ClangRenameTests/18/33
  Clang-Unit :: Rename/./ClangRenameTests/19/33
  Clang-Unit :: Rename/./ClangRenameTests/2/33
  Clang-Unit :: Rename/./ClangRenameTests/20/33
  Clang-Unit :: Rename/./ClangRenameTests/21/33
  Clang-Unit :: Rename/./ClangRenameTests/22/33
  Clang-Unit :: Rename/./ClangRenameTests/23/33
  Clang-Unit :: Rename/./ClangRenameTests/24/33
  Clang-Unit :: Rename/./ClangRenameTests/25/33
  Clang-Unit :: Rename/./ClangRenameTests/26/33
  Clang-Unit :: Rename/./ClangRenameTests/27/33
  Clang-Unit :: Rename/./ClangRenameTests/28/33
  Clang-Unit :: Rename/./ClangRenameTests/29/33
  Clang-Unit :: Rename/./ClangRenameTests/3/33
  Clang-Unit :: Rename/./ClangRenameTests/30/33
  Clang-Unit :: Rename/./ClangRenameTests/31/33
  Clang-Unit :: Rename/./ClangRenameTests/32/33
  Clang-Unit :: Rename/./ClangRenameTests/4/33
  Clang-Unit :: Rename/./ClangRenameTests/5/33
  Clang-Unit :: Rename/./ClangRenameTests/6/33
  Clang-Unit :: Rename/./ClangRenameTests/7/33
  Clang-Unit :: Rename/./ClangRenameTests/8/33
  Clang-Unit :: Rename/./ClangRenameTests/9/33


Testing Time: 5.04s

Total Discovered Tests: 18068
  Skipped:     4 (0.02%)
  Passed : 18026 (99.77%)
  Failed :    38 (0.21%)
cmake \
  -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \
  -DCMAKE_C_COMPILER=clang \
  -DCMAKE_CXX_COMPILER=clang++ \
  -DLLVM_ENABLE_WERROR=OFF \
  -DLLVM_USE_LINKER=gold \
  -DBUILD_SHARED_LIBS=ON \
  -DCMAKE_BUILD_TYPE=RELEASE \
  -DCMAKE_INSTALL_PREFIX=.. \
  -DLLVM_ENABLE_ASSERTIONS=ON \
  -DLLVM_ENABLE_PROJECTS=clang \
  -GNinja \

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 clangd flang:driver flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Multiple diagnostics in flight at once!" assert, and runaway memory usage in release builds, on C++20 testcase with PCH
4 participants