[clang] [clang-tools-extra] Fix OOM in FormatDiagnostic (PR #108187)

Vakhurin Sergei via cfe-commits cfe-commits at lists.llvm.org
Thu Sep 12 10:35:56 PDT 2024


================
@@ -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)
----------------
igelbox wrote:

I researched over the codebase and found many easy to prove usage patterns:
- `DiagID` comes from the same `Diag` object, e.g. https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Diagnostic.cpp#L536
  => they're interchangeable
- code compares the result level with `> DiagnosticsEngine::Warning`, e.g. https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Driver.cpp#L277
or `>= DiagnosticsEngine::Error` - https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L16351 
or `if (CurLevel < DiagnosticsEngine::Error)` - https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L498
  but both `Fatal` and `Error` are greater than `Warning` (or not less than `Error`)
  => these comparisons aren't affected at all

as well as some not so easy to prove:
- https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/ParsedAST.cpp#L578
  but it checks levels of a particular DiagIDs
  => probably we shouldn't rely on how many errors we faced before this checks (many of not so)
- https://github.com/llvm/llvm-project/blob/main/clang/tools/diagtool/ShowEnabledWarnings.cpp#L138
  ! here I'm not sure should we prevent downgrading some `Fatal`s to `Error`s here if the current diagnostic is in the `fatal_too_many_errors` "state"
- https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h#L217
  !! the hardest one to prove coz it has so many usages

Well, for now I would do it in a simple/hopeful way - just use the `DiagID` argument. But, if it doesn't seem safe to reviewers - I could add smth like `size_t count_in_flight__fatal_too_many_errors` to the `DiagnosticsEngine` to mimic the original behavior.

https://github.com/llvm/llvm-project/pull/108187


More information about the cfe-commits mailing list