[webkit-reviews] review granted: [Bug 22174] Simplified op_call by nixing its responsibility for moving the value of "this" into the first argument slot : [Attachment 25044] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 's request for review:
Bug 22174: Simplified op_call by nixing its responsibility for moving the value
of "this" into the first argument slot
https://bugs.webkit.org/show_bug.cgi?id=22174

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +// This macro rewinds to the previous call frame because CTI functions that
> +// throw stack overflow exceptions execute after the call frame has
> +// optimistically moved forward.
> +#define CTI_THROW_STACK_OVERFLOW() do { \
> +    CallFrame* oldCallFrame = ARG_callFrame->callerFrame(); \
> +    JSGlobalData* globalData = ARG_globalData; \
> +    globalData->exception = createStackOverflowError(oldCallFrame); \
> +    globalData->throwReturnAddress = CTI_RETURN_ADDRESS; \
> +    ARG_setCallFrame(oldCallFrame); \
> +    CTI_SET_RETURN_ADDRESS(reinterpret_cast<void*>(ctiVMThrowTrampoline)); \

> +} while (0);

I understand why we want to use a macro here so we can get at the ARG fields,
but I think we should put as much of this work as possible into a function. For
one thing, macros are harder to read and maintain than functions because of all
the backslashes and special rules about identifiers and. For another, since
this is an exceptional case it's nice to keep the code out of the way in a
separate function so it has minimal performance impact when not executed. It
looks like the existing code was using a separate function, but you eliminated
it. Would you reconsider?

> +	   * fast/js/global-recursion-on-full-stack-expected.txt: This test
passes
> +	   a little differently now, because the register layout has changed.

This comment seems vague, perhaps intentionally so. Why specifically don't we
get call stack size exception logged now?

r=me