ARM: Do not tail-call to an externally-defined function with weak linkage

[olista01]

Summary

Externally-defined functions with weak linkage should not be
tail-called on ARM or AArch64, as the AAELF spec requires normal calls
to undefined weak functions to be replaced with a NOP or jump to the
next instruction. The behaviour of branch instructions in this
situation (as used for tail calls) is implementation-defined, so we
cannot rely on the linker replacing the tail call with a return.

[t.p.northover]

Hi Oliver,

Is this an armlink work-around? The ABI looks suspiciously like it was written specifically to be compatible with some recalcitrant linker. It's even phrased as a platform-dependent issue (especially so on AArch32).

GNU's linker on Linux (both AArch32 & AArch64) seems to cope fine, for example. So I don't think we'd want to disable the optimisation there. Or anywhere without evidence of problems, really.

Cheers.

Tim.

[olista01]

I originally stated that "we cannot rely on the linker replacing the tail call with a return", but after further thought I now realise that it is impossible for the linker to replace a tail-call with a return, as it may need to insert a stack pointer adjustment, but it does not know how big the adjustment must be.

Both armlink and arm-none-eabi-ld replace branch instructions to undefined weak symbols with a NOP, so this is not just an armlink problem.

The AAELF spec only states this for "platforms that do not support dynamic pre-emption of symbols", so I have modified the patch to only prevent the tail call for bare-metal systems.

[t.p.northover]

Hi Oliver,

I'm really sorry, I've realised I was confused yesterday. For some reason I'd convinced myself that the nop/"branch-to-next" was the correct behaviour and that's what I was checking for. I see more clearly what you mean now.

So going onto the patch itself, I think it's in the wrong place. The "tail call" instructions inserted at IR level are (usually) more of a hint than a strict requirement. The backends decide what really should and shouldn't be made a tail call, using the "isEligibleForTailCallOptimisation" in XYZISelLowering.cpp (at least in the AArch64 & ARM cases).

My confusion also affects where this is valid. It's almost certainly needed for Linux -static compilations too. Darwin platforms appear to be closest to the pre-emptable Linux situation.

Cheers.

Tim.

[olista01]

Moved this to the ARM and AArch64 backends, removed all changes to TailCallElim.

I have taken out the restriction based on target platform, as both bare-metal and Linux could need this.

[t.p.northover]

Hi Oliver,

This looks good to me. Thanks for updating it.

Tim.

[olista01]

Thanks, committed revision 215890.

lib/Target/AArch64/AArch64ISelLowering.cpp

if (i->hasByValAttr())
       return false;
 
   if (getTargetMachine().Options.GuaranteedTailCallOpt) {
     if (IsTailCallConvention(CalleeCC) && CCMatch)
       return true;
     return false;
   }
 
+  // Externally-defined functions with weak linkage should not be
+  // tail-called on AArch64 when the OS does not support dynamic
+  // pre-emption of symbols, as the AAELF spec requires normal calls
+  // to undefined weak functions to be replaced with a NOP or jump to the
+  // next instruction. The behaviour of branch instructions in this
+  // situation (as used for tail calls) is implementation-defined, so we
+  // cannot rely on the linker replacing the tail call with a return.
+  if (GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee)) {
+    const GlobalValue *GV = G->getGlobal();
+    if (GV->hasExternalWeakLinkage())
+      return false;
+  }
+
   // Now we search for cases where we can use a tail call without changing the
   // ABI. Sibcall is used in some places (particularly gcc) to refer to this
   // concept.
 
   // I want anyone implementing a new calling convention to think long and hard
   // about this assert.
   assert((!isVarArg || CalleeCC == CallingConv::C) &&
          "Unexpected variadic calling convention");

lib/Target/ARM/ARMISelLowering.cpp

