[llvm] r338481 - [x86] Fix a really subtle miscompile due to a somewhat glaring bug in
Chandler Carruth via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 31 20:01:58 PDT 2018
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
+
+...
More information about the llvm-commits
mailing list