Page MenuHomePhabricator

[lld][ELF][test] Improve deplib.s
ClosedPublic

Authored by jhenderson on Mar 26 2020, 7:51 AM.

Details

Summary

The test had a few style issues, and I noticed a hole in the coverage (namely that the search order wasn't tested). Adding cases for the hole in turn meant other cases weren't important.

The .so test case isn't important, since the code is shared code, so I've removed it. Additionally, I've modified the usage of the "bar" directive to show that an unneeded library must still be present, or the link will fail, even though it isn't linked in.

Diff Detail

Event Timeline

jhenderson created this revision.Mar 26 2020, 7:51 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: emaste. · View Herald Transcript

Remove unneeded test input.

Harbormaster completed remote builds in B50543: Diff 252848.
MaskRay added inline comments.Mar 26 2020, 9:00 AM
lld/test/ELF/deplibs.s
4–5

Up to you, but Inputs/deplibs-lib_foo.s can also be removed. It is small and thus inlining it helps reading.

echo '.globl foo; foo:' | llvm-mc -filetype=obj -triple=x86_64 - -o %tfoo.o (most -unknown-linux are not useful)

62

One space after PLAIN-DIR-NEXT:

68

is not -> may not be

jhenderson marked 3 inline comments as done.

Address review comments:

  • Deleted foo input file.
  • Fix spacing.
  • Improved comment.

Fix other test due to removal of input file.

MaskRay accepted this revision.Mar 30 2020, 1:12 PM

Thanks! With one nit.

lld/test/ELF/deplibs-colon-prefix.s
5 ↗(On Diff #253551)

Nit: delete -unknown-linux as this is a generic ELF behavior.

This revision is now accepted and ready to land.Mar 30 2020, 1:12 PM
grimar accepted this revision.Mar 31 2020, 2:08 AM

LGTM. Have a suggestion inlined though.

lld/test/ELF/deplibs.s
63

To me it looks a bit overcomplicated to use --check-prefixes here. The COMMON part is just a single line,
but having --check-prefixes=COMMON,CWD, --check-prefixes=COMMON,PLAIN-DIR and --check-prefixes=COMMON,LIBA-DIR
seems reads slightly harder than the following:

# RUN: ld.lld %t.o -o /dev/null -L %t.dir --trace 2>&1 | \
# RUN:   FileCheck %s -DOBJ=%t.o -DDIR=%t.dir --check-prefix=CWD \
# RUN:                --implicit-check-not=foo.a --implicit-check-not=libbar.so

# CWD:      [[OBJ]]
# CWD-NEXT: {{^foo\.a}}

# RUN: rm %t.cwd/foo.a
# RUN: ld.lld %t.o -o /dev/null -L %t.dir --trace 2>&1 | \
# RUN:   FileCheck %s -DOBJ=%t.o -DDIR=%t.dir --check-prefix=PLAIN-DIR \
# RUN:                --implicit-check-not=foo.a --implicit-check-not=libbar.so

# PLAIN-DIR:      [[OBJ]]
# PLAIN-DIR-NEXT: {{^foo\.a}}

# RUN: rm %t.dir/foo.a
# RUN: ld.lld %t.o -o /dev/null -L %t.dir --trace 2>&1 | \
# RUN:   FileCheck %s -DOBJ=%t.o -DDIR=%t.dir --check-prefix=LIBA-DIR \
# RUN:                --implicit-check-not=foo.a --implicit-check-not=libbar.so

# LIBA-DIR:      [[OBJ]]
# LIBA-DIR-NEXT: {{^foo\.a}}

Up to you probably.

Remove "-unknown-linux" and adopted @grimar's suggestion.

The update LG too.

This revision was automatically updated to reflect the committed changes.