Page MenuHomePhabricator

[lld][ELF][test] Add additional LTO testing
ClosedPublic

Authored by jhenderson on Sep 17 2020, 3:47 AM.

Details

Summary

The additional testing is testing we previously had in a downstream testsuite.

Diff Detail

Event Timeline

jhenderson created this revision.Sep 17 2020, 3:47 AM
jhenderson requested review of this revision.Sep 17 2020, 3:47 AM
grimar added inline comments.Sep 17 2020, 4:38 AM
lld/test/ELF/lto/archive-mixed.ll
26 ↗(On Diff #292453)

nit: in the llvm-ar block above, a.bc.b.o.a follows the a.bc.b.bc.a. I'd suggest to keep the order here.

lld/test/ELF/lto/undef-mixed2.s
3 ↗(On Diff #292453)

I'd put "REQUIRES" on the first line.

5 ↗(On Diff #292453)

Any reason not to use split-file instead of adding a new input?

lld/test/ELF/lto/wrap-2.ll
2–43

Probably worth adding a general comment about what this test case validates?

5

%t.bc.bc.so looks a bit strange, though I see why you name it like this.
Do %t.bc-bc.so reads better? (the same for other .so).

16

Perhaps %t2.o, since you have %t1.o already?

jhenderson marked 6 inline comments as done.

Address @grimar's comments (added additional comments, used split-file, renamed various things). Also renamed two tests using split-file which are not specifically .ll or .s.

lld/test/ELF/lto/undef-mixed2.s
5 ↗(On Diff #292453)

That's what happens when you try to write/copy tests whilst half asleep. Mostly, it was becuase this test was derived from undef-mixed.s originally. I'm happy to use split-file though.

grimar accepted this revision.Sep 17 2020, 6:16 AM

LGTM. Worth waiting for @MaskRay or someone else opinion too.

This revision is now accepted and ready to land.Sep 17 2020, 6:16 AM

LGTM. Worth waiting for @MaskRay or someone else opinion too.

Will do. Thanks for the quick feedback.

LGTM too, thanks for adding the tests.

I have a suggestion to a test. If you exclude lto/wrap-2.ll from this patch, it will look good to me.

lld/test/ELF/lto/internalize-basic.ll
14

You can enhance the test by deleting hidden. The important property is isUsedInRegularObj, not visibility.

lld/test/ELF/lto/wrap-2.ll
2–43

Is it related to D85782? My feeling is https://reviews.llvm.org/D85782#2211636

I think we need to be careful making this promise. This is not intentionally guaranteed.

Remove hidden.

lld/test/ELF/lto/internalize-basic.ll
14

Done, but I noticed that foo is also hidden. Any idea why?

lld/test/ELF/lto/wrap-2.ll
2–43

No, I don't think so, unless I misunderstood something. D85782 is for intra-module references, i.e. where the reference and definition are within the same input file. This test is about inter-module references, i.e. references between different modules.

The cases I've actually added are [Thin]LTO + native object, where one or other defines the symbol to be wrapped (as well as the wrapped version), and the other references that symbol. My understanding is that this behaviour should be stable.

jhenderson marked an inline comment as done.Sep 18 2020, 2:49 AM
MaskRay accepted this revision.Sep 18 2020, 9:58 AM

LGTM.

lld/test/ELF/lto/internalize-basic.ll
14

I think that is fine. It adds some variants to further demonstrate the property after the main property has been tested.

MaskRay added inline comments.Sep 18 2020, 10:00 AM
lld/test/ELF/lto/wrap-2.ll
2–43

Thanks. Inter-module references via --wrap are something we should make a promise. Intra-module --wrap is fragile (not supported by GCC and even in LLVM's LTO framework it is fragile as well).

Thanks for the reviews. I'll land this later today.

lld/test/ELF/lto/wrap-2.ll
2–43

Makes sense to me.

This revision was automatically updated to reflect the committed changes.