[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
Wed Aug 1 17:01:48 PDT 2018
I mean, that's what I assert after the check. I'm happy to switch these
around whichever way you think would be most clear. I leaned towards
checking the store flag because what I really want is to check the
*instruction* being used, not just whether we've set a particular flag on a
particular operand... WDYT though? I don't feel strongly here either way.
On Wed, Aug 1, 2018 at 12:05 PM Craig Topper via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> 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
>>>
>> _______________________________________________
> 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/9d734e7a/attachment.html>
More information about the llvm-commits
mailing list