Summary: | JSC: fast/js/stack-trace.html fails due to erroneous line number for LLint frame | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | mark.lam, webkit.review.bot | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Mark Lam
2012-08-14 17:27:43 PDT
The issue is not due to getLineNumberForCallFrame() returning the wrong line number as original suspected. getLineNumberForCallFrame() does not have enough context to safely reverse the line number PC. Instead, the llint runtime glue code should adjust the bytecode PC if needed just before it throws an exception. The PC should only be adjusted if we're throwing from a call site. The PC should not be adjusted for exceptions thrown from non-call bytecodes. Created attachment 159464 [details]
Fix.
Created attachment 159468 [details]
rev 1: Removed some unused code.
Comment on attachment 159468 [details] rev 1: Removed some unused code. Attachment 159468 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13543407 Comment on attachment 159468 [details] rev 1: Removed some unused code. Attachment 159468 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13533935 Comment on attachment 159468 [details] rev 1: Removed some unused code. Attachment 159468 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13550027 Created attachment 159505 [details]
rev 2: Made adjustPCIfAtCallSite() conditional under ENABLE(LLINT).
Comment on attachment 159505 [details] rev 2: Made adjustPCIfAtCallSite() conditional under ENABLE(LLINT). View in context: https://bugs.webkit.org/attachment.cgi?id=159505&action=review > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2618 > +#if ENABLE(LLINT) > +Instruction* CodeBlock::adjustPCIfAtCallSite(Instruction* potentialReturnPC) > +{ > + ASSERT(potentialReturnPC); > + > + unsigned returnPCOffset = potentialReturnPC - instructions().begin(); > + Instruction* adjustedPC; > + unsigned opcodeLength; > + > + // If we are at a callsite, the LLInt stores the PC after the call > + // instruction rather than the PC of the call instruction. This requires > + // some correcting. If so, we can rely on the fact that the preceding > + // instruction must be one of the call instructions, so either it's a > + // call_varargs or it's a call, construct, or eval. > + // > + // If we are not at a call site, then we need to guard against the > + // possibility of peeking past the start of the bytecode range for this > + // codeBlock. Hence, we do a bounds check before we peek at the > + // potential "preceding" instruction. > + // The bounds check is done by comparing the offset of the potential > + // returnPC with the length of the opcode. If there is room for a call > + // instruction before the returnPC, then the offset of the returnPC must > + // be greater than the size of the call opcode we're looking for. > + > + // The determination of the call instruction present (if we are at a > + // callsite) depends on the following assumptions. So, assert that > + // they are still true: > + ASSERT(OPCODE_LENGTH(op_call_varargs) <= OPCODE_LENGTH(op_call)); > + ASSERT(OPCODE_LENGTH(op_call) == OPCODE_LENGTH(op_construct)); > + ASSERT(OPCODE_LENGTH(op_call) == OPCODE_LENGTH(op_call_eval)); > + > + // Check for the case of a preceeding op_call_varargs: > + opcodeLength = OPCODE_LENGTH(op_call_varargs); > + adjustedPC = potentialReturnPC - opcodeLength; > + if ((returnPCOffset >= opcodeLength) > + && (adjustedPC->u.pointer == bitwise_cast<void*>(llint_op_call_varargs))) { Does this correctly handle the case where the call site was compiled in something other than llint? Comment on attachment 159505 [details] rev 2: Made adjustPCIfAtCallSite() conditional under ENABLE(LLINT). > Source/JavaScriptCore/llint/LLIntExceptions.cpp:87 > + fixupPCforExceptionIfNeeded(exec); > genericThrow(globalData, exec, globalData->exception, pc - exec->codeBlock()->instructions().begin()); It looks like you're fixing up exec->currentVPC(). But isn't the expression "pc - exec->codeBlock()->instructions().begin()" still using the wrong pc? Comment on attachment 159505 [details] rev 2: Made adjustPCIfAtCallSite() conditional under ENABLE(LLINT). Clearing flags on attachment: 159505 Committed r126093: <http://trac.webkit.org/changeset/126093> All reviewed patches have been landed. Closing bug. |