ARMTargetLowering::IsEligibleForTailCallOptimization(SDValue Callee,
 
   // Thumb1 PIC calls to external symbols use BX, so they can be tail calls,
   // but we need to make sure there are enough registers; the only valid
   // registers are the 4 used for parameters.  We don't currently do this
   // case.
   if (Subtarget->isThumb1Only())
     return false;
 
+  // Externally-defined functions with weak linkage should not be
+  // tail-called on ARM when the OS does not support dynamic
+  // pre-emption of symbols, as the AAELF spec requires normal calls
+  // to undefined weak functions to be replaced with a NOP or jump to the
+  // next instruction. The behaviour of branch instructions in this
+  // situation (as used for tail calls) is implementation-defined, so we
+  // cannot rely on the linker replacing the tail call with a return.
+  if (GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee)) {
+    const GlobalValue *GV = G->getGlobal();
+    if (GV->hasExternalWeakLinkage())
+      return false;
+  }
+
   // If the calling conventions do not match, then we'd better make sure the
   // results are returned in the same way as what the caller expects.
   if (!CCMatch) {
     SmallVector<CCValAssign, 16> RVLocs1;
     ARMCCState CCInfo1(CalleeCC, false, DAG.getMachineFunction(), RVLocs1,
                        *DAG.getContext(), Call);
     CCInfo1.AnalyzeCallResult(Ins, CCAssignFnForNode(CalleeCC, true, isVarArg));
 

test/CodeGen/AArch64/tail-call.ll

 ; RUN: llc -verify-machineinstrs < %s -mtriple=aarch64-none-linux-gnu -tailcallopt | FileCheck %s
 
 declare fastcc void @callee_stack0()
 declare fastcc void @callee_stack8([8 x i32], i64)
 declare fastcc void @callee_stack16([8 x i32], i64, i64)
+declare extern_weak fastcc void @callee_weak()
 
 define fastcc void @caller_to0_from0() nounwind {
 ; CHECK-LABEL: caller_to0_from0:
 ; CHECK-NEXT: // BB
 
   tail call fastcc void @callee_stack0()
   ret void
 
; otherwise either %a or %b will be wrongly clobbered.
   tail call fastcc void @callee_stack16([8 x i32] undef, i64 %b, i64 %a)
   ret void
 
 ; CHECK: ldp {{x[0-9]+}}, {{x[0-9]+}}, [sp, #16]
 ; CHECK: stp {{x[0-9]+}}, {{x[0-9]+}}, [sp, #16]
 ; CHECK-NEXT: add sp, sp, #16
 ; CHECK-NEXT: b callee_stack16
 }
+
+
+; Weakly-referenced extern functions cannot be tail-called, as AAELF does
+; not define the behaviour of branch instructions to undefined weak symbols.
+define fastcc void @caller_weak() {
+; CHECK-LABEL: caller_weak:
+; CHECK: bl callee_weak
+  tail call void @callee_weak()
+  ret void
+}

test/CodeGen/ARM/tail-call.ll

 ; RUN: llc -mtriple armv7 -O0 -o - < %s | FileCheck %s -check-prefix CHECK-TAIL
 ; RUN: llc -mtriple armv7 -O0 -disable-tail-calls -o - < %s \
 ; RUN:   | FileCheck %s -check-prefix CHECK-NO-TAIL
 
 declare i32 @callee(i32 %i)
+declare extern_weak fastcc void @callee_weak()
 
 define i32 @caller(i32 %i) {
 entry:
   %r = tail call i32 @callee(i32 %i)
   ret i32 %r
 }
 
 ; CHECK-TAIL-LABEL: caller
 ; CHECK-TAIL: b callee
 
 ; CHECK-NO-TAIL-LABEL: caller
 ; CHECK-NO-TAIL: push {lr}
 ; CHECK-NO-TAIL: bl callee
 ; CHECK-NO-TAIL: pop {lr}
 ; CHECK-NO-TAIL: bx lr
 
+
+; Weakly-referenced extern functions cannot be tail-called, as AAELF does
+; not define the behaviour of branch instructions to undefined weak symbols.
+define fastcc void @caller_weak() {
+; CHECK-LABEL: caller_weak:
+; CHECK: bl callee_weak
+  tail call void @callee_weak()
+  ret void
+}