[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:
> stuij wrote:
> > 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?
> Likely that the best naming scheme means renaming everything, which is not ideal as it will mean updating a lot of tests.
> 
> I think the requirement is probably the best so V7 requires Arm V7 etc. This would imply that only ARMV4PILongThunk is needed.
> 
> The awkward case is the LongLdrPcThunk as that can be used in Arm state for non-interworking. This would end up with a monstrosity like `ARMV5AndNotInterworkingV4LongLdrPcThunk`. It is probably less confusing to say `ARMV5LongLdrPcThunk` with a comment that says can be used for ARMV4T if not interworking.
> 
> Probably best err on the side of readable names with the one exception rather than precise but unreadable names. 
Right, so we're back to minimum arch requirement encoding so to speak. Yes I'd prefer that. And can live with ARMV5LongLdrPcThunk.

So the name changes included in this patch would then be:
`ARMV4PILongThunk`
`ARMV5LongLdrPcThunk`
`ARMV4ABSLongBxThunk` (as there will also be an `ARMV4PILongBxThunk` in follow-up patch)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139888