We have a code like this. 4297 SpeculateInt32Operand op1(this, node->child1()); 4298 SpeculateInt32Operand op2(this, node->child2()); 4299 GPRTemporary result(this, Reuse, op1, op2); 4300 4301 GPRReg gpr1 = op1.gpr(); 4302 GPRReg gpr2 = op2.gpr(); 4303 GPRReg gprResult = result.gpr(); 4304 4305 if (!shouldCheckOverflow(node->arithMode())) 4306 m_jit.add32(gpr1, gpr2, gprResult); 4307 else { 4308 MacroAssembler::Jump check = m_jit.branchAdd32(MacroAssembler::Overflow, gpr1, gpr2, gprResult); 4309 4310 if (gpr1 == gprResult) 4311 speculationCheck(Overflow, JSValueRegs(), 0, check, SpeculationRecovery(SpeculativeAdd, gprResult, gpr2)); 4312 else if (gpr2 == gprResult) 4313 speculationCheck(Overflow, JSValueRegs(), 0, check, SpeculationRecovery(SpeculativeAdd, gprResult, gpr1)); 4314 else 4315 speculationCheck(Overflow, JSValueRegs(), 0, check); And recovery side code is the following. 1114 jit.sub32(recovery->src(), recovery->dest()); If gpr1 = gpr2 = gprResult (this can happen if child1 and child2 are the same and GPRTemporary Reuse works), the result becomes zero accidentally.
We reliably reproduce this with --useDoublePredictionFuzzerAgent=1.
Created attachment 366859 [details] Patch WIP
Created attachment 366865 [details] Patch
Comment on attachment 366865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366865&action=review > Source/JavaScriptCore/ChangeLog:18 > + We do not need to have this mechanism in FTL (because FTL handles registers and stack places better, we do not need to > + have such a peephole optimization), so only DFG has this bug. This is found by injecting Double prediction into value > + profiling. I wouldn't even say this. The FTL would also benefit from this as it alleviates register pressure. Instead, I'd just say you're removing dead code. > Source/JavaScriptCore/dfg/DFGOSRExit.h:62 > + SpeculativeAddSelf, Can't this just be detected by asking if src() == dst()?
Comment on attachment 366865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366865&action=review > Source/JavaScriptCore/dfg/DFGOSRExit.cpp:1129 > + // If A + A = A (int32_t) overflows, A can be recovereted by ((static_cast<int32_t>(A) >> 1) ^ 0x8000000). typo: "recovereted" => "recovered"
Comment on attachment 366865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366865&action=review >> Source/JavaScriptCore/ChangeLog:18 >> + profiling. > > I wouldn't even say this. The FTL would also benefit from this as it alleviates register pressure. > > Instead, I'd just say you're removing dead code. OK, I'll rephrase this part. >> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:1129 >> + // If A + A = A (int32_t) overflows, A can be recovereted by ((static_cast<int32_t>(A) >> 1) ^ 0x8000000). > > typo: "recovereted" => "recovered" Fixed. >> Source/JavaScriptCore/dfg/DFGOSRExit.h:62 >> + SpeculativeAddSelf, > > Can't this just be detected by asking if src() == dst()? We can detect it, but I think having another type seems cleaner given that we have two types SpeculativeAdd and SpeculativeAddImmediate.
Comment on attachment 366865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366865&action=review >>> Source/JavaScriptCore/dfg/DFGOSRExit.h:62 >>> + SpeculativeAddSelf, >> >> Can't this just be detected by asking if src() == dst()? > > We can detect it, but I think having another type seems cleaner given that we have two types SpeculativeAdd and SpeculativeAddImmediate. Kind of. SpeculativeAdd just means A and B are registers. It doesn't care what registers are. Immediate makes more sense because it means we don't need a register for the purposes of recovery.
(In reply to Saam Barati from comment #7) > Comment on attachment 366865 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=366865&action=review > > >>> Source/JavaScriptCore/dfg/DFGOSRExit.h:62 > >>> + SpeculativeAddSelf, > >> > >> Can't this just be detected by asking if src() == dst()? > > > > We can detect it, but I think having another type seems cleaner given that we have two types SpeculativeAdd and SpeculativeAddImmediate. > > Kind of. SpeculativeAdd just means A and B are registers. It doesn't care > what registers are. Immediate makes more sense because it means we don't > need a register for the purposes of recovery. Anyways, I don't have a super strong opinion here. I'll let you decide what you prefer.
Comment on attachment 366865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366865&action=review >>>>> Source/JavaScriptCore/dfg/DFGOSRExit.h:62 >>>>> + SpeculativeAddSelf, >>>> >>>> Can't this just be detected by asking if src() == dst()? >>> >>> We can detect it, but I think having another type seems cleaner given that we have two types SpeculativeAdd and SpeculativeAddImmediate. >> >> Kind of. SpeculativeAdd just means A and B are registers. It doesn't care what registers are. Immediate makes more sense because it means we don't need a register for the purposes of recovery. > > Anyways, I don't have a super strong opinion here. I'll let you decide what you prefer. Personally, I like having a separate types here because these types are corresponding to the way how to recover the value from the given arguments. By having separate types, I think our recovery side code can be simple, which has the `switch` on how to recover the value, and each clause maintains the recovery method corresponding to this type :)
Committed r243959: <https://trac.webkit.org/changeset/243959>
<rdar://problem/49664247>