[PATCH] D33485: [ELF] - Do not allow -r to eat comdats.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/InputFiles.cpp:306
     switch (Sec.sh_type) {
-    case SHT_GROUP:
-      this->Sections[I] = &InputSection::Discarded;
-      if (ComdatGroups.insert(getShtGroupSignature(ObjSections, Sec)).second)
+    case SHT_GROUP: {
+      bool New =
----------------
ruiu wrote:
> This needs an explanation.
Done.


================
Comment at: ELF/InputFiles.cpp:322
       }
-      break;
+    } break;
     case SHT_SYMTAB:
----------------
ruiu wrote:
> Format
It is just formatting in this way :(


================
Comment at: ELF/InputSection.cpp:302
+  // First entry is a flag word, we leave it unchanged.
+  auto *P = reinterpret_cast<typename ELFT::Word *>(Buf);
+  *P++ = Ents[0];
----------------
ruiu wrote:
> Avoid ELFT::Word` in general. You can use read32 instead. You can also remove ELFT from template parameter list if you use Config->Wordsize.
Done. (Not sure why did you mention Config->Wordsize though, all values here are 4 bytes I believe).


================
Comment at: ELF/InputSection.cpp:701
+    copyGroup<ELFT>(Buf + OutSecOff,
+                    this->template getDataAs<typename ELFT::Word>());
+    return;
----------------
ruiu wrote:
> Do not use ELFT::Word.
Done.


================
Comment at: ELF/InputSection.h:324
+  template <class ELFT, class EntTy>
+  void copyGroup(uint8_t *Buf, llvm::ArrayRef<EntTy> Ents);
 };
----------------
ruiu wrote:
> I prefer more specific name -- like copyShtGroup, as `Group` sounds too generic.
Done.


================
Comment at: ELF/OutputSections.cpp:129-141
+  if (Type == SHT_GROUP) {
+    // sh_link field for SHT_GROUP sections should contain the section index of
+    // the symbol table.
+    this->Link = InX::SymTab->OutSec->SectionIndex;
+
+    // sh_link then contain index of an entry in symbol table section which
+    // provides signature of the section group.
----------------
ruiu wrote:
> This is too much details to write in this funciton.
Done.


================
Comment at: ELF/OutputSections.cpp:276
 
 static uint64_t getOutFlags(InputSectionBase *S) {
+  if (Config->Relocatable)
----------------
ruiu wrote:
> This looks a bit tricky. We should turn off SHF_COMPRESSED when we decompress sections.
May be. This patch just removes SHF_GROUP here.


https://reviews.llvm.org/D33485