[X86_32] Miscompilation with __llvm_retpoline_push

[llvmbot]

Bugzilla Link	36329
Resolution	FIXED
Resolved on	Feb 14, 2018 02:10
Version	6.0
OS	Linux
Blocks	#35152
Attachments	Config file for kernel build
Reporter	LLVM Bugzilla Contributor
CC	@zmodem,@tstellar

Extended Description

After a call via __llvm_retpoline_push, code in the calling function seems confused about where %esp points.

Reproduced with llvm/clang master (r324745 / r324741 resp.) and also with 6.0 branch (r324726 / r324719 resp) with r326645 added to the latter.

To reproduce:

git clone git://git.infradead.org/linux-retpoline.git
cd linux-retpoline
git checkout clang
cp /where/you/downloaded/the/attached/config .config
make CC=/where/is/your/clang bzImage
qemu-system-i386 -display none -serial stdio -kernel arch/x86/boot/bzImage -append earlyprintk=ttyS0,keep

Observe the output from mp_register_ioapic():
[ 0.000000] mp_register_ioapic, 0 fec00000 0 c1b31e88
[ 0.000000] At line 412, gsi_base is 0
[ 0.000000] At line 425, gsi_base is -1043707140
[ 0.000000] At line 427, gsi_base is -1043707140

Between line 412 and 425, gsi_base got clobbered.

Now uncomment the CFLAGS_io_apic_b line in arch/x86/kernel/apic/Makefile and repeat, to disable the retpoline. Observe the problem go away.

To eliminate the external thunks as a cause of this problem, also retest with
CFLAGS_io_apic_b.o += -mno-retpoline-external-thunk -mretpoline
The problem persists even when we let LLVM emit its own thunks.

[llvmbot]

assigned to @rnk

[llvmbot]

http://david.woodhou.se/io_apic_b.i
http://david.woodhou.se/io_apic_b_llvm_retpoline_v6.s
http://david.woodhou.se/io_apic_b_noretpoline_v6.s

[llvmbot]

In both .s files, observe that at about .Ltmp22, the 'gsi_base' variable is spilled from %edi to the stack at %esp+12:

.Ltmp22:
addl $12, %esp
movl %edi, 12(%esp) # 4-byte Spill

The no-retpoline version does a plain indirect call:

calll	*pv_mmu_ops+132

.Ltmp28:

... while the retpoline version uses __llvm_retpoline_push:

pushl	pv_mmu_ops+132

.Ltmp28:
calll __llvm_retpoline_push

They both continue to call bad_ioapic_register() which will return zero. Thus taking the conditional branch to .LBB0_11:

calll	bad_ioapic_register
testl	%eax, %eax

.Ltmp29:
.loc 2 419 6 is_stmt 0 # arch/x86/kernel/apic/io_apic_b.c:419:6
je .LBB0_11

Now compare the code at .LBB0_11 in both versions. The retpoline version has this:

.LBB0_11:
.Ltmp34:
#DEBUG_VALUE: mp_register_ioapic:address <- %esi
#DEBUG_VALUE: mp_register_ioapic:cfg <- [DW_OP_plus_uconst 8] [%ebp+0]
.loc 2 0 1 is_stmt 0 # arch/x86/kernel/apic/io_apic_b.c:0:1
movl %ebx, 12(%esp) # 4-byte Spill
movl %edi, 20(%esp) # 4-byte Spill

.. while the non-retpoline version has this:

.LBB0_11:
.Ltmp33:
#DEBUG_VALUE: mp_register_ioapic:gsi_base <- [DW_OP_plus_uconst 12] [%esp+0]
#DEBUG_VALUE: mp_register_ioapic:address <- %esi
#DEBUG_VALUE: mp_register_ioapic:cfg <- [DW_OP_plus_uconst 8] [%ebp+0]
.loc 2 0 1 is_stmt 0 # arch/x86/kernel/apic/io_apic_b.c:0:1
movl %ebx, 8(%esp) # 4-byte Spill
movl %edi, 16(%esp) # 4-byte Spill

