Bug 66571

Summary: Keep track of topCallFrame for Stack traces
Product: WebKit Reporter: Juan C. Montemayor <j.mont>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ggaren, j.mont, oliver, slewis, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 66956    
Bug Blocks:    
Attachments:
Description Flags
Proposed patch
none
updated patch
none
updated patch
none
updated arm-ready patch
none
Complete results of performance tests made on this patch
none
sunspider test results
none
v8 results (outside web browser)
none
v8 results (in web browser)
none
kraken results
none
new and improved patch
none
patch
none
new perf test results none

Description Juan C. Montemayor 2011-08-19 11:37:15 PDT
This is the first patch of two needed to implement stack traces in JSC. This patch adds a TopCallFrame to JSC in order to have that information when an error is thrown to create a stack trace.
Comment 1 Juan C. Montemayor 2011-08-19 11:44:22 PDT
Created attachment 104538 [details]
Proposed patch
Comment 2 Geoffrey Garen 2011-08-19 12:53:29 PDT
Comment on attachment 104538 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104538&action=review

> Source/JavaScriptCore/runtime/JSObject.h:433
>  inline JSObject* asObject(JSValue value)
>  {
> +    if (!value.isCell())
> +        return 0;
>      return asObject(value.asCell());

This change is no good. No code should call asObject on a value if the value is not a cell (and therefore not an object). It's akin to a bad C++ cast.

Is this change required to make your patch work?
Comment 3 Juan C. Montemayor 2011-08-19 13:27:02 PDT
> This change is no good. No code should call asObject on a value if the value is not a cell (and therefore not an object). It's akin to a bad C++ cast.
> 
> Is this change required to make your patch work?

Let me re-check....
Comment 4 Juan C. Montemayor 2011-08-19 14:00:14 PDT
Created attachment 104559 [details]
updated patch

I removed the extra code and one useless newline. You were right, the change was not required to make my patch work
Comment 5 Darin Adler 2011-08-19 16:09:41 PDT
Comment on attachment 104559 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104559&action=review

> Source/JavaScriptCore/interpreter/CallFrame.h:170
> +        : globalData(global)
> +        , oldCallFrame(global->topCallFrame) 

Formatting mistake. These lines should be indented four more spaces.

> Source/JavaScriptCore/interpreter/CallFrame.h:183
> +    };
>  } // namespace JSC

Formatting mistake. There should be a blank line here.
Comment 6 Darin Adler 2011-08-19 16:11:53 PDT
Comment on attachment 104559 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104559&action=review

