[PATCH] D50077: [LLD][ELF][ARM] Add support for Armv5 and Armv6 compatible Thunks

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Thanks for the comments, I'll post another patch shortly.



================
Comment at: ELF/Driver.cpp:1463
+      // Adjust thunk section spacing to match shorter Thumb BL range.
+      Target->ThunkSectionSpacing = 0x400000 - 0x7500;
     // FIXME: These warnings can be removed when lld only uses these features
----------------
ruiu wrote:
> Currently, target is essentially a read-only object which only its constructor can initialize its members. So, if this needs to be a variable, can you define a function that returns an appropriate value, instead of precompute a value and mutate the Target member?
Yes, will do.


================
Comment at: ELF/Thunks.cpp:485
+      0x00, 0xc0, 0x9f, 0xe5, // P:  ldr ip, [pc] ; L2
+      0x0c, 0xf0, 0x8f, 0xe0, // L1: add pc, pc, ip
+      0x00, 0x00, 0x00, 0x00, // L2: .word S - (P + (L1 - P) + 8)
----------------
efriedma wrote:
> I think `add pc, pc, ip` isn't an interworking branch on pre-v7 targets, so you can't use this sequence if the destination is a Thumb function.
Yes you are correct. I've checked in the Arm ARM under interworking and add only comes up from v7 onwards. I'll update with with add ip, pc, ip ; bx ip.


================
Comment at: ELF/Thunks.cpp:622
+  }
+  fatal("unrecognized relocation type");
+}
----------------
grimar wrote:
> Does it make sense to include the file name and/or at least relocation value in this error message?
I'll improve it to add relocation type and symbol name. We don't have access to the filename at this point and given that this error shouldn't occur if all the input files have the right build attributes it is probably not worth threading it through.  


================
Comment at: ELF/Thunks.cpp:646
+      // currently implemented.
+      fatal("Thunks not supported for Architecture Armv6-m");
+  }
----------------
grimar wrote:
> Is it possible to test this error?
> Also, `Thunks` should be lower case (and perhaps `Architecture` too).
Yes, will correct and add tests for the error messages.


https://reviews.llvm.org/D50077