[clang] Multilib error fixes (PR #110804)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 2 01:47:07 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Simon Tatham (statham-arm)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/110804.diff
7 Files Affected:
- (modified) clang/docs/Multilib.rst (+2-2)
- (modified) clang/include/clang/Driver/Multilib.h (+4-4)
- (modified) clang/lib/Driver/Driver.cpp (+1-1)
- (modified) clang/lib/Driver/Multilib.cpp (+17-17)
- (modified) clang/lib/Driver/ToolChains/BareMetal.cpp (+17-1)
- (modified) clang/test/Driver/baremetal-multilib-custom-error.yaml (+1-1)
- (modified) clang/unittests/Driver/MultilibTest.cpp (+1-1)
``````````diff
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..fbf62da132c3b9 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() ||
@@ -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.isFatalError()) {
- D.Diag(clang::diag::err_drv_multilib_custom_error) << M.getFatalError();
- 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<>
@@ -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..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.isFatalError())
+ 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";
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"(
``````````
</details>
https://github.com/llvm/llvm-project/pull/110804
More information about the cfe-commits
mailing list