Modified: trunk/Source/_javascript_Core/ChangeLog (195237 => 195238)
--- trunk/Source/_javascript_Core/ChangeLog 2016-01-18 21:27:53 UTC (rev 195237)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-01-18 22:15:30 UTC (rev 195238)
@@ -1,3 +1,38 @@
+2016-01-18 Saam barati <[email protected]>
+
+ FTL doesn't do proper spilling for exception handling when GetById/Snippets go to slow path
+ https://bugs.webkit.org/show_bug.cgi?id=153186
+
+ Reviewed by Michael Saboff.
+
+ Michael was investigating a bug he found while doing the new JSC calling
+ convention work and it turns out to be a latent bug in FTL try/catch machinery.
+ After I looked at the code again, I realized that what I had previously
+ written is wrong in a subtle way. The FTL callOperation machinery will remove
+ its result register from the set of registers it needs to spill. This is not
+ correct when we have try/catch. We may want to do value recovery on
+ the value that the result register is prior to the call after the call
+ throws an exception. The case that we were solving before was when the
+ resultRegister == baseRegister in a GetById, or left/rightRegister == resultRegister in a Snippet.
+ This code is correct in wanting to spill in that case, even though it might spill
+ when we don't need it to (i.e the result is not needed for value recovery). Once I
+ investigated this bug further, I realized that the previous rule is just a
+ partial subset of the rule that says we should spill anytime the result is
+ a register we might do value recovery on. This patch implements the rule that
+ says we always want to spill the result when we will do value recovery on it
+ if an exception is thrown.
+
+ * ftl/FTLCompile.cpp:
+ (JSC::FTL::mmAllocateDataSection):
+ * tests/stress/ftl-try-catch-getter-throw-interesting-value-recovery.js: Added.
+ (assert):
+ (random):
+ (identity):
+ (let.o2.get f):
+ (let.o3.get f):
+ (foo):
+ (i.else):
+
2016-01-18 Konstantin Tokarev <[email protected]>
[MIPS] LLInt: fix calculation of Global Offset Table
Modified: trunk/Source/_javascript_Core/ftl/FTLCompile.cpp (195237 => 195238)
--- trunk/Source/_javascript_Core/ftl/FTLCompile.cpp 2016-01-18 21:27:53 UTC (rev 195237)
+++ trunk/Source/_javascript_Core/ftl/FTLCompile.cpp 2016-01-18 22:15:30 UTC (rev 195238)
@@ -555,16 +555,10 @@
// be preserved to the stack before the call, that way the OSR exit
// exception handler can recover them into the proper registers.
exit.gatherRegistersToSpillForCallIfException(stackmaps, record);
- } else if (exitDescriptorImpl.m_exceptionType == ExceptionType::GetById) {
+ } else if (exitDescriptorImpl.m_exceptionType == ExceptionType::GetById || exitDescriptorImpl.m_exceptionType == ExceptionType::BinaryOpGenerator) {
GPRReg result = record.locations[0].directGPR();
- GPRReg base = record.locations[1].directGPR();
- if (base == result)
- callOperationExit->registersToPreserveForCallThatMightThrow.set(base);
- } else if (exitDescriptorImpl.m_exceptionType == ExceptionType::BinaryOpGenerator) {
- GPRReg result = record.locations[0].directGPR();
- GPRReg left = record.locations[1].directGPR();
- GPRReg right = record.locations[2].directGPR();
- if (result == left || result == right)
+ RegisterSet registersNeededForOSRExit = record.usedRegisterSet();
+ if (registersNeededForOSRExit.get(result))
callOperationExit->registersToPreserveForCallThatMightThrow.set(result);
}
}
@@ -664,19 +658,24 @@
bool addedUniqueExceptionJump = addNewExceptionJumpIfNecessary(iter->value[i].index);
MacroAssembler::Label begin = slowPathJIT.label();
- if (result == base) {
- // This situation has a really interesting story. We may have a GetById inside
- // a try block where LLVM assigns the result and the base to the same register.
- // The inline cache may miss and we may end up at this slow path callOperation.
- // Then, suppose the base and the result are both the same register, so the return
- // value of the C call gets stored into the original base register. If the operationGetByIdOptimize
- // throws, it will return "undefined" and we will be stuck with "undefined" in the base
- // register that we would like to do value recovery on. We combat this situation from ever
- // taking place by ensuring we spill the original base value and then recover it from
- // the spill slot as the first step in OSR exit.
- if (OSRExit* exit = exceptionHandlerManager.callOperationOSRExit(iter->value[i].index))
- exit->spillRegistersToSpillSlot(slowPathJIT, osrExitFromGenericUnwindStackSpillSlot);
- }
+ // We take care to always spill the result whenever we need to do value recovery
+ // on it in the OSR exit. This is because the callOperation(.) machinery doesn't
+ // ever spill the result value. It actually takes care to never spill the result
+ // because it overwrites it with the result of the call. But, with exceptions and
+ // OSR exits, we may need the result value prior to the call during OSR exit.
+ // We take care to mark it for spillage now.
+ //
+ // This also handles another really interesting register preservation story:
+ // We may have a GetById/Snippet inside a try block where LLVM assigns the result
+ // and the base to the same register. The inline cache may miss and we may end up
+ // at this slow path callOperation. Then, because the base and the result are both
+ // the same register, the return value of the C call gets stored into the original
+ // base register. If the operationGetByIdOptimize throws, it will return "undefined"
+ // and we will be stuck with "undefined" in the base register that we would like to
+ // do value recovery on. We combat this situation from ever taking place by ensuring
+ // we spill the original base value (i.e the result register).
+ if (OSRExit* exit = exceptionHandlerManager.callOperationOSRExit(iter->value[i].index))
+ exit->spillRegistersToSpillSlot(slowPathJIT, osrExitFromGenericUnwindStackSpillSlot);
MacroAssembler::Call call = callOperation(
state, usedRegisters, slowPathJIT, codeOrigin, addedUniqueExceptionJump ? &exceptionJumpsToLink.last().first : &exceptionTarget,
operationGetByIdOptimize, result, CCallHelpers::TrustedImmPtr(gen.stubInfo()),
@@ -718,6 +717,9 @@
MacroAssembler::Label begin = slowPathJIT.label();
+ if (OSRExit* exit = exceptionHandlerManager.callOperationOSRExit(iter->value[i].index))
+ exit->spillRegistersToSpillSlot(slowPathJIT, osrExitFromGenericUnwindStackSpillSlot);
+
MacroAssembler::Call call = callOperation(
state, usedRegisters, slowPathJIT, codeOrigin, addedUniqueExceptionJump ? &exceptionJumpsToLink.last().first : &exceptionTarget,
gen.slowPathFunction(), InvalidGPRReg,
@@ -790,12 +792,10 @@
binaryOp.m_slowPathStarts.append(slowPathJIT.label());
bool addedUniqueExceptionJump = addNewExceptionJumpIfNecessary(iter->value[i].index);
- if (result == left || result == right) {
- // This situation has a really interesting register preservation story.
- // See comment above for GetByIds.
- if (OSRExit* exit = exceptionHandlerManager.callOperationOSRExit(iter->value[i].index))
- exit->spillRegistersToSpillSlot(slowPathJIT, osrExitFromGenericUnwindStackSpillSlot);
- }
+ // This situation has a really interesting register preservation story.
+ // See comment above for GetByIds.
+ if (OSRExit* exit = exceptionHandlerManager.callOperationOSRExit(iter->value[i].index))
+ exit->spillRegistersToSpillSlot(slowPathJIT, osrExitFromGenericUnwindStackSpillSlot);
callOperation(state, usedRegisters, slowPathJIT, codeOrigin, addedUniqueExceptionJump ? &exceptionJumpsToLink.last().first : &exceptionTarget,
binaryOp.slowPathFunction(), result, left, right).call();
Added: trunk/Source/_javascript_Core/tests/stress/ftl-try-catch-getter-throw-interesting-value-recovery.js (0 => 195238)
--- trunk/Source/_javascript_Core/tests/stress/ftl-try-catch-getter-throw-interesting-value-recovery.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/ftl-try-catch-getter-throw-interesting-value-recovery.js 2016-01-18 22:15:30 UTC (rev 195238)
@@ -0,0 +1,65 @@
+function assert(b) {
+ if (!b)
+ throw new Error("bad value")
+}
+noInline(assert);
+
+function random() {
+ return "blah";
+}
+noInline(random);
+
+function identity(x) {
+ return x;
+}
+noInline(identity);
+
+let o1 = {
+ g: 20,
+ y: 40,
+ f: "get f"
+};
+
+let o2 = {
+ g: "g",
+ y: "y",
+ get f() {
+ return "get f";
+ }
+}
+
+let o4 = {};
+
+let o3 = {
+ get f() {
+ throw new Error("blah");
+ }
+}
+
+function foo(o, a) {
+ let x = o.g;
+ let y = o.y;
+ let oo = identity(o);
+ let j = random();
+ try {
+ j = o.f;
+ } catch(e) {
+ assert(j === "blah");
+ assert(oo === o3);
+ return x + y + 1;
+ }
+ return x + y;
+}
+
+noInline(foo);
+for (let i = 0; i < 100000; i++) {
+ if (i % 3 == 0) {
+ assert(foo(o1) === 60);
+ } else if (i % 3 === 1) {
+ assert(foo(o2) === "gy");
+ } else {
+ foo(o4);
+ }
+}
+
+foo(o3);
