This is an archive of the discontinued LLVM Phabricator instance.

[LTO] Prevent asm undefined references to be dropped from the output
ClosedPublic

Authored by davide on Sep 15 2016, 11:18 AM.

Diff Detail

Event Timeline

davide updated this revision to Diff 71526.Sep 15 2016, 11:18 AM
davide retitled this revision from to [LTO] Prevent asm undefined references to be dropped from the output.
davide updated this object.
davide added reviewers: tejohnson, rafael.
davide added a subscriber: llvm-commits.

Test?
ThinLTO Path?

tejohnson edited edge metadata.Sep 15 2016, 11:40 AM

Thanks! With a few changes noted below this can be fixed for ThinLTO as well.

lib/LTO/LTO.cpp
354

Do this in addSymbolToGlobalRes instead, so it is added for ThinLTO as well

lib/LTO/LTOBackend.cpp
291

Ditto in thinBackend below

Also it is not clear to me why we track this globally instead of per-file? What is an ASM reference a local symbol for instance?

davide added inline comments.Sep 15 2016, 12:20 PM
lib/LTO/LTOBackend.cpp
291

thinBackend() is also used by clang and it's not entirely clear to me what should I pass to it as AsmUndefinedReferences. If you know that off of your mind, I can change it. Otherwise I can commit the patch as is for full LTO and you can modify for thin accordingly (sorry, my knowledge of ThinLTO is still rather limited, and I don't want to delay this further).

Also it is not clear to me why we track this globally instead of per-file? What is an ASM reference a local symbol for instance?

Yeah at least for ThinLTO perhaps we should call IRObjectFile::CollectAsmUndefinedRefs in the thinBackend to collect this for each module? That would address the issue Davide mentioned about that being called from clang (distributed backends case). For regular LTO it is essentially global since we merge the module.

lib/LTO/LTO.cpp
310

Shouldn't this be: an undefined reference "of" a symbol "used" in asm?

The problem is that after merge, if two local symbols have a name conflict in LTO, one of them will be renamed.
I'd like to see a test with such cases.

mehdi_amini requested changes to this revision.Sep 15 2016, 1:10 PM
mehdi_amini added a reviewer: mehdi_amini.
This revision now requires changes to proceed.Sep 15 2016, 1:10 PM

The problem is that after merge, if two local symbols have a name conflict in LTO, one of them will be renamed.
I'd like to see a test with such cases.

Hmm, so how would we catch/handle this? The uses from module level assembly won't be renamed, right? (I know that is an issue for inline assembly). That seems orthogonal to the issue of making sure their defs are not dropped by updating llvm.compiler.used appropriately (which could be fixed by doing what I suggested for ThinLTO - call CollectAsmUndefinedRefs from the backend).

That seems orthogonal to the issue of making sure their defs are not dropped by updating llvm.compiler.used appropriately which could be fixed by doing what I suggested for ThinLTO - call CollectAsmUndefinedRefs from the backend).

Right, this would solves it (but for the lack of renaming).

Another way is to do it on the individual modules when they're loaded, pre-merge.

davide updated this revision to Diff 71552.Sep 15 2016, 2:07 PM
davide edited edge metadata.

Teresa, is this what you had in mind? Also added a ThinLTO test, hopefully it's correct.

mehdi_amini added inline comments.Sep 15 2016, 2:16 PM
lib/LTO/LTO.cpp
368

Why don't you update compiler.used here before merging? This way the StringSet could be local to this function and not global to the LTO class/lifetime I believe.

Teresa, is this what you had in mind? Also added a ThinLTO test, hopefully it's correct.

Yes, thanks. One suggestion to merge the new tests below.

test/tools/gold/X86/asm_undefined2_thin.ll
6 ↗(On Diff #71552)

I suppose since the test is basically the same you could copy these RUN lines into asm_undefined2.ll so it would test both regular and thin LTO.

davide updated this revision to Diff 71558.Sep 15 2016, 2:36 PM
davide edited edge metadata.

Unified tests as per Teresa's request.

davide updated this revision to Diff 71658.Sep 16 2016, 8:39 AM

Update after Teresa's comments.

mehdi_amini accepted this revision.Sep 16 2016, 9:02 AM
mehdi_amini edited edge metadata.

LGTM.

With a llvm-lto2 version of the test, it'll be perfect ;)

This revision is now accepted and ready to land.Sep 16 2016, 9:02 AM

LGTM.

With a llvm-lto2 version of the test, it'll be perfect ;)

I tried to write that but there are issues providing the resolution because IRObjectFile will report the same symbol twice. I added a comment to https://llvm.org/bugs/show_bug.cgi?id=30396 so that we can add a test for this once we fix IRObjectFile =)

Thanks for adding the tracking of this!

This revision was automatically updated to reflect the committed changes.