[clang] [clang-tools-extra] Revert "Fix OOM in FormatDiagnostic" (PR #108838)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 16 07:49:14 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tidy
@llvm/pr-subscribers-clang-modules
Author: Aaron Ballman (AaronBallman)
<details>
<summary>Changes</summary>
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
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]
``````````
</details>
https://github.com/llvm/llvm-project/pull/108838
More information about the cfe-commits
mailing list