Bug 22174 - Simplified op_call by nixing its responsibility for moving the value of "this" into the first argument slot
Summary: Simplified op_call by nixing its responsibility for moving the value of "this...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-10 23:04 PST by Geoffrey Garen
Modified: 2008-11-12 16:51 PST (History)
0 users

See Also:


Attachments
patch (53.03 KB, patch)
2008-11-10 23:08 PST, Geoffrey Garen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2008-11-10 23:04:40 PST
Patch coming.
Comment 1 Geoffrey Garen 2008-11-10 23:08:44 PST
Created attachment 25044 [details]
patch
Comment 2 Darin Adler 2008-11-11 08:50:27 PST
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
Comment 3 Geoffrey Garen 2008-11-11 16:24:04 PST
I'm going to land and then fix up the macro, so Gavin can start resolving conflicts.
Comment 4 Geoffrey Garen 2008-11-11 16:32:59 PST
Committed revision 38322.
Comment 5 Geoffrey Garen 2008-11-12 16:51:33 PST
Committed revision 38349.