[PATCH] D13814: Add support for SHF_MERGE sections

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/InputSection.cpp:62
@@ -35,2 +61,3 @@
 
 template <class ELFT>
+static typename ELFFile<ELFT>::uintX_t
----------------
Needs a brief comment to describe what this function is for.

As I understand, this function is to fix addend if it is referring a SHF_MERGE'ed section so that the addend becomes an offset from start of the entry, but I needed to read this code carefully.

================
Comment at: ELF/InputSection.cpp:78
@@ +77,3 @@
+  const InputSectionBase<ELFT> *Section = File.getSection(*Sym);
+  if (Section && isa<MergeInputSection<ELFT>>(Section))
+    return Addend % Section->getSectionHdr()->sh_entsize;
----------------
Don't we have isa_or_null?

Oh we don't. Nevermind.

================
Comment at: ELF/InputSection.cpp:162-171
@@ -106,2 +161,12 @@
+
+template <class ELFT>
+unsigned MergeInputSection<ELFT>::getOffset(const Elf_Sym &Sym) const {
+  uintX_t EntSize = this->Header->sh_entsize;
+  ArrayRef<uint8_t> Data = this->getSectionData();
+  uintX_t Value = Sym.st_value;
+  if (Value + EntSize > Data.size())
+    error("zed");
+  Data = Data.slice(Value, EntSize);
+  return static_cast<MergeOutputSection<ELFT> *>(this->OutSec)->getOffset(Data);
 }
 
----------------
This function is called indirectly for each relocation, and this basically does string search from a large vector. That's probably too slow.

You may want to keep a vector of offsets for each element of MergeInputSection, so that you don't have to do string search.

================
Comment at: ELF/InputSection.h:68
@@ +67,3 @@
+
+// This corresponds to a merge section of an input file.
+template <class ELFT> class MergeInputSection : public InputSectionBase<ELFT> {
----------------
s/merge/SHF_MERGE/ to make it clear that it's for ELF merge sections.

================
Comment at: ELF/OutputSections.cpp:533
@@ +532,3 @@
+template <class ELFT> void MergeOutputSection<ELFT>::writeTo(uint8_t *Buf) {
+  for (const std::pair<StringRef, unsigned> &P : Offsets) {
+    StringRef Data = P.first;
----------------
unsinged -> uint64_t or uintX_t to support cross-linking huge sections on 32 bit platform.

(In reality, LLD will always run out of virtual memory space if we have such huge chunks on 32-bit platform, but still unsigned doesn't feel correct. If it is size_t, it may make sense.)

================
Comment at: ELF/OutputSections.cpp:684-696
@@ -601,8 +683,15 @@
+      uintX_t VA = 0;
       if (Sym.st_shndx == SHN_ABS) {
         ESym->st_shndx = SHN_ABS;
+        VA = Sym.st_value;
       } else {
-        const InputSection<ELFT> *Sec = File->getSection(Sym);
-        ESym->st_shndx = Sec->OutSec->SectionIndex;
-        VA += Sec->OutSec->getVA() + Sec->OutSecOff;
+        const InputSectionBase<ELFT> *Section = File->getSection(Sym);
+        const OutputSectionBase<ELFT> *OutSec = Section->OutSec;
+        ESym->st_shndx = OutSec->SectionIndex;
+        VA += OutSec->getVA();
+        if (auto *S = dyn_cast<InputSection<ELFT>>(Section))
+          VA += Sym.st_value + S->OutSecOff;
+        else
+          VA += cast<MergeInputSection<ELFT>>(Section)->getOffset(Sym);
       }
       ESym->st_value = VA;
----------------
You may want to separate this as a function.

================
Comment at: ELF/OutputSections.h:208
@@ +207,3 @@
+private:
+  llvm::MapVector<StringRef, unsigned> Offsets;
+};
----------------
This member needs a comment because of its importance.

================
Comment at: ELF/Writer.cpp:412-413
@@ -407,4 +411,4 @@
 
   for (const std::unique_ptr<ObjectFile<ELFT>> &F : Symtab.getObjectFiles()) {
-    for (InputSection<ELFT> *C : F->getSections()) {
+    for (InputSectionBase<ELFT> *C : F->getSections()) {
       if (!C || C == &InputSection<ELFT>::Discarded)
----------------
Can you run two loops, one for regular sections and the other for merged sections, to improve readability? I don't like large loops.


http://reviews.llvm.org/D13814