Fix OOM in FormatDiagnostic #108187
Merged
Fix OOM in FormatDiagnostic #108187
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
igelbox
commented
Sep 11, 2024
igelbox
commented
Sep 11, 2024
igelbox
commented
Sep 11, 2024
igelbox
force-pushed
the
fix--race-condition-caused--infinity-cycle--memory-leak
branch
from
f26c4f3 to
aee4cf7
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang-driver Author: Vakhurin Sergei (igelbox) ChangesResolves: #70930 (and probably latest comments from clangd/clangd#251) which causes The Main Idea: TODO (if it will be requested by reviewer):
Patch is 42.21 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/108187.diff 15 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 200bb87a5ac3cb..4c75b422701148 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -380,7 +380,6 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic(
++Context.Stats.ErrorsIgnoredNOLINT;
// Ignored a warning, should ignore related notes as well
LastErrorWasIgnored = true;
- Context.DiagEngine->Clear();
for (const auto &Error : SuppressionErrors)
Context.diag(Error);
return;
@@ -457,7 +456,6 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic(
if (Info.hasSourceManager())
checkFilters(Info.getLocation(), Info.getSourceManager());
- Context.DiagEngine->Clear();
for (const auto &Error : SuppressionErrors)
Context.diag(Error);
}
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index 0c7836c2ea569c..1eecab4f6e49a2 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -183,6 +183,41 @@ struct DiagnosticStorage {
DiagnosticStorage() = default;
};
+/// An allocator for DiagnosticStorage objects, which uses a small cache to
+/// objects, used to reduce malloc()/free() traffic for partial diagnostics.
+class DiagStorageAllocator {
+ static const unsigned NumCached = 16;
+ DiagnosticStorage Cached[NumCached];
+ DiagnosticStorage *FreeList[NumCached];
+ unsigned NumFreeListEntries;
+
+public:
+ DiagStorageAllocator();
+ ~DiagStorageAllocator();
+
+ /// Allocate new storage.
+ DiagnosticStorage *Allocate() {
+ if (NumFreeListEntries == 0)
+ return new DiagnosticStorage;
+
+ DiagnosticStorage *Result = FreeList[--NumFreeListEntries];
+ Result->NumDiagArgs = 0;
+ Result->DiagRanges.clear();
+ Result->FixItHints.clear();
+ return Result;
+ }
+
+ /// Free the given storage object.
+ void Deallocate(DiagnosticStorage *S) {
+ if (S >= Cached && S <= Cached + NumCached) {
+ FreeList[NumFreeListEntries++] = S;
+ return;
+ }
+
+ delete S;
+ }
+};
+
/// Concrete class used by the front-end to report problems and issues.
///
/// This massages the diagnostics (e.g. handling things like "report warnings
@@ -520,27 +555,6 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
void *ArgToStringCookie = nullptr;
ArgToStringFnTy ArgToStringFn;
- /// ID of the "delayed" diagnostic, which is a (typically
- /// fatal) diagnostic that had to be delayed because it was found
- /// while emitting another diagnostic.
- unsigned DelayedDiagID;
-
- /// First string argument for the delayed diagnostic.
- std::string DelayedDiagArg1;
-
- /// Second string argument for the delayed diagnostic.
- std::string DelayedDiagArg2;
-
- /// Third string argument for the delayed diagnostic.
- std::string DelayedDiagArg3;
-
- /// Optional flag value.
- ///
- /// Some flags accept values, for instance: -Wframe-larger-than=<value> and
- /// -Rpass=<value>. The content of this string is emitted after the flag name
- /// and '='.
- std::string FlagValue;
-
public:
explicit DiagnosticsEngine(IntrusiveRefCntPtr<DiagnosticIDs> Diags,
IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts,
@@ -945,50 +959,11 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
void Report(const StoredDiagnostic &storedDiag);
- /// Determine whethere there is already a diagnostic in flight.
- bool isDiagnosticInFlight() const {
- return CurDiagID != std::numeric_limits<unsigned>::max();
- }
-
- /// Set the "delayed" diagnostic that will be emitted once
- /// the current diagnostic completes.
- ///
- /// If a diagnostic is already in-flight but the front end must
- /// report a problem (e.g., with an inconsistent file system
- /// state), this routine sets a "delayed" diagnostic that will be
- /// emitted after the current diagnostic completes. This should
- /// only be used for fatal errors detected at inconvenient
- /// times. If emitting a delayed diagnostic causes a second delayed
- /// diagnostic to be introduced, that second delayed diagnostic
- /// will be ignored.
- ///
- /// \param DiagID The ID of the diagnostic being delayed.
- ///
- /// \param Arg1 A string argument that will be provided to the
- /// diagnostic. A copy of this string will be stored in the
- /// DiagnosticsEngine object itself.
- ///
- /// \param Arg2 A string argument that will be provided to the
- /// diagnostic. A copy of this string will be stored in the
- /// DiagnosticsEngine object itself.
- ///
- /// \param Arg3 A string argument that will be provided to the
- /// diagnostic. A copy of this string will be stored in the
- /// DiagnosticsEngine object itself.
- void SetDelayedDiagnostic(unsigned DiagID, StringRef Arg1 = "",
- StringRef Arg2 = "", StringRef Arg3 = "");
-
- /// Clear out the current diagnostic.
- void Clear() { CurDiagID = std::numeric_limits<unsigned>::max(); }
-
- /// Return the value associated with this diagnostic flag.
- StringRef getFlagValue() const { return FlagValue; }
-
private:
// This is private state used by DiagnosticBuilder. We put it here instead of
// in DiagnosticBuilder in order to keep DiagnosticBuilder a small lightweight
- // object. This implementation choice means that we can only have one
- // diagnostic "in flight" at a time, but this seems to be a reasonable
+ // object. This implementation choice means that we can only have a few
+ // diagnostics "in flight" at a time, but this seems to be a reasonable
// tradeoff to keep these objects small. Assertions verify that only one
// diagnostic is in flight at a time.
friend class Diagnostic;
@@ -997,18 +972,6 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
friend class DiagnosticIDs;
friend class PartialDiagnostic;
- /// Report the delayed diagnostic.
- void ReportDelayed();
-
- /// The location of the current diagnostic that is in flight.
- SourceLocation CurDiagLoc;
-
- /// The ID of the current diagnostic that is in flight.
- ///
- /// This is set to std::numeric_limits<unsigned>::max() when there is no
- /// diagnostic in flight.
- unsigned CurDiagID;
-
enum {
/// The maximum number of arguments we can hold.
///
@@ -1018,7 +981,7 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
MaxArguments = DiagnosticStorage::MaxArguments,
};
- DiagnosticStorage DiagStorage;
+ DiagStorageAllocator DiagAllocator;
DiagnosticMapping makeUserMapping(diag::Severity Map, SourceLocation L) {
bool isPragma = L.isValid();
@@ -1038,8 +1001,8 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
/// Used to report a diagnostic that is finally fully formed.
///
/// \returns true if the diagnostic was emitted, false if it was suppressed.
- bool ProcessDiag() {
- return Diags->ProcessDiag(*this);
+ bool ProcessDiag(const DiagnosticBuilder &DiagBuilder) {
+ return Diags->ProcessDiag(*this, DiagBuilder);
}
/// @name Diagnostic Emission
@@ -1054,14 +1017,10 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
// Sema::Diag() patterns.
friend class Sema;
- /// Emit the current diagnostic and clear the diagnostic state.
+ /// Emit the diagnostic
///
/// \param Force Emit the diagnostic regardless of suppression settings.
- bool EmitCurrentDiagnostic(bool Force = false);
-
- unsigned getCurrentDiagID() const { return CurDiagID; }
-
- SourceLocation getCurrentDiagLoc() const { return CurDiagLoc; }
+ bool EmitDiagnostic(const DiagnosticBuilder &DB, bool Force = false);
/// @}
};
@@ -1114,40 +1073,7 @@ class DiagnosticErrorTrap {
///
class StreamingDiagnostic {
public:
- /// An allocator for DiagnosticStorage objects, which uses a small cache to
- /// objects, used to reduce malloc()/free() traffic for partial diagnostics.
- class DiagStorageAllocator {
- static const unsigned NumCached = 16;
- DiagnosticStorage Cached[NumCached];
- DiagnosticStorage *FreeList[NumCached];
- unsigned NumFreeListEntries;
-
- public:
- DiagStorageAllocator();
- ~DiagStorageAllocator();
-
- /// Allocate new storage.
- DiagnosticStorage *Allocate() {
- if (NumFreeListEntries == 0)
- return new DiagnosticStorage;
-
- DiagnosticStorage *Result = FreeList[--NumFreeListEntries];
- Result->NumDiagArgs = 0;
- Result->DiagRanges.clear();
- Result->FixItHints.clear();
- return Result;
- }
-
- /// Free the given storage object.
- void Deallocate(DiagnosticStorage *S) {
- if (S >= Cached && S <= Cached + NumCached) {
- FreeList[NumFreeListEntries++] = S;
- return;
- }
-
- delete S;
- }
- };
+ using DiagStorageAllocator = clang::DiagStorageAllocator;
protected:
mutable DiagnosticStorage *DiagStorage = nullptr;
@@ -1236,11 +1162,6 @@ class StreamingDiagnostic {
protected:
StreamingDiagnostic() = default;
- /// Construct with an external storage not owned by itself. The allocator
- /// is a null pointer in this case.
- explicit StreamingDiagnostic(DiagnosticStorage *Storage)
- : DiagStorage(Storage) {}
-
/// Construct with a storage allocator which will manage the storage. The
/// allocator is not a null pointer in this case.
explicit StreamingDiagnostic(DiagStorageAllocator &Alloc)
@@ -1271,9 +1192,26 @@ class StreamingDiagnostic {
class DiagnosticBuilder : public StreamingDiagnostic {
friend class DiagnosticsEngine;
friend class PartialDiagnostic;
+ friend class Diagnostic;
mutable DiagnosticsEngine *DiagObj = nullptr;
+ /// The location of the current diagnostic that is in flight.
+ SourceLocation CurDiagLoc;
+
+ /// The ID of the current diagnostic that is in flight.
+ ///
+ /// This is set to std::numeric_limits<unsigned>::max() when there is no
+ /// diagnostic in flight.
+ unsigned CurDiagID;
+
+ /// Optional flag value.
+ ///
+ /// Some flags accept values, for instance: -Wframe-larger-than=<value> and
+ /// -Rpass=<value>. The content of this string is emitted after the flag name
+ /// and '='.
+ mutable std::string FlagValue;
+
/// Status variable indicating if this diagnostic is still active.
///
// NOTE: This field is redundant with DiagObj (IsActive iff (DiagObj == 0)),
@@ -1287,16 +1225,8 @@ class DiagnosticBuilder : public StreamingDiagnostic {
DiagnosticBuilder() = default;
- explicit DiagnosticBuilder(DiagnosticsEngine *diagObj)
- : StreamingDiagnostic(&diagObj->DiagStorage), DiagObj(diagObj),
- IsActive(true) {
- assert(diagObj && "DiagnosticBuilder requires a valid DiagnosticsEngine!");
- assert(DiagStorage &&
- "DiagnosticBuilder requires a valid DiagnosticStorage!");
- DiagStorage->NumDiagArgs = 0;
- DiagStorage->DiagRanges.clear();
- DiagStorage->FixItHints.clear();
- }
+ DiagnosticBuilder(DiagnosticsEngine *diagObj, SourceLocation CurDiagLoc,
+ unsigned CurDiagID);
protected:
/// Clear out the current diagnostic.
@@ -1322,7 +1252,7 @@ class DiagnosticBuilder : public StreamingDiagnostic {
if (!isActive()) return false;
// Process the diagnostic.
- bool Result = DiagObj->EmitCurrentDiagnostic(IsForceEmit);
+ bool Result = DiagObj->EmitDiagnostic(*this, IsForceEmit);
// This diagnostic is dead.
Clear();
@@ -1333,13 +1263,7 @@ class DiagnosticBuilder : public StreamingDiagnostic {
public:
/// Copy constructor. When copied, this "takes" the diagnostic info from the
/// input and neuters it.
- DiagnosticBuilder(const DiagnosticBuilder &D) : StreamingDiagnostic() {
- DiagObj = D.DiagObj;
- DiagStorage = D.DiagStorage;
- IsActive = D.IsActive;
- IsForceEmit = D.IsForceEmit;
- D.Clear();
- }
+ DiagnosticBuilder(const DiagnosticBuilder &D);
template <typename T> const DiagnosticBuilder &operator<<(const T &V) const {
assert(isActive() && "Clients must not add to cleared diagnostic!");
@@ -1371,7 +1295,7 @@ class DiagnosticBuilder : public StreamingDiagnostic {
return *this;
}
- void addFlagValue(StringRef V) const { DiagObj->FlagValue = std::string(V); }
+ void addFlagValue(StringRef V) const { FlagValue = std::string(V); }
};
struct AddFlagValue {
@@ -1546,12 +1470,7 @@ const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
inline DiagnosticBuilder DiagnosticsEngine::Report(SourceLocation Loc,
unsigned DiagID) {
- assert(CurDiagID == std::numeric_limits<unsigned>::max() &&
- "Multiple diagnostics in flight at once!");
- CurDiagLoc = Loc;
- CurDiagID = DiagID;
- FlagValue.clear();
- return DiagnosticBuilder(this);
+ return DiagnosticBuilder(this, Loc, DiagID);
}
const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
@@ -1570,20 +1489,25 @@ inline DiagnosticBuilder DiagnosticsEngine::Report(unsigned DiagID) {
/// currently in-flight diagnostic.
class Diagnostic {
const DiagnosticsEngine *DiagObj;
+ SourceLocation CurDiagLoc;
+ unsigned CurDiagID;
+ std::string FlagValue;
+ const DiagnosticStorage &DiagStorage;
std::optional<StringRef> StoredDiagMessage;
public:
- explicit Diagnostic(const DiagnosticsEngine *DO) : DiagObj(DO) {}
- Diagnostic(const DiagnosticsEngine *DO, StringRef storedDiagMessage)
- : DiagObj(DO), StoredDiagMessage(storedDiagMessage) {}
+ Diagnostic(const DiagnosticsEngine *DO, const DiagnosticBuilder &DiagBuilder);
+ Diagnostic(const DiagnosticsEngine *DO, SourceLocation CurDiagLoc,
+ unsigned CurDiagID, const DiagnosticStorage &DiagStorage,
+ StringRef storedDiagMessage);
const DiagnosticsEngine *getDiags() const { return DiagObj; }
- unsigned getID() const { return DiagObj->CurDiagID; }
- const SourceLocation &getLocation() const { return DiagObj->CurDiagLoc; }
+ unsigned getID() const { return CurDiagID; }
+ const SourceLocation &getLocation() const { return CurDiagLoc; }
bool hasSourceManager() const { return DiagObj->hasSourceManager(); }
SourceManager &getSourceManager() const { return DiagObj->getSourceManager();}
- unsigned getNumArgs() const { return DiagObj->DiagStorage.NumDiagArgs; }
+ unsigned getNumArgs() const { return DiagStorage.NumDiagArgs; }
/// Return the kind of the specified index.
///
@@ -1593,8 +1517,7 @@ class Diagnostic {
/// \pre Idx < getNumArgs()
DiagnosticsEngine::ArgumentKind getArgKind(unsigned Idx) const {
assert(Idx < getNumArgs() && "Argument index out of range!");
- return (DiagnosticsEngine::ArgumentKind)
- DiagObj->DiagStorage.DiagArgumentsKind[Idx];
+ return (DiagnosticsEngine::ArgumentKind)DiagStorage.DiagArgumentsKind[Idx];
}
/// Return the provided argument string specified by \p Idx.
@@ -1602,7 +1525,7 @@ class Diagnostic {
const std::string &getArgStdStr(unsigned Idx) const {
assert(getArgKind(Idx) == DiagnosticsEngine::ak_std_string &&
"invalid argument accessor!");
- return DiagObj->DiagStorage.DiagArgumentsStr[Idx];
+ return DiagStorage.DiagArgumentsStr[Idx];
}
/// Return the specified C string argument.
@@ -1610,8 +1533,7 @@ class Diagnostic {
const char *getArgCStr(unsigned Idx) const {
assert(getArgKind(Idx) == DiagnosticsEngine::ak_c_string &&
"invalid argument accessor!");
- return reinterpret_cast<const char *>(
- DiagObj->DiagStorage.DiagArgumentsVal[Idx]);
+ return reinterpret_cast<const char *>(DiagStorage.DiagArgumentsVal[Idx]);
}
/// Return the specified signed integer argument.
@@ -1619,7 +1541,7 @@ class Diagnostic {
int64_t getArgSInt(unsigned Idx) const {
assert(getArgKind(Idx) == DiagnosticsEngine::ak_sint &&
"invalid argument accessor!");
- return (int64_t)DiagObj->DiagStorage.DiagArgumentsVal[Idx];
+ return (int64_t)DiagStorage.DiagArgumentsVal[Idx];
}
/// Return the specified unsigned integer argument.
@@ -1627,7 +1549,7 @@ class Diagnostic {
uint64_t getArgUInt(unsigned Idx) const {
assert(getArgKind(Idx) == DiagnosticsEngine::ak_uint &&
"invalid argument accessor!");
- return DiagObj->DiagStorage.DiagArgumentsVal[Idx];
+ return DiagStorage.DiagArgumentsVal[Idx];
}
/// Return the specified IdentifierInfo argument.
@@ -1636,7 +1558,7 @@ class Diagnostic {
assert(getArgKind(Idx) == DiagnosticsEngine::ak_identifierinfo &&
"invalid argument accessor!");
return reinterpret_cast<IdentifierInfo *>(
- DiagObj->DiagStorage.DiagArgumentsVal[Idx]);
+ DiagStorage.DiagArgumentsVal[Idx]);
}
/// Return the specified non-string argument in an opaque form.
@@ -1644,37 +1566,32 @@ class Diagnostic {
uint64_t getRawArg(unsigned Idx) const {
assert(getArgKind(Idx) != DiagnosticsEngine::ak_std_string &&
"invalid argument accessor!");
- return DiagObj->DiagStorage.DiagArgumentsVal[Idx];
+ return DiagStorage.DiagArgumentsVal[Idx];
}
/// Return the number of source ranges associated with this diagnostic.
- unsigned getNumRanges() const {
- return DiagObj->DiagStorage.DiagRanges.size();
- }
+ unsigned getNumRanges() const { return DiagStorage.DiagRanges.size(); }
/// \pre Idx < getNumRanges()
const CharSourceRange &getRange(unsigned Idx) const {
assert(Idx < getNumRanges() && "Invalid diagnostic range index!");
- return DiagObj->DiagStorage.DiagRanges[Idx];
+ return DiagStorage.DiagRanges[Idx];
}
/// Return an array reference for this diagnostic's ranges.
- ArrayRef<CharSourceRange> getRanges() const {
- return DiagObj->DiagStorage.DiagRanges;
- }
+ ArrayRef<CharSourceRange> getRanges() const { return DiagStorage.DiagRanges; }
- unsigned getNumFixItHints() const {
- return DiagObj->DiagStorage.FixItHints.size();
- }
+ unsigned getNumFixItHints() const { return DiagStorage.FixItHints.size(); }
const FixItHint &getFixItHint(unsigned Idx) const {
assert(Idx < getNumFixItHints() && "Invalid index!");
- return DiagObj->DiagStorage.FixItHints[Idx];
+ return DiagStorage.FixItHints[Idx];
}
- ArrayRef<FixItHint> getFixItHints() const {
- return DiagObj->DiagStorage.FixItHints;
- }
+ ArrayRef<FixItHint> getFixItHints() const { return DiagStorage.FixItHints; }
+
+ /// Return the value associated with this diagnostic flag.
+ StringRef getFlagValue() const { return FlagValue; }
/// Format this diagnostic into a string, substituting the
/// formal arguments into the %0 slots.
diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h
index 8b976bdac6dc51..f16fea6e798a95 100644
--- a/clang/include/clang/Basic/DiagnosticIDs.h
+++ b/clang/include/clang/Basic/DiagnosticIDs.h
@@ -22,6 +22,7 @@
namespace clang {
class DiagnosticsEngine;
+ class DiagnosticBuilder;
class SourceLocation;
// Import the diagnostic enums themselves.
@@ -367,11 +368,13 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
///
/// \returns \c true if the diagnostic was emitted, \c false if it was
/// suppressed.
- bool ProcessDiag(DiagnosticsEngine &Diag) const;
+ bool ProcessDiag(DiagnosticsEngine &Diag,
+ const DiagnosticBuilder &DiagBuilder) const;
/// Used to emit a diagnostic that is finally fully formed,
/// ignoring suppression.
- void EmitDiag(DiagnosticsEngine &Diag, Level DiagLevel) const;
+ void EmitDiag(DiagnosticsEngine &Diag, const DiagnosticBuilder &DiagBuilder,
+ Level DiagLevel) const;
/// Whether the diagnostic may leave the AST in a state where some
/// invariants can break.
diff --git a/clang/include/clang/Basic/PartialDiagnostic.h b/clang/include/clang/Basic/PartialDiagnostic.h
index 507d789c54ff9b..4bf6049d08fdb4 100644
--- a/clang/include/clang/Basic/PartialDiagnostic.h
+++ b/clang/include/clang/Basic/PartialDiagnostic.h
@@ -166,13 +166,10 @@ class PartialDiagnostic : public StreamingDiagnostic {
void EmitToString(DiagnosticsEngine &Diags,
SmallVectorImpl<char> &Buf) const {
- // FIXME: It should be possible to render a diagno...
[truncated]
|
There was a problem hiding this comment.
Thank you for working on this! I'm not certain that there is a reasonable way to add test coverage for the changes, but if you can devise a test, that would be appreciated.
Have you run your changes with address and ub sanitizers to see if there are some uncaught edge cases?
The changes are mostly looking good to me, but more sets of eyes on the changes would be appreciated.
Not quire sure I get the point about UB-sanitizers. I tested this changes against my proprietary code which caused the issue with OOM/infinite-loop and it works just fine. I can and will build the |
I wonder there should be some tests like:
but not sure yet where to find such a tests-folder. I'm just a newbie in clang development workflow, so I wonder is someone more experienced would point me that. |
Thanks! Mostly just trying to verify correctness given the breadth of changes; I didn't spot any UB, but it's easy for that to creep in.
Welcome! Most of our test coverage lives in |
igelbox
commented
Sep 13, 2024
|
Whew, managed to add both tests. |
vvd170501
reviewed
Sep 14, 2024
AaronBallman
approved these changes
Sep 16, 2024
I suppose so.. or what are my options? The Merge button isn't active of course. |
|
@igelbox Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
I've merged on your behalf (I sometime struggle to tell who has commit privileges and who doesn't), thank you! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/7831 Here is the relevant piece of the build log for the reference |
|
@igelbox: That seems to be a problem with this patch! If it is something you cannot fix/get a review up for very quickly so we can accept/get it in, we'll have to revert this. Please let us know ASAP what you'd like to do. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/6654 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/89/builds/6426 Here is the relevant piece of the build log for the reference |
AaronBallman
added a commit
that referenced
this pull request
Sep 16, 2024
This reverts commit e5d2556.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/5825 Here is the relevant piece of the build log for the reference |
AaronBallman
added a commit
that referenced
this pull request
Sep 16, 2024
Reverting due to build failures found in #108187
|
I've reverted; @igelbox, you can create a new PR that addresses the failures found by post-commit CI and I'll review them. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/134/builds/5347 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/4601 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/7472 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/9025 Here is the relevant piece of the build log for the reference |
9 tasks
AaronBallman
pushed a commit
that referenced
this pull request
Sep 18, 2024
Resolves: #70930 (and probably latest comments from clangd/clangd#251) by fixing racing for the shared DiagStorage value which caused messing with args inside the storage and then formatting the following message with getArgSInt(1) == 2: def err_module_odr_violation_function : Error< "%q0 has different definitions in different modules; " "%select{definition in module '%2'|defined here}1 " "first difference is " which causes HandleSelectModifier to go beyond the ArgumentLen so the recursive call to FormatDiagnostic was made with DiagStr > DiagEnd that leads to infinite while (DiagStr != DiagEnd). The Main Idea: Reuse the existing DiagStorageAllocator logic to make all DiagnosticBuilders having independent states. Also, encapsulating the rest of state (e.g. ID and Loc) into DiagnosticBuilder. The last attempt failed - #108187 (comment) so was reverted - #108838
tmsri
pushed a commit
to tmsri/llvm-project
that referenced
this pull request
Sep 19, 2024
Resolves: llvm#70930 (and probably latest comments from clangd/clangd#251) by fixing racing for the shared DiagStorage value which caused messing with args inside the storage and then formatting the following message with getArgSInt(1) == 2: def err_module_odr_violation_function : Error< "%q0 has different definitions in different modules; " "%select{definition in module '%2'|defined here}1 " "first difference is " which causes HandleSelectModifier to go beyond the ArgumentLen so the recursive call to FormatDiagnostic was made with DiagStr > DiagEnd that leads to infinite while (DiagStr != DiagEnd). The Main Idea: Reuse the existing DiagStorageAllocator logic to make all DiagnosticBuilders having independent states. Also, encapsulating the rest of state (e.g. ID and Loc) into DiagnosticBuilder. The last attempt failed - llvm#108187 (comment) so was reverted - llvm#108838
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves: #70930 (and probably latest comments from clangd/clangd#251)
by fixing racing for the shared
DiagStoragevalue which caused messing with args inside the storage and then formatting the following message withgetArgSInt(1)== 2:which causes
HandleSelectModifierto go beyond theArgumentLenso the recursive call toFormatDiagnosticwas made withDiagStr>DiagEndthat leads to infinitewhile (DiagStr != DiagEnd).The Main Idea:
Reuse the existing
DiagStorageAllocatorlogic to make allDiagnosticBuilders having independent states.Also, encapsulating the rest of state (e.g. ID and Loc) into
DiagnosticBuilder.TODO (if it will be requested by reviewer):
clangdto OOM into a small public example.. probably I must try using this instead)Diag.CurDiagID != diag::fatal_too_many_errorsDiagStorageAllocatorat all and makeDiagnosticBuilderhaving they ownDiagnosticStoragecoz it seems pretty small so should fit the stack for short-livingDiagnosticBuilderinstances