[PATCH] D58255: [lld-link] preserve @llvm.used symbols in LTO

Tue Feb 19 15:35:14 PST 2019

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM

================
Comment at: lld/COFF/InputFiles.cpp:691-692
     Symbols.push_back(Sym);
+    if (ObjSym.isUsed())
+      Config->GCRoot.push_back(Sym);
   }
----------------
inglorion wrote:
> ruiu wrote:
> > 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.
> 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.
> 
> 2. 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.
Fair enough.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58255/new/

https://reviews.llvm.org/D58255