[clang] 028571d - [clang][Driver] Correct tool search path priority
David Spickett via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 22 06:04:17 PDT 2020
Thanks for the heads up Nico. Yes that was a mistake, it was reviewed
as https://reviews.llvm.org/D79842 but somehow I told arc to land it
as the duplicate diff instead.
I'll look into the failure.
On Mon, 22 Jun 2020 at 13:48, Nico Weber <thakis at chromium.org> wrote:
>
> Hi,
>
> https://reviews.llvm.org/D79988 is apparently a "Restricted Differential Revision" and I don't have permissions to do that. This being an open source project, this is not something we do. I'm guessing it's not intentional?
>
> This also breaks check-clang on macOS: http://45.33.8.238/mac/15950/step_7.txt Please take a look and revert if it takes a while to investigate.
>
> Nico
>
> On Mon, Jun 22, 2020 at 4:41 AM David Spickett via cfe-commits <cfe-commits at lists.llvm.org> wrote:
>>
>>
>> Author: David Spickett
>> Date: 2020-06-22T09:41:13+01:00
>> New Revision: 028571d60843cb87e2637ef69ee09090d4526c62
>>
>> URL: https://github.com/llvm/llvm-project/commit/028571d60843cb87e2637ef69ee09090d4526c62
>> DIFF: https://github.com/llvm/llvm-project/commit/028571d60843cb87e2637ef69ee09090d4526c62.diff
>>
>> LOG: [clang][Driver] Correct tool search path priority
>>
>> Summary:
>> As seen in:
>> https://bugs.llvm.org/show_bug.cgi?id=45693
>>
>> When clang looks for a tool it has a set of
>> possible names for it, in priority order.
>> Previously it would look for these names in
>> the program path. Then look for all the names
>> in the PATH.
>>
>> This means that aarch64-none-elf-gcc on the PATH
>> would lose to gcc in the program path.
>> (which was /usr/bin in the bug's case)
>>
>> This changes that logic to search each name in both
>> possible locations, then move to the next name.
>> Which is more what you would expect to happen when
>> using a non default triple.
>>
>> (-B prefixes maybe should follow this logic too,
>> but are not changed in this patch)
>>
>> Subscribers: kristof.beyls, cfe-commits
>>
>> Tags: #clang
>>
>> Differential Revision: https://reviews.llvm.org/D79988
>>
>> Added:
>> clang/test/Driver/program-path-priority.c
>>
>> Modified:
>> clang/lib/Driver/Driver.cpp
>> clang/test/lit.cfg.py
>>
>> Removed:
>>
>>
>>
>> ################################################################################
>> diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
>> index a48761af400f..27477553963c 100644
>> --- a/clang/lib/Driver/Driver.cpp
>> +++ b/clang/lib/Driver/Driver.cpp
>> @@ -4717,13 +4717,11 @@ void Driver::generatePrefixedToolNames(
>> }
>>
>> static bool ScanDirForExecutable(SmallString<128> &Dir,
>> - ArrayRef<std::string> Names) {
>> - for (const auto &Name : Names) {
>> - llvm::sys::path::append(Dir, Name);
>> - if (llvm::sys::fs::can_execute(Twine(Dir)))
>> - return true;
>> - llvm::sys::path::remove_filename(Dir);
>> - }
>> + const std::string &Name) {
>> + llvm::sys::path::append(Dir, Name);
>> + if (llvm::sys::fs::can_execute(Twine(Dir)))
>> + return true;
>> + llvm::sys::path::remove_filename(Dir);
>> return false;
>> }
>>
>> @@ -4736,8 +4734,9 @@ std::string Driver::GetProgramPath(StringRef Name, const ToolChain &TC) const {
>> for (const auto &PrefixDir : PrefixDirs) {
>> if (llvm::sys::fs::is_directory(PrefixDir)) {
>> SmallString<128> P(PrefixDir);
>> - if (ScanDirForExecutable(P, TargetSpecificExecutables))
>> - return std::string(P.str());
>> + for (const auto &TargetSpecificExecutable : TargetSpecificExecutables)
>> + if (ScanDirForExecutable(P, TargetSpecificExecutable))
>> + return std::string(P.str());
>> } else {
>> SmallString<128> P((PrefixDir + Name).str());
>> if (llvm::sys::fs::can_execute(Twine(P)))
>> @@ -4746,17 +4745,25 @@ std::string Driver::GetProgramPath(StringRef Name, const ToolChain &TC) const {
>> }
>>
>> const ToolChain::path_list &List = TC.getProgramPaths();
>> - for (const auto &Path : List) {
>> - SmallString<128> P(Path);
>> - if (ScanDirForExecutable(P, TargetSpecificExecutables))
>> - return std::string(P.str());
>> - }
>> + for (const auto &TargetSpecificExecutable : TargetSpecificExecutables) {
>> + // For each possible name of the tool look for it in
>> + // program paths first, then the path.
>> + // Higher priority names will be first, meaning that
>> + // a higher priority name in the path will be found
>> + // instead of a lower priority name in the program path.
>> + // E.g. <triple>-gcc on the path will be found instead
>> + // of gcc in the program path
>> + for (const auto &Path : List) {
>> + SmallString<128> P(Path);
>> + if (ScanDirForExecutable(P, TargetSpecificExecutable))
>> + return std::string(P.str());
>> + }
>>
>> - // If all else failed, search the path.
>> - for (const auto &TargetSpecificExecutable : TargetSpecificExecutables)
>> + // Fall back to the path
>> if (llvm::ErrorOr<std::string> P =
>> llvm::sys::findProgramByName(TargetSpecificExecutable))
>> return *P;
>> + }
>>
>> return std::string(Name);
>> }
>>
>> diff --git a/clang/test/Driver/program-path-priority.c b/clang/test/Driver/program-path-priority.c
>> new file mode 100644
>> index 000000000000..48f23917812e
>> --- /dev/null
>> +++ b/clang/test/Driver/program-path-priority.c
>> @@ -0,0 +1,106 @@
>> +/// Don't create symlinks on Windows
>> +// UNSUPPORTED: system-windows
>> +
>> +/// Check the priority used when searching for tools
>> +/// Names and locations are usually in this order:
>> +/// <triple>-tool, tool, <default triple>-tool
>> +/// program path, PATH
>> +/// (from highest to lowest priority)
>> +/// A higher priority name found in a lower priority
>> +/// location will win over a lower priority name in a
>> +/// higher priority location.
>> +/// Prefix dirs (added with -B) override the location,
>> +/// so only name priority is accounted for, unless we fail to find
>> +/// anything at all in the prefix.
>> +
>> +/// Copy clang to a new dir which will be its
>> +/// "program path" for these tests
>> +// RUN: rm -rf %t && mkdir -p %t
>> +// RUN: ln -s %clang %t/clang
>> +
>> +/// No gccs at all, nothing is found
>> +// RUN: env "PATH=" %t/clang -### -target notreal-none-elf %s 2>&1 | \
>> +// RUN: FileCheck --check-prefix=NO_NOTREAL_GCC %s
>> +// NO_NOTREAL_GCC-NOT: notreal-none-elf-gcc
>> +// NO_NOTREAL_GCC-NOT: /gcc
>> +
>> +/// <triple>-gcc in program path is found
>> +// RUN: touch %t/notreal-none-elf-gcc && chmod +x %t/notreal-none-elf-gcc
>> +// RUN: env "PATH=" %t/clang -### -target notreal-none-elf %s 2>&1 | \
>> +// RUN: FileCheck --check-prefix=PROG_PATH_NOTREAL_GCC %s
>> +// PROG_PATH_NOTREAL_GCC: notreal-none-elf-gcc
>> +
>> +/// <triple>-gcc on the PATH is found
>> +// RUN: mkdir -p %t/env
>> +// RUN: rm %t/notreal-none-elf-gcc
>> +// RUN: touch %t/env/notreal-none-elf-gcc && chmod +x %t/env/notreal-none-elf-gcc
>> +// RUN: env "PATH=%t/env/" %t/clang -### -target notreal-none-elf %s 2>&1 | \
>> +// RUN: FileCheck --check-prefix=ENV_PATH_NOTREAL_GCC %s
>> +// ENV_PATH_NOTREAL_GCC: env/notreal-none-elf-gcc
>> +
>> +/// <triple>-gcc in program path is preferred to one on the PATH
>> +// RUN: touch %t/notreal-none-elf-gcc && chmod +x %t/notreal-none-elf-gcc
>> +// RUN: env "PATH=%t/env/" %t/clang -### -target notreal-none-elf %s 2>&1 | \
>> +// RUN: FileCheck --check-prefix=BOTH_NOTREAL_GCC %s
>> +// BOTH_NOTREAL_GCC: notreal-none-elf-gcc
>> +// BOTH_NOTREAL_GCC-NOT: env/notreal-none-elf-gcc
>> +
>> +/// On program path, <triple>-gcc is preferred to plain gcc
>> +// RUN: touch %t/gcc && chmod +x %t/gcc
>> +// RUN: env "PATH=" %t/clang -### -target notreal-none-elf %s 2>&1 | \
>> +// RUN: FileCheck --check-prefix=NOTREAL_GCC_PREFERRED %s
>> +// NOTREAL_GCC_PREFERRED: notreal-none-elf-gcc
>> +// NOTREAL_GCC_PREFERRED-NOT: /gcc
>> +
>> +/// <triple>-gcc on the PATH is preferred to gcc in program path
>> +// RUN: rm %t/notreal-none-elf-gcc
>> +// RUN: env "PATH=%t/env/" %t/clang -### -target notreal-none-elf %s 2>&1 | \
>> +// RUN: FileCheck --check-prefix=NOTREAL_PATH_OVER_GCC_PROG %s
>> +// NOTREAL_PATH_OVER_GCC_PROG: env/notreal-none-elf-gcc
>> +// NOTREAL_PATH_OVER_GCC_PROG-NOT: /gcc
>> +
>> +/// <triple>-gcc on the PATH is preferred to gcc on the PATH
>> +// RUN: rm %t/gcc
>> +// RUN: touch %t/env/gcc && chmod +x %t/env/gcc
>> +// RUN: env "PATH=%t/env/" %t/clang -### -target notreal-none-elf %s 2>&1 | \
>> +// RUN: FileCheck --check-prefix=NOTREAL_PATH_OVER_GCC_PATH %s
>> +// NOTREAL_PATH_OVER_GCC_PATH: env/notreal-none-elf-gcc
>> +// NOTREAL_PATH_OVER_GCC_PATH-NOT: /gcc
>> +
>> +/// <default-triple>-gcc has lowest priority so <triple>-gcc
>> +/// on PATH beats default triple in program path
>> +// RUN: touch %t/%target_triple-gcc && chmod +x %t/%target_triple-gcc
>> +// RUN: env "PATH=%t/env/" %t/clang -### -target notreal-none-elf %s 2>&1 | \
>> +// RUN: FileCheck --check-prefix=DEFAULT_TRIPLE_GCC %s
>> +// DEFAULT_TRIPLE_GCC: env/notreal-none-elf-gcc
>> +
>> +/// plain gcc on PATH beats default triple in program path
>> +// RUN: rm %t/env/notreal-none-elf-gcc
>> +// RUN: env "PATH=%t/env/" %t/clang -### -target notreal-none-elf %s 2>&1 | \
>> +// RUN: FileCheck --check-prefix=DEFAULT_TRIPLE_NO_NOTREAL %s
>> +// DEFAULT_TRIPLE_NO_NOTREAL: env/gcc
>> +// DEFAULT_TRIPLE_NO_NOTREAL-NOT: -gcc
>> +
>> +/// default triple only chosen when no others are present
>> +// RUN: rm %t/env/gcc
>> +// RUN: env "PATH=%t/env/" %t/clang -### -target notreal-none-elf %s 2>&1 | \
>> +// RUN: FileCheck --check-prefix=DEFAULT_TRIPLE_NO_OTHERS %s
>> +// DEFAULT_TRIPLE_NO_OTHERS: -gcc
>> +// DEFAULT_TRIPLE_NO_OTHERS-NOT: notreal-none-elf-gcc
>> +// DEFAULT_TRIPLE_NO_OTHERS-NOT: /gcc
>> +
>> +/// -B paths are searched separately so default triple will win
>> +/// if put in one of those even if other paths have higher priority names
>> +// RUN: mkdir -p %t/prefix
>> +// RUN: mv %t/%target_triple-gcc %t/prefix
>> +// RUN: touch %t/notreal-none-elf-gcc && chmod +x %t/notreal-none-elf-gcc
>> +// RUN: env "PATH=" %t/clang -### -target notreal-none-elf %s -B %t/prefix 2>&1 | \
>> +// RUN: FileCheck --check-prefix=DEFAULT_TRIPLE_IN_PREFIX %s
>> +// DEFAULT_TRIPLE_IN_PREFIX: prefix/{{.*}}-gcc
>> +// DEFAULT_TRIPLE_IN_PREFIX-NOT: notreal-none-elf-gcc
>> +
>> +/// Only if there is nothing in the prefix will we search other paths
>> +// RUN: rm %t/prefix/%target_triple-gcc
>> +// RUN: env "PATH=" %t/clang -### -target notreal-none-elf %s -B %t/prefix 2>&1 | \
>> +// RUN: FileCheck --check-prefix=EMPTY_PREFIX_DIR %s
>> +// EMPTY_PREFIX_DIR: notreal-none-elf-gcc
>>
>> diff --git a/clang/test/lit.cfg.py b/clang/test/lit.cfg.py
>> index 413f81175420..b1f4abe4ec3a 100644
>> --- a/clang/test/lit.cfg.py
>> +++ b/clang/test/lit.cfg.py
>> @@ -46,6 +46,8 @@
>> config.substitutions.append(
>> ('%src_include_dir', config.clang_src_dir + '/include'))
>>
>> +config.substitutions.append(
>> + ('%target_triple', config.target_triple))
>>
>> # Propagate path to symbolizer for ASan/MSan.
>> llvm_config.with_system_environment(
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list