[PATCH] D65909: ELF: Move sections referred to by __start_/__stop_ symbols into the main partition.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lld/ELF/MarkLive.cpp:299
 template <class ELFT> void MarkLive<ELFT>::moveToMain() {
-  for (InputFile *file : objectFiles)
-    for (Symbol *s : file->getSymbols())
+  StringSet<> startStopSymbolNames;
+  for (InputFile *file : objectFiles) {
----------------
ruiu wrote:
> MaskRay wrote:
> > ruiu wrote:
> > > MaskRay wrote:
> > > > `DenseSet<StringRef>` can avoid allocation for strings.
> > > In this patch you scan over all input files and their symbol tables to see if there's a symbol starting with "__start_" or "__stop_", but do you have to do this that way? I mean
> > > 
> > > 1. most section names are not a valid C identifier in the first place,
> > > 2. if section whose name "foo" exists, look up the symbol table with "__start_foo" and "__end_foo" to see if there's a symbol that refer to the start/end of the sectoin
> > > 
> > > Am I missing somehting?
> > The 
> > 
> > ```
> > for (InputFile *file : objectFiles)
> >   for (Symbol *s : file->getSymbols())
> > ```
> > 
> > loop already exists because STT_GNU_IFUNC and STT_TLS are special cased. The loop gets reused to check `__start_` and `__stop_`. I am not sure an alternative approach will optimize it.
> Yeah, performance-wise I don't know which is better, but I feel like looking up the symbol table is more natural than scanning all symbols if we are handling symbol names.
With regard to point 1, in certain sanitizer builds every global will have an associated C identifier named section. So depending on your build mode the number may not be that small.

That said, I couldn't measure a significant difference in link speed between the two options even in a HWASAN build of Chromium (if anything, the symbol lookup seems to be a little faster), so I'd be fine with either option. The symbol lookup turns out to be less code, so let's go with that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65909