Bug 47839 - JSC interpreter regressions after r69940
Summary: JSC interpreter regressions after r69940
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-18 11:24 PDT by Peter Gal
Modified: 2010-10-22 03:54 PDT (History)
4 users (show)

See Also:


Attachments
patch (2.13 KB, patch)
2010-10-21 01:23 PDT, Zoltan Herczeg
ossy: review-
ossy: commit-queue-
Details | Formatted Diff | Diff
Patch for Oszi (1.78 KB, patch)
2010-10-21 03:11 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Gal 2010-10-18 11:24:43 PDT
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.
Comment 1 Zoltan Herczeg 2010-10-21 01:13:12 PDT
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)".
Comment 2 Zoltan Herczeg 2010-10-21 01:23:25 PDT
Created attachment 71403 [details]
patch
Comment 3 Csaba Osztrogonác 2010-10-21 01:50:08 PDT
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());
Comment 4 Zoltan Herczeg 2010-10-21 03:11:44 PDT
Created attachment 71414 [details]
Patch for Oszi

I don't like it. Anyway it works.
Comment 5 Csaba Osztrogonác 2010-10-21 04:05:07 PDT
(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. :)
Comment 6 WebKit Commit Bot 2010-10-22 03:54:34 PDT
Comment on attachment 71414 [details]
Patch for Oszi

Clearing flags on attachment: 71414

Committed r70299: <http://trac.webkit.org/changeset/70299>
Comment 7 WebKit Commit Bot 2010-10-22 03:54:40 PDT
All reviewed patches have been landed.  Closing bug.