Bug 131449

Summary: Ensure that LLINT accessing of the ProtoCallFrame is big endian friendly
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, mhahnenberg, mmirman, msaboff, oliver, tpopela
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 128743    
Attachments:
Description Flags
the patch
none
revised patch. mhahnenberg: review+

Description Mark Lam 2014-04-09 12:34:37 PDT
Patch coming.
Comment 1 Mark Lam 2014-04-09 12:44:05 PDT
Created attachment 228975 [details]
the patch
Comment 2 Mark Hahnenberg 2014-04-09 12:46:48 PDT
Comment on attachment 228975 [details]
the patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:194
> +    loadp ProtoCallFrame::paddedArgCount[protoCallFrame], temp3

Doesn't this change the behavior? We will end up loading an extra 32 bits. Was the old behavior a bug?
Comment 3 Mark Lam 2014-04-09 12:50:27 PDT
(In reply to comment #2)
> (From update of attachment 228975 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228975&action=review
> 
> > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:194
> > +    loadp ProtoCallFrame::paddedArgCount[protoCallFrame], temp3
> 
> Doesn't this change the behavior? We will end up loading an extra 32 bits. Was the old behavior a bug?

Yes, the old behavior is a bug.  Alternatively, we can change ProtoCallFrame::paddedArgCount to an unsigned instead if a size_t.
Comment 4 Mark Hahnenberg 2014-04-09 12:53:06 PDT
Comment on attachment 228975 [details]
the patch

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

>>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:194
>>> +    loadp ProtoCallFrame::paddedArgCount[protoCallFrame], temp3
>> 
>> Doesn't this change the behavior? We will end up loading an extra 32 bits. Was the old behavior a bug?
> 
> Yes, the old behavior is a bug.  Alternatively, we can change ProtoCallFrame::paddedArgCount to an unsigned instead if a size_t.

Hmm, I'm unfamiliar with ProtoCallFrame. Does the size of the struct need to be consistent between 32-bit and 64-bit? If so then we should definitely change the type to uint32_t.
Comment 5 Mark Hahnenberg 2014-04-09 13:03:03 PDT
Comment on attachment 228975 [details]
the patch

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

>>>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:194
>>>> +    loadp ProtoCallFrame::paddedArgCount[protoCallFrame], temp3
>>> 
>>> Doesn't this change the behavior? We will end up loading an extra 32 bits. Was the old behavior a bug?
>> 
>> Yes, the old behavior is a bug.  Alternatively, we can change ProtoCallFrame::paddedArgCount to an unsigned instead if a size_t.
> 
> Hmm, I'm unfamiliar with ProtoCallFrame. Does the size of the struct need to be consistent between 32-bit and 64-bit? If so then we should definitely change the type to uint32_t.

Okay, looks like the ProtoCallFrame is the dummy call frame we use when we're about to start executing JS. I vote that we make ProtoCallFrame's arg count match the type of CallFrame, which looks like an int32_t. (Register::payload returns an int32_t, which then gets casted to a size_t inside CallFrame::argumentCountIncludingThis). So maybe uint32_t makes the most sense here.
Comment 6 Mark Lam 2014-04-09 13:17:43 PDT
Created attachment 228978 [details]
revised patch.
Comment 7 Mark Hahnenberg 2014-04-09 13:19:54 PDT
Comment on attachment 228978 [details]
revised patch.

r=me!
Comment 8 Mark Lam 2014-04-09 13:24:39 PDT
Thanks.  Landed in r167031: <http://trac.webkit.org/r167031>.
Comment 9 Geoffrey Garen 2014-04-09 14:02:28 PDT
Does this fix a bug? If so, it should come with a description of the bug and a regression test.
Comment 10 Mark Lam 2014-04-09 14:06:55 PDT
(In reply to comment #9)
> Does this fix a bug? If so, it should come with a description of the bug and a regression test.

The bug only manifests on a big endian system and was caught as soon as Tomas ran the JSC stress tests.  I’ll try to create a more isolated test case and post that later.
Comment 11 Geoffrey Garen 2014-04-09 14:23:49 PDT
> The bug only manifests on a big endian system and was caught as soon as Tomas ran the JSC stress tests.  I’ll try to create a more isolated test case and post that later.

If this patch fixes a bug that's already covered by existing regression test(s), then you don't need to write a new regression test, and instead you should mention that the existing regression test(s) in your ChangeLog.
Comment 12 Mark Lam 2014-04-09 14:24:40 PDT
(In reply to comment #11)
> > The bug only manifests on a big endian system and was caught as soon as Tomas ran the JSC stress tests.  I’ll try to create a more isolated test case and post that later.
> 
> If this patch fixes a bug that's already covered by existing regression test(s), then you don't need to write a new regression test, and instead you should mention that the existing regression test(s) in your ChangeLog.

I will update the ChangeLog comment.
Comment 13 Mark Lam 2014-04-09 14:53:45 PDT
ChangeLog comment updated in r167037: <http://trac.webkit.org/changeset/167037>