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

[George Rimar via llvm-commits]

grimar 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))
----------------
ruiu wrote:
> 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");
>   }
> 
But don't you want at least to check that there are __start_/__stop symbols referenced to keep sections alive ?
And also we can check if we are using LS then:

```
  if (!usingScript() && isValidCIdentifier(S)) {
  if (SymbolBody *B = Symtab->find(("__start_" + S).str()))
    if (B->isUndefined())
      return true;
  if (SymbolBody *B = Symtab->find(("__start_" + S).str()))
    if (B->isUndefined())
      return true;
    return false;
}
```

Or do you mean Symtab->find() would be slow ? But as far I understand there are not too much sections that are isValidCIdentifiers.


http://reviews.llvm.org/D17502