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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu 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:
> rocallahan wrote:
> > 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`?
> But you don't really care even if it is `EhInputSection` or `SyntheticSection`, no? I mean an assigned value would not be used but doesn't harm.
Ah, OK, got it.


================
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:
> 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`?
But you don't really care even if it is `EhInputSection` or `SyntheticSection`, no? I mean an assigned value would not be used but doesn't harm.


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

https://reviews.llvm.org/D54747