[darwin/x86] Model cxx_fast_tlscc as not preserving r11

[thakis]

Summary

If a TLV stub is called from a dylib, dyld_stub_binder will clobber r11.

Fixes PR51703.

[rjmccall]

Hmm, I wonder if there's a compatibility problem for changing this; I hadn't realized this was so old. It dates to 2015 (r252837), in a commit by Manman.

[thakis]

What's the next step here? Do you want to run some internal tests? Do you want me to run some tests? Use a different approach?

[rjmccall]

Sorry, I'm not entirely sure who owns this feature right now, I'm asking around.

[thakis]

Are there any owners to be found? :)

[thakis]

Whats the next step here?

[thakis]

roughly monthly ping :)

[thakis]

Should we just land this and see what (if any) breaks?

We do know that it fixes a miscompile of fairly basic code.

[hans]

Hmm, I think the potential problem here is that dropping R11 from CSR_64_TLS_Darwin does not only change the calling code (which is what we want, and seems like a safe change), but will also change the callee, i.e. the TLS wrapper functions wherever they're generated, to no longer preserve R11 (which they did before, although not when called through the dyld_stub_binder). That would cause a problem if the caller and callee are built by different compiler versions (e.g. an older program calling some newer (system?) library code). I don't know whether that could happen though?

(These registers are also tied up with "split CSRs" somehow, which was added by https://github.com/llvm/llvm-project/commit/abc7c1d1d2d44b4d7416c714a13239872d95a72e followed by AArch64-specific code in https://github.com/llvm/llvm-project/commit/cbe4f9417d1cbb0192173439501fedb782949bad and then x86_64 in https://github.com/llvm/llvm-project/commit/ed967f37520fec398aa419514de552e1a917c9ec
But I'm not sure how relevant that is..)

[thakis]

rjmccall, do you have any opinion on what (if anything) should happen here? At the moment, using TLV across dylibs on darwin is basically broken (see https://github.com/llvm/llvm-project/issues/51045).

This is a fairly simple fix. It carries some risk, but maybe not having it is worse?

[rjmccall]

I think I can speak authoritatively at this point that the right way to understand this convention is:

• Functions defined using this convention must promise to preserve all registers.
• As a general rule, calls that are not direct calls within a linkage unit may invalidate linker scratch registers such as r11.
• Therefore, calls to the function may rely on all non-linker-scratch registers as being preserved, but may only rely on linker scratch registers being preserved if the call is a direct call to a callee known to be within the current linkage unit.

I'll leave it to LLVM folks to decide how they want to model that.

llvm/lib/Target/X86/X86CallingConv.td

 
 def CSR_Win64 : CalleeSavedRegs<(add CSR_Win64_NoSSE,
                                      (sequence "XMM%u", 6, 15))>;
 
 def CSR_Win64_SwiftError : CalleeSavedRegs<(sub CSR_Win64, R12)>;
 def CSR_Win64_SwiftTail : CalleeSavedRegs<(sub CSR_Win64, R13, R14)>;
 
 // The function used by Darwin to obtain the address of a thread-local variable
-// uses rdi to pass a single parameter and rax for the return value. All other
-// GPRs are preserved.
+// uses rdi to pass a single parameter and rax for the return value. The
+// dynamic linker can clobber r11. All other GPRs are preserved.
 def CSR_64_TLS_Darwin : CalleeSavedRegs<(add CSR_64, RCX, RDX, RSI,
-                                             R8, R9, R10, R11)>;
+                                             R8, R9, R10)>;
 
 // CSRs that are handled by prologue, epilogue.
 def CSR_64_CXX_TLS_Darwin_PE : CalleeSavedRegs<(add RBP)>;
 
 // CSRs that are handled explicitly via copies.
 def CSR_64_CXX_TLS_Darwin_ViaCopy : CalleeSavedRegs<(sub CSR_64_TLS_Darwin, RBP)>;
 
 // All GPRs - except r11

llvm/test/CodeGen/X86/cxx_tlscc64.ll

 @sum1 = internal thread_local global i32 0, align 4
 
 declare void @_ZN1SC1Ev(%struct.S*)
 declare void @_ZN1SD1Ev(%struct.S*)
 declare i32 @_tlv_atexit(void (i8*)*, i8*, i8*)
 
 ; Every GPR should be saved - except rdi, rax, and rsp
 ; CHECK-LABEL: _ZTW2sg
-; CHECK-NOT: pushq %r11
 ; CHECK-NOT: pushq %r10
 ; CHECK-NOT: pushq %r9
 ; CHECK-NOT: pushq %r8
 ; CHECK-NOT: pushq %rsi
 ; CHECK-NOT: pushq %rdx
 ; CHECK-NOT: pushq %rcx
 ; CHECK-NOT: pushq %rbx
 ; CHECK: callq
 ; CHECK: jne
 ; CHECK: callq
 ; CHECK: tlv_atexit
 ; CHECK: callq
 ; CHECK-NOT: popq %rbx
 ; CHECK-NOT: popq %rcx
 ; CHECK-NOT: popq %rdx
 ; CHECK-NOT: popq %rsi
 ; CHECK-NOT: popq %r8
 ; CHECK-NOT: popq %r9
 ; CHECK-NOT: popq %r10
-; CHECK-NOT: popq %r11
 
 ; CHECK-O0-LABEL: _ZTW2sg
-; CHECK-O0: pushq %r11
 ; CHECK-O0: pushq %r10
 ; CHECK-O0: pushq %r9
 ; CHECK-O0: pushq %r8
 ; CHECK-O0: pushq %rsi
 ; CHECK-O0: pushq %rdx
 ; CHECK-O0: pushq %rcx
 ; CHECK-O0: callq
 ; CHECK-O0: jne
 ; CHECK-O0: callq
 ; CHECK-O0: tlv_atexit
 ; CHECK-O0: callq
 ; CHECK-O0: popq %rcx
 ; CHECK-O0: popq %rdx
 ; CHECK-O0: popq %rsi
 ; CHECK-O0: popq %r8
 ; CHECK-O0: popq %r9
 ; CHECK-O0: popq %r10
-; CHECK-O0: popq %r11
 define cxx_fast_tlscc nonnull %struct.S* @_ZTW2sg() nounwind {
   %.b.i = load i1, i1* @__tls_guard, align 1
   br i1 %.b.i, label %__tls_init.exit, label %init.i
 
 init.i:
   store i1 true, i1* @__tls_guard, align 1
   tail call void @_ZN1SC1Ev(%struct.S* nonnull @sg) #2
   %1 = tail call i32 @_tlv_atexit(void (i8*)* nonnull bitcast (void (%struct.S*)* @_ZN1SD1Ev to void (i8*)*), i8* nonnull getelementptr inbounds (%struct.S, %struct.S* @sg, i64 0, i32 0), i8* nonnull @__dso_handle) #2
   br label %__tls_init.exit
 
 __tls_init.exit:
   ret %struct.S* @sg
 }
 
 ; CHECK-LABEL: _ZTW4sum1
-; CHECK-NOT: pushq %r11
 ; CHECK-NOT: pushq %r10
 ; CHECK-NOT: pushq %r9
 ; CHECK-NOT: pushq %r8
 ; CHECK-NOT: pushq %rsi
 ; CHECK-NOT: pushq %rdx
 ; CHECK-NOT: pushq %rcx
 ; CHECK-NOT: pushq %rbx
 ; CHECK: callq
 ; CHECK-O0-LABEL: _ZTW4sum1
-; CHECK-O0-NOT: pushq %r11
 ; CHECK-O0-NOT: pushq %r10
 ; CHECK-O0-NOT: pushq %r9
 ; CHECK-O0-NOT: pushq %r8
 ; CHECK-O0-NOT: pushq %rsi
 ; CHECK-O0-NOT: pushq %rdx
 ; CHECK-O0-NOT: pushq %rcx
 ; CHECK-O0-NOT: pushq %rbx
-; CHECK-O0-NOT: movq %r11
 ; CHECK-O0-NOT: movq %r10
 ; CHECK-O0-NOT: movq %r9
 ; CHECK-O0-NOT: movq %r8
 ; CHECK-O0-NOT: movq %rsi
 ; CHECK-O0-NOT: movq %rdx
 ; CHECK-O0-NOT: movq %rcx
 ; CHECK-O0-NOT: movq %rbx
 ; CHECK-O0: callq
 define cxx_fast_tlscc nonnull i32* @_ZTW4sum1() nounwind {
   ret i32* @sum1
 }
 
 ; Make sure at O0 we don't overwrite RBP.
 ; CHECK-O0-LABEL: _ZTW4sum2
 ; CHECK-O0: pushq %rbp
 ; CHECK-O0: movq %rsp, %rbp
 ; CHECK-O0-NOT: movq %r{{.*}}, (%rbp) 
 define cxx_fast_tlscc i32* @_ZTW4sum2() #0 {
   ret i32* @sum1
 }
 
 ; Make sure at O0, we don't generate spilling/reloading of the CSRs.
 ; CHECK-O0-LABEL: tls_test2
