Summary: | Ensure that LLINT accessing of the ProtoCallFrame is big endian friendly | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
Component: | JavaScriptCore | Assignee: | 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
Mark Lam
2014-04-09 12:34:37 PDT
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> |