[PATCH] D33436: [ARM] Create relocation for Thumb functions calling ARM fns.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I've added a couple of inline comments and I hope I can answer the question about whether the relocations are harmless.

The ABI only permits a linker to do interworking (such as changing a BL to BLX) for a relocation if the symbol has type STT_FUNC and the relocation is one of R_ARM_CALL, R_ARM_JUMP24, R_ARM_THM_CALL, R_ARM_THM_JUMP22, R_ARM_THM_JUMP19, which match ARM BL/BLX, ARM B, Thumb BL/BLX, Thumb B.w, Thumb B.w<cc>. The narrow 16-bit Thumb branch instructions are excluded. The linker should give an error messages if it detects that an ARM/Thumb state change is required outside of these conditions. So I think that adding a relocation wouldn't make correct code generated by the assembler incorrect, and it would give the linker a chance to detect the error. I note that for ARM state callers we just mark all the branch type relocations as IsResolved=false, and I suggest we do the same for wide Thumb branch type relocations as well.

If there is a BL/BLX to an internal label that doesn't have type STT_FUNC (and may not have the thumb bit set) then the linker is not supposed to try and do interworking. According to the ABI it is the programmers responsibility to be in the correct ARM/Thumb state before writing the instruction in this case. I think the ideal behaviour would be to issue an error message, but we could rely on the linker to do it.

As an aside the intent behind the wording in the ABI was to expose as many branch relocations to the linker as possible as there was existing functionality in embedded tool chains to do things like substitute functions (for example ARM $Sub$$, and the ld symbol wrapping facility). It was thought at the time that the trade off of more information and a potentially simpler assembler was worth having more work to do at link time was worthwhile. The ABI doesn't forbid toolchains from resolving relocations at assembly time, but it does put the burden of doing correct interworking (or issuing an error when it isn't possible) if the relocation is resolved at assembly time.



================
Comment at: lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp:732
       IsResolved = false;
+    // When an ARM function is called from a Thumb function, produce a
+    // relocation so the linker will use the correct branch instruction.
----------------
I don't think that this is complete enough as it doesn't handle the wide unconditional b.w and wide conditional b<cc> cases. The ABI [*] requires the linker to do interworking for these cases. It is rare to to see b<cc> as a function call requiring interworking but b.w is often used in tail calls.

For example if you add to your new test case in thumb_caller

```
        b.w     internal_arm_fn
        b.w     global_thumb_fn
        b.w     global_arm_fn
        beq.w   internal_arm_fn
        beq.w   global_arm_fn
        beq.w   global_thumb_fn
```
You'll get no relocations for any of these cases. There is no way the assembler can fix up the b.w and beq.w as there is immediate version of bx that can be substituted like blx. The only solution is a linker generated thunk/stub/veneer or an error message if the ARM target is mismatched.

[*] Call and jump relocations on page 33 of http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044f/IHI0044F_aaelf.pdf 



================
Comment at: lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp:740
   // symbol's thumb-ness to get interworking right.
   if (A && ((unsigned)Fixup.getKind() == ARM::fixup_arm_thumb_blx ||
             (unsigned)Fixup.getKind() == ARM::fixup_arm_blx ||
----------------
In ARM state this bit of code marks all BL, BLX, B and B<cc> as IsResolved=false so all these cases are handled correctly by the linker from ARM state. I recommend that we do the same thing for Thumb wide branch instructions. 


https://reviews.llvm.org/D33436