Bug 94051

Summary: JSC: fast/js/stack-trace.html fails due to erroneous line number for LLint frame
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: 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 Flags
Fix.
none
rev 1: Removed some unused code.
gyuyoung.kim: commit-queue-
rev 2: Made adjustPCIfAtCallSite() conditional under ENABLE(LLINT). none

Description Mark Lam 2012-08-14 17:27:43 PDT
When the VM tries to get the stack trace, it will call getLineNumberForCallFrame() in interpreter.cpp.  If the the top frame is a LLint (interpreted) frame, then getLineNumberForCallFrame() will return a wrong line number.  This is because the LLint records the return PC after a call site and not the PC of the call instruction itself.  getLineNumberForCallFrame() needs to take this into account.

This issue causes a failure in webkit test fast/js/stack-trace.html when we disable both the baseline and DFG JITs.

Steps to reproduce:
1. In Options::initialize() (in runtime/Options.cpp), set:

    useJIT() = false;
    useDFGJIT() = false;

    Build JSC and webkit.

2. Run webkit test fast/js/stack-trace.html.  The test will fail.

    [7439/7504] fast/js/stack-trace.html failed unexpectedly (text diff)
Comment 1 Mark Lam 2012-08-16 00:04:58 PDT
rdar://problem/12110968
Comment 2 Mark Lam 2012-08-20 09:14:10 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.
Comment 3 Mark Lam 2012-08-20 10:09:57 PDT
Created attachment 159464 [details]
Fix.
Comment 4 Mark Lam 2012-08-20 10:28:33 PDT
Created attachment 159468 [details]
rev 1: Removed some unused code.
Comment 5 Gyuyoung Kim 2012-08-20 11:05:42 PDT
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 6 Early Warning System Bot 2012-08-20 11:17:00 PDT
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 7 Early Warning System Bot 2012-08-20 11:28:12 PDT
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
Comment 8 Mark Lam 2012-08-20 13:23:57 PDT
Created attachment 159505 [details]
rev 2: Made adjustPCIfAtCallSite() conditional under ENABLE(LLINT).
Comment 9 Filip Pizlo 2012-08-20 15:09:16 PDT
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 10 Geoffrey Garen 2012-08-20 15:28:43 PDT
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 11 WebKit Review Bot 2012-08-20 16:48:15 PDT
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>
Comment 12 WebKit Review Bot 2012-08-20 16:48:19 PDT
All reviewed patches have been landed.  Closing bug.