Page MenuHomePhabricator

[Test] Make llvm and lld tests pass when $USER matches `bar`
ClosedPublic

Authored by bbaren on Jan 7 2020, 1:17 PM.

Diff Detail

Event Timeline

bbaren created this revision.Jan 7 2020, 1:17 PM

I left similar comments on D72357 too

lld/test/COFF/start-lib.ll
24–25

I'm not sure what this is supposed to be. Can it just be Name: bar? Ditto for TEST3-NOT below

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test
6 ↗(On Diff #236673)

Read from stdin to avoid the filename showing up, e.g.

# RUN: llvm-readobj --symbols < %t2.o | FileCheck %s --implicit-check-not=bar

Then the rest of the test can remain unchanged

llvm/test/tools/llvm-objcopy/ELF/tail-merged-string-tables.test
7–8 ↗(On Diff #236673)

Same here, read from stdin

MaskRay added inline comments.Jan 7 2020, 1:58 PM
lld/test/COFF/start-lib.ll
36–38

Is {{ }}foo good enough?

The map file is something like 00001010 00000000 0 bar

jhenderson added inline comments.Jan 8 2020, 1:04 AM
llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test
6 ↗(On Diff #236673)

This will collide with D72357. As noted in that review, I'd prefer just changing the patterns to "Name: bar" (e.g. --implicit-check-not="Name: bar").

llvm/test/tools/llvm-objcopy/ELF/tail-merged-string-tables.test
7–8 ↗(On Diff #236673)

See my above comment/comment in D72357.

bbaren updated this revision to Diff 241748.Jan 31 2020, 9:04 AM
bbaren marked 10 inline comments as done.

Take D72358 into account; simplify some regexes

MaskRay added inline comments.Jan 31 2020, 9:23 AM
lld/test/ELF/lto/linker-script-symbols-assign.ll
6

Use llvm-nm %t2.lto.o

It does not include a file name.

lld/test/ELF/undefined-glob.s
17

Use llvm-nm

bbaren updated this revision to Diff 246548.Feb 25 2020, 12:53 PM

Use llvm-nm where appropriate

bbaren marked 2 inline comments as done.Feb 25 2020, 12:59 PM
bbaren added inline comments.
lld/test/COFF/start-lib.ll
24–25

Name: bar doesn’t work – the map file contains

00001010 00000000     0                 bar

However, @MaskRay’s suggestion below applies here as well. Changed to {{ }}bar.

36–38

Yes, that works great. Changed.

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test
6 ↗(On Diff #236673)

Obviated by D72358.

llvm/test/tools/llvm-objcopy/ELF/tail-merged-string-tables.test
7–8 ↗(On Diff #236673)

Also obviated by D72358.

MaskRay added inline comments.Feb 25 2020, 1:32 PM
lld/test/COFF/start-lib.ll
24–25

Maybe it is clearer to match the full line.

lld/test/ELF/lto/linker-script-symbols-assign.ll
6

| count 0

bbaren updated this revision to Diff 246567.Feb 25 2020, 2:04 PM

Use count 0 to check for empty output

bbaren marked an inline comment as done.Feb 25 2020, 2:04 PM
jhenderson added inline comments.Feb 26 2020, 1:00 AM
lld/test/COFF/start-lib.ll
24–25

If that's the format, then the TEST2-NOT lines either side of this line won't work...

Also, perhaps consider {{ }}bar{{$}, since {{ }}bar could match somebody with a space in their path before "bar".

bbaren updated this revision to Diff 246727.Feb 26 2020, 7:41 AM
bbaren marked an inline comment as done.

Check for end-of-line in start-lib.ll, and fix assertion issue

lld/test/COFF/start-lib.ll
24–25

Ah, good point. Switched everywhere to {{ }}symbolName{{$}}.

jhenderson added inline comments.Feb 26 2020, 8:16 AM
lld/test/COFF/start-lib.ll
36–38

Possibly want to change these lines too to have {{$}} at the end?

bbaren updated this revision to Diff 247920.Mar 3 2020, 8:50 AM

Add some more end-of-line checks

bbaren marked an inline comment as done.Mar 3 2020, 8:50 AM
MaskRay accepted this revision.Mar 3 2020, 10:23 AM
MaskRay added inline comments.
lld/test/COFF/start-lib.ll
24–25

It may be helpful to add ; TEST2: Address Size Align Out In Symbol before the first check line.

The sames applies to TEST3

This revision is now accepted and ready to land.Mar 3 2020, 10:23 AM

Does this fix ninja -C /path/to/build/directory check-all for you?

bbaren added a comment.Mar 3 2020, 2:58 PM

Does this fix ninja -C /path/to/build/directory check-all for you?

It does for me, at least.

This revision was automatically updated to reflect the committed changes.