[PATCH] D79842: [clang][Driver] Correct tool search path priority

stevewan added a comment. 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

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett added a comment. 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

[PATCH] D79842: [clang][Driver] Correct tool search path priority

stevewan added a comment. Yes, this issue was hit with the reland applied. When given a `--` format LLVM_DEFAULT_TARGET_TRIPLE, Clang would expand the target triple to `---`, and therefore causes the name mismatch between what the driver searches for and what the test case creates as the

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett added a comment. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D79842: [clang][Driver] Correct tool search path priority

stevewan added a comment. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett closed this revision. DavidSpickett added a comment. Relanded as d6efc9811646edbfe13f06c2676fb469f1c155b1 . Change was to get the default triple from clang itself, as on MacOS this can be different to the value

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett updated this revision to Diff 271345. DavidSpickett added a comment. Moved pipe (|) to end of the first lines to make it clearer that there's a continuation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79842/new/

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett marked 2 inline comments as done. DavidSpickett added inline comments. Comment at: clang/test/Driver/program-path-priority.c:22 +/// No gccs at all, nothing is found +// RUN: env "PATH=" %t/clang -### -target notreal-none-elf %s 2>&1 \ +// RUN: | FileCheck

[PATCH] D79842: [clang][Driver] Correct tool search path priority

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. Looks great! Please give other reviews a day or two to respond in case they have comments. Comment at: clang/test/Driver/program-path-priority.c:22 +/// No gccs at all,

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett marked 4 inline comments as done. DavidSpickett added inline comments. Comment at: clang/test/Driver/program-path-priority.c:20 +// No gccs at all, nothing is found +// RUN: env "PATH=" %t/clang -### -target notreal-none-elf %s 2>&1 \ +// RUN: | FileCheck

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett updated this revision to Diff 270716. DavidSpickett added a comment. 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 Repository: rG LLVM Github Monorepo

[PATCH] D79842: [clang][Driver] Correct tool search path priority

MaskRay added inline comments. Comment at: clang/test/Driver/program-path-priority.c:15 +// "program path" for these tests +// RUN: rm -rf %t +// RUN: mkdir -p %t `rm -rf %t && mkdir -p %t` You can do this on one line Comment at:

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett added a comment. 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. Repository: rG LLVM Github

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett updated this revision to Diff 265694. DavidSpickett added a comment. - Addressed comments from nickdesaulniers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79842/new/ https://reviews.llvm.org/D79842 Files:

[PATCH] D79842: [clang][Driver] Correct tool search path priority

nickdesaulniers added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4730 + return std::string(P.str()); + } } else { style nit: are the `{}` on the `for` necessary? Comment at:

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett marked an inline comment as done. DavidSpickett added inline comments. Comment at: clang/test/Driver/program-path-priority.c:72 +// -gcc has lowest priority +// RUN: default_triple=$(%t/clang --version | grep -oP "(?<=Target:\s).*") +// RUN: touch

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett updated this revision to Diff 264201. DavidSpickett added a comment. 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) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett updated this revision to Diff 264187. DavidSpickett added a comment. Update from arc to hopefully run harbormaster builds again. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79842/new/ https://reviews.llvm.org/D79842 Files:

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett updated this revision to Diff 263696. DavidSpickett added a comment. - Updated test to look for forward or backslash in expected file paths. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79842/new/ https://reviews.llvm.org/D79842 Files: clang/lib/Driver/Driver.cpp

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett updated this revision to Diff 263686. DavidSpickett added a comment. - Fix spelling - Rework explanatory comments to be a bit clearer. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79842/new/ https://reviews.llvm.org/D79842 Files: clang/lib/Driver/Driver.cpp

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett created this revision. DavidSpickett added reviewers: rogfer01, ddunbar, chandlerc. Herald added subscribers: cfe-commits, kristof.beyls. Herald added a project: clang. DavidSpickett marked an inline comment as done. DavidSpickett added inline comments. Comment at:

[PATCH] D79842: [clang][Driver] Correct tool search path priority

DavidSpickett marked an inline comment as done. DavidSpickett added inline comments. Comment at: clang/test/Driver/program-path-priority.c:72 +// -gcc has lowest priority +// RUN: default_triple=$(%t/clang --version | grep -oP "(?<=Target:\s).*") +// RUN: touch