fix: replace report_fatal_error with Diags and exit #147959
Merged
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
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-clang Author: woruyu (woruyu) ChangesSummary Full diff: https://github.com/llvm/llvm-project/pull/147959.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/SanitizerSpecialCaseList.h b/clang/include/clang/Basic/SanitizerSpecialCaseList.h
index cf7485909e409..72cdcf7c467f0 100644
--- a/clang/include/clang/Basic/SanitizerSpecialCaseList.h
+++ b/clang/include/clang/Basic/SanitizerSpecialCaseList.h
@@ -14,6 +14,7 @@
#ifndef LLVM_CLANG_BASIC_SANITIZERSPECIALCASELIST_H
#define LLVM_CLANG_BASIC_SANITIZERSPECIALCASELIST_H
+#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/LLVM.h"
#include "clang/Basic/Sanitizers.h"
#include "llvm/ADT/StringRef.h"
@@ -37,8 +38,8 @@ class SanitizerSpecialCaseList : public llvm::SpecialCaseList {
std::string &Error);
static std::unique_ptr<SanitizerSpecialCaseList>
- createOrDie(const std::vector<std::string> &Paths,
- llvm::vfs::FileSystem &VFS);
+ createOrDie(const std::vector<std::string> &Paths, llvm::vfs::FileSystem &VFS,
+ DiagnosticsEngine &Diags);
// Query ignorelisted entries if any bit in Mask matches the entry's section.
bool inSection(SanitizerMask Mask, StringRef Prefix, StringRef Query,
diff --git a/clang/lib/Basic/NoSanitizeList.cpp b/clang/lib/Basic/NoSanitizeList.cpp
index 96f79fb2a2a29..1ae304fbd2132 100644
--- a/clang/lib/Basic/NoSanitizeList.cpp
+++ b/clang/lib/Basic/NoSanitizeList.cpp
@@ -22,7 +22,8 @@ using namespace clang;
NoSanitizeList::NoSanitizeList(const std::vector<std::string> &NoSanitizePaths,
SourceManager &SM)
: SSCL(SanitizerSpecialCaseList::createOrDie(
- NoSanitizePaths, SM.getFileManager().getVirtualFileSystem())),
+ NoSanitizePaths, SM.getFileManager().getVirtualFileSystem(),
+ SM.getDiagnostics())),
SM(SM) {}
NoSanitizeList::~NoSanitizeList() = default;
diff --git a/clang/lib/Basic/SanitizerSpecialCaseList.cpp b/clang/lib/Basic/SanitizerSpecialCaseList.cpp
index f7bc1d5545d75..51a09b341f495 100644
--- a/clang/lib/Basic/SanitizerSpecialCaseList.cpp
+++ b/clang/lib/Basic/SanitizerSpecialCaseList.cpp
@@ -30,11 +30,16 @@ SanitizerSpecialCaseList::create(const std::vector<std::string> &Paths,
std::unique_ptr<SanitizerSpecialCaseList>
SanitizerSpecialCaseList::createOrDie(const std::vector<std::string> &Paths,
- llvm::vfs::FileSystem &VFS) {
+ llvm::vfs::FileSystem &VFS,
+ DiagnosticsEngine &Diags) {
std::string Error;
if (auto SSCL = create(Paths, VFS, Error))
return SSCL;
- llvm::report_fatal_error(StringRef(Error));
+ unsigned DiagID = Diags.getCustomDiagID(clang::DiagnosticsEngine::Error,
+ "failed to load NoSanitize file: %0");
+
+ Diags.Report(DiagID) << Error;
+ exit(1);
}
void SanitizerSpecialCaseList::createSanitizerSections() {
|
|
@hstk30-hw This patch is now ready for review. |
|
Need a test case at least. |
llvmbot
added
the
clang:driver
Done! |
|
@shafik Take a look, pls. |
woruyu
force-pushed
the
fix/report-error-sanitize-file
branch
2 times, most recently
from
a4236f9 to
4920812
Compare
|
Hi,@hstk30-hw, @shafik, @AaronBallman just wanted to check if there’s anything further needed from my side for this patch. I’d be happy to help clarify or adjust anything. |
clang/lib/AST/ASTContext.cpp
Outdated
| SourceManager &SM) { | ||
| std::string Error; | ||
| if (!NoSanitizeL->init(LangOpts.NoSanitizeFiles, Error)) { | ||
| const std::string &Path = LangOpts.NoSanitizeFiles.front(); |
There was a problem hiding this comment.
I think this assumes that the first file in the list is the one we cannot read; what happens if we can read that one fine but a subsequent file is where we struggle? We'd report the wrong path to the user in that case, which would be hard for them to reason about.
There was a problem hiding this comment.
There are two kind of error for NoSanitizeL. The first is can't open file, the second is error parsing file, so I think using CustomDiagID is better!
There was a problem hiding this comment.
so I think using CustomDiagID is better!
Custom diagnostics are not an acceptable solution IMO; the reason is because they don't get a warning group associated with them and so users have no way to silence those diagnostics. We have some uses of the API in Clang but in every case they're the wrong tool to use.
We actually document this on the API itself, but not very clearly:
(I'll fix up the comment)There was a problem hiding this comment.
Custom diagnostics are not an acceptable solution IMO;
Thanks for the feedback! To confirm: is the correct fix to define dedicated diagnostics in a .td file like this:
def err_no_sanitize_file_open_fail : Error<
"failed to load NoSanitize file: can't open file '%0'">;
def err_no_sanitize_file_parse_fail : Error<
"failed to load NoSanitize file: error parsing file '%0'">;
def SanitizeIgnorelist : DiagGroup<"sanitize-ignorelist">;
Then use them accordingly in ASTContext::initSanitizers.
There was a problem hiding this comment.
Almost! You can split the diagnostics up if that's easier for you, but another approach would be:
def err_no_sanitize_file_failure : Error<
"failed to %select{load|parse}0 sanitizer ignorelist file '%1'">;
where you only have a single diagnostic but pass in an argument to switch between load and parse.
Note, you don't need:
def SanitizeIgnorelist : DiagGroup<"sanitize-ignorelist">;
because diagnostic groups are only for warnings, and you're producing errors.
There was a problem hiding this comment.
I decided to go with:
def err_sanitize_ignorelist_failure : Error<
"failed to sanitize ignorelist file: %0">;
instead of
def err_no_sanitize_file_failure : Error<
"failed to %select{load|parse}0 sanitizer ignorelist file '%1'">;
for two reasons:
1.The %select logic is essentially string formatting and can be handled downstream where the error message is constructed.
2.Changing SpecialCaseList::createInternal to propagate structured error info adds unnecessary complexity, especially given its general-purpose nature.
There was a problem hiding this comment.
1.The %select logic is essentially string formatting and can be handled downstream where the error message is constructed.
Wellllll kind of yes but also kind of no. We try to avoid tying together strings like that because there are downstreams which want to localize diagnostic messages (and we'd like to someday do the same upstream). It's easier for localizers to handle %select for any various languages instead of having to try to reword diagnostics so that they still make sense with arbitrary string concatenation. So it works but we prefer %select whenever possible.
2.Changing SpecialCaseList::createInternal to propagate structured error info adds unnecessary complexity, especially given its general-purpose nature.
We currently have to thread through the error string, the suggestion is to use an enumeration as well as a string. e.g.,
"failed to %enum_select<SpecialCaseListFailure>{%Open{open}|%Parse{parse}}0 sanitizer ignorelist file: %1">; as the diagnostic message, and then use std::pair<unsigned, std::string> for passing the diagnostic information around, where the first parameter would be something like: diag::SpecialCaseListFailure::Open. So when you go to emit the diagnostic, it would effectively be: Diag(Loc, diag::err_no_sanitize_file_failure) << Error.first << Error.second;
There was a problem hiding this comment.
Done! By the way, I find many diagnose to modify such as err_drv_malformed_sanitizer_coverage_allowlist, err_drv_malformed_sanitizer_ignorelist and so on, but I get a redefine error of SpecialCaseListFailure, so I change it to %select.
|
Additionally, I noticed the same issue occurs with |
woruyu
force-pushed
the
fix/report-error-sanitize-file
branch
from
2184d03 to
49447c0
Compare
|
we can't use diagnostics in llvm-tools, so keep the overload function |
AaronBallman
approved these changes
Jul 25, 2025
Yes, please — I don’t have commit access. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/14696 Here is the relevant piece of the build log for the reference |
|
Reverting the changes because these failures look related to the patch. I think we're missing a call to |
AaronBallman
added a commit
that referenced
this pull request
Jul 25, 2025
This reverts commit 9d3dd8e.
AaronBallman
added a commit
that referenced
this pull request
Jul 25, 2025
I took a closer look at the build logs and I suspect the issue might be related to this code: The failing cases seem to be triggered during C++20 module builds, and from what I can tell, reproducing them locally is quite difficult. That said, this made me reconsider the core design of the merged patch. This way, we can ensure that a valid object is always returned, and we can avoid unexpected crashes (or stack dumps) due to misuse. To me, this seems like a safer and more maintainable direction. |
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Jul 28, 2025
report_fatal_error is not a good way to report diagnostics to the users, so this switches to using actual diagnostic reporting mechanisms instead. Fixes llvm#147187
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Jul 28, 2025
ajaden-codes
pushed a commit
to Jaddyen/llvm-project
that referenced
this pull request
Jul 28, 2025
report_fatal_error is not a good way to report diagnostics to the users, so this switches to using actual diagnostic reporting mechanisms instead. Fixes llvm#147187
ajaden-codes
pushed a commit
to Jaddyen/llvm-project
that referenced
this pull request
Jul 28, 2025
I don't think reverting to the previous approach is the right choice, certainly not back to using |
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.
Summary
This PR resolves #147187