[llvm] r338481 - [x86] Fix a really subtle miscompile due to a somewhat glaring bug in
Tom Stellard via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 4 15:34:47 PDT 2018
On 07/31/2018 08:08 PM, Chandler Carruth wrote:
> +tstellar at redhat.com <mailto:tstellar at redhat.com> - We almost certainly want to pull this into whatever the next release is. =[
>
> +Craig Topper <mailto:craig.topper at gmail.com> for reference....
>
fyi, this was committed right before the release_70 branch was
created, so it will be in the 7.0.0 release.
-Tom
> On Tue, Jul 31, 2018 at 8:02 PM Chandler Carruth via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>
> Author: chandlerc
> Date: Tue Jul 31 20:01:58 2018
> New Revision: 338481
>
> URL: http://llvm.org/viewvc/llvm-project?rev=338481&view=rev
> Log:
> [x86] Fix a really subtle miscompile due to a somewhat glaring bug in
> EFLAGS copy lowering.
>
> If you have a branch of LLVM, you may want to cherrypick this. It is
> extremely unlikely to hit this case empirically, but it will likely
> manifest as an "impossible" branch being taken somewhere, and will be
> ... very hard to debug.
>
> Hitting this requires complex conditions living across complex control
> flow combined with some interesting memory (non-stack) initialized with
> the results of a comparison. Also, because you have to arrange for an
> EFLAGS copy to be in *just* the right place, almost anything you do to
> the code will hide the bug. I was unable to reduce anything remotely
> resembling a "good" test case from the place where I hit it, and so
> instead I have constructed synthetic MIR testing that directly exercises
> the bug in question (as well as the good behavior for completeness).
>
> The issue is that we would mistakenly assume any SETcc with a valid
> condition and an initial operand that was a register and a virtual
> register at that to be a register *defining* SETcc...
>
> It isn't though....
>
> This would in turn cause us to test some other bizarre register,
> typically the base pointer of some memory. Now, testing this register
> and using that to branch on doesn't make any sense. It even fails the
> machine verifier (if you are running it) due to the wrong register
> class. But it will make it through LLVM, assemble, and it *looks*
> fine... But wow do you get a very unsual and surprising branch taken in
> your actual code.
>
> The fix is to actually check what kind of SETcc instruction we're
> dealing with. Because there are a bunch of them, I just test the
> may-store bit in the instruction. I've also added an assert for sanity
> that ensure we are, in fact, *defining* the register operand. =D
>
> Modified:
> llvm/trunk/lib/Target/X86/X86FlagsCopyLowering.cpp
> llvm/trunk/test/CodeGen/X86/flags-copy-lowering.mir
>
> Modified: llvm/trunk/lib/Target/X86/X86FlagsCopyLowering.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86FlagsCopyLowering.cpp?rev=338481&r1=338480&r2=338481&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/X86/X86FlagsCopyLowering.cpp (original)
> +++ llvm/trunk/lib/Target/X86/X86FlagsCopyLowering.cpp Tue Jul 31 20:01:58 2018
> @@ -730,9 +730,12 @@ CondRegArray X86FlagsCopyLoweringPass::c
> for (MachineInstr &MI :
> llvm::reverse(llvm::make_range(MBB.begin(), TestPos))) {
> X86::CondCode Cond = X86::getCondFromSETOpc(MI.getOpcode());
> - if (Cond != X86::COND_INVALID && MI.getOperand(0).isReg() &&
> - TRI->isVirtualRegister(MI.getOperand(0).getReg()))
> + if (Cond != X86::COND_INVALID && !MI.mayStore() && MI.getOperand(0).isReg() &&
> + TRI->isVirtualRegister(MI.getOperand(0).getReg())) {
> + assert(MI.getOperand(0).isDef() &&
> + "A non-storing SETcc should always define a register!");
> CondRegs[Cond] = MI.getOperand(0).getReg();
> + }
>
> // Stop scanning when we see the first definition of the EFLAGS as prior to
> // this we would potentially capture the wrong flag state.
>
> Modified: llvm/trunk/test/CodeGen/X86/flags-copy-lowering.mir
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/flags-copy-lowering.mir?rev=338481&r1=338480&r2=338481&view=diff
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/flags-copy-lowering.mir (original)
> +++ llvm/trunk/test/CodeGen/X86/flags-copy-lowering.mir Tue Jul 31 20:01:58 2018
> @@ -90,6 +90,18 @@
> call void @foo()
> ret i64 0
> }
> +
> + define i32 @test_existing_setcc(i64 %a, i64 %b) {
> + entry:
> + call void @foo()
> + ret i32 0
> + }
> +
> + define i32 @test_existing_setcc_memory(i64 %a, i64 %b) {
> + entry:
> + call void @foo()
> + ret i32 0
> + }
> ...
> ---
> name: test_branch
> @@ -936,3 +948,110 @@ body: |
> ; CHECK: %8:gr64 = CMOVE64rr %0, %1, implicit killed $eflags
>
> ...
> +---
> +name: test_existing_setcc
> +# CHECK-LABEL: name: test_existing_setcc
> +liveins:
> + - { reg: '$rdi', virtual-reg: '%0' }
> + - { reg: '$rsi', virtual-reg: '%1' }
> +body: |
> + bb.0:
> + successors: %bb.1, %bb.2, %bb.3
> + liveins: $rdi, $rsi
> +
> + %0:gr64 = COPY $rdi
> + %1:gr64 = COPY $rsi
> + CMP64rr %0, %1, implicit-def $eflags
> + %2:gr8 = SETAr implicit $eflags
> + %3:gr8 = SETAEr implicit $eflags
> + %4:gr64 = COPY $eflags
> + ; CHECK: CMP64rr %0, %1, implicit-def $eflags
> + ; CHECK-NEXT: %[[A_REG:[^:]*]]:gr8 = SETAr implicit $eflags
> + ; CHECK-NEXT: %[[AE_REG:[^:]*]]:gr8 = SETAEr implicit $eflags
> + ; CHECK-NOT: COPY{{( killed)?}} $eflags
> +
> + ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
> + CALL64pcrel32 @foo, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp, implicit-def $eax
> + ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
> +
> + $eflags = COPY %4
> + JA_1 %bb.1, implicit $eflags
> + JB_1 %bb.2, implicit $eflags
> + JMP_1 %bb.3
> + ; CHECK-NOT: $eflags =
> + ;
> + ; CHECK: TEST8rr %[[A_REG]], %[[A_REG]], implicit-def $eflags
> + ; CHECK-NEXT: JNE_1 %bb.1, implicit killed $eflags
> + ; CHECK-SAME: {{$[[:space:]]}}
> + ; CHECK-NEXT: bb.4:
> + ; CHECK-NEXT: successors: {{.*$}}
> + ; CHECK-SAME: {{$[[:space:]]}}
> + ; CHECK-NEXT: TEST8rr %[[AE_REG]], %[[AE_REG]], implicit-def $eflags
> + ; CHECK-NEXT: JE_1 %bb.2, implicit killed $eflags
> + ; CHECK-NEXT: JMP_1 %bb.3
> +
> + bb.1:
> + %5:gr32 = MOV32ri64 42
> + $eax = COPY %5
> + RET 0, $eax
> +
> + bb.2:
> + %6:gr32 = MOV32ri64 43
> + $eax = COPY %6
> + RET 0, $eax
> +
> + bb.3:
> + %7:gr32 = MOV32r0 implicit-def dead $eflags
> + $eax = COPY %7
> + RET 0, $eax
> +
> +...
> +---
> +name: test_existing_setcc_memory
> +# CHECK-LABEL: name: test_existing_setcc_memory
> +liveins:
> + - { reg: '$rdi', virtual-reg: '%0' }
> + - { reg: '$rsi', virtual-reg: '%1' }
> +body: |
> + bb.0:
> + successors: %bb.1, %bb.2
> + liveins: $rdi, $rsi
> +
> + %0:gr64 = COPY $rdi
> + %1:gr64 = COPY $rsi
> + CMP64rr %0, %1, implicit-def $eflags
> + SETEm %0, 1, $noreg, -16, $noreg, implicit $eflags
> + %2:gr64 = COPY $eflags
> + ; CHECK: CMP64rr %0, %1, implicit-def $eflags
> + ; We cannot reuse this SETE because it stores the flag directly to memory,
> + ; so we have two SETEs here. FIXME: It'd be great if something could fold
> + ; these automatically. If not, maybe we want to unfold SETcc instructions
> + ; writing to memory so we can reuse them.
> + ; CHECK-NEXT: SETEm {{.*}} implicit $eflags
> + ; CHECK-NEXT: %[[E_REG:[^:]*]]:gr8 = SETEr implicit $eflags
> + ; CHECK-NOT: COPY{{( killed)?}} $eflags
> +
> + ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
> + CALL64pcrel32 @foo, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp, implicit-def $eax
> + ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
> +
> + $eflags = COPY %2
> + JE_1 %bb.1, implicit $eflags
> + JMP_1 %bb.2
> + ; CHECK-NOT: $eflags =
> + ;
> + ; CHECK: TEST8rr %[[E_REG]], %[[E_REG]], implicit-def $eflags
> + ; CHECK-NEXT: JNE_1 %bb.1, implicit killed $eflags
> + ; CHECK-NEXT: JMP_1 %bb.2
> +
> + bb.1:
> + %3:gr32 = MOV32ri64 42
> + $eax = COPY %3
> + RET 0, $eax
> +
> + bb.2:
> + %4:gr32 = MOV32ri64 43
> + $eax = COPY %4
> + RET 0, $eax
> +
> +...
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list