Bug 47839 – JSC interpreter regressions after r69940

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

[Peter Gal]

After the 69940 revision, the interpreter build was broken. That was fixed, but there are still a "few" regressions,
running WebKitTools/Scripts/run-javascriptcore-tests results: 348 regressions found.

This is tested with a Qt build, but I assume that there is no Qt specific code inside the parser. So the regressions are on all platforms.

[Zoltan Herczeg]

Bug:

JSValue result = JSC::resolveBase(callFrame, ident, callFrame->scopeChain(), isStrictPut);
if (!result) {
    callFrame->r(dst) = result;
    ASSERT(callFrame->r(dst).jsValue());
} else
    callFrame->globalData().exception = createErrorForInvalidGlobalAssignment(callFrame, ident.ustring());

"if (!result)" is wrong, because it should test the opposite. Although "if (!!result)" is ok, I refactored the code with "UNLIKELY(!result)".

[Zoltan Herczeg]

Created attachment 71403 [details]
patch

[Csaba Osztrogonác]

Comment on attachment 71403 [details]
patch

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

> JavaScriptCore/interpreter/Interpreter.cpp:292
> -    if (!result) {
> -        callFrame->r(dst) = result;
> -        ASSERT(callFrame->r(dst).jsValue());
> -    } else
> +    if (UNLIKELY(!result)) {
>          callFrame->globalData().exception = createErrorForInvalidGlobalAssignment(callFrame, ident.ustring());
> +        return;
> +    }
> +    callFrame->r(dst) = result;
> +    ASSERT(callFrame->r(dst).jsValue());
>  }

I don't think we need to change the order of cases. 
I prefer only fixing the conditon and add LIKELY 
hint to the compiler, like this:

if (LIKELY(result)) {
    callFrame->r(dst) = result;
    ASSERT(callFrame->r(dst).jsValue());
} else
    callFrame->globalData().exception = createErrorForInvalidGlobalAssignment(callFrame, ident.ustring());

[Zoltan Herczeg]

Created attachment 71414 [details]
Patch for Oszi

I don't like it. Anyway it works.

[Csaba Osztrogonác]

(In reply to comment #4)
> Created an attachment (id=71414) [details]
> Patch for Oszi
> 
> I don't like it. Anyway it works.

I do like it. :)

[WebKit Commit Bot]

Comment on attachment 71414 [details]
Patch for Oszi

Clearing flags on attachment: 71414

Committed r70299: <http://trac.webkit.org/changeset/70299>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.