Bug 22174 – Simplified op_call by nixing its responsibility for moving the value of "this" into the first argument slot

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

[Geoffrey Garen]

Patch coming.

[Geoffrey Garen]

Created attachment 25044 [details]
patch

[Darin Adler]

Comment on attachment 25044 [details]
patch

> +// 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

[Geoffrey Garen]

I'm going to land and then fix up the macro, so Gavin can start resolving conflicts.

[Geoffrey Garen]

Committed revision 38322.

[Geoffrey Garen]

Committed revision 38349.