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

[Rui Ueyama via Phabricator via lldb-commits]

ruiu added a comment.

Overall looking good.



================
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:
> 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.


================
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;
----------------
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;


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


================
Comment at: lld/ELF/MarkLive.cpp:321
     bool IsLinkOrder = (Sec->Flags & SHF_LINK_ORDER);
     bool IsRel = (Sec->Type == SHT_REL || Sec->Type == SHT_RELA);
----------------
Then you can simplify the condition to

  if (!IsAlloc && !IsLinkOrder && !IsRel && !Sec->Debug)
    Sec->Live = true;


================
Comment at: lld/ELF/MarkLive.cpp:330
 
+  if (AnyDebugSections)
+    // Mark debug sections as live in any object file that has a live
----------------
Remove this variable and just visit all sections even if there's no debug info (which shouldn't take too much time anyway.)


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

https://reviews.llvm.org/D54747