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