[PATCH] D17502: [ELF] - Fix of 22906, referencing __start or __stop should keep the section from GC.

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/MarkLive.cpp:83
@@ +82,3 @@
+static bool isRetained(SymbolTable<ELFT> *Symtab, InputSectionBase<ELFT> *Sec) {
+  StringRef Name = Sec->getOutputSectionName();
+  if (!isValidCIdentifier(Name))
----------------
emaste wrote:
> grimar wrote:
> > emaste wrote:
> > > Is the input->output section name mapping necessary? Those section names are not valid C identifiers anyhow.
> > I think it is. Thats mirrors logic of
> > 
> > ```
> > void Writer<ELFT>::addStartStopSymbols(OutputSectionBase<ELFT> *Sec) {
> >   StringRef S = Sec->getName();
> >   if (!isValidCIdentifier(S))
> >     return;
> > ...
> > ```
> > which uses output section name to create start/stop identificator name. 
> > Also since we can use linker script to place sections to any output section, it seems to be correct for me.
> > 
> Ah yes, sorry - I glossed over the arbitrary mapping that might come from the linker script and only looked at the built-in set of `.text.` etc.
This still seems to be overkill and would probably be slow. I don't feel that we really have to worry about a combination of __start/__stop and linker script. If you are using linker scripts, you can easily keep all sections using KEEP command, and you don't really have to depend on __start/__stop symbols because you can freely define any symbols using linker scripts.

I'd prefer a simpler implementation. You can check only input section names instead of output section names by adding this code to isReserved in MarkLive.cpp.

  default:
    StringRef S = Sec->getSectionName();

    // We do not want to reclaim sections if they can be referred
    // by __start_* and __stop_* symbols.
    if (isValidCIdentifier(S))
      return true;

    return S.startswith(".ctors") || S.startswith(".dtors") ||
           S.startswith(".init") || S.startswith(".fini") ||
           S.startswith(".jcr");
  }



http://reviews.llvm.org/D17502