[PATCH] D28481: Respect section groups in GC

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/InputFiles.cpp:295-303
+      if (ComdatGroups
+              .insert(
+                  CachedHashStringRef(getShtGroupSignature(ObjSections, Sec)))
               .second)
         continue;
       for (uint32_t SecIndex : getShtGroupEntries(Sec)) {
         if (SecIndex >= Size)
----------------
Revert these cosmetic changes.


================
Comment at: ELF/InputFiles.cpp:340
+
+    auto GroupSections = getShtGroupEntries(Sec);
+    for (uint32_t SecIndex : GroupSections) {
----------------
Use `ArrayRef<typename ELFT::Word>` instead of `auto`.


================
Comment at: ELF/InputFiles.cpp:342-344
+      if (SecIndex >= Size)
+        fatal(toString(this) +
+              ": invalid section index in group: " + Twine(SecIndex));
----------------
It shouldn't fail this time because we have the same error check above, so you don't need this check. Then this can be written as

  for (uint32_t SecIndex : GroupSections)
    if (Sections[SecIndex] != &InputSection<ELFT>::Discarded)
      Sections[SecIndex]->GroupSections = GroupSections;


================
Comment at: ELF/InputFiles.cpp:345-346
+              ": invalid section index in group: " + Twine(SecIndex));
+      if (!Sections[SecIndex] ||
+          Sections[SecIndex] == &InputSection<ELFT>::Discarded)
+        continue;
----------------
It cannot be nullptr, no? I think this can only be Discarded or a regular section.


================
Comment at: ELF/InputSection.h:95
 
+  ArrayRef<typename ELFT::Word> GroupSections;
+
----------------
This needs a comment. Please describe that if sections are grouped by SHT_GROUP, we handle the sections as a unit in garbage collection, because that is needed for at least ASAN although it is not clearly documented in the standard document.


================
Comment at: ELF/MarkLive.cpp:83
                              std::function<void(ResolvedReloc<ELFT>)> Fn) {
   if (Sec.AreRelocsRela) {
     for (const typename ELFT::Rela &Rel : Sec.relas())
----------------
// Visit relocations.


================
Comment at: ELF/MarkLive.cpp:90
   }
   if (Sec.DependentSection)
     Fn({Sec.DependentSection, 0});
----------------
// Visit a reverse link of SHF_LINK_ORDER if exists.


================
Comment at: ELF/MarkLive.cpp:92
     Fn({Sec.DependentSection, 0});
+  for (auto SecIdx : Sec.GroupSections) {
+    InputSectionBase<ELFT> *SameGroupSection =
----------------
// Visit SHT_GROUP members if this section is in a group.


================
Comment at: ELF/MarkLive.cpp:171
 template <class ELFT> static bool isReserved(InputSectionBase<ELFT> *Sec) {
+  // Sections that are part of a group follow the usual GC rules.
+  if (Sec->GroupSections.size() > 0)
----------------
Please explain why you want that behavior, in addition to how this code behaves.


Repository:
  rL LLVM

https://reviews.llvm.org/D28481