Bug 136103

Summary: [Win] Assertion fails when running JSC stress tests.
Product: WebKit Reporter: peavo
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue, darin, fpizlo, mark.lam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description peavo 2014-08-20 08:39:59 PDT
When I run the JSC stress tests on Windows, I get a few asserts in the function specializationKindFor in the InlineCallFrame struct (CodeOrigin.h).
This happens because the kind parameter has the value -2.

    static CodeSpecializationKind specializationKindFor(Kind kind)
    {
        switch (kind) {
        case Call:
        case GetterCall:
        case SetterCall:
            return CodeForCall;
        case Construct:
            return CodeForConstruct;
        }
        RELEASE_ASSERT_NOT_REACHED();
        return CodeForCall;
    }

Since enums are signed on Windows, and only 2 bits are allocated for the Kind member in the InlineCallFrame struct, the values we can represent are -2, -1, 0, 1.
We should use an extra bit in the bitfield, so we also can represent the values 2, and 3. 

See http://rogerkarlsson.com/blogs/programming/enum-bit-field-visual-studio/ for a similar case.
Comment 1 peavo 2014-08-20 09:38:02 PDT
Created attachment 236873 [details]
Patch
Comment 2 Darin Adler 2014-08-20 09:55:59 PDT
Comment on attachment 236873 [details]
Patch

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

> Source/JavaScriptCore/bytecode/CodeOrigin.h:143
> +    static CodeSpecializationKind specializationKindFor(unsigned kind)

This change should not be necessary. What happens when you don’t change this? Some kind of failure to compile?

> Source/JavaScriptCore/bytecode/CodeOrigin.h:163
> +    unsigned kind : 2;

I believe our WebKit coding style pattern in cases like this is to put a comment off to the right with the name of the enum type. See examples like the ones in DFGNode.h, RenderObject.h, StyleRareInheritedData.h, and StyleRareNonInheritedData.h.
Comment 3 peavo 2014-08-20 10:03:12 PDT
Created attachment 236874 [details]
Patch
Comment 4 Darin Adler 2014-08-20 10:04:15 PDT
Comment on attachment 236874 [details]
Patch

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

> Source/JavaScriptCore/bytecode/CodeOrigin.h:143
> -    static CodeSpecializationKind specializationKindFor(Kind kind)
> +    static CodeSpecializationKind specializationKindFor(unsigned kind)

This change should not be necessary. What happens when you don’t change this? Some kind of failure to compile?
Comment 5 Darin Adler 2014-08-20 10:04:45 PDT
Comment on attachment 236874 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/CodeOrigin.h:143
>> +    static CodeSpecializationKind specializationKindFor(unsigned kind)
> 
> This change should not be necessary. What happens when you don’t change this? Some kind of failure to compile?

There is likely a better way to fix this than to remove the type here.
Comment 6 peavo 2014-08-20 10:07:44 PDT
(In reply to comment #5)
> (From update of attachment 236874 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236874&action=review
> 
> >> Source/JavaScriptCore/bytecode/CodeOrigin.h:143
> >> +    static CodeSpecializationKind specializationKindFor(unsigned kind)
> > 
> > This change should not be necessary. What happens when you don’t change this? Some kind of failure to compile?
> 
> There is likely a better way to fix this than to remove the type here.

MSVC was not able to convert from unsigned to the Kind enum type.
Maybe replace with a static_cast when calling the function instead?
Comment 7 Mark Lam 2014-08-20 10:13:20 PDT
Comment on attachment 236874 [details]
Patch

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

>>>> Source/JavaScriptCore/bytecode/CodeOrigin.h:143
>>>> +    static CodeSpecializationKind specializationKindFor(unsigned kind)
>>> 
>>> This change should not be necessary. What happens when you don’t change this? Some kind of failure to compile?
>> 
>> There is likely a better way to fix this than to remove the type here.
> 
> MSVC was not able to convert from unsigned to the Kind enum type.
> Maybe replace with a static_cast when calling the function instead?

I see that we’re now using enum classes everywhere.  How about using “enum class Kind : unsigned”?
Comment 8 Darin Adler 2014-08-20 10:16:39 PDT
Comment on attachment 236874 [details]
Patch

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

>>>>> Source/JavaScriptCore/bytecode/CodeOrigin.h:143
>>>>> +    static CodeSpecializationKind specializationKindFor(unsigned kind)
>>>> 
>>>> This change should not be necessary. What happens when you don’t change this? Some kind of failure to compile?
>>> 
>>> There is likely a better way to fix this than to remove the type here.
>> 
>> MSVC was not able to convert from unsigned to the Kind enum type.
>> Maybe replace with a static_cast when calling the function instead?
> 
> I see that we’re now using enum classes everywhere.  How about using “enum class Kind : unsigned”?

Yes, we should put the static_cast into the specializationKind() function; note that the two other callers in DFGByteCodeParser.cpp would not require a static_cast. Also seems fine to try Mark’s suggestion of moving to an enum class, but I think that will lead to even more changes and I am not sure it’s worth it.
Comment 9 peavo 2014-08-20 10:27:45 PDT
Created attachment 236876 [details]
Patch
Comment 10 peavo 2014-08-20 10:28:18 PDT
(In reply to comment #8)
> (From update of attachment 236874 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236874&action=review
> 
> >>>>> Source/JavaScriptCore/bytecode/CodeOrigin.h:143
> >>>>> +    static CodeSpecializationKind specializationKindFor(unsigned kind)
> >>>> 
> >>>> This change should not be necessary. What happens when you don’t change this? Some kind of failure to compile?
> >>> 
> >>> There is likely a better way to fix this than to remove the type here.
> >> 
> >> MSVC was not able to convert from unsigned to the Kind enum type.
> >> Maybe replace with a static_cast when calling the function instead?
> > 
> > I see that we’re now using enum classes everywhere.  How about using “enum class Kind : unsigned”?
> 
> Yes, we should put the static_cast into the specializationKind() function; note that the two other callers in DFGByteCodeParser.cpp would not require a static_cast. Also seems fine to try Mark’s suggestion of moving to an enum class, but I think that will lead to even more changes and I am not sure it’s worth it.

Ok, thanks, updated patch :)
Comment 11 Brent Fulgham 2014-08-20 11:00:50 PDT
(In reply to comment #7)
> (From update of attachment 236874 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236874&action=review
> 
> >>>> Source/JavaScriptCore/bytecode/CodeOrigin.h:143
> >>>> +    static CodeSpecializationKind specializationKindFor(unsigned kind)
> >>> 
> >>> This change should not be necessary. What happens when you don’t change this? Some kind of failure to compile?
> >> 
> >> There is likely a better way to fix this than to remove the type here.
> > 
> > MSVC was not able to convert from unsigned to the Kind enum type.
> > Maybe replace with a static_cast when calling the function instead?
> 
> I see that we’re now using enum classes everywhere.  How about using “enum class Kind : unsigned”?

Yes. See <https://bugs.webkit.org/show_bug.cgi?id=134252> as well. We've hit this numerous times.
Comment 12 WebKit Commit Bot 2014-08-20 11:11:29 PDT
Comment on attachment 236876 [details]
Patch

Clearing flags on attachment: 236876

Committed r172801: <http://trac.webkit.org/changeset/172801>
Comment 13 WebKit Commit Bot 2014-08-20 11:11:35 PDT
All reviewed patches have been landed.  Closing bug.