[PATCH] D139888: [lld][ARM] support absolute thunks for Armv4T Thumb and interworking

[Ties Stuij via Phabricator via llvm-commits]

stuij added inline comments.


================
Comment at: lld/ELF/Thunks.cpp:733
 
-bool ARMV5PILongThunk::isCompatibleWith(const InputSection &isec,
-                                        const Relocation &rel) const {
-  // Thumb branch relocations can't use BLX
-  return rel.type != R_ARM_THM_JUMP19 && rel.type != R_ARM_THM_JUMP24;
+void ARMV4ABSLongThunk::writeLong(uint8_t *buf) {
+  const uint8_t data[] = {
----------------
peter.smith wrote:
> It looks like the original naming convention for Thunks has broken down a bit. When I first saw the sequence I left a comment that you can't use this for V4 as it can't handle interworking, only saw the ARMLongBXThunk later. 
> 
> As I understand it the thunk has a code sequence that is V4 ABS (non-interworking), but V5 ABS (interworking). The new v4 ABS Long BX thunk is only needed for V4 interworking.
> 
> Although a more significant name change, perhaps:
> `ARMV4V5LongLdrPcThunk`
> `ARMV4LongBxThunk`
> 
> Not sure what the capitalisation should be for Ldr and Bx. As instructions they need to be all one case, but this makes it difficult to read the class name.
> 
> 
> As I understand it the thunk has a code sequence that is V4 ABS (non-interworking), but V5 ABS (interworking). The new v4 ABS Long BX thunk is only needed for V4 interworking.

Correct, LDR doesn't do state change for Armv4T.

Regarding naming, V6 also uses the V5 thunks, so the thunk name would become:
`ARMV4V5V6LongLdrPcThunk`

Similarly for PI thunks that would become:
`ARMV4V5V6PILongThunk`

I *think* I'm ok with this as it does make the intent clear, but it's not pretty. Thoughts?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139888/new/

https://reviews.llvm.org/D139888