[PATCH] [ELF]: Initial implementation for ARM static linking

[Denis Protivensky]

>>! In D6446#13, @atanasyan wrote:
> Just curious, does the only test case cover the most part of new functionality?
The code is not ready yet, it doesn't even link statically, so I'm not sure if adding test cases won't result in a needless work.

>>! In D6446#14, @shankarke wrote:
> As Simon mentioned, This change needs more tests as it includes changes to handle dynamic linking too. 
I didn't plan to add support for dynamic linking right now, I'm still working on the static linking.

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMELFFile.h:25
@@ +24,3 @@
+
+  ARMELFFile(std::unique_ptr<MemoryBuffer> mb, bool atomizeStrings,
+                 std::error_code &ec)
----------------
atanasyan wrote:
> Strange indentation here. I recommend you to run the `clang-format` other all new files.
Thanks, didn't know about that tool. It revealed couple more formatting misses.

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMELFFile.h:103-108
@@ +102,8 @@
+      return readAddend_THM_CALL(ap);
+    case llvm::ELF::R_ARM_CALL:
+    case llvm::ELF::R_ARM_JUMP24:
+      return readAddend_ARM_CALL(ap);
+    case llvm::ELF::R_ARM_THM_MOVW_ABS_NC:
+    case llvm::ELF::R_ARM_THM_MOVT_ABS:
+      return readAddend_THM_MOV(ap);
+    case llvm::ELF::R_ARM_THM_JUMP11:
----------------
atanasyan wrote:
> shankarke wrote:
> > atanasyan wrote:
> > > shankarke wrote:
> > > > All of the above would need to be done in the relocation  handler. We should not interpret the bits in the Reader, this way the Reader is agnostic to the symbol value.
> > > > 
> > > > 
> > > But the relocation handler does not know about used relocation section format (.rel/.rela) so it cannot decide how and where to read an addendum. So I think it is a good approach to read addendums here, construct `Reference` objects, and call the `Reference::addend()` method further.
> > I have two concerns (a), (b) :-
> > 
> > a) Doing this in the Reader may not be idempotent (Need to consider cases like partial linking and ELF to yam conversions and back to atoms).
> > b) We may be losing some data because of the transformation in the Reader too, to preserve what was in the object file.
> > 
> > ELFLinkingContext::isRelaOutputFormat() can be used to figure out if the Target is using Rela/Rel. In addition the Target knows the relocation types that it handles.
> > 
> > 
> In general, I agree with you. As far as I can see ARM relocation addendums can be read in the `ARMRelocationHandler` class.
> 
> MIPS has some unusual relocations like `R_MIPS_HI16` / `R_MIPS_LO16`.  These relocations form pairs and to apply each of such relocation we need to know addendums from both paired relocations. The bad thing is that the paired relocations can belong to different atoms. So it is difficult to search a paired relocation in the `applyRelocation` method. That is why I have to read addendums for MIPS target in the `MipsELFFile` class. I think Denis’ implementation is inspired by this code.
> 
> By the way, if anybody knows a better solution I would be happy to hear it.
I used MIPS implementation as a reference, while I initially planned to put addend calculation to the relocation handler. Okay, I'll move the code to the relocation handler.

Here's just a little thing that I don't like in this solution, but it's not critical of course:
some relocations will have `Reference::addend` field filled, because they are processed and formed in the relocation pass. And other relocations will have `Reference::addend` empty, and this value will be calculated in-palce. I'm not sure if `Reference`'s addends are used somewhere before the relocation handler, but for me it was more consistent when they all were filled before and came to `applyRelocation` already formed.

//By the way, if anybody knows a better solution I would be happy to hear it.//
If there are limitations when putting addend calculation to `ELFFile`, isn't it better to add a separate pass to process these initial addends (and maybe cross-references between them as for MIPS) for example, after relocation pass?
While it requires more work and additional instance, it looks cleaner, and also removes that little miss I described above for the relocation handler.

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMExecutableWriter.h:36
@@ +35,3 @@
+
+    _gotFile->addAtom(*new (_gotFile->_alloc) GLOBAL_OFFSET_TABLEAtom(*_gotFile));
+  }
----------------
shankarke wrote:
> This should be done only for dynamic executables.
This is needed for handling STT_GNU_IFUNC. While this handing is not here, I've already implemented it and want to send as a separate diff.

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMLinkingContext.h:45
@@ +44,3 @@
+    case llvm::ELF::R_ARM_TLS_DTPOFF32:
+    case llvm::ELF::R_ARM_TLS_TPOFF32:
+    case llvm::ELF::R_ARM_COPY:
----------------
shankarke wrote:
> Why is this a dynamic relocation ?
Because these all marked as dynamic in the ARM ELF reference.

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp:158-161
@@ +157,6 @@
+        }
+            // Process all references.
+            llvm::dbgs()
+            << "Defined Atoms"
+            << "\n");
+    for (const auto &atom : mf->defined()) {
----------------
shankarke wrote:
> You may need to manually indent.
Will do.

================
Comment at: lib/ReaderWriter/ELF/ARM/TODO.rst:8
@@ +7,3 @@
+* Dynamic executable linking
+* DSO linking
+* PLT entries' generation for images larger than 2^28 bytes (see Sec. A.3 of the ELF reference)
----------------
shankarke wrote:
> you should also add ARM interoperability.
What does that mean? Can you please explain so I understand what I'm adding?

http://reviews.llvm.org/D6446