Bug 136103 – [Win] Assertion fails when running JSC stress tests.

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

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.

Created attachment 236873 [details]
Patch

[Darin Adler]

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.

Created attachment 236874 [details]
Patch

[Darin Adler]

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?

[Darin Adler]

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.

(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?

[Mark Lam]

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”?

[Darin Adler]

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.

Created attachment 236876 [details]
Patch

(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 :)

[Brent Fulgham]

(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.

[WebKit Commit Bot]

Comment on attachment 236876 [details]
Patch

Clearing flags on attachment: 236876

Committed r172801: <http://trac.webkit.org/changeset/172801>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.