-; CHECK-O0-NOT: pushq %r11
 ; CHECK-O0-NOT: pushq %r10
 ; CHECK-O0-NOT: pushq %r9
 ; CHECK-O0-NOT: pushq %r8
 ; CHECK-O0-NOT: pushq %rsi
 ; CHECK-O0-NOT: pushq %rdx
 ; CHECK-O0: callq {{.*}}tls_helper
 ; CHECK-O0-NOT: popq %rdx
 ; CHECK-O0-NOT: popq %rsi
 ; CHECK-O0-NOT: popq %r8
 ; CHECK-O0-NOT: popq %r9
 ; CHECK-O0-NOT: popq %r10
-; CHECK-O0-NOT: popq %r11
 ; CHECK-O0: ret
 %class.C = type { i32 }
 @tC = internal thread_local global %class.C zeroinitializer, align 4
 declare cxx_fast_tlscc void @tls_helper()
 define cxx_fast_tlscc %class.C* @tls_test2() #1 {
   call cxx_fast_tlscc void @tls_helper()
   ret %class.C* @tC
 }
 
 ; Make sure we do not allow tail call when caller and callee have different
 ; calling conventions.
 declare %class.C* @_ZN1CD1Ev(%class.C* readnone returned %this)
 ; CHECK-LABEL: tls_test
 ; CHECK: callq {{.*}}tlv_atexit
 define cxx_fast_tlscc void @tls_test() {
 entry:
   store i32 0, i32* getelementptr inbounds (%class.C, %class.C* @tC, i64 0, i32 0), align 4
   %0 = tail call i32 @_tlv_atexit(void (i8*)* bitcast (%class.C* (%class.C*)* @_ZN1CD1Ev to void (i8*)*), i8* bitcast (%class.C* @tC to i8*), i8* nonnull @__dso_handle) #1
   ret void
 }
 
 @ssp_var = internal thread_local global i8 0, align 1
 
 ; CHECK-LABEL: test_ssp
-; CHECK-NOT: pushq %r11
 ; CHECK-NOT: pushq %r10
 ; CHECK-NOT: pushq %r9
 ; CHECK-NOT: pushq %r8
 ; CHECK-NOT: pushq %rsi
 ; CHECK-NOT: pushq %rdx
 ; CHECK-NOT: pushq %rcx
 ; CHECK-NOT: pushq %rbx
 ; CHECK: callq
 define cxx_fast_tlscc nonnull i8* @test_ssp() #2 {
   ret i8* @ssp_var
 }
 attributes #0 = { nounwind "frame-pointer"="all" }
 attributes #1 = { nounwind }
 attributes #2 = { nounwind sspreq }