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

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

rocallahan marked an inline comment as done.
rocallahan 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;
----------------
ruiu wrote:
> 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.
> #     if a file contains one or more debug info sections, and
  #     if at least one of them are marked live, then
  #     all debug sections should be marked live.

That is not quite right. We are not marking any debuginfo sections live until the final pass. The first condition is "if a file contains one or more non-debuginfo sections marked live for non-LSDA data..."

Anyway the diff I just posted contains the change you wanted here.


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

https://reviews.llvm.org/D54747