This is an archive of the discontinued LLVM Phabricator instance.

[COFF] added test for thinlto
ClosedPublic

Authored by inglorion on Feb 22 2017, 2:54 PM.

Details

Summary

Creates bitcode files suitable for use with ThinLTO, then checks that the linker can build an executable from them.

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion created this revision.Feb 22 2017, 2:54 PM

Maybe the testing is obvious to someone use to COFF, in which case I'm just out-of-place :)
Otherwise I'd rather have comment explaining what every check is doing.

test/COFF/thinlto.ll
9 ↗(On Diff #89430)

is .obj the standard extension for object on Windows? (and .lib for archive?)

11 ↗(On Diff #89430)

I don't know what is the /include directive doing? Is it preventing dead-stripping somehow? Or affecting the symbol resolution otherwise?

34 ↗(On Diff #89430)

I don't know enough about COFF to know if what you're testing is relevant here. Especially I have no idea why you're checking the HEADERS (or what you're checking).

106 ↗(On Diff #89430)

Why do we check this exact sequence of assembly? Overall I don't know what this test is enforcing / verifying right now.

pcc edited edge metadata.Feb 22 2017, 3:16 PM

For this test I wouldn't expect to see anything more than a basic test that uses temporaries to verify that importing is working. See http://llvm-cs.pcc.me.uk/tools/lld/test/ELF/lto/thinlto.ll for example.

pcc added a comment.Feb 22 2017, 3:21 PM

See also r295901 which changes the ELF test to be closer to what I'd expect this test to be doing.

inglorion updated this revision to Diff 89440.Feb 22 2017, 4:15 PM
inglorion edited the summary of this revision. (Show Details)

@pcc: How is this instead?

inglorion added inline comments.Feb 22 2017, 4:19 PM
test/COFF/thinlto.ll
9 ↗(On Diff #89430)

Yes.

11 ↗(On Diff #89430)

Right. It instructs the linker to retain that symbol, regardless of what it would otherwise do.

106 ↗(On Diff #89430)

This was essentially performing the same checks as lto.ll. However, per @pcc's suggestion, I've changed the test to be much simpler and not actually look at the disassembly.

pcc accepted this revision.Feb 22 2017, 4:44 PM

Nit: I'd use llvm-nm to check the symbol tables of the object files so that we're checking the inputs that the linker actually reads, rather than a byproduct of LTO. Other than that, LGTM.

This revision is now accepted and ready to land.Feb 22 2017, 4:44 PM
mehdi_amini added inline comments.Feb 22 2017, 4:59 PM
test/COFF/thinlto.ll
106 ↗(On Diff #89430)

Thanks, looks good!
(and I agree with the idea of using llvm-nm where possible/appropriate)

Nit: I'd use llvm-nm to check the symbol tables of the object files so that we're checking the inputs that the linker actually reads, rather than a byproduct of LTO.

That was my original plan, but it looks like llvm-nm only tells us about main. Is that enough? Is there a way to get it to give us more information? I wrote the test the way I wrote it because I felt just looking at the output from llvm-nm doesn't really tell us if ThinLTO is working as intended.

pcc added a comment.Feb 23 2017, 11:36 AM

Nit: I'd use llvm-nm to check the symbol tables of the object files so that we're checking the inputs that the linker actually reads, rather than a byproduct of LTO.

That was my original plan, but it looks like llvm-nm only tells us about main. Is that enough? Is there a way to get it to give us more information? I wrote the test the way I wrote it because I felt just looking at the output from llvm-nm doesn't really tell us if ThinLTO is working as intended.

Do you mean that main.exe.lto.obj only contains a definition of main? That is what I'd expect to see if foo was imported into main and inlined.

Nit: I'd use llvm-nm to check the symbol tables of the object files so that we're checking the inputs that the linker actually reads, rather than a byproduct of LTO.

That was my original plan, but it looks like llvm-nm only tells us about main. Is that enough? Is there a way to get it to give us more information? I wrote the test the way I wrote it because I felt just looking at the output from llvm-nm doesn't really tell us if ThinLTO is working as intended.

If there is no undefined reference to foo from main, then it means it worked :)

inglorion updated this revision to Diff 89554.Feb 23 2017, 1:12 PM

use llvm-nm instead of looking at the bitcode

inglorion edited the summary of this revision. (Show Details)Feb 23 2017, 1:12 PM
mehdi_amini added inline comments.Feb 23 2017, 1:15 PM
test/COFF/thinlto.ll
8 ↗(On Diff #89554)

assuming llvm-nm on COFF looks like ELF, what you need to check is not that main is present, but that foo isn't.

pcc added inline comments.Feb 23 2017, 1:15 PM
test/COFF/thinlto.ll
8 ↗(On Diff #89554)

; CHECK-NOT: U foo?

inglorion updated this revision to Diff 89561.Feb 23 2017, 2:00 PM

check for absence of undefined foo, instead of presence of main

This revision was automatically updated to reflect the committed changes.