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

[Rui Ueyama via Phabricator via lldb-commits]

ruiu added inline comments.


================
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:
> > > 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.
> I'm assuming that `.eh_frame` sections don't have associated debuginfo so even if such a section is enqueued somehow, it should not cause debuginfo to be included for that object file. Is it impossible for a `.eh_frame` section to be enqueued here? Ditto for a `SyntheticSection`?
> 
> To put it another way, the logic here is supposed to be: "debuginfo can only be associated with Regular or Merge sections, so it only makes sense to mark files as having 'live code or data' possibly associated with debuginfo for those section types."
No debug sections are attached to synthetic sections, but I don't think that logically matters. The logic we want to implement here is

 1. if a file contains one or more debug info sections, and
 2. if at least one of them are marked live, then
 3. all debug sections should be marked live.

If `Sec->Debug` is true, it is a debug section, and vice versa. If Sec->File is a non-null, it is a file that the section belongs to.

So, in the above logic, there's no logic that depends on the type of the section. That's why I think it is better to not assert any expected type for a debug section. As long as `!IsLSDA && Sec->File`, that File should be marked live here.


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

https://reviews.llvm.org/D54747