[PATCH] D28481: Respect section groups in GC

[Evgeniy Stepanov via Phabricator via llvm-commits]

eugenis 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)
----------------
ruiu wrote:
> Revert these cosmetic changes.
Done, but this file is not clang-format clean.


================
Comment at: ELF/InputFiles.cpp:342-344
+      if (SecIndex >= Size)
+        fatal(toString(this) +
+              ": invalid section index in group: " + Twine(SecIndex));
----------------
ruiu wrote:
> 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;
The check above runs for discarded groups only.


================
Comment at: ELF/InputFiles.cpp:345-346
+              ": invalid section index in group: " + Twine(SecIndex));
+      if (!Sections[SecIndex] ||
+          Sections[SecIndex] == &InputSection<ELFT>::Discarded)
+        continue;
----------------
ruiu wrote:
> It cannot be nullptr, no? I think this can only be Discarded or a regular section.
These cases above don't seem to set the section pointer:
   case SHT_SYMTAB:
    case SHT_SYMTAB_SHNDX:
    case SHT_STRTAB:
    case SHT_NULL:



Repository:
  rL LLVM

https://reviews.llvm.org/D28481