[clang][Driver] Correct tool search path priority

[DavidSpickett]

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)

[DavidSpickett]

clang/test/Driver/program-path-priority.c

73

Pretty sure I'm stretching the limits here, probably not suitable for Windows. I hoped to be able to capture the default triple in a CHECK line, then use it in a following RUN line. (though thinking about how FileCheck works, that isn't possible)

[DavidSpickett]

• Fix spelling
• Rework explanatory comments to be a bit clearer.

[DavidSpickett]

• Updated test to look for forward or backslash in expected file paths.

[DavidSpickett]

Update from arc to hopefully run harbormaster builds again.

[DavidSpickett]

Added target triple as a substitution so the tests
don't have to grep for it.
(which I didn't manage to verify on Windows in any case)

[DavidSpickett]

clang/test/Driver/program-path-priority.c

73

Resolved by adding a substitution for the default triple.

[nickdesaulniers]

clang/lib/Driver/Driver.cpp

4725

style nit: are the {} on the for necessary?

clang/test/Driver/program-path-priority.c

4

s/defaul/default/

[DavidSpickett]

• Addressed comments from nickdesaulniers

[DavidSpickett]

Comments on cfe-dev (http://lists.llvm.org/pipermail/cfe-dev/2020-May/065432.html) also in favour of removing the default triple lookup. I tend to agree but think it makes more sense to land this first and address it in a follow up.

[MaskRay]

clang/test/Driver/program-path-priority.c

16

rm -rf %t && mkdir -p %t
You can do this on one line

18

clang can be of several hundred bytes. Copying it is expensive. Use ln -s and add UNSUPPORTED: system-windows with a comment saying "Don't create symlinks on Windows" or similar

20

You can use /// to make comments stand out from RUN and CHECK lines.

21

Place \ on the previous line to emphasize it has a continuation. Add two spaces before the continuation line to emphasize it is a continuation line.

40

Just use / since you are going to use ln -s chmod +x etc

[DavidSpickett]

Address comments from MaskRay.

• Use triple slash for actual comments
• symlink clang, don't run test on Windows
• Merged some commands onto one line e.g. touch/chmod

[DavidSpickett]

clang/test/Driver/program-path-priority.c

21

I think I've done this but I didn't quite understand the \ part of the comment. The first lines already have one.

[MaskRay]

Looks great! Please give other reviews a day or two to respond in case they have comments.

clang/test/Driver/program-path-priority.c

23

Nit: this may be the preference of some reviewers, but they tend to favor | \ in the end of the last line. | makes it clear that it has a continuation line (and if you type the first line with |, your shell will wait for the next line)

[DavidSpickett]

Moved pipe (|) to end of the first lines to
make it clearer that there's a continuation.

[DavidSpickett]

clang/test/Driver/program-path-priority.c

23

Ah now I understand. Yes that looks clearer so I've done that too.

[DavidSpickett]

Committed but made a mistake with arc so it landed with the wrong diff number. In any case a failure on MacOS was reported so reverted while I investigate that.

[DavidSpickett]

Relanded as d6efc9811646edbfe13f06c2676fb469f1c155b1. Change was to get the default triple from clang itself, as on MacOS this can be different to the value you set via CMake.

Closing this review as the diff shown was committed as 028571d60843cb87e2637ef69ee09090d4526c62 but wasn't closed automatically because I had the wrong diff link in the commit msg.

[stevewan]

The test was failing on Linux if I set LLVM_DEFAULT_TARGET_TRIPLE. For example if I set it to`powerpc64le-linux-gnu` clang actually uses "powerpc64le-unknown-linux-gnu".

Would you be able to provide a fix to this?

[DavidSpickett]

I saw similar behaviour on Mac OSX and fixed that in the reland. (https://reviews.llvm.org/rGd6efc9811646edbfe13f06c2676fb469f1c155b1)

Are you still seeing a failure with that applied?

[stevewan]

Yes, this issue was hit with the reland applied. When given a <arch>-<sys>-<abi> format LLVM_DEFAULT_TARGET_TRIPLE, Clang would expand the target triple to <arch>-<vendor>-<sys>-<abi>, and therefore causes the name mismatch between what the driver searches for and what the test case creates as the dummy tool.

[DavidSpickett]

Right, I see the issue.

The code that gets the default triple name (https://reviews.llvm.org/D13340?id=36227#inline-108606) looks up the one you have in cmake, not the actual default which you get in --version. We could "fix" this by doing so when we make the tool name as well, but this breaks whatever mips toolchain was using that. (their tools won't be mips-unknown-elf-<whatever>)

So yes it looks up powerpc64le-linux-gnu but shows powerpc64le-unknown-linux-gnu. Can't go back to using cmake's value because on Mac OS, cmake has x86_64-darwin, clang has x86_64-darwin<version>. Writing to both is a short term option so I will try that and fold it into https://reviews.llvm.org/D83055. (will add you on review once I update it)

(this whole default triple lookup should probably go but I'd really like to do that in its own commit)

[stevewan]

In D79842#2138857, @DavidSpickett wrote:

Right, I see the issue.

The code that gets the default triple name (https://reviews.llvm.org/D13340?id=36227#inline-108606) looks up the one you have in cmake, not the actual default which you get in --version. We could "fix" this by doing so when we make the tool name as well, but this breaks whatever mips toolchain was using that. (their tools won't be mips-unknown-elf-<whatever>)

So yes it looks up powerpc64le-linux-gnu but shows powerpc64le-unknown-linux-gnu. Can't go back to using cmake's value because on Mac OS, cmake has x86_64-darwin, clang has x86_64-darwin<version>. Writing to both is a short term option so I will try that and fold it into https://reviews.llvm.org/D83055. (will add you on review once I update it)

(this whole default triple lookup should probably go but I'd really like to do that in its own commit)

Thanks, @DavidSpickett.

clang/lib/Driver/Driver.cpp

void Driver::generatePrefixedToolNames(
 
   // Allow the discovery of tools prefixed with LLVM's default target triple.
   std::string DefaultTargetTriple = llvm::sys::getDefaultTargetTriple();
   if (DefaultTargetTriple != TargetTriple)
     Names.emplace_back((DefaultTargetTriple + "-" + Tool).str());
 }
 
 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) {
   return false;
 }
 
 std::string Driver::GetProgramPath(StringRef Name, const ToolChain &TC) const {
   SmallVector<std::string, 2> TargetSpecificExecutables;
   generatePrefixedToolNames(Name, TC, TargetSpecificExecutables);
 
   // Respect a limited subset of the '-Bprefix' functionality in GCC by
   // attempting to use this prefix when looking for program paths.
   for (const auto &PrefixDir : PrefixDirs) {
     if (llvm::sys::fs::is_directory(PrefixDir)) {
       SmallString<128> P(PrefixDir);
-      if (ScanDirForExecutable(P, TargetSpecificExecutables))
+      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)))
         return std::string(P.str());
     }
   }
 
   const ToolChain::path_list &List = TC.getProgramPaths();
+  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, TargetSpecificExecutables))
+      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);
 }
 
 std::string Driver::GetTemporaryPath(StringRef Prefix, StringRef Suffix) const {
   SmallString<128> Path;
   std::error_code EC = llvm::sys::fs::createTemporaryFile(Prefix, Suffix, Path);
   if (EC) {

clang/test/Driver/program-path-priority.c

+// Check the priority used when searching for tools
+// Names and locations are usually in this order:
+// <triple>-tool, tool, <defaul 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
+// RUN: mkdir -p %t
+// RUN: cp %clang %t
+
+// 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
+// RUN: 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
+// RUN: 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
+// RUN: 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
+// RUN: 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
+// RUN: 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
+// RUN: default_triple=$(%t/clang --version | grep -oP "(?<=Target:\s).*")
+// RUN: touch %t/${default_triple}-gcc
+// RUN: chmod +x %t/${default_triple}-gcc
+
+// <triple>-gcc on PATH beats default triple in program path
+// RUN: touch %t/${default_triple}-gcc
+// RUN: chmod +x %t/${default_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/${default_triple}-gcc %t/prefix
+// RUN: touch %t/notreal-none-elf-gcc
+// RUN: 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/${default_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