[flang-commits] [clang] [clang-tools-extra] [flang] Fix OOM in FormatDiagnostic (2nd attempt) (PR #108866)
Vakhurin Sergei via flang-commits
flang-commits at lists.llvm.org
Mon Sep 16 10:48:05 PDT 2024
https://github.com/igelbox created https://github.com/llvm/llvm-project/pull/108866
The last attempt failed - https://github.com/llvm/llvm-project/pull/108187#issuecomment-2353122096
so was reverted - #108838
Not quite sure how to build targets available, so I run the same `cmake` and the `ninja` w/o arguments:
- [x] [build-unified-tree](https://lab.llvm.org/buildbot/#/builders/157/builds/7831)
`cmake -DLLVM_TARGETS_TO_BUILD=PowerPC -DLLVM_INSTALL_UTILS=ON -DCMAKE_CXX_STANDARD=17 '-DLLVM_LIT_ARGS=-vj 256' -DFLANG_ENABLE_WERROR=ON -DLLVM_ENABLE_ASSERTIONS=ON '-DLLVM_ENABLE_PROJECTS=llvm;mlir;flang;clang' -DCMAKE_BUILD_TYPE=Release -GNinja ../llvm`
- [ ] [build-unified-tree](https://lab.llvm.org/buildbot/#/builders/190/builds/5825)
`cmake -DCMAKE_BUILD_TYPE=Release '-DLLVM_ENABLE_PROJECTS=llvm;cross-project-tests;lld;clang-tools-extra;clang' ../llvm`
- [ ] [build-unified-tree](https://lab.llvm.org/buildbot/#/builders/89/builds/6426)
- [x] ~[build-unified-tree](https://lab.llvm.org/buildbot/#/builders/153/builds/9025)~
ignored, the same error
- [ ] [test-build-clangd-clangd-index-server-clangd-indexer-check-clangd](https://lab.llvm.org/buildbot/#/builders/134/builds/5347)
- [ ] [ninja check](https://lab.llvm.org/buildbot/#/builders/65/builds/4601)
- [ ] [test-build-clangd-clangd-index-server-clangd-indexer-check-clangd](https://lab.llvm.org/buildbot/#/builders/134/builds/5347)
- [ ] [test-build-unified-tree-check-all](https://lab.llvm.org/buildbot/#/builders/56/builds/7472)
- [ ] [compile-openmp](https://lab.llvm.org/buildbot/#/builders/140/builds/6654)
>From aee4cf70dedaa3c8b7b6508238e01f92d60c631c Mon Sep 17 00:00:00 2001
From: Sergei <igelbox at gmail.com>
Date: Tue, 10 Sep 2024 16:19:05 +0000
Subject: [PATCH 1/5] fix quick OOM in FormatDiagnostic
---
.../ClangTidyDiagnosticConsumer.cpp | 2 -
clang/include/clang/Basic/Diagnostic.h | 269 ++++++------------
clang/include/clang/Basic/DiagnosticIDs.h | 7 +-
clang/include/clang/Basic/PartialDiagnostic.h | 5 +-
clang/include/clang/Sema/Sema.h | 6 +-
clang/lib/Basic/Diagnostic.cpp | 86 +++---
clang/lib/Basic/DiagnosticIDs.cpp | 22 +-
clang/lib/Basic/SourceManager.cpp | 23 +-
clang/lib/Frontend/Rewrite/FixItRewriter.cpp | 4 +-
clang/lib/Frontend/TextDiagnosticPrinter.cpp | 2 +-
clang/lib/Sema/Sema.cpp | 19 +-
clang/lib/Sema/SemaBase.cpp | 2 +-
clang/lib/Serialization/ASTReader.cpp | 15 +-
clang/unittests/Basic/DiagnosticTest.cpp | 4 -
clang/unittests/Driver/DXCModeTest.cpp | 5 -
15 files changed, 174 insertions(+), 297 deletions(-)
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 diagnostic to a string without
- // messing with the state of the diagnostics engine.
DiagnosticBuilder DB(Diags.Report(getDiagID()));
Emit(DB);
- Diagnostic(&Diags).FormatDiagnostic(Buf);
+ Diagnostic(&Diags, DB).FormatDiagnostic(Buf);
DB.Clear();
- Diags.Clear();
}
/// Clear out this partial diagnostic, giving it a new diagnostic ID
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 68c782a15c6f1b..d343ee99e9718c 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -626,10 +626,10 @@ class Sema final : public SemaBase {
const llvm::MapVector<FieldDecl *, DeleteLocs> &
getMismatchingDeleteExpressions() const;
- /// Cause the active diagnostic on the DiagosticsEngine to be
- /// emitted. This is closely coupled to the SemaDiagnosticBuilder class and
+ /// Cause the built diagnostic to be emitted on the DiagosticsEngine.
+ /// This is closely coupled to the SemaDiagnosticBuilder class and
/// should not be used elsewhere.
- void EmitCurrentDiagnostic(unsigned DiagID);
+ void EmitDiagnostic(unsigned DiagID, const DiagnosticBuilder &DB);
void addImplicitTypedef(StringRef Name, QualType T);
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index 66776daa5e1493..fc436687a50a1e 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -126,9 +126,7 @@ void DiagnosticsEngine::Reset(bool soft /*=false*/) {
TrapNumErrorsOccurred = 0;
TrapNumUnrecoverableErrorsOccurred = 0;
- CurDiagID = std::numeric_limits<unsigned>::max();
LastDiagLevel = DiagnosticIDs::Ignored;
- DelayedDiagID = 0;
if (!soft) {
// Clear state related to #pragma diagnostic.
@@ -143,23 +141,6 @@ void DiagnosticsEngine::Reset(bool soft /*=false*/) {
}
}
-void DiagnosticsEngine::SetDelayedDiagnostic(unsigned DiagID, StringRef Arg1,
- StringRef Arg2, StringRef Arg3) {
- if (DelayedDiagID)
- return;
-
- DelayedDiagID = DiagID;
- DelayedDiagArg1 = Arg1.str();
- DelayedDiagArg2 = Arg2.str();
- DelayedDiagArg3 = Arg3.str();
-}
-
-void DiagnosticsEngine::ReportDelayed() {
- unsigned ID = DelayedDiagID;
- DelayedDiagID = 0;
- Report(ID) << DelayedDiagArg1 << DelayedDiagArg2 << DelayedDiagArg3;
-}
-
DiagnosticMapping &
DiagnosticsEngine::DiagState::getOrAddMapping(diag::kind Diag) {
std::pair<iterator, bool> Result =
@@ -497,39 +478,31 @@ void DiagnosticsEngine::setSeverityForAll(diag::Flavor Flavor,
}
void DiagnosticsEngine::Report(const StoredDiagnostic &storedDiag) {
- assert(CurDiagID == std::numeric_limits<unsigned>::max() &&
- "Multiple diagnostics in flight at once!");
-
- CurDiagLoc = storedDiag.getLocation();
- CurDiagID = storedDiag.getID();
- DiagStorage.NumDiagArgs = 0;
-
- DiagStorage.DiagRanges.clear();
+ DiagnosticStorage DiagStorage;
DiagStorage.DiagRanges.append(storedDiag.range_begin(),
storedDiag.range_end());
- DiagStorage.FixItHints.clear();
DiagStorage.FixItHints.append(storedDiag.fixit_begin(),
storedDiag.fixit_end());
assert(Client && "DiagnosticConsumer not set!");
Level DiagLevel = storedDiag.getLevel();
- Diagnostic Info(this, storedDiag.getMessage());
+ Diagnostic Info(this, storedDiag.getLocation(), storedDiag.getID(),
+ DiagStorage, storedDiag.getMessage());
Client->HandleDiagnostic(DiagLevel, Info);
if (Client->IncludeInDiagnosticCounts()) {
if (DiagLevel == DiagnosticsEngine::Warning)
++NumWarnings;
}
-
- CurDiagID = std::numeric_limits<unsigned>::max();
}
-bool DiagnosticsEngine::EmitCurrentDiagnostic(bool Force) {
+bool DiagnosticsEngine::EmitDiagnostic(const DiagnosticBuilder &DB,
+ bool Force) {
assert(getClient() && "DiagnosticClient not set!");
bool Emitted;
if (Force) {
- Diagnostic Info(this);
+ Diagnostic Info(this, DB);
// Figure out the diagnostic level of this message.
DiagnosticIDs::Level DiagLevel
@@ -538,24 +511,51 @@ bool DiagnosticsEngine::EmitCurrentDiagnostic(bool Force) {
Emitted = (DiagLevel != DiagnosticIDs::Ignored);
if (Emitted) {
// Emit the diagnostic regardless of suppression level.
- Diags->EmitDiag(*this, DiagLevel);
+ Diags->EmitDiag(*this, DB, DiagLevel);
}
} else {
// Process the diagnostic, sending the accumulated information to the
// DiagnosticConsumer.
- Emitted = ProcessDiag();
+ Emitted = ProcessDiag(DB);
}
- // Clear out the current diagnostic object.
- Clear();
+ return Emitted;
+}
- // If there was a delayed diagnostic, emit it now.
- if (!Force && DelayedDiagID)
- ReportDelayed();
+DiagnosticBuilder::DiagnosticBuilder(DiagnosticsEngine *diagObj,
+ SourceLocation CurDiagLoc,
+ unsigned CurDiagID)
+ : StreamingDiagnostic(diagObj->DiagAllocator), DiagObj(diagObj),
+ CurDiagLoc(CurDiagLoc), CurDiagID(CurDiagID), IsActive(true) {
+ assert(diagObj && "DiagnosticBuilder requires a valid DiagnosticsEngine!");
+}
- return Emitted;
+DiagnosticBuilder::DiagnosticBuilder(const DiagnosticBuilder &D)
+ : StreamingDiagnostic() {
+ CurDiagLoc = D.CurDiagLoc;
+ CurDiagID = D.CurDiagID;
+ FlagValue = D.FlagValue;
+ DiagObj = D.DiagObj;
+ DiagStorage = D.DiagStorage;
+ D.DiagStorage = nullptr;
+ Allocator = D.Allocator;
+ IsActive = D.IsActive;
+ IsForceEmit = D.IsForceEmit;
+ D.Clear();
}
+Diagnostic::Diagnostic(const DiagnosticsEngine *DO,
+ const DiagnosticBuilder &DiagBuilder)
+ : DiagObj(DO), CurDiagLoc(DiagBuilder.CurDiagLoc),
+ CurDiagID(DiagBuilder.CurDiagID), FlagValue(DiagBuilder.FlagValue),
+ DiagStorage(*DiagBuilder.getStorage()) {}
+
+Diagnostic::Diagnostic(const DiagnosticsEngine *DO, SourceLocation CurDiagLoc,
+ unsigned CurDiagID, const DiagnosticStorage &DiagStorage,
+ StringRef storedDiagMessage)
+ : DiagObj(DO), CurDiagLoc(CurDiagLoc), CurDiagID(CurDiagID),
+ DiagStorage(DiagStorage), StoredDiagMessage(storedDiagMessage) {}
+
DiagnosticConsumer::~DiagnosticConsumer() = default;
void DiagnosticConsumer::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
@@ -1210,13 +1210,13 @@ bool ForwardingDiagnosticConsumer::IncludeInDiagnosticCounts() const {
return Target.IncludeInDiagnosticCounts();
}
-PartialDiagnostic::DiagStorageAllocator::DiagStorageAllocator() {
+DiagStorageAllocator::DiagStorageAllocator() {
for (unsigned I = 0; I != NumCached; ++I)
FreeList[I] = Cached + I;
NumFreeListEntries = NumCached;
}
-PartialDiagnostic::DiagStorageAllocator::~DiagStorageAllocator() {
+DiagStorageAllocator::~DiagStorageAllocator() {
// Don't assert if we are in a CrashRecovery context, as this invariant may
// be invalidated during a crash.
assert((NumFreeListEntries == NumCached ||
diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp
index cd42573968b212..4631f7f7e38858 100644
--- a/clang/lib/Basic/DiagnosticIDs.cpp
+++ b/clang/lib/Basic/DiagnosticIDs.cpp
@@ -571,8 +571,7 @@ DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
}
// If explicitly requested, map fatal errors to errors.
- if (Result == diag::Severity::Fatal &&
- Diag.CurDiagID != diag::fatal_too_many_errors && Diag.FatalsAsError)
+ if (Result == diag::Severity::Fatal && Diag.FatalsAsError)
Result = diag::Severity::Error;
// Custom diagnostics always are emitted in system headers.
@@ -748,8 +747,9 @@ StringRef DiagnosticIDs::getNearestOption(diag::Flavor Flavor,
/// ProcessDiag - This is the method used to report a diagnostic that is
/// finally fully formed.
-bool DiagnosticIDs::ProcessDiag(DiagnosticsEngine &Diag) const {
- Diagnostic Info(&Diag);
+bool DiagnosticIDs::ProcessDiag(DiagnosticsEngine &Diag,
+ const DiagnosticBuilder &DiagBuilder) const {
+ Diagnostic Info(&Diag, DiagBuilder);
assert(Diag.getClient() && "DiagnosticClient not set!");
@@ -815,22 +815,24 @@ bool DiagnosticIDs::ProcessDiag(DiagnosticsEngine &Diag) const {
// stop a flood of bogus errors.
if (Diag.ErrorLimit && Diag.NumErrors > Diag.ErrorLimit &&
DiagLevel == DiagnosticIDs::Error) {
- Diag.SetDelayedDiagnostic(diag::fatal_too_many_errors);
+ Diag.Report(diag::fatal_too_many_errors);
return false;
}
}
// Make sure we set FatalErrorOccurred to ensure that the notes from the
// diagnostic that caused `fatal_too_many_errors` won't be emitted.
- if (Diag.CurDiagID == diag::fatal_too_many_errors)
+ if (Info.getID() == diag::fatal_too_many_errors)
Diag.FatalErrorOccurred = true;
// Finally, report it.
- EmitDiag(Diag, DiagLevel);
+ EmitDiag(Diag, DiagBuilder, DiagLevel);
return true;
}
-void DiagnosticIDs::EmitDiag(DiagnosticsEngine &Diag, Level DiagLevel) const {
- Diagnostic Info(&Diag);
+void DiagnosticIDs::EmitDiag(DiagnosticsEngine &Diag,
+ const DiagnosticBuilder &DiagBuilder,
+ Level DiagLevel) const {
+ Diagnostic Info(&Diag, DiagBuilder);
assert(DiagLevel != DiagnosticIDs::Ignored && "Cannot emit ignored diagnostics!");
Diag.Client->HandleDiagnostic((DiagnosticsEngine::Level)DiagLevel, Info);
@@ -838,8 +840,6 @@ void DiagnosticIDs::EmitDiag(DiagnosticsEngine &Diag, Level DiagLevel) const {
if (DiagLevel == DiagnosticIDs::Warning)
++Diag.NumWarnings;
}
-
- Diag.CurDiagID = ~0U;
}
bool DiagnosticIDs::isUnrecoverable(unsigned DiagID) const {
diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp
index d6ec26af80aadd..65a8a7253e054f 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -130,13 +130,8 @@ ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM,
// the file could also have been removed during processing. Since we can't
// really deal with this situation, just create an empty buffer.
if (!BufferOrError) {
- if (Diag.isDiagnosticInFlight())
- Diag.SetDelayedDiagnostic(diag::err_cannot_open_file,
- ContentsEntry->getName(),
- BufferOrError.getError().message());
- else
- Diag.Report(Loc, diag::err_cannot_open_file)
- << ContentsEntry->getName() << BufferOrError.getError().message();
+ Diag.Report(Loc, diag::err_cannot_open_file)
+ << ContentsEntry->getName() << BufferOrError.getError().message();
return std::nullopt;
}
@@ -153,12 +148,7 @@ ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM,
// ContentsEntry::getSize() could have the wrong size. Use
// MemoryBuffer::getBufferSize() instead.
if (Buffer->getBufferSize() >= std::numeric_limits<unsigned>::max()) {
- if (Diag.isDiagnosticInFlight())
- Diag.SetDelayedDiagnostic(diag::err_file_too_large,
- ContentsEntry->getName());
- else
- Diag.Report(Loc, diag::err_file_too_large)
- << ContentsEntry->getName();
+ Diag.Report(Loc, diag::err_file_too_large) << ContentsEntry->getName();
return std::nullopt;
}
@@ -168,12 +158,7 @@ ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM,
// have come from a stat cache).
if (!ContentsEntry->isNamedPipe() &&
Buffer->getBufferSize() != (size_t)ContentsEntry->getSize()) {
- if (Diag.isDiagnosticInFlight())
- Diag.SetDelayedDiagnostic(diag::err_file_modified,
- ContentsEntry->getName());
- else
- Diag.Report(Loc, diag::err_file_modified)
- << ContentsEntry->getName();
+ Diag.Report(Loc, diag::err_file_modified) << ContentsEntry->getName();
return std::nullopt;
}
diff --git a/clang/lib/Frontend/Rewrite/FixItRewriter.cpp b/clang/lib/Frontend/Rewrite/FixItRewriter.cpp
index 44dfaf20eae73f..7309553e3bc0b4 100644
--- a/clang/lib/Frontend/Rewrite/FixItRewriter.cpp
+++ b/clang/lib/Frontend/Rewrite/FixItRewriter.cpp
@@ -200,10 +200,8 @@ void FixItRewriter::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
/// Emit a diagnostic via the adapted diagnostic client.
void FixItRewriter::Diag(SourceLocation Loc, unsigned DiagID) {
// When producing this diagnostic, we temporarily bypass ourselves,
- // clear out any current diagnostic, and let the downstream client
- // format the diagnostic.
+ // and let the downstream client format the diagnostic.
Diags.setClient(Client, false);
- Diags.Clear();
Diags.Report(Loc, DiagID);
Diags.setClient(this, false);
}
diff --git a/clang/lib/Frontend/TextDiagnosticPrinter.cpp b/clang/lib/Frontend/TextDiagnosticPrinter.cpp
index b2fb762537573e..dac5c44fe92566 100644
--- a/clang/lib/Frontend/TextDiagnosticPrinter.cpp
+++ b/clang/lib/Frontend/TextDiagnosticPrinter.cpp
@@ -80,7 +80,7 @@ static void printDiagnosticOptions(raw_ostream &OS,
if (!Opt.empty()) {
OS << (Started ? "," : " [")
<< (Level == DiagnosticsEngine::Remark ? "-R" : "-W") << Opt;
- StringRef OptValue = Info.getDiags()->getFlagValue();
+ StringRef OptValue = Info.getFlagValue();
if (!OptValue.empty())
OS << "=" << OptValue;
Started = true;
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 46ddd360870b4f..973eedf0fe22f5 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -1589,7 +1589,7 @@ LangAS Sema::getDefaultCXXMethodAddrSpace() const {
return LangAS::Default;
}
-void Sema::EmitCurrentDiagnostic(unsigned DiagID) {
+void Sema::EmitDiagnostic(unsigned DiagID, const DiagnosticBuilder &DB) {
// FIXME: It doesn't make sense to me that DiagID is an incoming argument here
// and yet we also use the current diag ID on the DiagnosticsEngine. This has
// been made more painfully obvious by the refactor that introduced this
@@ -1597,9 +1597,9 @@ void Sema::EmitCurrentDiagnostic(unsigned DiagID) {
// eliminated. If it truly cannot be (for example, there is some reentrancy
// issue I am not seeing yet), then there should at least be a clarifying
// comment somewhere.
+ Diagnostic DiagInfo(&Diags, DB);
if (std::optional<TemplateDeductionInfo *> Info = isSFINAEContext()) {
- switch (DiagnosticIDs::getDiagnosticSFINAEResponse(
- Diags.getCurrentDiagID())) {
+ switch (DiagnosticIDs::getDiagnosticSFINAEResponse(DiagInfo.getID())) {
case DiagnosticIDs::SFINAE_Report:
// We'll report the diagnostic below.
break;
@@ -1612,13 +1612,11 @@ void Sema::EmitCurrentDiagnostic(unsigned DiagID) {
// Make a copy of this suppressed diagnostic and store it with the
// template-deduction information.
if (*Info && !(*Info)->hasSFINAEDiagnostic()) {
- Diagnostic DiagInfo(&Diags);
(*Info)->addSFINAEDiagnostic(DiagInfo.getLocation(),
PartialDiagnostic(DiagInfo, Context.getDiagAllocator()));
}
Diags.setLastDiagnosticIgnored(true);
- Diags.Clear();
return;
case DiagnosticIDs::SFINAE_AccessControl: {
@@ -1629,7 +1627,7 @@ void Sema::EmitCurrentDiagnostic(unsigned DiagID) {
if (!AccessCheckingSFINAE && !getLangOpts().CPlusPlus11)
break;
- SourceLocation Loc = Diags.getCurrentDiagLoc();
+ SourceLocation Loc = DiagInfo.getLocation();
// Suppress this diagnostic.
++NumSFINAEErrors;
@@ -1637,16 +1635,13 @@ void Sema::EmitCurrentDiagnostic(unsigned DiagID) {
// Make a copy of this suppressed diagnostic and store it with the
// template-deduction information.
if (*Info && !(*Info)->hasSFINAEDiagnostic()) {
- Diagnostic DiagInfo(&Diags);
(*Info)->addSFINAEDiagnostic(DiagInfo.getLocation(),
PartialDiagnostic(DiagInfo, Context.getDiagAllocator()));
}
Diags.setLastDiagnosticIgnored(true);
- Diags.Clear();
- // Now the diagnostic state is clear, produce a C++98 compatibility
- // warning.
+ // Now produce a C++98 compatibility warning.
Diag(Loc, diag::warn_cxx98_compat_sfinae_access_control);
// The last diagnostic which Sema produced was ignored. Suppress any
@@ -1659,14 +1654,12 @@ void Sema::EmitCurrentDiagnostic(unsigned DiagID) {
// Make a copy of this suppressed diagnostic and store it with the
// template-deduction information;
if (*Info) {
- Diagnostic DiagInfo(&Diags);
(*Info)->addSuppressedDiagnostic(DiagInfo.getLocation(),
PartialDiagnostic(DiagInfo, Context.getDiagAllocator()));
}
// Suppress this diagnostic.
Diags.setLastDiagnosticIgnored(true);
- Diags.Clear();
return;
}
}
@@ -1676,7 +1669,7 @@ void Sema::EmitCurrentDiagnostic(unsigned DiagID) {
Context.setPrintingPolicy(getPrintingPolicy());
// Emit the diagnostic.
- if (!Diags.EmitCurrentDiagnostic())
+ if (!Diags.EmitDiagnostic(DB))
return;
// If this is not a note, and we're in a template instantiation
diff --git a/clang/lib/Sema/SemaBase.cpp b/clang/lib/Sema/SemaBase.cpp
index a2f12d622e8ccc..5c24f21b469b01 100644
--- a/clang/lib/Sema/SemaBase.cpp
+++ b/clang/lib/Sema/SemaBase.cpp
@@ -26,7 +26,7 @@ SemaBase::ImmediateDiagBuilder::~ImmediateDiagBuilder() {
Clear();
// Dispatch to Sema to emit the diagnostic.
- SemaRef.EmitCurrentDiagnostic(DiagID);
+ SemaRef.EmitDiagnostic(DiagID, *this);
}
PartialDiagnostic SemaBase::PDiag(unsigned DiagID) {
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 0ee53e43dff39c..720843e8b8b96f 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -1382,7 +1382,7 @@ bool ASTReader::ReadVisibleDeclContextStorage(ModuleFile &M,
void ASTReader::Error(StringRef Msg) const {
Error(diag::err_fe_pch_malformed, Msg);
- if (PP.getLangOpts().Modules && !Diags.isDiagnosticInFlight() &&
+ if (PP.getLangOpts().Modules &&
!PP.getHeaderSearchInfo().getModuleCachePath().empty()) {
Diag(diag::note_module_cache_path)
<< PP.getHeaderSearchInfo().getModuleCachePath();
@@ -1391,10 +1391,7 @@ void ASTReader::Error(StringRef Msg) const {
void ASTReader::Error(unsigned DiagID, StringRef Arg1, StringRef Arg2,
StringRef Arg3) const {
- if (Diags.isDiagnosticInFlight())
- Diags.SetDelayedDiagnostic(DiagID, Arg1, Arg2, Arg3);
- else
- Diag(DiagID) << Arg1 << Arg2 << Arg3;
+ Diag(DiagID) << Arg1 << Arg2 << Arg3;
}
void ASTReader::Error(llvm::Error &&Err) const {
@@ -2713,7 +2710,7 @@ InputFile ASTReader::getInputFile(ModuleFile &F, unsigned ID, bool Complain) {
// For an overridden file, there is nothing to validate.
if (!Overridden && FileChange.Kind != Change::None) {
- if (Complain && !Diags.isDiagnosticInFlight()) {
+ if (Complain) {
// Build a list of the PCH imports that got us here (in reverse).
SmallVector<ModuleFile *, 4> ImportStack(1, &F);
while (!ImportStack.back()->ImportedBy.empty())
@@ -3689,10 +3686,8 @@ llvm::Error ASTReader::ReadASTBlock(ModuleFile &F,
SourceMgr.AllocateLoadedSLocEntries(F.LocalNumSLocEntries,
SLocSpaceSize);
if (!F.SLocEntryBaseID) {
- if (!Diags.isDiagnosticInFlight()) {
- Diags.Report(SourceLocation(), diag::remark_sloc_usage);
- SourceMgr.noteSLocAddressSpaceUsage(Diags);
- }
+ Diags.Report(SourceLocation(), diag::remark_sloc_usage);
+ SourceMgr.noteSLocAddressSpaceUsage(Diags);
return llvm::createStringError(std::errc::invalid_argument,
"ran out of source locations");
}
diff --git a/clang/unittests/Basic/DiagnosticTest.cpp b/clang/unittests/Basic/DiagnosticTest.cpp
index 74690193917165..bc6258fdf4f004 100644
--- a/clang/unittests/Basic/DiagnosticTest.cpp
+++ b/clang/unittests/Basic/DiagnosticTest.cpp
@@ -17,9 +17,6 @@ using namespace llvm;
using namespace clang;
void clang::DiagnosticsTestHelper(DiagnosticsEngine &diag) {
- unsigned delayedDiagID = 0U;
-
- EXPECT_EQ(diag.DelayedDiagID, delayedDiagID);
EXPECT_FALSE(diag.DiagStates.empty());
EXPECT_TRUE(diag.DiagStatesByLoc.empty());
EXPECT_TRUE(diag.DiagStateOnPushStack.empty());
@@ -104,7 +101,6 @@ TEST(DiagnosticTest, softReset) {
// Check for private variables of DiagnosticsEngine differentiating soft reset
DiagnosticsTestHelper(Diags);
- EXPECT_FALSE(Diags.isDiagnosticInFlight());
EXPECT_TRUE(Diags.isLastDiagnosticIgnored());
}
diff --git a/clang/unittests/Driver/DXCModeTest.cpp b/clang/unittests/Driver/DXCModeTest.cpp
index 41ab30bc81d5f9..2a079a62f1bc13 100644
--- a/clang/unittests/Driver/DXCModeTest.cpp
+++ b/clang/unittests/Driver/DXCModeTest.cpp
@@ -51,7 +51,6 @@ static void validateTargetProfile(
EXPECT_TRUE(C);
EXPECT_EQ(Diags.getNumErrors(), NumOfErrors);
EXPECT_STREQ(DiagConsumer->Errors.back().c_str(), ExpectError.data());
- Diags.Clear();
DiagConsumer->clear();
}
@@ -160,7 +159,6 @@ TEST(DxcModeTest, ValidatorVersionValidation) {
DiagConsumer->Errors.back().c_str(),
"invalid validator version : 0.1; if validator major version is 0, "
"minor version must also be 0");
- Diags.Clear();
DiagConsumer->clear();
Args = TheDriver.ParseArgStrings({"-validator-version", "1"}, false,
@@ -176,7 +174,6 @@ TEST(DxcModeTest, ValidatorVersionValidation) {
EXPECT_STREQ(DiagConsumer->Errors.back().c_str(),
"invalid validator version : 1; format of validator version is "
"\"<major>.<minor>\" (ex:\"1.4\")");
- Diags.Clear();
DiagConsumer->clear();
Args = TheDriver.ParseArgStrings({"-validator-version", "-Tlib_6_7"}, false,
@@ -193,7 +190,6 @@ TEST(DxcModeTest, ValidatorVersionValidation) {
DiagConsumer->Errors.back().c_str(),
"invalid validator version : -Tlib_6_7; format of validator version is "
"\"<major>.<minor>\" (ex:\"1.4\")");
- Diags.Clear();
DiagConsumer->clear();
Args = TheDriver.ParseArgStrings({"-validator-version", "foo"}, false,
@@ -210,7 +206,6 @@ TEST(DxcModeTest, ValidatorVersionValidation) {
DiagConsumer->Errors.back().c_str(),
"invalid validator version : foo; format of validator version is "
"\"<major>.<minor>\" (ex:\"1.4\")");
- Diags.Clear();
DiagConsumer->clear();
}
>From 81e030d41982ee868311a09919e31ae1609b1ae9 Mon Sep 17 00:00:00 2001
From: Sergei <igelbox at gmail.com>
Date: Thu, 12 Sep 2024 17:12:06 +0000
Subject: [PATCH 2/5] address review
---
clang/include/clang/Basic/Diagnostic.h | 4 ++--
clang/lib/Basic/Diagnostic.cpp | 10 +++++-----
clang/lib/Basic/DiagnosticIDs.cpp | 3 ++-
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index 1eecab4f6e49a2..cee5375dc6285c 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -1225,7 +1225,7 @@ class DiagnosticBuilder : public StreamingDiagnostic {
DiagnosticBuilder() = default;
- DiagnosticBuilder(DiagnosticsEngine *diagObj, SourceLocation CurDiagLoc,
+ DiagnosticBuilder(DiagnosticsEngine *DiagObj, SourceLocation CurDiagLoc,
unsigned CurDiagID);
protected:
@@ -1499,7 +1499,7 @@ class Diagnostic {
Diagnostic(const DiagnosticsEngine *DO, const DiagnosticBuilder &DiagBuilder);
Diagnostic(const DiagnosticsEngine *DO, SourceLocation CurDiagLoc,
unsigned CurDiagID, const DiagnosticStorage &DiagStorage,
- StringRef storedDiagMessage);
+ StringRef StoredDiagMessage);
const DiagnosticsEngine *getDiags() const { return DiagObj; }
unsigned getID() const { return CurDiagID; }
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index fc436687a50a1e..484e8bf14f4fc8 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -522,12 +522,12 @@ bool DiagnosticsEngine::EmitDiagnostic(const DiagnosticBuilder &DB,
return Emitted;
}
-DiagnosticBuilder::DiagnosticBuilder(DiagnosticsEngine *diagObj,
+DiagnosticBuilder::DiagnosticBuilder(DiagnosticsEngine *DiagObj,
SourceLocation CurDiagLoc,
unsigned CurDiagID)
- : StreamingDiagnostic(diagObj->DiagAllocator), DiagObj(diagObj),
+ : StreamingDiagnostic(DiagObj->DiagAllocator), DiagObj(DiagObj),
CurDiagLoc(CurDiagLoc), CurDiagID(CurDiagID), IsActive(true) {
- assert(diagObj && "DiagnosticBuilder requires a valid DiagnosticsEngine!");
+ assert(DiagObj && "DiagnosticBuilder requires a valid DiagnosticsEngine!");
}
DiagnosticBuilder::DiagnosticBuilder(const DiagnosticBuilder &D)
@@ -552,9 +552,9 @@ Diagnostic::Diagnostic(const DiagnosticsEngine *DO,
Diagnostic::Diagnostic(const DiagnosticsEngine *DO, SourceLocation CurDiagLoc,
unsigned CurDiagID, const DiagnosticStorage &DiagStorage,
- StringRef storedDiagMessage)
+ StringRef StoredDiagMessage)
: DiagObj(DO), CurDiagLoc(CurDiagLoc), CurDiagID(CurDiagID),
- DiagStorage(DiagStorage), StoredDiagMessage(storedDiagMessage) {}
+ DiagStorage(DiagStorage), StoredDiagMessage(StoredDiagMessage) {}
DiagnosticConsumer::~DiagnosticConsumer() = default;
diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp
index 4631f7f7e38858..d45bb0f392d457 100644
--- a/clang/lib/Basic/DiagnosticIDs.cpp
+++ b/clang/lib/Basic/DiagnosticIDs.cpp
@@ -571,7 +571,8 @@ DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
}
// If explicitly requested, map fatal errors to errors.
- if (Result == diag::Severity::Fatal && Diag.FatalsAsError)
+ if (Result == diag::Severity::Fatal &&
+ DiagID != diag::fatal_too_many_errors && Diag.FatalsAsError)
Result = diag::Severity::Error;
// Custom diagnostics always are emitted in system headers.
>From b526067cd6a132add5a8fd6536aa3865e56908d9 Mon Sep 17 00:00:00 2001
From: Sergei <igelbox at gmail.com>
Date: Thu, 12 Sep 2024 23:58:22 +0000
Subject: [PATCH 3/5] add tests
---
clang/test/PCH/race-condition.cpp | 41 ++++++++++++++++++++++++
clang/unittests/Basic/DiagnosticTest.cpp | 15 +++++++++
2 files changed, 56 insertions(+)
create mode 100644 clang/test/PCH/race-condition.cpp
diff --git a/clang/test/PCH/race-condition.cpp b/clang/test/PCH/race-condition.cpp
new file mode 100644
index 00000000000000..752b0cc3ff6286
--- /dev/null
+++ b/clang/test/PCH/race-condition.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -fallow-pch-with-compiler-errors -std=c++20 -x c++-header -emit-pch %s -o %t -verify
+// RUN: %clang_cc1 -fallow-pch-with-compiler-errors -std=c++20 -include-pch %t %s -verify
+#ifndef HEADER_H
+#define HEADER_H
+
+#include "bad_include.h"
+// expected-error at 6{{'bad_include.h' file not found}}
+
+template <bool, class = void> struct enable_if {};
+template <class T> struct enable_if<true, T> { typedef T type; };
+template <bool B, class T = void> using enable_if_t = typename enable_if<B, T>::type;
+
+template <typename> struct meta { static constexpr int value = 0; };
+template <> struct meta<int> { static constexpr int value = 1; };
+template <> struct meta<float> { static constexpr int value = 2; };
+
+namespace N {
+inline namespace inner {
+
+template <class T>
+constexpr enable_if_t<meta<T>::value == 0, void> midpoint(T) {}
+
+template <class U>
+constexpr enable_if_t<meta<U>::value == 1, void> midpoint(U) {}
+
+template <class F>
+constexpr enable_if_t<meta<F>::value == 2, void> midpoint(F) {}
+
+} // namespace inner
+} // namespace N
+
+#else
+
+// expected-error at 27{{'N::midpoint' has different definitions in different modules; defined here first difference is 1st parameter with type 'F'}}
+// expected-error at 24{{'N::midpoint' has different definitions in different modules; defined here first difference is 1st parameter with type 'U'}}
+// expected-note at 21{{but in '' found 1st parameter with type 'T'}}
+int x = N::something;
+// expected-error at 37{{no member named 'something' in namespace 'N'}}
+// expected-note at 21{{but in '' found 1st parameter with type 'T'}}
+
+#endif
diff --git a/clang/unittests/Basic/DiagnosticTest.cpp b/clang/unittests/Basic/DiagnosticTest.cpp
index bc6258fdf4f004..691d74f697f278 100644
--- a/clang/unittests/Basic/DiagnosticTest.cpp
+++ b/clang/unittests/Basic/DiagnosticTest.cpp
@@ -80,6 +80,21 @@ TEST(DiagnosticTest, fatalsAsError) {
}
}
+TEST(DiagnosticTest, tooManyErrorsIsAlwaysFatal) {
+ DiagnosticsEngine Diags(new DiagnosticIDs(), new DiagnosticOptions,
+ new IgnoringDiagConsumer());
+ Diags.setFatalsAsError(true);
+
+ // Report a fatal_too_many_errors diagnostic to ensure that still
+ // acts as a fatal error despite downgrading fatal errors to errors.
+ Diags.Report(diag::fatal_too_many_errors);
+ EXPECT_TRUE(Diags.hasFatalErrorOccurred());
+
+ // Ensure that the severity of that diagnostic is really "fatal".
+ EXPECT_EQ(Diags.getDiagnosticLevel(diag::fatal_too_many_errors, {}),
+ DiagnosticsEngine::Level::Fatal);
+}
+
// Check that soft RESET works as intended
TEST(DiagnosticTest, softReset) {
DiagnosticsEngine Diags(new DiagnosticIDs(), new DiagnosticOptions,
>From 0dc0631b1927fdd4b1ae12ee29f1a7fed0719c3d Mon Sep 17 00:00:00 2001
From: Sergei <igelbox at gmail.com>
Date: Sat, 14 Sep 2024 19:57:32 +0000
Subject: [PATCH 4/5] fix misleading comments and namings
---
clang/include/clang/Basic/Diagnostic.h | 33 ++++++++++----------------
clang/lib/Basic/Diagnostic.cpp | 19 +++++++--------
2 files changed, 22 insertions(+), 30 deletions(-)
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index cee5375dc6285c..3b1efdb12824c7 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -964,8 +964,7 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
// 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. Assertions verify that only one
- // diagnostic is in flight at a time.
+ // tradeoff to keep these objects small.
friend class Diagnostic;
friend class DiagnosticBuilder;
friend class DiagnosticErrorTrap;
@@ -1196,14 +1195,8 @@ class DiagnosticBuilder : public StreamingDiagnostic {
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;
+ SourceLocation DiagLoc;
+ unsigned DiagID;
/// Optional flag value.
///
@@ -1225,8 +1218,8 @@ class DiagnosticBuilder : public StreamingDiagnostic {
DiagnosticBuilder() = default;
- DiagnosticBuilder(DiagnosticsEngine *DiagObj, SourceLocation CurDiagLoc,
- unsigned CurDiagID);
+ DiagnosticBuilder(DiagnosticsEngine *DiagObj, SourceLocation DiagLoc,
+ unsigned DiagID);
protected:
/// Clear out the current diagnostic.
@@ -1485,25 +1478,25 @@ 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 CurDiagLoc;
- unsigned CurDiagID;
+ 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 CurDiagLoc,
- unsigned CurDiagID, const DiagnosticStorage &DiagStorage,
+ Diagnostic(const DiagnosticsEngine *DO, SourceLocation DiagLoc,
+ unsigned DiagID, const DiagnosticStorage &DiagStorage,
StringRef StoredDiagMessage);
const DiagnosticsEngine *getDiags() const { return DiagObj; }
- unsigned getID() const { return CurDiagID; }
- const SourceLocation &getLocation() const { return CurDiagLoc; }
+ unsigned getID() const { return DiagID; }
+ const SourceLocation &getLocation() const { return DiagLoc; }
bool hasSourceManager() const { return DiagObj->hasSourceManager(); }
SourceManager &getSourceManager() const { return DiagObj->getSourceManager();}
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index 484e8bf14f4fc8..2dc0f6b09f0de8 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -523,17 +523,16 @@ bool DiagnosticsEngine::EmitDiagnostic(const DiagnosticBuilder &DB,
}
DiagnosticBuilder::DiagnosticBuilder(DiagnosticsEngine *DiagObj,
- SourceLocation CurDiagLoc,
- unsigned CurDiagID)
+ SourceLocation DiagLoc, unsigned DiagID)
: StreamingDiagnostic(DiagObj->DiagAllocator), DiagObj(DiagObj),
- CurDiagLoc(CurDiagLoc), CurDiagID(CurDiagID), IsActive(true) {
+ DiagLoc(DiagLoc), DiagID(DiagID), IsActive(true) {
assert(DiagObj && "DiagnosticBuilder requires a valid DiagnosticsEngine!");
}
DiagnosticBuilder::DiagnosticBuilder(const DiagnosticBuilder &D)
: StreamingDiagnostic() {
- CurDiagLoc = D.CurDiagLoc;
- CurDiagID = D.CurDiagID;
+ DiagLoc = D.DiagLoc;
+ DiagID = D.DiagID;
FlagValue = D.FlagValue;
DiagObj = D.DiagObj;
DiagStorage = D.DiagStorage;
@@ -546,14 +545,14 @@ DiagnosticBuilder::DiagnosticBuilder(const DiagnosticBuilder &D)
Diagnostic::Diagnostic(const DiagnosticsEngine *DO,
const DiagnosticBuilder &DiagBuilder)
- : DiagObj(DO), CurDiagLoc(DiagBuilder.CurDiagLoc),
- CurDiagID(DiagBuilder.CurDiagID), FlagValue(DiagBuilder.FlagValue),
+ : DiagObj(DO), DiagLoc(DiagBuilder.DiagLoc),
+ DiagID(DiagBuilder.DiagID), FlagValue(DiagBuilder.FlagValue),
DiagStorage(*DiagBuilder.getStorage()) {}
-Diagnostic::Diagnostic(const DiagnosticsEngine *DO, SourceLocation CurDiagLoc,
- unsigned CurDiagID, const DiagnosticStorage &DiagStorage,
+Diagnostic::Diagnostic(const DiagnosticsEngine *DO, SourceLocation DiagLoc,
+ unsigned DiagID, const DiagnosticStorage &DiagStorage,
StringRef StoredDiagMessage)
- : DiagObj(DO), CurDiagLoc(CurDiagLoc), CurDiagID(CurDiagID),
+ : DiagObj(DO), DiagLoc(DiagLoc), DiagID(DiagID),
DiagStorage(DiagStorage), StoredDiagMessage(StoredDiagMessage) {}
DiagnosticConsumer::~DiagnosticConsumer() = default;
>From 57b1dabf3522eae47fd7f8ea26c12bf1a97819ab Mon Sep 17 00:00:00 2001
From: Sergei <igelbox at gmail.com>
Date: Mon, 16 Sep 2024 17:19:30 +0000
Subject: [PATCH 5/5] fix main-branch build failures
---
flang/lib/Frontend/TextDiagnosticPrinter.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/flang/lib/Frontend/TextDiagnosticPrinter.cpp b/flang/lib/Frontend/TextDiagnosticPrinter.cpp
index 8b00fb69b3cefb..dc182d68a1a979 100644
--- a/flang/lib/Frontend/TextDiagnosticPrinter.cpp
+++ b/flang/lib/Frontend/TextDiagnosticPrinter.cpp
@@ -45,7 +45,7 @@ static void printRemarkOption(llvm::raw_ostream &os,
// warning could be printed i.e. [-Wunknown-warning-option]
os << " [" << (level == clang::DiagnosticsEngine::Remark ? "-R" : "-W")
<< opt;
- llvm::StringRef optValue = info.getDiags()->getFlagValue();
+ llvm::StringRef optValue = info.getFlagValue();
if (!optValue.empty())
os << "=" << optValue;
os << ']';
More information about the flang-commits
mailing list