This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] [llvm-symbolizer] Fix failing tests
ClosedPublic

Authored by hintonda on Apr 2 2019, 6:51 PM.

Details

Summary

Don't use simple single word strings for matching as they can
match a substring in the path and give erroneous results.

Event Timeline

hintonda created this revision.Apr 2 2019, 6:51 PM
mattd added a comment.Apr 2 2019, 8:57 PM

LGTM, I'll let @jhenderson weigh-in to see what he says.

jhenderson added inline comments.Apr 3 2019, 1:41 AM
llvm/test/tools/llvm-objcopy/ELF/regex.test
55

Nit: I know this was there before, but would you mind indenting this match, so that it lines up with the other patterns, please?

#REGEX2-NOT: Name: foobaz
#REGEX2:     Name: bar
#REGEX2-NOT: Name: rebar
llvm/test/tools/llvm-symbolizer/ignore-undefined-symbols.s
3

Is implicit-check-not a regex pattern or just a literal check (I think it's a literal check)? Does this still fail when bar appears instead of foo?

hintonda marked an inline comment as done.Apr 3 2019, 9:06 AM
hintonda added inline comments.
llvm/test/tools/llvm-symbolizer/ignore-undefined-symbols.s
3

Yes, you're correct, it's a literal check. I can reproduce by using my login -- which obviously isn't a symbol:

$ bin/llvm-symbolizer --obj=x.o 0 | bin/FileCheck /Users/dhinton/projects/llvm_project/monorepo/llvm-project/llvm/test/tools/llvm-symbolizer/ignore-undefined-symbols.s --implicit-check-not=dhinton
command line:1:22: error: CHECK-NOT: excluded string found in input
-implicit-check-not='dhinton'
                     ^
<stdin>:2:8: note: found here
/Users/dhinton/projects/llvm_project/monorepo/build/Debug/../../llvm-project/llvm/test/tools/llvm-symbolizer/ignore-undefined-symbols.s:12:0
       ^~~~~~~

I'll see if I can come up with a better solution.

hintonda marked an inline comment as done.Apr 3 2019, 9:21 AM
hintonda added inline comments.
llvm/test/tools/llvm-symbolizer/ignore-undefined-symbols.s
3

Okay, prepending {{.*}} to ignore-undefined-symbols below fixes the problem. Will update shortly. Thanks for the feedback.

hintonda updated this revision to Diff 193529.Apr 3 2019, 9:50 AM
  • Fix formatting and implicit-check-not matching.
jhenderson added inline comments.Apr 3 2019, 9:53 AM
llvm/test/tools/llvm-symbolizer/ignore-undefined-symbols.s
3

You may be able to use --implicit-check-not={{^}}bar{{$}}. I haven't tested that though.

3

I'm not sure I follow how {{.*}} helps fix this? Isn't the problem to do with the use of "foo" without other checks?

3

"bar", I mean.

hintonda marked an inline comment as done.Apr 3 2019, 10:10 AM
hintonda added inline comments.
llvm/test/tools/llvm-symbolizer/ignore-undefined-symbols.s
3

This works as well. Would you prefer this change?

hintonda updated this revision to Diff 193537.Apr 3 2019, 10:16 AM
  • Move the regex expression(s) to the parameter.
rupprecht added inline comments.Apr 3 2019, 3:34 PM
llvm/test/tools/llvm-objcopy/ELF/regex.test
50–52

These tests fails because the match is embedded in the filename depending on the username. So you can skip the filename by searching for "Symbols [", since FileCheck checks for things in order.

e.g.

# REGEX1:     Symbols [
# REGEX1-NOT:   Name: foobaz
# REGEX1-NOT:   Name: bar
# REGEX1-NOT:   Name: rebar
...

(It should even work without the Name: part, but it's clearer to leave that in)

hintonda updated this revision to Diff 193629.Apr 3 2019, 4:56 PM
  • Address comments.
This revision is now accepted and ready to land.Apr 4 2019, 1:26 AM
jhenderson added inline comments.Apr 4 2019, 1:29 AM
llvm/test/tools/llvm-symbolizer/ignore-undefined-symbols.s
3

For reference, I figured out what the issue was, so I'm going to clarify it for any future readers. The full text of the ignore-undefined-symbols.s:12:0 line is an absolute path, IIRC, so could be "/foo/bar/ignore-undefined-symbol.s:12:0". By adding {{.*}} to the pattern, the positive match includes everything on the line. CHECK-NOT (and therefore --implicit-check-not) only matches against things not matched by other CHECKs, so this avoids the issue.

This revision was automatically updated to reflect the committed changes.