> Source/JavaScriptCore/interpreter/CallFrame.h:166
> +    class TopCallFrame {

I don’t think the class name here is right. It should be something like TopCallFrameSetter.

Since this class is only used in three places in Interpreter.cpp I think it shouldn’t go into CallFrame.h. It can go into Interpreter.cpp.

> Source/JavaScriptCore/interpreter/CallFrame.h:168
> +        TopCallFrame(JSGlobalData* global, CallFrame* callFrame)

I suggest taking a reference to global data instead of a pointer.
Comment 7 Juan C. Montemayor 2011-08-19 16:37:47 PDT
Created attachment 104592 [details]
updated patch

Thanks for the feedback Darin\!
Comment 8 Juan C. Montemayor 2011-08-19 16:39:58 PDT
Geoff: I tested my patch in both 64 and 32 bit, since I changed the patchOffsetGetByIdSlowCaseCall. Are there any other architectures that I should test it on?
Comment 9 Juan C. Montemayor 2011-08-22 13:37:00 PDT
Comment on attachment 104592 [details]
updated patch

getting weird results with performance tests... I have to re-check them
Comment 10 Geoffrey Garen 2011-08-22 13:55:06 PDT
You'll need to update the offsets for ARM as well. Gavin can help you with this.
Comment 11 Geoffrey Garen 2011-08-22 15:14:49 PDT
Comment on attachment 104592 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104592&action=review

Some of the setting and resetting of topCallFrame seems haphazard in the Interpreter.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:711
>      CodeBlock* codeBlock = callFrame->codeBlock();
>      bool isInterrupt = false;
> +    callFrame->globalData().topCallFrame = callFrame;

Why does the first act of throwing an exception need to be to set topCallFrame? In theory, topCallFrame was already set when we entered the function that is now throwing an exception.

On the other hand, I think you're missing an update of topCallFrame after you've thrown an exception and decided to return to a handler. You can put the update into unwindCallFrame().

> Source/JavaScriptCore/interpreter/Interpreter.cpp:934
>  JSValue Interpreter::executeCall(CallFrame* callFrame, JSObject* function, CallType callType, const CallData& callData, JSValue thisValue, const ArgList& args)
>  {
> +    callFrame->globalData().topCallFrame = callFrame;

Why here? Seems unnecessary.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:977
>  JSValue Interpreter::executeCall(CallFra
>          }
>  
>          newCallFrame->init(newCodeBlock, 0, callDataScopeChain, callFrame->addHostCallFrameFlag(), argCount, function);
> +        TopCallFrameSetter topCallFrameSetter(callFrame->globalData(), newCallFrame);

Here, you caught the CallTypeJS case, but missed the non-JS case.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1030
>  JSObject* Interpreter::executeConstruct(CallFrame* callFrame, JSObject* constructor, ConstructType constructType, const ConstructData& constructData, const ArgList& args)
>  {
> +    callFrame->globalData().topCallFrame = callFrame;

Why here? Seems unnecessary.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1055
> +    TopCallFrameSetter topCallFrameSetter(callFrame->globalData(), newCallFrame);
> +

This is too soon, since newCallFrame is not yet fully initialized, and can change later in the function.

A good rule of thumb is to update topCallFrame right after calling CallFrame::init, since that means you've just created a new CallFrame and fully initialized it.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1217
>  JSValue Interpreter::execute(EvalExecutable* eval, CallFrame* callFrame, JSValue thisValue, ScopeChainNode* scopeChain)
>  {
> +    callFrame->globalData().topCallFrame = callFrame;

Why here? Seems unnecessary.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1227
>  JSValue Interpreter::execute(EvalExecutable* eval, CallFrame* callFrame, JSValue thisValue, int globalRegisterOffset, ScopeChainNode* scopeChain)
>  {
>      ASSERT(isValidThisObject(thisValue, callFrame));
> +    callFrame->globalData().topCallFrame = callFrame;

Why here? Seems unnecessary.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1658
>  #endif
>  
> +    *topCallFrameSlot = callFrame;
>  #if ENABLE(COMPUTED_GOTO_INTERPRETER)

Why here? Seems unnecessary.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4188
>                  goto vm_throw;
>              functionReturnValue = result;
>  
> +            *topCallFrameSlot = callFrame;

Why here? callEval() should restore topCallFrame for you.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4257
> +            *topCallFrameSlot = newCallFrame;
>              newCallFrame->init(0, vPC + OPCODE_LENGTH(op_call), scopeChain, callFrame, argCount, asObject(v));

Slightly better to switch these two lines of code, so topCallFrame doesn't point at newCallFrame until newCallFrame is in a valid state.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4427
> +            *topCallFrameSlot = newCallFrame;
>              newCallFrame->init(0, vPC + OPCODE_LENGTH(op_call_varargs), scopeChain, callFrame, argCount, asObject(v));

Same comment here.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4438
> +            *topCallFrameSlot = callFrame;

This needs to happen before CHECK_FOR_EXCEPTION(), to ensure that you put things back into a valid state before jumping somewhere else.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4518
> +        *topCallFrameSlot = callFrame;
>          if (callFrame->hasHostCallFrameFlag())
>              return returnValue;

Slightly better to change around the order of these two statements, since you don't need to update topCallFrame if you're returning back to C++. (The C++ code should update for you.)

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4761
>              CHECK_FOR_EXCEPTION();
>              functionReturnValue = returnValue;
>  
> +            *topCallFrameSlot = callFrame;

Update needs to happen before CHECK_FOR_EXCEPTION.

You missed the initial setting of topCallFrame in the ConstructTypeHost case here.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4940
> +        *topCallFrameSlot = callFrame;

Why here? Seems unnecessary, if exception unwinding does its job correctly.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4964
>          codeBlock = callFrame->codeBlock();
> +        *topCallFrameSlot = callFrame;
>          vPC = codeBlock->instructions().begin() + handler->target;

If you fix throwException to update topCallFrame correctly, this line shouldn't be necessary.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:5124
> +        *topCallFrameSlot = callFrame;
>          vPC = codeBlock->instructions().begin() + handler->target;

Same comment here.
Comment 12 Juan C. Montemayor 2011-08-22 16:42:34 PDT
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:711
> >      CodeBlock* codeBlock = callFrame->codeBlock();
> >      bool isInterrupt = false;
> > +    callFrame->globalData().topCallFrame = callFrame;
> 
> Why does the first act of throwing an exception need to be to set topCallFrame? 

in throwException(), addErrorInfo() needs the updated callFrame to work properly. I update the topCallFrame before the first if statement to set it in case it branches.

> On the other hand, I think you're missing an update of topCallFrame after you've thrown an exception and decided to return to a handler. You can put the update into unwindCallFrame().

If I only update the callFrame inside unwindCallFrame, my code would fail jsc-tests. As I mentioned before, addErrorInfo() needs the updated callFrame to work, and addErrorInfo() is called before unwindCallFrame()

Should I keep it like this or what would be the right course of action?
Comment 13 Geoffrey Garen 2011-08-22 17:36:59 PDT
(In reply to comment #12)
> > > Source/JavaScriptCore/interpreter/Interpreter.cpp:711
> > >      CodeBlock* codeBlock = callFrame->codeBlock();
> > >      bool isInterrupt = false;
> > > +    callFrame->globalData().topCallFrame = callFrame;
> > 
> > Why does the first act of throwing an exception need to be to set topCallFrame? 
> 
> in throwException(), addErrorInfo() needs the updated callFrame to work properly. I update the topCallFrame before the first if statement to set it in case it branches.

If you saw cases where addErrorInfo() saw the wrong callFrame, perhaps they were caused by some of the cases of missing updates I listed. If you do the job right, addErrorInfo, like all C++ code, should always have a coherent view of topCallFrame.

> > On the other hand, I think you're missing an update of topCallFrame after you've thrown an exception and decided to return to a handler. You can put the update into unwindCallFrame().
> 
> If I only update the callFrame inside unwindCallFrame, my code would fail jsc-tests. As I mentioned before, addErrorInfo() needs the updated callFrame to work, and addErrorInfo() is called before unwindCallFrame()

The update at the top of Interpreter::throwException will not make topCallFrame correct after you've thrown an exception and decided to return to a handler. Consider this case:

(function () { try { (function() { throw "error"; })(); } catch(exception) { // topCallFrame is now wrong. } })();

topCallFrame needs to point to the callFrame of the exception handler, which can be zero or more frames up the stack from the frame that throws the exception.
Comment 14 Juan C. Montemayor 2011-08-23 12:51:15 PDT
Created attachment 104894 [details]
updated arm-ready patch

I added the changes suggested by Geoff. I decided to update both in unwindCallFrame() and when an exception is thrown. I also added some asserts where I removed updates to topCallFrame just to make sure that the callframes at those points were correct. This patch has also been modified to work with arm processors. Hopefully I didn't miss anything
Comment 15 Juan C. Montemayor 2011-08-24 11:09:19 PDT
Created attachment 105022 [details]
Complete results of performance tests made on this patch
Comment 16 Juan C. Montemayor 2011-08-24 11:10:09 PDT
Created attachment 105023 [details]
sunspider test results
Comment 17 Juan C. Montemayor 2011-08-24 11:10:52 PDT
Created attachment 105024 [details]
v8 results (outside web browser)
Comment 18 Juan C. Montemayor 2011-08-24 11:11:28 PDT
Created attachment 105025 [details]
v8 results (in web browser)
Comment 19 Juan C. Montemayor 2011-08-24 11:12:15 PDT
Created attachment 105026 [details]
kraken results
Comment 20 Juan C. Montemayor 2011-08-24 11:26:42 PDT
Created attachment 105028 [details]
new and improved patch

Removed some assert statements. I included some perfromance test results that I ran yesterday/today. It looks like no measurable speed regression was introduced by this patch.
Comment 21 Juan C. Montemayor 2011-08-24 14:57:12 PDT
Created attachment 105076 [details]
patch

added one more TopCallFrameSetter in execute(CallFrameClosure) to assure that we are updating and resetting topCallFrame before and after we execute arbitrary js code that can change our topCallFrame
Comment 22 Juan C. Montemayor 2011-08-24 14:57:55 PDT
performance figures are looking very similar to the ones I posted. I'll keep running them and upload the new ones.
Comment 23 Juan C. Montemayor 2011-08-24 15:38:28 PDT
Created attachment 105085 [details]
new perf test results
Comment 24 Geoffrey Garen 2011-08-24 17:54:18 PDT
Comment on attachment 105076 [details]
patch

r=me
Comment 25 WebKit Review Bot 2011-08-24 18:25:51 PDT
Comment on attachment 105076 [details]
patch

Clearing flags on attachment: 105076

Committed r93755: <http://trac.webkit.org/changeset/93755>
Comment 26 WebKit Review Bot 2011-08-24 18:25:57 PDT
All reviewed patches have been landed.  Closing bug.