The additional testing is testing we previously had in a downstream testsuite.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
lld/test/ELF/lto/archive-mixed.ll | ||
---|---|---|
26 | 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 | I'd put "REQUIRES" on the first line. | |
5 | Any reason not to use split-file instead of adding a new input? | |
lld/test/ELF/lto/wrap-2.ll | ||
2 | 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. | |
16 | Perhaps %t2.o, since you have %t1.o already? |
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 | 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. |
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–39 | 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–39 | 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. |
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. |
lld/test/ELF/lto/wrap-2.ll | ||
---|---|---|
2–39 | 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–39 | Makes sense to me. |
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.