Bug 131449 – Ensure that LLINT accessing of the ProtoCallFrame is big endian friendly

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

[Mark Lam]

Patch coming.

[Mark Lam]

Created attachment 228975 [details]
the patch

[Mark Hahnenberg]

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?

[Mark Lam]

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

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

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.

[Mark Lam]

Created attachment 228978 [details]
revised patch.

[Mark Hahnenberg]

Comment on attachment 228978 [details]
revised patch.

r=me!

[Mark Lam]

Thanks.  Landed in r167031: <http://trac.webkit.org/r167031>.

[Geoffrey Garen]

Does this fix a bug? If so, it should come with a description of the bug and a regression test.

[Mark Lam]

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

[Geoffrey Garen]

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

[Mark Lam]

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

[Mark Lam]

ChangeLog comment updated in r167037: <http://trac.webkit.org/changeset/167037>