Patch coming.
Created attachment 228975 [details] the patch
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?
(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 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 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.
Created attachment 228978 [details] revised patch.
Comment on attachment 228978 [details] revised patch. r=me!
Thanks. Landed in r167031: <http://trac.webkit.org/r167031>.
Does this fix a bug? If so, it should come with a description of the bug and a regression test.
(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.
> 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.
(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.
ChangeLog comment updated in r167037: <http://trac.webkit.org/changeset/167037>