Bug 47839

Summary: JSC interpreter regressions after r69940
Product: WebKit Reporter: Peter Gal <galpeter>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, oliver, ossy, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
patch
ossy: review-, ossy: commit-queue-
Patch for Oszi none

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.