Page MenuHomePhabricator

[clang][Driver] Correct tool search path priority
ClosedPublic

Authored by DavidSpickett on May 13 2020, 2:44 AM.

Details

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)

Diff Detail

Event Timeline

DavidSpickett created this revision.May 13 2020, 2:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2020, 2:44 AM
DavidSpickett marked an inline comment as done.May 13 2020, 2:49 AM
DavidSpickett added inline comments.
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 marked an inline comment as not done.May 13 2020, 3:20 AM
  • Fix spelling
  • Rework explanatory comments to be a bit clearer.
  • Updated test to look for forward or backslash in expected file paths.

Update from arc to hopefully run harbormaster builds again.

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 marked an inline comment as done.May 15 2020, 4:04 AM
DavidSpickett marked an inline comment as done.
DavidSpickett added inline comments.
clang/test/Driver/program-path-priority.c
73

Resolved by adding a substitution for the default triple.

clang/lib/Driver/Driver.cpp
4740

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

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

s/defaul/default/

  • Addressed comments from nickdesaulniers
DavidSpickett marked 2 inline comments as done.May 22 2020, 4:02 AM

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 added inline comments.Jun 6 2020, 1:43 PM
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 updated this revision to Diff 270716.EditedJun 15 2020, 4:16 AM

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 marked 4 inline comments as done.Jun 15 2020, 4:20 AM
DavidSpickett added inline comments.
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 accepted this revision.EditedJun 15 2020, 10:28 AM

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)

This revision is now accepted and ready to land.Jun 15 2020, 10:28 AM

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

DavidSpickett marked 2 inline comments as done.Jun 17 2020, 5:43 AM
DavidSpickett added inline comments.
clang/test/Driver/program-path-priority.c
23

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

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 closed this revision.Jun 25 2020, 2:15 AM

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.

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?

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?

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.

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 added a comment.EditedJul 8 2020, 2:06 PM

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.