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