[PATCH] D14382: [ELF2] - Basic implementation of -r/--relocatable

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Config.h:70
@@ -69,2 +69,3 @@
   bool PrintGcSections;
+  bool Relocatable = false;
   bool Shared;
----------------
Remove ' = false'.

================
Comment at: ELF/InputSection.cpp:99
@@ -98,3 +98,3 @@
 
 template <class ELFT>
 template <bool isRela>
----------------
What does this function do? It needs a comment.

I figured that out myself. So, this function seems to do this.

  // This function copies section contents to Buf, assuming that the current section
  // contains relocations. Usually this function is not called because we apply and
  // consume relocation ourselves as a static linker. However, if -r is specified,
  // we are a producer of an object file, so instead of applying, we
  // copy all relocations to a resulting object file and let the final static
  // linker resolve all relocations.
  // We can't use memcpy to copy relocations because we need to update symbol table
  // offset and section index for each relocation. So we copy relocations one by one.

================
Comment at: ELF/InputSection.cpp:103
@@ +102,3 @@
+                                              RelIteratorRange<isRela> Rels) {
+  typedef Elf_Rel_Impl<ELFT, isRela> RelType;
+  size_t Num = Rels.end() - Rels.begin();
----------------
s/RelType/RelTy/

(I think we used this name at somewhere else.)

================
Comment at: ELF/InputSection.cpp:106
@@ +105,3 @@
+  for (size_t I = 0; I < Num; ++I) {
+    const RelType &RI = *(Rels.begin() + I);
+    uint32_t SymIndex = RI.getSymbol(Config->Mips64EL);
----------------
I believe `RI` originally stands for Relocation Iterator. Using RI as a name of a relocation is probably inappropriate. Please rename Rel.

================
Comment at: ELF/InputSection.cpp:118
@@ +117,3 @@
+    if (SymIndex < SymTab->sh_info)
+      fatal("Not implemented relocation section type");
+
----------------
  fatal("Relocation against local symbols is not supported yet")

================
Comment at: ELF/InputSection.cpp:288-291
@@ +287,6 @@
+  if (RelocatingSection) {
+    if (Header->sh_type == SHT_RELA)
+      this->fixupRelocations(Buf + OutSecOff, BufEnd, EObj.relas(Header));
+    else
+      this->fixupRelocations(Buf + OutSecOff, BufEnd, EObj.rels(Header));
+    return;
----------------
OK, so these functions are called after you copy all relocations to Buf. But inside fixupRelocations, you copy all relocations to Buf again. That's not wasting but confusing. I'd rename fixupRelocations to copyRelocations and move this function call before memcpy in this function.

================
Comment at: ELF/InputSection.h:70
@@ -69,3 +69,3 @@
   // section.
-  uintX_t getOffset(uintX_t Offset);
+  uintX_t getOffset(uintX_t Offset = 0);
 
----------------
I'm not a big fan of optional parameters because it adds complexity (even though it is small.) And this function converts a given offset to a different offset starting from a different point of origin, so it is weird to call this function without any argument.

================
Comment at: ELF/OutputSections.cpp:1402-1403
@@ -1377,3 +1401,4 @@
                      });
-    return;
+    if (!Config->Relocatable)
+      return;
   }
----------------
Please try to separate the control flow for -r from the regular control flow. (Or, in general, please do not try to add small `if`s for various flags to achieve something.)

!StrTabSec.isDynamic() does not make sense for -r. As I understand, what you want to do is to set DynsymIndex for each symbol if -r is given, so that you can use the indices in fixRelocations() later. In that case, you don't actually want to sort anything (is this correct?). So, please add

  if (Config->Relocatable) {
    size_t I = 0;
    for (const std::pair<SymbolBody *, size_t> &P : Symbols)
      P.first->DynsymIndex = ++I;
    return;
  }

before `if (!StrTabSec.isDynamic())`.

================
Comment at: ELF/OutputSections.cpp:1410
@@ -1384,3 +1409,3 @@
     std::stable_sort(Symbols.begin(), Symbols.end(), sortMipsSymbols);
-  size_t I = 0;
+  size_t I = NumLocals;
   for (const std::pair<SymbolBody *, size_t> &P : Symbols)
----------------
I do not understand -- isn't this an incompatible change if you do not use -r?

================
Comment at: ELF/OutputSections.h:302
@@ +301,3 @@
+template <class ELFT>
+class StaticRelocSection final : public OutputSection<ELFT> {
+public:
----------------
I do not want to define a derived class of OutputSection. All other subclasses are of OutputSectionBase.

================
Comment at: ELF/OutputSections.h:305
@@ +304,3 @@
+  StaticRelocSection(StringRef Name, uint32_t Type, uintX_t Flags, bool IsRela);
+  void addSection(InputSectionBase<ELFT> *C) override;
+  void finalize() override;
----------------
Do not use C for InputSections because C stands for Chunk. We have removed Chunk, so it no longer makes sense. I'd name S.

================
Comment at: ELF/OutputSections.h:309
@@ +308,3 @@
+private:
+  // Output section of section to which relocation applies.
+  OutputSectionBase<ELFT> *RelocOutSec = nullptr;
----------------
  // The output section to which relocation would be applied.

================
Comment at: ELF/Writer.cpp:103
@@ -102,2 +102,3 @@
   }
+  unsigned dummySectionsNum() const { return Config->Relocatable ? 1 : 2; }
 
----------------
This needs an explanation.

================
Comment at: ELF/Writer.cpp:1310
@@ -1299,1 +1309,3 @@
 
+template <class ELFT> void Writer<ELFT>::assignAddressesRelocatable() {
+  Out<ELFT>::ElfHeader->setSize(sizeof(Elf_Ehdr));
----------------
This needs a comment.

================
Comment at: ELF/Writer.cpp:1472
@@ -1443,3 +1471,3 @@
   EHdr->e_entry = getEntryAddr<ELFT>();
-  EHdr->e_phoff = sizeof(Elf_Ehdr);
+  EHdr->e_phoff = Config->Relocatable ? 0 : sizeof(Elf_Ehdr);
   EHdr->e_shoff = SectionHeaderOff;
----------------
  if (!Config->Relocatable) {
    EHdr->e_phoff = sizeof(Elf_Ehdr);
    EHdr->e_phentsize = sizeof(Elf_Phdr);
  }


http://reviews.llvm.org/D14382