https://github.com/statham-arm created
https://github.com/llvm/llvm-project/pull/110804
This PR contains two separate patches updating the "custom error message"
feature in `multilib.yaml` (#105684):
* Change the YAML keyword `FatalError` to `Error`, as @petrhosek requested
after the previous PR was already landed
* Improve ergonomics of the error report: it's better to have it after the long
list of available multilibs, not before, so that it appears in the most
sensible place for a fatal error message and doesn't get scrolled off the
user's screen.
>From 593cc72bc80ee79a8ae5b4191a91a48fed0314be Mon Sep 17 00:00:00 2001
From: Simon Tatham <simon.tat...@arm.com>
Date: Mon, 30 Sep 2024 16:12:00 +0100
Subject: [PATCH 1/2] [clang][Driver] Rename "FatalError" key to "Error" in
multilib.yaml.
This is a late-breaking change to #105684, suggested after the
original patch was already landed.
---
clang/docs/Multilib.rst | 4 ++--
clang/include/clang/Driver/Multilib.h | 8 +++----
clang/lib/Driver/Driver.cpp | 2 +-
clang/lib/Driver/Multilib.cpp | 24 +++++++++----------
clang/lib/Driver/ToolChains/BareMetal.cpp | 2 +-
.../baremetal-multilib-custom-error.yaml | 2 +-
clang/unittests/Driver/MultilibTest.cpp | 2 +-
7 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/clang/docs/Multilib.rst b/clang/docs/Multilib.rst
index 6d77fda3623b20..7637d0db9565b8 100644
--- a/clang/docs/Multilib.rst
+++ b/clang/docs/Multilib.rst
@@ -202,8 +202,8 @@ For a more comprehensive example see
# If there is no multilib available for a particular set of flags, and the
# other multilibs are not adequate fallbacks, then you can define a variant
- # record with a FatalError key in place of the Dir key.
- - FatalError: this multilib collection has no hard-float ABI support
+ # record with an Error key in place of the Dir key.
+ - Error: this multilib collection has no hard-float ABI support
Flags: [--target=thumbv7m-none-eabi, -mfloat-abi=hard]
diff --git a/clang/include/clang/Driver/Multilib.h
b/clang/include/clang/Driver/Multilib.h
index 1a79417111eece..dbed70f4f9008f 100644
--- a/clang/include/clang/Driver/Multilib.h
+++ b/clang/include/clang/Driver/Multilib.h
@@ -54,7 +54,7 @@ class Multilib {
// Some Multilib objects don't actually represent library directories you can
// select. Instead, they represent failures of multilib selection, of the
// form 'Sorry, we don't have any library compatible with these constraints'.
- std::optional<std::string> FatalError;
+ std::optional<std::string> Error;
public:
/// GCCSuffix, OSSuffix & IncludeSuffix will be appended directly to the
@@ -63,7 +63,7 @@ class Multilib {
Multilib(StringRef GCCSuffix = {}, StringRef OSSuffix = {},
StringRef IncludeSuffix = {}, const flags_list &Flags =
flags_list(),
StringRef ExclusiveGroup = {},
- std::optional<StringRef> FatalError = std::nullopt);
+ std::optional<StringRef> Error = std::nullopt);
/// Get the detected GCC installation path suffix for the multi-arch
/// target variant. Always starts with a '/', unless empty
@@ -94,9 +94,9 @@ class Multilib {
bool operator==(const Multilib &Other) const;
- bool isFatalError() const { return FatalError.has_value(); }
+ bool isError() const { return Error.has_value(); }
- const std::string &getFatalError() const { return FatalError.value(); }
+ const std::string &getErrorMessage() const { return Error.value(); }
};
raw_ostream &operator<<(raw_ostream &OS, const Multilib &M);
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index fba6a8853c3960..b8536a706a8fa2 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -2314,7 +2314,7 @@ bool Driver::HandleImmediateArgs(Compilation &C) {
if (C.getArgs().hasArg(options::OPT_print_multi_lib)) {
for (const Multilib &Multilib : TC.getMultilibs())
- if (!Multilib.isFatalError())
+ if (!Multilib.isError())
llvm::outs() << Multilib << "\n";
return false;
}
diff --git a/clang/lib/Driver/Multilib.cpp b/clang/lib/Driver/Multilib.cpp
index e8a27fe9de473a..a4ba1e75748fdd 100644
--- a/clang/lib/Driver/Multilib.cpp
+++ b/clang/lib/Driver/Multilib.cpp
@@ -33,9 +33,9 @@ using namespace llvm::sys;
Multilib::Multilib(StringRef GCCSuffix, StringRef OSSuffix,
StringRef IncludeSuffix, const flags_list &Flags,
StringRef ExclusiveGroup,
- std::optional<StringRef> FatalError)
+ std::optional<StringRef> Error)
: GCCSuffix(GCCSuffix), OSSuffix(OSSuffix), IncludeSuffix(IncludeSuffix),
- Flags(Flags), ExclusiveGroup(ExclusiveGroup), FatalError(FatalError) {
+ Flags(Flags), ExclusiveGroup(ExclusiveGroup), Error(Error) {
assert(GCCSuffix.empty() ||
(StringRef(GCCSuffix).front() == '/' && GCCSuffix.size() > 1));
assert(OSSuffix.empty() ||
@@ -126,8 +126,8 @@ bool MultilibSet::select(const Driver &D, const
Multilib::flags_list &Flags,
// If this multilib is actually a placeholder containing a fatal
// error message written by the multilib.yaml author, display that
// error message, and return failure.
- if (M.isFatalError()) {
- D.Diag(clang::diag::err_drv_multilib_custom_error) << M.getFatalError();
+ if (M.isError()) {
+ D.Diag(clang::diag::err_drv_multilib_custom_error) <<
M.getErrorMessage();
return false;
}
@@ -173,7 +173,7 @@ static const VersionTuple MultilibVersionCurrent(1, 0);
struct MultilibSerialization {
std::string Dir; // if this record successfully selects a library dir
- std::string FatalError; // if this record reports a fatal error message
+ std::string Error; // if this record reports a fatal error message
std::vector<std::string> Flags;
std::string Group;
};
@@ -217,15 +217,15 @@ struct MultilibSetSerialization {
template <> struct llvm::yaml::MappingTraits<MultilibSerialization> {
static void mapping(llvm::yaml::IO &io, MultilibSerialization &V) {
io.mapOptional("Dir", V.Dir);
- io.mapOptional("FatalError", V.FatalError);
+ io.mapOptional("Error", V.Error);
io.mapRequired("Flags", V.Flags);
io.mapOptional("Group", V.Group);
}
static std::string validate(IO &io, MultilibSerialization &V) {
- if (V.Dir.empty() && V.FatalError.empty())
- return "one of the 'Dir' and 'FatalError' keys must be specified";
- if (!V.Dir.empty() && !V.FatalError.empty())
- return "the 'Dir' and 'FatalError' keys may not both be specified";
+ if (V.Dir.empty() && V.Error.empty())
+ return "one of the 'Dir' and 'atalError' keys must be specified";
+ if (!V.Dir.empty() && !V.Error.empty())
+ return "the 'Dir' and 'Error' keys may not both be specified";
if (StringRef(V.Dir).starts_with("/"))
return "paths must be relative but \"" + V.Dir + "\" starts with \"/\"";
return std::string{};
@@ -311,8 +311,8 @@ MultilibSet::parseYaml(llvm::MemoryBufferRef Input,
multilib_list Multilibs;
Multilibs.reserve(MS.Multilibs.size());
for (const auto &M : MS.Multilibs) {
- if (!M.FatalError.empty()) {
- Multilibs.emplace_back("", "", "", M.Flags, M.Group, M.FatalError);
+ if (!M.Error.empty()) {
+ Multilibs.emplace_back("", "", "", M.Flags, M.Group, M.Error);
} else {
std::string Dir;
if (M.Dir != ".")
diff --git a/clang/lib/Driver/ToolChains/BareMetal.cpp
b/clang/lib/Driver/ToolChains/BareMetal.cpp
index 8aed9ed6e18817..d5c70f02b5e752 100644
--- a/clang/lib/Driver/ToolChains/BareMetal.cpp
+++ b/clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -187,7 +187,7 @@ static void findMultilibsFromYAML(const ToolChain &TC,
const Driver &D,
D.Diag(clang::diag::warn_drv_missing_multilib) << llvm::join(Flags, " ");
std::stringstream ss;
for (const Multilib &Multilib : Result.Multilibs)
- if (!Multilib.isFatalError())
+ if (!Multilib.isError())
ss << "\n" << llvm::join(Multilib.flags(), " ");
D.Diag(clang::diag::note_drv_available_multilibs) << ss.str();
}
diff --git a/clang/test/Driver/baremetal-multilib-custom-error.yaml
b/clang/test/Driver/baremetal-multilib-custom-error.yaml
index c006bb4072ce2f..761e595757f346 100644
--- a/clang/test/Driver/baremetal-multilib-custom-error.yaml
+++ b/clang/test/Driver/baremetal-multilib-custom-error.yaml
@@ -44,7 +44,7 @@ Variants:
Flags:
- --target=thumbv8.1m.main-unknown-none-eabi
-- FatalError: mve-softfloat is not supported
+- Error: mve-softfloat is not supported
Flags:
- --target=thumbv8.1m.main-unknown-none-eabi
- -march=thumbv8.1m.main+mve
diff --git a/clang/unittests/Driver/MultilibTest.cpp
b/clang/unittests/Driver/MultilibTest.cpp
index dfeef7c2077c72..c03e117d993045 100644
--- a/clang/unittests/Driver/MultilibTest.cpp
+++ b/clang/unittests/Driver/MultilibTest.cpp
@@ -282,7 +282,7 @@ Variants: []
)"));
EXPECT_TRUE(
StringRef(Diagnostic)
- .contains("one of the 'Dir' and 'FatalError' keys must be
specified"))
+ .contains("one of the 'Dir' and 'Error' keys must be specified"))
<< Diagnostic;
EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
>From 765e431243b8ee6df9e118d7c1b21b270bf68c11 Mon Sep 17 00:00:00 2001
From: Simon Tatham <simon.tat...@arm.com>
Date: Wed, 2 Oct 2024 09:26:02 +0100
Subject: [PATCH 2/2] [clang][Driver] Improve multilib custom error reporting.
If `multilib.yaml` reports a custom error message for some unsupported
configuration, it's not very helpful to display that error message
_first_, and then follow it up with a huge list of all the multilib
configurations that _are_ supported.
In interactive use, the list is likely to scroll the most important
message off the top of the user's window, leaving them with just a
long list of available libraries, without a visible explanation of
_why_ clang just printed that long list. Also, in general, it makes
more intuitive sense to print the message last that shows why
compilation can't continue, because that's where users are most likely
to look for the reason why something stopped.
---
clang/lib/Driver/Multilib.cpp | 14 +++++++-------
clang/lib/Driver/ToolChains/BareMetal.cpp | 16 ++++++++++++++++
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/clang/lib/Driver/Multilib.cpp b/clang/lib/Driver/Multilib.cpp
index a4ba1e75748fdd..fbf62da132c3b9 100644
--- a/clang/lib/Driver/Multilib.cpp
+++ b/clang/lib/Driver/Multilib.cpp
@@ -100,6 +100,7 @@ bool MultilibSet::select(const Driver &D, const
Multilib::flags_list &Flags,
llvm::SmallVectorImpl<Multilib> &Selected) const {
llvm::StringSet<> FlagSet(expandFlags(Flags));
Selected.clear();
+ bool AnyErrors = false;
// Decide which multilibs we're going to select at all.
llvm::DenseSet<StringRef> ExclusiveGroupsSelected;
@@ -124,12 +125,11 @@ bool MultilibSet::select(const Driver &D, const
Multilib::flags_list &Flags,
}
// If this multilib is actually a placeholder containing a fatal
- // error message written by the multilib.yaml author, display that
- // error message, and return failure.
- if (M.isError()) {
- D.Diag(clang::diag::err_drv_multilib_custom_error) <<
M.getErrorMessage();
- return false;
- }
+ // error message written by the multilib.yaml author, then set a
+ // flag that will cause a failure return. Our caller will display
+ // the error message.
+ if (M.isError())
+ AnyErrors = true;
// Select this multilib.
Selected.push_back(M);
@@ -139,7 +139,7 @@ bool MultilibSet::select(const Driver &D, const
Multilib::flags_list &Flags,
// round.
std::reverse(Selected.begin(), Selected.end());
- return !Selected.empty();
+ return !AnyErrors && !Selected.empty();
}
llvm::StringSet<>
diff --git a/clang/lib/Driver/ToolChains/BareMetal.cpp
b/clang/lib/Driver/ToolChains/BareMetal.cpp
index d5c70f02b5e752..75ca0552ce63a5 100644
--- a/clang/lib/Driver/ToolChains/BareMetal.cpp
+++ b/clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -186,10 +186,26 @@ static void findMultilibsFromYAML(const ToolChain &TC,
const Driver &D,
return;
D.Diag(clang::diag::warn_drv_missing_multilib) << llvm::join(Flags, " ");
std::stringstream ss;
+
+ // If multilib selection didn't complete successfully, report a list
+ // of all the configurations the user could have provided.
for (const Multilib &Multilib : Result.Multilibs)
if (!Multilib.isError())
ss << "\n" << llvm::join(Multilib.flags(), " ");
D.Diag(clang::diag::note_drv_available_multilibs) << ss.str();
+
+ // Now report any custom error messages requested by the YAML. We do
+ // this after displaying the list of available multilibs, because
+ // that list is probably large, and (in interactive use) risks
+ // scrolling the useful error message off the top of the user's
+ // terminal.
+ for (const Multilib &Multilib : Result.SelectedMultilibs)
+ if (Multilib.isError())
+ D.Diag(clang::diag::err_drv_multilib_custom_error) <<
Multilib.getErrorMessage();
+
+ // If there was an error, clear the SelectedMultilibs vector, in
+ // case it contains partial data.
+ Result.SelectedMultilibs.clear();
}
static constexpr llvm::StringLiteral MultilibFilename = "multilib.yaml";
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
