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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
