[PATCH] D17529: ELF: Implement ICF.

[Rafael Ávila de Espíndola via llvm-commits]

rafael added inline comments.

================
Comment at: ELF/ICF.cpp:118
@@ +117,3 @@
+  ELFFile<ELFT> &EObj = S->File->getObj();
+  const Elf_Shdr *RelSec = S->RelocSections[0];
+
----------------
Assert that there only one? Return 0 if there is none?

================
Comment at: ELF/ICF.cpp:120
@@ +119,3 @@
+
+  if (RelSec->sh_type == SHT_RELA) {
+    auto Rels = EObj.relas(RelSec);
----------------
It might be cleaner to just say RelSec->sh_size/RelSec->sh_entsize;

================
Comment at: ELF/ICF.cpp:128
@@ +127,3 @@
+
+// Returns a hash value for S. Note that relocation information is not
+// included in the hash value.
----------------
Not exactly true since the number of relocations is included.

Since this is the only use of numRels and it is only used for hashing, how about just using RelSec->sh_size?

================
Comment at: ELF/ICF.cpp:150
@@ +149,3 @@
+  const Elf_Shdr &H = *S->getSectionHdr();
+  return H.sh_type == SHT_PROGBITS &&
+         (H.sh_flags & SHF_ALLOC) &&
----------------
Why only PROGBITS? OK if it is just to keep it simple for now.

================
Comment at: ELF/ICF.cpp:168
@@ +167,3 @@
+// for new groups.
+template <class ELFT>
+bool ICF<ELFT>::partition(InputSection<ELFT> **Begin, InputSection<ELFT> **End,
----------------
Return true if a new group was created.

This could also return void and have the caller check NextId.

================
Comment at: ELF/ICF.cpp:232
@@ +231,3 @@
+    const Elf_Shdr *RB = B->RelocSections[0];
+    if (RA->sh_type != RB->sh_type)
+      return false;
----------------
I don't think this is possible.

It is all SHT_RELA or SHT_REL.

================
Comment at: ELF/ICF.cpp:247
@@ +246,3 @@
+         A->getSize() == B->getSize() &&
+         A->getAlign() == B->getAlign() &&
+         A->getSectionData() == B->getSectionData();
----------------
It is possible to merge when the alignment is different, we just have to keep the larger one. A fixme for now is fine.


================
Comment at: ELF/ICF.cpp:262
@@ +261,3 @@
+  if (EA - IA != EB - IB)
+    return false;
+  for (; IA != EA; ++IA, ++IB) {
----------------
The same 'if' is in equalsContant. Do you need it here?

================
Comment at: ELF/ICF.cpp:268
@@ +267,3 @@
+      continue;
+    if (!SA || !SB)
+      return false;
----------------
If both are null they could still be equal, no?

FIXME for now is fine.

================
Comment at: ELF/ICF.cpp:272
@@ +271,3 @@
+    InputSection<ELFT> *Y = dyn_cast<InputSection<ELFT>>(SB);
+    if (!X || !Y || X->GroupId != Y->GroupId)
+      return false;
----------------
This is also conservatively correct, right?

Two relocations pointing to the string "foobar" in a SHF_MERGE section are equivalent.

================
Comment at: ELF/ICF.cpp:311
@@ +310,3 @@
+  // the same group are consecutive in the vector.
+  std::sort(V.begin(), V.end(),
+            [](InputSection<ELFT> *A, InputSection<ELFT> *B) {
----------------
Do we need stable_sort to make this deterministic?

================
Comment at: ELF/InputSection.cpp:31
@@ +30,3 @@
+  // If GC is disabled, all sections are considered live by default.
+  Live = Config && !Config->GcSections;
+}
----------------
When is Config null?

================
Comment at: ELF/InputSection.cpp:75
@@ -70,3 +74,3 @@
 InputSectionBase<ELFT> *
-InputSectionBase<ELFT>::getRelocTarget(const Elf_Rel &Rel) {
+InputSectionBase<ELFT>::getRelocTarget(const Elf_Rel &Rel) const {
   // Global symbol
----------------
Can you commit the const fixes first?

================
Comment at: ELF/OutputSections.cpp:1151
@@ -1150,3 +1151,3 @@
       InputSectionBase<ELFT> *Target = S->getRelocTarget(*RelI);
-      if (Target != &InputSection<ELFT>::Discarded && Target->isLive()) {
         uint32_t CieOffset = Offset + 4 - ID;
----------------
Can you commit the s/isLife/Life/ change first?

================
Comment at: ELF/Symbols.h:236
@@ +235,3 @@
+  // If this is null, the symbol is an absolute symbol.
+  InputSectionBase<ELFT> *&Section;
+
----------------
Not sure I follow. Why is this not redundant with the Repl pointer in the section?


http://reviews.llvm.org/D17529