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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/InputFiles.cpp:230
@@ +229,3 @@
+      if (Config->Relocatable) {
+        // If relocatable output is choosen, relocation sections are handled as
+        // regular input sections, because no relocations are applied during link.
----------------
choosen -> chosen

================
Comment at: ELF/InputSection.cpp:109
@@ +108,3 @@
+template <bool isRela>
+void InputSectionBase<ELFT>::copyRelocations(uint8_t *Buf, uint8_t *BufEnd,
+                                             RelIteratorRange<isRela> Rels) {
----------------
You are not using BufEnd. Please remove.

================
Comment at: ELF/InputSection.cpp:112-113
@@ +111,4 @@
+  typedef Elf_Rel_Impl<ELFT, isRela> RelTy;
+  size_t Num = Rels.end() - Rels.begin();
+  for (size_t I = 0; I < Num; ++I) {
+    const RelTy &Rel = *(Rels.begin() + I);
----------------
Why don't you use range-based for?

================
Comment at: ELF/OutputSections.cpp:763
@@ +762,3 @@
+  else if (RelocOutSec != Sec->RelocatingSection->OutSec)
+    error("Sections to which the relocations applies doesn`t match");
+
----------------
grimar wrote:
> rafael wrote:
> > Can you get here? If so, add a test, if not, use an assert.
> I just assumed that users who will try linker probably will use the release version. So if them meet that error situation, there is no way they see the assert, it will not be debug build for them.
> error() is universal way to prompt here. I would use it or fatal() here.
So, the point is that

 - if this is not reachable except by programmer's error (by a bug), assert is better
 - if this is reachable by feeding incompatible files, giving incorrect combination of flags, etc, error is appropriate

================
Comment at: ELF/Writer.cpp:844
@@ -836,2 +843,3 @@
   case InputSectionBase<ELFT>::Regular:
-    Sec = new OutputSection<ELFT>(Key.Name, Key.Type, Key.Flags);
+    if (C->RelocatingSection)
+      Sec = new StaticRelocSection<ELFT>(Key.Name, Key.Type, Key.Flags,
----------------
Checking for `RelocatingSection` to see if an input section is a relocation section is an obscure way to distinguish two different kind of sections. Maybe we should define RelInputSection for relocation sections?


http://reviews.llvm.org/D14382