[RISCV] Just reporting an error shouldn't generate a crash diagnostic #134040
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
The code uses report_fatal_error() to report user errors and exit. Unfortunately that function's GenCrashDiag argument defaults to true so it appears to the user as if they should report a bug. Added a tiny wrapper which ensures we don't generate the crash diagnostic.
Member
|
@llvm/pr-subscribers-mc @llvm/pr-subscribers-backend-risc-v Author: Paul Bowen-Huggett (paulhuggett) ChangesWanting to examine some of generated code, I tried MCA with the command: llvm-mca -mtriple=riscv32-unknown-unknown -mcpu=rocket -iterations=300 core_list_join.sI was greeted with the following error message: On beginning to investigate the “bug”, I discovered that the code was simply attempting to report a user error. It used report_fatal_error() to do so but with the “bool GenCrashDiag” argument enabled (the default). This tiny change adds a wrapper function which calls report_fatal_error() as before but with GenCrashDiag disabled. Full diff: https://github.com/llvm/llvm-project/pull/134040.diff 1 Files Affected:
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
index d5f08ac05f82b..d74c92f992d08 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
@@ -51,6 +51,14 @@ namespace RISCV {
#include "RISCVGenSearchableTables.inc"
} // namespace RISCV
+// Report an error but don't perform ask the user to report a bug.
+[[noreturn]] static void reportError(const char *Reason) {
+ report_fatal_error(Reason, false);
+}
+[[noreturn]] static void reportError(Error Err) {
+ report_fatal_error(std::move(Err), false);
+}
+
namespace RISCVABI {
ABI computeTargetABI(const Triple &TT, const FeatureBitset &FeatureBits,
StringRef ABIName) {
@@ -87,7 +95,7 @@ ABI computeTargetABI(const Triple &TT, const FeatureBitset &FeatureBits,
if ((TargetABI == RISCVABI::ABI::ABI_ILP32E ||
(TargetABI == ABI_Unknown && IsRVE && !IsRV64)) &&
FeatureBits[RISCV::FeatureStdExtD])
- report_fatal_error("ILP32E cannot be used with the D ISA extension");
+ reportError("ILP32E cannot be used with the D ISA extension");
if (TargetABI != ABI_Unknown)
return TargetABI;
@@ -95,7 +103,7 @@ ABI computeTargetABI(const Triple &TT, const FeatureBitset &FeatureBits,
// If no explicit ABI is given, try to compute the default ABI.
auto ISAInfo = RISCVFeatures::parseFeatureBits(IsRV64, FeatureBits);
if (!ISAInfo)
- report_fatal_error(ISAInfo.takeError());
+ reportError(ISAInfo.takeError());
return getTargetABI((*ISAInfo)->computeDefaultABI());
}
@@ -127,12 +135,12 @@ namespace RISCVFeatures {
void validate(const Triple &TT, const FeatureBitset &FeatureBits) {
if (TT.isArch64Bit() && !FeatureBits[RISCV::Feature64Bit])
- report_fatal_error("RV64 target requires an RV64 CPU");
+ reportError("RV64 target requires an RV64 CPU");
if (!TT.isArch64Bit() && !FeatureBits[RISCV::Feature32Bit])
- report_fatal_error("RV32 target requires an RV32 CPU");
+ reportError("RV32 target requires an RV32 CPU");
if (FeatureBits[RISCV::Feature32Bit] &&
FeatureBits[RISCV::Feature64Bit])
- report_fatal_error("RV32 and RV64 can't be combined");
+ reportError("RV32 and RV64 can't be combined");
}
llvm::Expected<std::unique_ptr<RISCVISAInfo>>
|
Contributor
|
This has a slightly frustrating history. Quite some time ago I tried to clean up cases where we didn't want the crash dialog, but the preference was to try to change the default which led to this RFC and a lengthy debate. Once it was clear there was no path forwards I probably should have returned to the original approach to adding the argument. Setting GenCrashDiag as you do here seems like a fine incremental improvement to me. |
Contributor
Author
Thanks. That makes sense of what initially seemed like curious and unusual behavior! |
Contributor
Author
|
Is there anyone in a position to do a formal review and (potentially) commit this for me, please? |
topperc
reviewed
Apr 14, 2025
|
|
||
| // Report an error but don't ask the user to report a bug. | ||
| [[noreturn]] static void reportError(const char *Reason) { | ||
| report_fatal_error(Reason, false); |
Collaborator
There was a problem hiding this comment.
Add /*gen_crash_diag=*/ in front of the false
| report_fatal_error(Reason, false); | ||
| } | ||
| [[noreturn]] static void reportError(Error Err) { | ||
| report_fatal_error(std::move(Err), false); |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/16058 Here is the relevant piece of the build log for the reference |
Contributor
Author
Thank you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Wanting to examine some of generated code, I tried MCA with the command:
I was greeted with the following error message:
On beginning to investigate the “bug”, I discovered that the code was simply attempting to report a user error. It used report_fatal_error() to do so but with the “bool GenCrashDiag” argument enabled (the default). This tiny change adds a wrapper function which calls report_fatal_error() as before but with GenCrashDiag disabled.