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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
