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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Driver.cpp:156-162
@@ +155,9 @@
+  Config->Relocatable = Args.hasArg(OPT_relocatable);
+  if (Config->Relocatable) {
+    // -strip-all is silently ignored.
+    Config->StripAll = false;
+    Config->Static = true;
+    if (Config->Shared)
+      error("-r and -shared may not be used together");
+  }
+
----------------
Move this to checkOptions.

================
Comment at: ELF/InputFiles.cpp:209
@@ +208,3 @@
+
+      if (Config->Relocatable) {
+        InputSection<ELFT> *S = new (this->Alloc) InputSection<ELFT>(this, &Sec);
----------------
This needs a description. (Any piece of code that does something obvious needs explanation.)

It is probably better to explain how you are handling object files if -r.



================
Comment at: ELF/InputSection.cpp:95-107
@@ -93,1 +94,15 @@
+
+    if (RelocatingSection) {
+      auto *P = reinterpret_cast<RelType *>(Buf);
+      Buf += sizeof(RelType);
+      // Relocation for local symbol here means that it is probably
+      // rel[a].eh_frame section which has references to
+      // sections in r_info field. Also needs fix for addend.
+      if (SymIndex < SymTab->sh_info)
+        error("Not implemented relocation section type");
+      SymbolBody &Body = *File.getSymbolBody(SymIndex)->repl();
+      P->r_offset += RelocatingSection->OutSecOff;
+      P->setSymbolAndType(Body.getSymbolTableIndex(), Type, Config->Mips64EL);
+      continue;
+    }
 
----------------
I don't think this is a right place to add this code. This is more like copying existing relocations rather than relocating. You want to create a new function to copy relocation sections instead of embedding here.

================
Comment at: ELF/InputSection.cpp:149-159
@@ -134,3 +148,13 @@
+
+  // If it is SHT_REL[A] section then RelocatingSection will be available.
+  // That happens if relocatable output was choosen. In that case we should fix
+  // the symbol indices within symtab table, fix addends in some cases and also
+  // other possible data.
+  SmallVector<const Elf_Shdr *, 1> Info;
+  if (RelocatingSection)
+    Info.emplace_back(Header);
+
   // Iterate over all relocation sections that apply to this section.
-  for (const Elf_Shdr *RelSec : RelocSections) {
+  SmallVector<const Elf_Shdr *, 1> &Relocs = RelocatingSection ? Info : RelocSections;
+  for (const Elf_Shdr *RelSec : Relocs) {
     if (RelSec->sh_type == SHT_RELA)
----------------
This code is not very readable -- you defined a vector with only one element and run a for-loop over that vector? It is probably better to just call a different function if we find that we are handling a relocation section.

================
Comment at: ELF/InputSection.h:120
@@ -119,1 +119,3 @@
 
+  // Section to which the relocation applies if this is a SHT_REL[A] section.
+  InputSection<ELFT> *RelocatingSection = nullptr;
----------------
  // If -r (or --relocatable) option is given, this member may have a non-null value.
  // During partial linking, we do not apply any relocations but just leave them as is,
  // expecting the final static linking applies them all at once.
  // In that case, relocation sections are handled as regular input sections, and their
  // RelocatingSection pointers point to their target sections.

================
Comment at: ELF/OutputSections.cpp:174
@@ -173,3 +173,3 @@
       if (NeedsGot)
-        P->setSymbolAndType(Body->getDynamicSymbolTableIndex(),
+        P->setSymbolAndType(Body->getSymbolTableIndex(),
                             LazyReloc ? Target->getPltReloc()
----------------
Why did you change the function name? If not relevant to this change, please revert.

================
Comment at: ELF/OutputSections.cpp:639
@@ +638,3 @@
+    : OutputSection<ELFT>(Name, sh_type, sh_flags) {
+  Elf_Shdr &Header = this->Header;
+  Header.sh_addralign = ELFT::Is64Bits ? 8 : 4;
----------------
Remove this temporary variable and directly assign to this->Header.

================
Comment at: ELF/OutputSections.cpp:646-649
@@ +645,6 @@
+void StaticRelocSection<ELFT>::addSection(InputSection<ELFT> *C) {
+  if (!LinkedOut)
+    LinkedOut = C->RelocatingSection->OutSec;
+  else if (LinkedOut != C->RelocatingSection->OutSec)
+    error("Sections to which the relocations applies doesn`t match");
+  OutputSection<ELFT>::addSection(C);
----------------
I'm not quite sure what you are doing here.

================
Comment at: ELF/OutputSections.cpp:654-655
@@ +653,4 @@
+template <class ELFT> void StaticRelocSection<ELFT>::finalize() {
+  if (Out<ELFT>::SymTab)
+    Header.sh_link = Out<ELFT>::SymTab->SectionIndex;
+  Header.sh_info = LinkedOut->SectionIndex;
----------------
Is it possible that we do not have the symbol table if -r is specified?

================
Comment at: ELF/OutputSections.cpp:942
@@ -917,2 +941,3 @@
                      });
-    return;
+    if (!Config->Relocatable)
+      return;
----------------
Why did you need this change?

================
Comment at: ELF/OutputSections.cpp:948
@@ -922,3 +947,3 @@
     Out<ELFT>::GnuHashTab->addSymbols(Symbols);
-  size_t I = 0;
+  size_t I = NumLocals;
   for (SymbolBody *B : Symbols)
----------------
What is this for?

================
Comment at: ELF/OutputSections.h:235-236
@@ -232,2 +234,4 @@
 
 template <class ELFT>
+class StaticRelocSection final : public OutputSection<ELFT> {
+public:
----------------
This needs explanation.

  // This section contains input relocation sections so that they are copied
  // to the output. Used only when -r is specified (-r lets the linker do partial
  // linking, which combines multiple .o files into single .o file).
  // Normally, we don't need this class because we interpret and consume
  // relocations ourselves instead of copying.

================
Comment at: ELF/OutputSections.h:243
@@ +242,3 @@
+private:
+  OutputSectionBase<ELFT> *LinkedOut = nullptr;
+};
----------------
What is this variable for?

================
Comment at: ELF/Writer.cpp:829-833
@@ -806,3 +828,7 @@
 
-  EHdr->e_type = Config->Shared ? ET_DYN : ET_EXEC;
+  if (Config->Relocatable)
+    EHdr->e_type = ET_REL;
+  else
+    EHdr->e_type = Config->Shared ? ET_DYN : ET_EXEC;
+
   EHdr->e_machine = FirstObj.getEMachine();
----------------
It looks a bit weird to use ?: in combination with `if`. I'd write

  if (Config->Shared)
    EHdr->e_type = ET_DYN;
  else if (Config->Relocatable)
    EHdr->e_type = ET_REL;
  else
    EHdr->e_type = ET_EXEC;


================
Comment at: ELF/Writer.cpp:837
@@ -810,3 +836,3 @@
   EHdr->e_entry = getEntryAddr();
-  EHdr->e_phoff = sizeof(Elf_Ehdr);
+  EHdr->e_phoff = EHdr->e_type == ET_REL ? 0 : sizeof(Elf_Ehdr);
   EHdr->e_shoff = SectionHeaderOff;
----------------
Use Config->Relocatable instead of EHdr->e_type==ET_REL

================
Comment at: ELF/Writer.cpp:840
@@ -813,3 +839,3 @@
   EHdr->e_ehsize = sizeof(Elf_Ehdr);
-  EHdr->e_phentsize = sizeof(Elf_Phdr);
+  EHdr->e_phentsize = EHdr->e_type == ET_REL ? 0 : sizeof(Elf_Phdr);
   EHdr->e_phnum = Phdrs.size();
----------------
Ditto


http://reviews.llvm.org/D14382