This is an archive of the discontinued LLVM Phabricator instance.

[lld][ELF][test] Add additional test coverage for LTO
ClosedPublic

Authored by jhenderson on Oct 20 2020, 2:11 AM.

Details

Summary

These are all inspired by existing test coverage we have in an internal testsuite.

Diff Detail

Event Timeline

jhenderson created this revision.Oct 20 2020, 2:11 AM
jhenderson requested review of this revision.Oct 20 2020, 2:11 AM

Add missing new line at EOF.

It feels like common-mixed1.ll and common-mixed2.ll are very similar and can be merged?

lld/test/ELF/lto/bitcode-wrapper.ll
52

I'd comment fields here. They are:
[Magic32, Version32, Offset32, Size32, CPUType32]

lld/test/ELF/lto/common-mixed2.ll
7 ↗(On Diff #299304)

Perhaps, use split-file or just echo rather than adding a new tiny input?

MaskRay edited reviewers, added: psmith; removed: peter.smith.Oct 20 2020, 9:56 AM
MaskRay added inline comments.Oct 20 2020, 4:00 PM
lld/test/ELF/lto/Inputs/common-mixed2.s
1 ↗(On Diff #299304)

This can be inlined into common-mixed2.ll

jhenderson planned changes to this revision.Oct 21 2020, 7:48 AM

I'll see if I can merge common-mixed1.ll and common-mixed2.ll and inline the input files. That should allow me to avoid renaming common4.ll too.

lld/test/ELF/lto/resolution.ll
22–23

This test is failing for reasons I've yet to discover. I thought it passed previously, but it's possible it didn't. Currently, it fails because both versions of b are present in the contents of .data, which clearly isn't correct (only one b appears in the symbol table).

Reworked resolution.ll and common.ll, fixing the former, and inlining the input files. Also undid the test renaming and made the wrap_bitcode.py script python3 compatible.

You need to update the patch description which still mentions "common-mixed1.ll".
The rest looks fine to me.

lld/test/ELF/lto/bitcode-wrapper.ll
27

aside: we probably want to lowercase this message.

jhenderson edited the summary of this revision. (Show Details)Oct 22 2020, 4:29 AM

You need to update the patch description which still mentions "common-mixed1.ll".
The rest looks fine to me.

Done, thanks.

grimar accepted this revision.Oct 22 2020, 4:32 AM

LGTM. But please wait for @MaskRay.

This revision is now accepted and ready to land.Oct 22 2020, 4:32 AM
jhenderson added inline comments.Oct 22 2020, 4:39 AM
lld/test/ELF/lto/bitcode-wrapper.ll
27

I took a quick look, and this is just one of several error messages within the BitcodeReader and BitcodeAnalyzer files, and most of them use upper-case, so that would probably want to be part of a much wider change. Think we'd need to discuss the preferred style of the library with whoever owns/maintains it before doing that.

grimar added inline comments.Oct 22 2020, 4:41 AM
lld/test/ELF/lto/bitcode-wrapper.ll
27

Think we'd need to discuss the preferred style of the library with whoever owns/maintains it before doing that.

perhaps we could just lower case all error messages on the lld side.

jhenderson added inline comments.Oct 22 2020, 5:20 AM
lld/test/ELF/lto/bitcode-wrapper.ll
27

I thought about that, but that would risk us occasionally converting strings to lower-case that shouldn't be (e.g. file paths, names, accronyms like ELF or DWARF etc).

grimar added inline comments.Oct 22 2020, 5:26 AM
lld/test/ELF/lto/bitcode-wrapper.ll
27

I meant to lower case just the first character. Perhaps it is a low risk here. Though probably
a more safe way is to convert particular error messages. I.e. if it comes from BitcodeReader/BitcodeAnalyzer we could do it in case if we can't just switch to lower case right there.

MaskRay accepted this revision.Oct 22 2020, 9:09 AM

LGTM.

lld/test/ELF/lto/bitcode-wrapper.ll
27

I'd prefer not transforming the error messages. If people search for "Invalid bitcode wrapper header", they can find occurrences in llvm/ and lld/test/ELF. If they happen to change the messages (e.g. to add more information; many BitcodeReader.cpp messages are duplicated in multiple places and are generally not so informational), they can update the lld one as well. Suredly they can use grep -i or rg -i, or not search for the first character, but that is extra hurdle.

We lose a bit of inconsistency, but the loss seems very small to me.

This revision was automatically updated to reflect the committed changes.