Page MenuHomePhabricator

Linker: Move special casing for available_externally in IRMover to clients. NFCI.
ClosedPublic

Authored by pcc on Feb 1 2017, 5:19 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Feb 1 2017, 5:19 PM
mehdi_amini added inline comments.Feb 1 2017, 5:45 PM
llvm/lib/LTO/LTO.cpp
481 ↗(On Diff #86751)

Do we have a test that shows that:

  1. if we first import an available_externally, then a later linkonce_odr (and/or strong) would overwrite?
  2. if we first import a linkonce_odr (and/or strong), a later available_externally wouldn't overwrite?
llvm/lib/Linker/IRMover.cpp
875 ↗(On Diff #86751)

No test changed, that's annoying that we're not covering this case (independently of the new LTO API). Any idea how to test this? (along with the removal below as well)

mehdi_amini accepted this revision.Feb 1 2017, 5:45 PM

LGTM otherwise.

This revision is now accepted and ready to land.Feb 1 2017, 5:45 PM
tejohnson added inline comments.Feb 1 2017, 6:56 PM
llvm/lib/Linker/LinkModules.cpp
398 ↗(On Diff #86751)

I guess removing this is required to make it NFC. Is it required for correctness too? I don't think so but want to make sure my understanding is right. Is it better to continue not mapping these in here (I think they get mapped later if referenced, right?) and not bother making this NFC?

pcc updated this revision to Diff 86763.Feb 1 2017, 9:05 PM
  • Add test
  • A change is required in a different place to make it NFC
pcc added inline comments.Feb 1 2017, 9:05 PM
llvm/lib/LTO/LTO.cpp
481 ↗(On Diff #86751)

I've added tests for both of those things, as well as a standalone test for the available_externally functionality (previously only a gold plugin test was covering this).

llvm/lib/Linker/IRMover.cpp
875 ↗(On Diff #86751)

The only direct users of IRMover are LinkModules and LTO. I guess we could write a unit test for IRMover perhaps? Doesn't seem worth it for this one change though.

llvm/lib/Linker/LinkModules.cpp
398 ↗(On Diff #86751)

Actually this change was needed for us to link available_externally at all (with only the change to shouldLink we would hit the 'return false;' here and never link any available_externally globals). But taking a closer look I found that we were linking too much, even unreferenced available_externally globals (which was a functional change). I've uploaded a new change that causes available_externally to be handled like linkonce and only linked if referenced.

Thanks for the tests! Still LGTM.

llvm/lib/Linker/IRMover.cpp
875 ↗(On Diff #86751)

I wouldn't suggest it :)

mehdi_amini added inline comments.Feb 1 2017, 9:12 PM
llvm/test/Linker/available_externally_a.ll
7 ↗(On Diff #86763)

Actually why isn't llvm-link including the available_externally?

pcc added inline comments.Feb 1 2017, 9:15 PM
llvm/test/Linker/available_externally_a.ll
7 ↗(On Diff #86763)

Because it is unreferenced, see discussion with Teresa in LinkModules.cpp.

mehdi_amini added inline comments.Feb 1 2017, 9:16 PM
llvm/test/Linker/available_externally_a.ll
7 ↗(On Diff #86763)

Oh right :)

This revision was automatically updated to reflect the committed changes.