Observe that the retpoline version is doing another spill to %esp+12 and clobbering the value of 'gsi_base' which was already there. It's got confused about where %esp is pointing; perhaps because of the thunk call or its preceding 'pushl'? Stepping through this with GDB I checked that %esp hasn't actually changed; what was %esp+12 then is also %esp+12 now.

[rnk]

Our use of PUSH for retpoline interacts badly with x86 call frame optimization. Here's a smaller example where things go off the rails:

#define FORCE_SPILL()
__asm volatile("" : : : "eax", "ebx", "ecx", "edx", "edi", "esi", "ebp")
int getval();
void useval(int);
void push_call_seq(int, int, int, int, int);

void (*fp)(int, int, int);

int main() {
int v = getval();
push_call_seq(0, 0, 0, 0, 0);
FORCE_SPILL();
fp(0, 0, 0);
useval(v);
}

Compile like so:
$ clang -mretpoline -mretpoline-external-thunk -mregparm=3 -S --target=i686-linux -O2 t.c -o -
...
main: # @​main

%bb.0: # %entry

    pushl   %ebp
    pushl   %ebx
    pushl   %edi
    pushl   %esi
    subl    $12, %esp
    calll   getval
    movl    %eax, 8(%esp)           # 4-byte Spill
    subl    $8, %esp
    xorl    %eax, %eax
    xorl    %edx, %edx
    xorl    %ecx, %ecx
    pushl   $0
    pushl   $0
    calll   five_args
    addl    $16, %esp
    #APP
    #NO_APP
    xorl    %eax, %eax
    xorl    %edx, %edx
    xorl    %ecx, %ecx
    pushl   fp
    calll   __x86_indirect_thunk
    movl    12(%esp), %eax          # 4-byte Reload
    calll   useval
    xorl    %eax, %eax
    addl    $12, %esp
    popl    %esi
    popl    %edi
    popl    %ebx
    popl    %ebp
    retl

...

And see how we spill at 8(%esp) and fill from 12(%esp) because we think we've done a PUSH that hasn't yet been balanced out with a pop. Honestly, I should've seen this coming. I was worried about it, but I didn't test enough for it.

[rnk]

Here's another example where things go off the rails in a different way:

#include <assert.h>
int f(int a, int b, int c) {
return a + b + c;
}
int attribute((optnone)) getcsr() {
static int v;
return ++v;
}
int (*volatile fp)(int, int, int) = &f;
int main() {
int a = getcsr();
int b = getcsr();
int c = getcsr();
int d = getcsr();
int x = fp(a, b, c);
int y = fp(a, b, c);
int z = fp(x, y, d);
assert(z == 16);
assert(y == 6);
assert(x == 6);
assert(d == 4);
}

This function doesn't profit from using push to setup arguments in memory, so it's not an x86 call frame optimization problem, it's something else. This one has a spill between the PUSH and the retpoline call:

    calll   _Z6getcsrv
    movl    %eax, 8(%esp)           # 4-byte Spill

...
pushl %edi
movl 8(%esp), %edi # 4-byte Reload
calll __llvm_retpoline_push

That 8(%esp) is at the wrong offset.

We might be able to fix this bug by bundling the PUSH and the CALL together in a way that prevents spills from being inserted between them. That's probably easy, but the call frame optimization bug is trickier.

[rnk]

This should be fixed with r325049. We give up if none of EAX, ECX, EDX, or EDI are available, though.

Let me know if this fixes the issues in the Linux kernel, and we'll merge it to 6.0 and 5.0 along with the rest if so.

[llvmbot]

As long as I don't try to build the i915 driver, the 32-bit kernel builds and boots with LLVM r325049. Thanks.

Tested with my tree at
http://git.infradead.org/users/dwmw2/linux-retpoline.git/shortlog/refs/heads/clang

[rnk]

Hans and Tom, I merged this and the other retpoline patches to the 5.0 and 6.0 branches.

[zmodem]

Hans and Tom, I merged this and the other retpoline patches to the 5.0 and
6.0 branches.

Awesome, thanks!