[llvm] r338481 - [x86] Fix a really subtle miscompile due to a somewhat glaring bug in
Craig Topper via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 1 12:04:48 PDT 2018
While I agree that the fix as implemented to check the mayStore flag will
work. It might be more obvious as "MI.getOperand(0).isDef()"
~Craig
On Tue, Jul 31, 2018 at 8:08 PM Chandler Carruth <chandlerc at gmail.com>
wrote:
> +tstellar at redhat.com <tstellar at redhat.com> - We almost certainly want to
> pull this into whatever the next release is. =[
>
> +Craig Topper <craig.topper at gmail.com> for reference....
>
> On Tue, Jul 31, 2018 at 8:02 PM Chandler Carruth via llvm-commits <
> 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
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180801/b030509c/attachment.html>
More information about the llvm-commits
mailing list