Page MenuHomePhabricator

[lld-link] preserve @llvm.used symbols in LTO
ClosedPublic

Authored by inglorion on Feb 14 2019, 3:04 PM.

Details

Summary

We translate @llvm.used to COFF by generating /include directives
in the .drectve section. However, in LTO links, this happens after
directives have already been processed, so the new directives do
not take effect. This change marks @llvm.used symbols as GCRoots
so that they are preserved as intended.

Fixes PR40733.

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion created this revision.Feb 14 2019, 3:04 PM
ruiu added inline comments.Feb 14 2019, 6:08 PM
lld/COFF/InputFiles.cpp
691–692 ↗(On Diff #186924)

It feels like it is cleaner to do this in MarkLive.cpp. When we execute markLive(), we have a list of Chunks, and we can iterate over the Chunks to add symbols to Worklist.

inglorion marked an inline comment as done.Feb 19 2019, 2:36 PM
inglorion added inline comments.
lld/COFF/InputFiles.cpp
691–692 ↗(On Diff #186924)

After looking into this, I'm not sure I like the suggested approach better. It would require

  1. Iterating over more symbols in markLive() - namely all symbols, instead of only those reachable from GC roots.
  1. Tracking which symbols are used, which we currently don't do.

The symbols this change matches are created first as DefinedRegular when reading from bitcode, then replaced with Undefined in LTO.cpp, then replaced with DefinedRegular when read from COFF objects. To do #2, we would need to either track the used attribute through these replacements, or we would need to re-parse the .drectve section of the COFF file to find the include directives. I don't think either option is particularly clean. And #1 sounds like extra work - even if it takes little real time, I'd rather not do it if we can avoid it.

By contrast, I think marking these symbols as GC roots is essentially the right thing to do. It's also how the /include command line flag is implemented.

ruiu accepted this revision.Feb 19 2019, 3:35 PM

LGTM

lld/COFF/InputFiles.cpp
691–692 ↗(On Diff #186924)

Fair enough.

This revision is now accepted and ready to land.Feb 19 2019, 3:35 PM
This revision was automatically updated to reflect the committed changes.