Page MenuHomePhabricator

[clang][Driver] Fix tool path priority test failures
ClosedPublic

Authored by DavidSpickett on Jul 2 2020, 8:13 AM.

Details

Summary

Failure type 1:
This test can fail when the path of the build includes the strings
we're checking for. E.g "/gcc" is found in ".../gcc_7.3.0/..."

To correct this look for '"' on the end of all matches. So that we
only match the end of paths printed by clang -###.
(which would be ".../gcc_7.3.0/.../gcc" for the example)

Also look for other gcc names like gcc-x.y.z in the first check.
This confirms that the copy of clang we made is isolated as expected.

Failure type 2:
If you use a triple like "powerpc64le-linux-gnu" clang actually reports
"powerpc64le-unknown-linux-gnu". Then it searches for the
former.

That combined with Mac OS adding a version number to cmake's triple
means we can't trust cmake or clang to give us the one default triple.
To fix the test, write to both names. As they don't overlap with our
fake triple, we're still showing that the lookup works.

Diff Detail

Event Timeline

DavidSpickett created this revision.Jul 2 2020, 8:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2020, 8:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
  • Added check for alternate gcc names in the first test.

The description appears to be wrapped at a very small column number. Can you please fix it?

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

\ can be removed.

  • Removed \ from regex, not needed for POSIX syntax.
  • Reflowed text in commit description.
DavidSpickett edited the summary of this revision. (Show Details)Tue, Jul 7, 2:09 AM
DavidSpickett marked an inline comment as done.
DavidSpickett added a subscriber: stevewan.
  • Write to cmake and clang's default triple to also fix failure reported by @stevewan.
DavidSpickett edited the summary of this revision. (Show Details)
DavidSpickett retitled this revision from [clang][Driver] Fix tool path priority test failure to [clang][Driver] Fix tool path priority test failures.Wed, Jul 8, 6:33 AM
DavidSpickett added a comment.EditedWed, Jul 8, 6:37 AM

For the record my logic here is that I could change the tool names to use the same name as --version. However that will break whatever mips toolchain needed this code in the first place. (https://reviews.llvm.org/D13340?id=36227#inline-108606)

This default triple lookup code had some support to be removed anyway, so I'd rather patch up this test first before doing so. So that it's easier to roll back if there are still users of it. Having tests of the existing behaviour is a good thing and I don't think writing to both potential default triples undermines that. (one will just get ignored)

MaskRay added inline comments.Wed, Jul 8, 11:06 AM
clang/test/Driver/program-path-priority.c
117

If $DEFAULT_TRIPLE == %target_triple, this mv command will fail.

The same applies to the rm below.

  • Updated rm/mv command lines to account for $DEFAULT_TRIPLE being the same as %target_triple.
DavidSpickett marked an inline comment as done.Thu, Jul 9, 2:24 AM
stevewan added inline comments.Thu, Jul 9, 4:14 PM
clang/test/Driver/program-path-priority.c
117

Maybe I'm not seeing something obvious here, but I'm not aware of the file -E usage, and on Linux I got file: invalid option -- 'E'.

  • mv with stderr redirected instead of using non standard file -E flag
DavidSpickett marked 2 inline comments as done.Fri, Jul 10, 1:52 AM
DavidSpickett added inline comments.
clang/test/Driver/program-path-priority.c
117

I was looking for a way to mimic [! -f <file>], which worked on the command line but not in a test. From my system's file:

-E      On filesystem errors (file not found etc), instead of handling the error as regu‐
         lar output as POSIX mandates and keep going, issue an error message and exit.

I didn't realise it was non-standard, so I've switched to mv || true with the error redirected so it won't confuse the verbose output.

stevewan added inline comments.Fri, Jul 10, 11:19 AM
clang/test/Driver/program-path-priority.c
117

I believe having just mv || true works, but I did like the idea of pre-checking for the existence of the file, and the test is more robust with the check. Is there a reason we wouldn't use test -f here?

DavidSpickett marked an inline comment as done.
  • Use more standard 'test -f' command before 'mv's
DavidSpickett marked 2 inline comments as done.Mon, Jul 13, 1:37 AM
DavidSpickett added inline comments.
clang/test/Driver/program-path-priority.c
117

Honestly it slipped my mind that [ ] was the same as test. Updated to test && move || true which you're right is clearer.
(one will always fail if the two triples are the same but at least we're only hiding the error we expect, rather than all errors mv could have)

stevewan accepted this revision.Mon, Jul 13, 8:46 AM

This LGTM, but since I'm not most familiar with the type 1 failure in description, let's see if other reviewers have further comments.

This revision is now accepted and ready to land.Mon, Jul 13, 8:46 AM
miyuki added a subscriber: miyuki.Tue, Jul 14, 9:50 AM

I have reproduced the type 1 failure and I confirm that this patch fixes the issue.

This revision was automatically updated to reflect the committed changes.
DavidSpickett marked an inline comment as done.