Bug 94051 – JSC: fast/js/stack-trace.html fails due to erroneous line number for LLint frame

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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)

[Mark Lam]

rdar://problem/12110968

[Mark Lam]

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.

[Mark Lam]

Created attachment 159464 [details]
Fix.

[Mark Lam]

Created attachment 159468 [details]
rev 1: Removed some unused code.

[Gyuyoung Kim]

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

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Mark Lam]

Created attachment 159505 [details]
rev 2: Made adjustPCIfAtCallSite() conditional under ENABLE(LLINT).

[Filip Pizlo]

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?

[Geoffrey Garen]

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?

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.