From 531253ab8c33cc69a927b28a1608675cd9ef709c Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 30 Sep 2024 16:12:00 +0100 Subject: [PATCH 1/4] [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 | 25 +++++++++---------- clang/lib/Driver/ToolChains/BareMetal.cpp | 2 +- .../baremetal-multilib-custom-error.yaml | 2 +- clang/unittests/Driver/MultilibTest.cpp | 2 +- 7 files changed, 22 insertions(+), 23 deletions(-) diff --git a/clang/docs/Multilib.rst b/clang/docs/Multilib.rst index 6d77fda3623b2..7637d0db9565b 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 1a79417111eec..dbed70f4f9008 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 FatalError; + std::optional 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 FatalError = std::nullopt); + std::optional 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 fba6a8853c396..b8536a706a8fa 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 e8a27fe9de473..cc34794b7dac4 100644 --- a/clang/lib/Driver/Multilib.cpp +++ b/clang/lib/Driver/Multilib.cpp @@ -32,10 +32,9 @@ using namespace llvm::sys; Multilib::Multilib(StringRef GCCSuffix, StringRef OSSuffix, StringRef IncludeSuffix, const flags_list &Flags, - StringRef ExclusiveGroup, - std::optional FatalError) + StringRef ExclusiveGroup, std::optional 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 +125,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 +172,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 Flags; std::string Group; }; @@ -217,15 +216,15 @@ struct MultilibSetSerialization { template <> struct llvm::yaml::MappingTraits { 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 +310,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 8aed9ed6e1881..d5c70f02b5e75 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 c006bb4072ce2..761e595757f34 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 dfeef7c2077c7..c03e117d99304 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 9401c95c5183b460f0bdff92f65e14132191ae51 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 2 Oct 2024 09:26:02 +0100 Subject: [PATCH 2/4] [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 | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/clang/lib/Driver/Multilib.cpp b/clang/lib/Driver/Multilib.cpp index cc34794b7dac4..9d5cdde98c797 100644 --- a/clang/lib/Driver/Multilib.cpp +++ b/clang/lib/Driver/Multilib.cpp @@ -99,6 +99,7 @@ bool MultilibSet::select(const Driver &D, const Multilib::flags_list &Flags, llvm::SmallVectorImpl &Selected) const { llvm::StringSet<> FlagSet(expandFlags(Flags)); Selected.clear(); + bool AnyErrors = false; // Decide which multilibs we're going to select at all. llvm::DenseSet ExclusiveGroupsSelected; @@ -123,12 +124,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); @@ -138,7 +138,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 d5c70f02b5e75..7e4cbe6b9b656 100644 --- a/clang/lib/Driver/ToolChains/BareMetal.cpp +++ b/clang/lib/Driver/ToolChains/BareMetal.cpp @@ -186,10 +186,27 @@ 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"; From 6acc753b01a94940978c6a57a7e321b804f969be Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 2 Oct 2024 16:54:12 +0100 Subject: [PATCH 3/4] remove another 'fatal' in a comment --- clang/lib/Driver/Multilib.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/clang/lib/Driver/Multilib.cpp b/clang/lib/Driver/Multilib.cpp index 9d5cdde98c797..2590dcf7f2ea4 100644 --- a/clang/lib/Driver/Multilib.cpp +++ b/clang/lib/Driver/Multilib.cpp @@ -123,10 +123,9 @@ bool MultilibSet::select(const Driver &D, const Multilib::flags_list &Flags, continue; } - // If this multilib is actually a placeholder containing a fatal - // 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 this multilib is actually a placeholder containing an 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; From e92221715ee537ce9fb776f945a26bfc442f4d6e Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Thu, 3 Oct 2024 16:45:48 +0100 Subject: [PATCH 4/4] =?UTF-8?q?remove=20=E2=85=98=20of=20another=20'fatal'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- clang/lib/Driver/Multilib.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/Driver/Multilib.cpp b/clang/lib/Driver/Multilib.cpp index 2590dcf7f2ea4..c56417c6f6d0b 100644 --- a/clang/lib/Driver/Multilib.cpp +++ b/clang/lib/Driver/Multilib.cpp @@ -221,7 +221,7 @@ template <> struct llvm::yaml::MappingTraits { } static std::string validate(IO &io, MultilibSerialization &V) { if (V.Dir.empty() && V.Error.empty()) - return "one of the 'Dir' and 'atalError' keys must be specified"; + return "one of the 'Dir' and 'Error' 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("/"))