[PATCH] D54747: Discard debuginfo for object files empty after GC

[Robert O'Callahan via Phabricator via llvm-commits]

rocallahan marked 4 inline comments as done.
rocallahan added inline comments.


================
Comment at: lld/ELF/MarkLive.cpp:190
+  // case we still need to mark the file.
+  if (S && !IsLSDA && Sec->File)
+    if (isa<InputSection>(Sec) || isa<MergeInputSection>(Sec))
----------------
ruiu wrote:
> ruiu wrote:
> > S is true only when it is an InputSection, so you have duplicate tests for this. It also looks like you don't have to care about whether a section is an input section or a merged section. Isn't this condition sufficient?
> > 
> >   if (Sec->File && !IsLSDA)
> >     Sec->getFile<ELFT>()->HasLiveCodeOrData = true;
> Move this code after `Sec->Live = true` so that when we visit the same section more than once, this piece of code runs only once.
We can't do that. The comment I added tries to explain why. Is it unclear?


================
Comment at: lld/ELF/MarkLive.cpp:190-191
+  // case we still need to mark the file.
+  if (S && !IsLSDA && Sec->File)
+    if (isa<InputSection>(Sec) || isa<MergeInputSection>(Sec))
+      Sec->getFile<ELFT>()->HasLiveCodeOrData = true;
----------------
rocallahan wrote:
> ruiu wrote:
> > ruiu wrote:
> > > S is true only when it is an InputSection, so you have duplicate tests for this. It also looks like you don't have to care about whether a section is an input section or a merged section. Isn't this condition sufficient?
> > > 
> > >   if (Sec->File && !IsLSDA)
> > >     Sec->getFile<ELFT>()->HasLiveCodeOrData = true;
> > Move this code after `Sec->Live = true` so that when we visit the same section more than once, this piece of code runs only once.
> We can't do that. The comment I added tries to explain why. Is it unclear?
I want to skip `EhInputSection` and `SyntheticSection` here. So I guess the advice to use `isa` here was wrong and I should revert to checking `kind() == Regular || kind() == Merge`?


================
Comment at: lld/ELF/MarkLive.cpp:313
   // remove its relocation section.
+  bool AnyDebugSections = false;
   for (InputSectionBase *Sec : InputSections) {
----------------
ruiu wrote:
> Let's remove this optimization -- I don't think we need this.
OK


================
Comment at: lld/ELF/MarkLive.cpp:321
     bool IsLinkOrder = (Sec->Flags & SHF_LINK_ORDER);
     bool IsRel = (Sec->Type == SHT_REL || Sec->Type == SHT_RELA);
----------------
ruiu wrote:
> Then you can simplify the condition to
> 
>   if (!IsAlloc && !IsLinkOrder && !IsRel && !Sec->Debug)
>     Sec->Live = true;
Yep.


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

https://reviews.llvm.org/D54747