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

[Peter Smith via Phabricator via llvm-commits]

peter.smith 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[] = {
----------------
stuij wrote:
> 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)
Yes that looks good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139888