Changeset View
Changeset View
Standalone View
Standalone View
devel/llvm60/files/patch-head-r337615.diff
- This file was added.
| Property | Old Value | New Value |
|---|---|---|
| fbsd:nokeywords | null | yes \ No newline at end of property |
| svn:eol-style | null | native \ No newline at end of property |
| svn:mime-type | null | text/plain \ No newline at end of property |
| r337615 | dim | 2018-08-11 12:42:12 +0200 (Sat, 11 Aug 2018) | 43 lines | |||||
| Pull in r338481 from upstream llvm trunk (by Chandler Carruth): | |||||
| [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 | |||||
| Noticed by: kib | |||||
| MFC after: 1 week | |||||
| Index: lib/Target/X86/X86FlagsCopyLowering.cpp | |||||
| =================================================================== | |||||
| --- lib/Target/X86/X86FlagsCopyLowering.cpp (revision 337614) | |||||
| +++ lib/Target/X86/X86FlagsCopyLowering.cpp (revision 337615) | |||||
| @@ -608,9 +608,12 @@ X86FlagsCopyLoweringPass::collectCondsInRegs(Machi | |||||
| for (MachineInstr &MI : llvm::reverse( | |||||
| llvm::make_range(MBB.instr_begin(), CopyDefI.getIterator()))) { | |||||
| 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. | |||||