[PATCH] Initial ELF/AArch64 Support

Renato Golin renato.golin at linaro.org
Tue Aug 5 01:20:53 PDT 2014


Hi Daniel,

I'm not an expert on linkers neither AARch64, but I agree that this is a great starting point. Thanks for working on this!

As Shankar said, tests would be good, following what's already in lld for other archs. Even if you can't link more than hello world, you do have some features in, and linking / disassembling / FileCheck'ing wouldn't be that hard to implement for a few basic cases. It would be good, however, to stress each part of the code more thoroughly with time, and since this is brand new code, it shouldn't take too long to go in, or we'll forget, and then ignore.

With tests and the comments addressed by others and me, I think this looks good to me, too.

cheers,
--renato

================
Comment at: lib/ReaderWriter/ELF/AArch64/AArch64RelocationHandler.cpp:32
@@ +31,3 @@
+/// \brief R_AARCH64_PREL32 - word32: S + A - P
+static void relocR_AARCH64_PREL32(uint8_t *location, uint64_t P, uint64_t S, int64_t A) {
+  int32_t result = (int32_t)((S + A) - P);
----------------
Forgot debug for this one, and some others. Is that intentional?

================
Comment at: lib/ReaderWriter/ELF/AArch64/AArch64RelocationHandler.cpp:352
@@ +351,3 @@
+    break;
+  case R_AARCH64_ABS64:
+    relocR_AARCH64_ABS64(location, relocVAddress, targetVAddress, ref.addend());
----------------
If I got it right, the ones that have debug messages here are not the ones that have it up there. Since we're mostly just calling them in the switch, why not move them all into one place, ie. the functions up there?

http://reviews.llvm.org/D4778






More information about the llvm-commits mailing list