[ARM] Create relocations for Thumb functions calling ARM fns in ELF.

[fhahn]

Summary

Without using a fixup in this case, BL will be used instead of BLX to
call internal ARM functions from Thumb functions.

[echristo]

One inline question, looks ok to me otherwise.

-eric

test/MC/ARM/thumb-far-jump.s

25 ↗

(On Diff #99879)

I think this is right, but I'm curious how we had it precisely inverted. What's up?

[peter.smith]

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.

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp

732

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

741

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.

[fhahn]

test/MC/ARM/thumb-far-jump.s

25 ↗

(On Diff #99879)

Without the changes in this patch, no relocations are generated for branches to internal labels that are in range of the branch, which was the case for the branch to end. That means BL is used for all such branches, even for branches from Thumb to ARM code.

With this patch, a relocation is always generated when jumping to non Thumb code. But currently isThumbFunc only tracks function symbols, while end is a non-function label, meaning a relocation is added even though it is not really necessary because in this case we are branch from Thumb code to Thumb code.

As Peter explained, additional relocations for non-function labels shouldn't cause problems, as long as there's no change in execution mode. And unless we are dealing with hand-written assembler that violates the ABI, the execution mode should only be changed by branches to function symbols AFAIK.

[echristo]

test/MC/ARM/thumb-far-jump.s

25 ↗

(On Diff #99879)

Seems reasonable. At any rate I trust you and Peter to figure this one out among you :)

[fhahn]

Updated patch to only emit relocations for branches to ELF function symbols. It seems like ARM::fixup_arm_thumb_bl isn't used for MachO binaries and I am not sure how/if we should deal with Windows binaries?

[peter.smith]

In general that looks good to me. I think it would be best not to hoist the check for STT_FUNC and STT_GNU_IFUNC into MC as this is common to all Targets and STT_GNU_IFUNC is defined in the STT_LOOS (OS specific range) and hence could mean something different on another platform/target.

lib/MC/MCSymbolELF.cpp

197

I think that we should be careful in putting this in MCSymbolELF. The STT_FUNC is generic ELF and common to all Targets but STT_GNU_IFUNC is defined in the OS specific range [STT_LOOS, STT_HIOS] so there is an outside chance that it is defined to something else on a non ARM Target.

[fhahn]

Addressed Peter's comment and removed hoisting the check for STT_FUNC and STT_GNU_IFUNC into MCSymbolElf.

[peter.smith]

LGTM; I'm happy with that. It will be worth waiting a bit to make sure there are no other objections from those in different timezones.

include/llvm/MC/MCSymbolELF.h

public:
   bool isBindingSet() const;
 
   void setIsWeakrefUsedInReloc() const;
   bool isWeakrefUsedInReloc() const;
 
   void setIsSignature() const;
   bool isSignature() const;
 
+  bool isFunction() const;
+
   static bool classof(const MCSymbol *S) { return S->isELF(); }
 
 private:
   void setIsBindingSet() const;
 };
 }
 
 #endif

lib/MC/MCSymbolELF.cpp

void MCSymbolELF::setIsSignature() const {
   uint32_t OtherFlags = getFlags() & ~(0x1 << ELF_IsSignature_Shift);
   setFlags(OtherFlags | (1 << ELF_IsSignature_Shift));
 }
 
 bool MCSymbolELF::isSignature() const {
   return getFlags() & (0x1 << ELF_IsSignature_Shift);
 }
 
+bool MCSymbolELF::isFunction() const {
+  return getType() == ELF::STT_FUNC || getType() == ELF::STT_GNU_IFUNC;
+}
+
 void MCSymbolELF::setIsBindingSet() const {
   uint32_t OtherFlags = getFlags() & ~(0x1 << ELF_BindingSet_Shift);
   setFlags(OtherFlags | (1 << ELF_BindingSet_Shift));
 }
 
 bool MCSymbolELF::isBindingSet() const {
   return getFlags() & (0x1 << ELF_BindingSet_Shift);
 }
 }

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp

if (IsResolved && (unsigned)Fixup.getKind() == ARM::fixup_arm_thumb_bl) {
 
     // If the symbol is external the linker will handle it.
     // FIXME: Should we handle it as an optimization?
 
     // If the symbol is out of range, produce a relocation and hope the
     // linker can handle it. GNU AS produces an error in this case.
     if (Sym->isExternal() || Value >= 0x400004)
       IsResolved = false;
+    // When an ARM function is called from a Thumb function, produce a
+    // relocation so the linker will use the correct branch instruction.
+    if (Sym->isELF() && dyn_cast<MCSymbolELF>(Sym)->isFunction() &&
+        !Asm.isThumbFunc(Sym))
+      IsResolved = false;
   }
   // We must always generate a relocation for BL/BLX instructions if we have
   // a symbol to reference, as the linker relies on knowing the destination
   // 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 ||
             (unsigned)Fixup.getKind() == ARM::fixup_arm_uncondbl ||
             (unsigned)Fixup.getKind() == ARM::fixup_arm_condbl))
     IsResolved = false;
 }
 
 /// getFixupKindNumBytes - The number of bytes the fixup may change.
 static unsigned getFixupKindNumBytes(unsigned Kind) {

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

 }
 
 void ARMTargetELFStreamer::emitLabel(MCSymbol *Symbol) {
   ARMELFStreamer &Streamer = getStreamer();
   if (!Streamer.IsThumb)
     return;
 
   Streamer.getAssembler().registerSymbol(*Symbol);
-  unsigned Type = cast<MCSymbolELF>(Symbol)->getType();
-  if (Type == ELF::STT_FUNC || Type == ELF::STT_GNU_IFUNC)
+  if (cast<MCSymbolELF>(Symbol)->isFunction())
     Streamer.EmitThumbFunc(Symbol);
 }
 
 void
 ARMTargetELFStreamer::AnnotateTLSDescriptorSequence(const MCSymbolRefExpr *S) {
   getStreamer().EmitFixup(S, FK_Data_4);
 }
 

