Bug 149531 - We should only expect a RareCaseProfile to exist if the rare case actually exists
Summary: We should only expect a RareCaseProfile to exist if the rare case actually ex...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-24 10:27 PDT by Mark Lam
Modified: 2015-09-24 11:40 PDT (History)
0 users

See Also:


Attachments
the fix. (6.01 KB, patch)
2015-09-24 10:44 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2015-09-24 10:27:31 PDT
The current code that calls rareCaseProfileForBytecodeOffset() always assumes that it will always return a non-null RareCaseProfile.  As a result, op_add in the baseline JIT is forced to add a dummy slow case that will never be taken, only to ensure that the RareCaseProfile for that bytecode is created, and it will always produce a counter value of 0 (since that path will never be taken).  Instead, we'll make the callers of rareCaseProfileForBytecodeOffset() check if the profile actually exist before dereferencing it.
Comment 1 Mark Lam 2015-09-24 10:44:03 PDT
Created attachment 261875 [details]
the fix.
Comment 2 Saam Barati 2015-09-24 11:25:43 PDT
Comment on attachment 261875 [details]
the fix.

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

r=me with comments.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:3974
> +    auto profile = rareCaseProfileForBytecodeOffset(bytecodeOffset);

nit: Can we write the actual type here?

> Source/JavaScriptCore/jit/JITArithmetic.cpp:797
>      if (!types.first().mightBeNumber() || !types.second().mightBeNumber()) {

We need 32-bit code too.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:826
> +    RELEASE_ASSERT(!(!types.first().mightBeNumber() || !types.second().mightBeNumber()));

I think this reads better as: 
RELEASE_ASSERT(types.first().mightBeNumber() && types.second().mightBeNumber()))
Comment 3 Mark Lam 2015-09-24 11:40:19 PDT
Thanks.  I've fixed the issues before landing.

Landed in r190213: <http://trac.webkit.org/r190213>.