[webkit-reviews] review denied: [Bug 118498] ASSERTION FAILED: callFrame == vm->topCallFrame || callFrame == callFrame->lexicalGlobalObject()->globalExec() || callFrame == callFrame->dynamicGlobalObject()->globalExec() in JSC::Interpreter::addStackTraceIfNecessary : [Attachment 206406] patch 3

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Chris <chris_curtis at apple.com>'s
request for review:
Bug 118498: ASSERTION FAILED: callFrame == vm->topCallFrame || callFrame ==
callFrame->lexicalGlobalObject()->globalExec() || callFrame ==
callFrame->dynamicGlobalObject()->globalExec() in
JSC::Interpreter::addStackTraceIfNecessary
https://bugs.webkit.org/show_bug.cgi?id=118498

Attachment 206406: patch 3
https://bugs.webkit.org/attachment.cgi?id=206406&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=206406&action=review


This is pretty cool, but it would be awesome to remove the code duplication.

> Source/JavaScriptCore/jit/JITStubs.cpp:1183
> -template<typename T> static T throwExceptionFromOpCall(JITStackFrame&
jitStackFrame, CallFrame* newCallFrame, ReturnAddressPtr& returnAddressSlot,
JSValue exception)
> +template<typename T> static T throwExceptionFromOpCall(JITStackFrame&
jitStackFrame, CallFrame* newCallFrame, ReturnAddressPtr& returnAddressSlot,
JSObject* (*createError)(ExecState*, JSValue))
>  {
> -    newCallFrame->callerFrame()->vm().exception = exception;
> -    return throwExceptionFromOpCall<T>(jitStackFrame, newCallFrame,
returnAddressSlot);
> +    CallFrame* callFrame = newCallFrame->callerFrame();
> +    jitStackFrame.callFrame = callFrame;
> +    callFrame->vm().topCallFrame = callFrame;
> +    callFrame->vm().exception = (*createError)(callFrame,
callFrame->callee());
> +    ASSERT(callFrame->vm().exception);
> +    returnToThrowTrampoline(&callFrame->vm(),
ReturnAddressPtr(newCallFrame->returnPC()), returnAddressSlot);
> +    return T();
> +}
> +template<typename T> static T throwExceptionFromOpCall(JITStackFrame&
jitStackFrame, CallFrame* newCallFrame, ReturnAddressPtr& returnAddressSlot,
JSObject* (*createError)(ExecState*))
> +{
> +    CallFrame* callFrame = newCallFrame->callerFrame();
> +    jitStackFrame.callFrame = callFrame;
> +    callFrame->vm().topCallFrame = callFrame;
> +    callFrame->vm().exception = (*createError)(callFrame);
> +    ASSERT(callFrame->vm().exception);
> +    returnToThrowTrampoline(&callFrame->vm(),
ReturnAddressPtr(newCallFrame->returnPC()), returnAddressSlot);
> +    return T();
>  }

This is cool.  But, I would recommend doing some more hacking to avoid this
duplicated code.  I think you're duplicating it because of different
createError signatures, right?

It seems like there are two equally decent ways of getting around this.  Both
of them will result in a net increase of code, but IMHO more code in this case
is better than duplicated code, particularly given the gnarly nature of this
code.

1) Create a base class called ErrorCreator like so: class ErrorCreator {
public: virtual JSObject* createError(ExecState*, JSValue); }.	Then have two
subclasses, one that wraps a "JSObject* (*createError)(ExecState*, JSValue)"
and another than wraps a "JSObject* (*createError)(ExecState*)".

2) Do the same but using a functor.  A functor just means that you omit the
base class, and have throwExceptionFromOpCall() be templatized on both T and
the type of the ErrorCreator.  This saves a virtual call and means you don't
have to write the code for the base class, but OTOH it's somewhat more
confusing (both to write and maintain) and leads to code duplication (each
instance of the template will mean the same code splatted again by the
compiler).

> Source/JavaScriptCore/jit/JITStubs.cpp:1478
> +	   return throwExceptionFromOpCall<void*>(stackFrame, callFrame,
STUB_RETURN_ADDRESS, &createStackOverflowError);

It ought not be necessary to say "&" here.  In C and C++, if you refer to a
function by saying "createStackOverflowError" and use it in a function pointer
context, you get the & for free.

Weird, I know - but we regularly omit the "&" when making function pointers.

> Source/JavaScriptCore/jit/JITStubs.cpp:2177
> +	   return throwExceptionFromOpCall<void*>(stackFrame, callFrame,
STUB_RETURN_ADDRESS, &createStackOverflowError);

Ditto.

> Source/JavaScriptCore/jit/JITStubs.cpp:2190
> +	   return throwExceptionFromOpCall<void*>(stackFrame, callFrame,
STUB_RETURN_ADDRESS, &createStackOverflowError);

Ditto.

> Source/JavaScriptCore/jit/JITStubs.cpp:2355
> +	   return throwExceptionFromOpCall<EncodedJSValue>(stackFrame,
callFrame, STUB_RETURN_ADDRESS, &createNotAFunctionError);

Ditto.

> Source/JavaScriptCore/jit/JITStubs.cpp:2481
> +	   return throwExceptionFromOpCall<EncodedJSValue>(stackFrame,
callFrame, STUB_RETURN_ADDRESS, &createNotAConstructorError);

Ditto.