test/MC/ARM/big-endian-thumb-fixup.s

 // RUN: llvm-mc -triple=armeb-eabi -mattr v7,vfp2 -filetype=obj < %s | llvm-objdump -s - | FileCheck %s
 
   .syntax unified
   .text
   .align  2
   .code 16
+  .thumb_func
 
 @ARM::fixup_arm_thumb_bl
 .section s_thumb_bl,"ax",%progbits
 // CHECK-LABEL: Contents of section s_thumb_bl
 // CHECK: 0000 f000f801
   bl thumb_bl_label
   nop
 thumb_bl_label:

test/MC/ARM/mixed-arm-thumb-bl-fixup.ll

+; RUN: llc -O0 < %s -mtriple armv7-linux-gnueabi -o - \
+; RUN:   | llvm-mc -triple armv7-linux-gnueabi -filetype=obj -o - \
+; RUN:    | llvm-readobj -r | FileCheck --check-prefix LINUX %s
+
+; RUN: llc -O0 < %s -mtriple armv7-linux-android -o - \
+; RUN:   | llvm-mc -triple armv7-linux-android -filetype=obj -o - \
+; RUN:    | llvm-readobj -r | FileCheck --check-prefix LINUX %s
+
+
+; RUN: llc -O0 < %s -mtriple armv7-apple-ios -o - \
+; RUN:   | llvm-mc -triple armv7-apple-ios -filetype=obj -o - \
+; RUN:    | llvm-readobj -r | FileCheck --check-prefix IOS %s
+
+
+define void @thumb_caller() #0 {
+  call void @internal_arm_fn()
+  call void @global_arm_fn()
+  call void @internal_thumb_fn()
+  call void @global_thumb_fn()
+  ret void
+}
+
+define void @arm_caller() #1 {
+  call void @internal_arm_fn()
+  call void @global_arm_fn()
+  call void @internal_thumb_fn()
+  call void @global_thumb_fn()
+  ret void
+}
+
+define internal void @internal_thumb_fn() #0 {
+  ret void
+}
+
+define void @global_thumb_fn() #0 {
+entry:
+  br label %end
+end:
+  br label %end
+  ret void
+}
+
+define internal void @internal_arm_fn() #1 {
+  ret void
+}
+
+define void @global_arm_fn() #1 {
+entry:
+  br label %end
+end:
+  br label %end
+  ret void
+}
+
+attributes #0 = { "target-features"="+thumb-mode" }
+attributes #1 = { "target-features"="-thumb-mode" }
+
+; LINUX: Section (3) .rel.text {
+; LINUX-NEXT: 0x2 R_ARM_THM_CALL internal_arm_fn 0x0
+; LINUX-NEXT: 0x6 R_ARM_THM_CALL global_arm_fn 0x0
+; LINUX-NEXT: 0xE R_ARM_THM_CALL global_thumb_fn 0x0
+; LINUX-NEXT: 0x1C R_ARM_CALL internal_arm_fn 0x0
+; LINUX-NEXT: 0x20 R_ARM_CALL global_arm_fn 0x0
+; LINUX-NEXT: 0x24 R_ARM_CALL internal_thumb_fn 0x0
+; LINUX-NEXT: 0x28 R_ARM_CALL global_thumb_fn 0x0
+; LINUX-NEXT: }
+
+; IOS:   Section __text {
+; IOS-NEXT: 0x2C 1 2 0 ARM_RELOC_BR24 0 __text
+; IOS-NEXT: 0x28 1 2 0 ARM_RELOC_BR24 0 __text
+; IOS-NEXT: 0x24 1 2 0 ARM_RELOC_BR24 0 __text
+; IOS-NEXT: 0x20 1 2 0 ARM_RELOC_BR24 0 __text
+; IOS-NEXT: 0x10 1 2 0 ARM_THUMB_RELOC_BR22 0 __text
+; IOS-NEXT: 0xC 1 2 0 ARM_THUMB_RELOC_BR22 0 __text
+; IOS-NEXT: 0x8 1 2 0 ARM_THUMB_RELOC_BR22 0 __text
+; IOS-NEXT: 0x4 1 2 0 ARM_THUMB_RELOC_BR22 0 __text
+; IOS-NEXT: }