Bug 136103 - [Win] Assertion fails when running JSC stress tests.
Summary: [Win] Assertion fails when running JSC stress tests.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-20 08:39 PDT by peavo
Modified: 2014-08-20 11:11 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.83 KB, patch)
2014-08-20 09:38 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (1.89 KB, patch)
2014-08-20 10:03 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (1.94 KB, patch)
2014-08-20 10:27 PDT, peavo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.