Closed
Bug 818138
Opened 12 years ago
Closed 12 years ago
enterFakeExitFrame can do a better job with the assembly it generates
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: bzbarsky, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
(Whiteboard: [js:t])
Attachments
(1 file)
1.94 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Right now, for something like enterFakeExitFrame(ION_FRAME_DOMGETTER) we get codegen like so on x86-84:
> movabsq $0x1, %r11 #bz Stick 1 in %r11
> push %r11 #bz Push 1
> movabsq $0x0, %r11 #bz Stick 0 in %r11
> push %r11 #bz Push 0
The problem is that it's not clear to me that the codeVal argument is always something that can be pushed as an immediate... But null sure is.
Assignee | ||
Updated•12 years ago
|
Assignee: general → evilpies
Reporter | ||
Comment 1•12 years ago
|
||
So what buildFakeExitFrame (which ends up with a pushl) does is this: 364 Push(Imm32(descriptor)); while enterFakeExitFrame does this: Push(ImmWord(uintptr_t(codeVal))); Push(ImmWord(uintptr_t(NULL))); The problem is that presumably fake and real exit frames need to match size, and real exit frames have: exitCodePatch_ = PushWithPatch(ImmWord(-1)); // Push VMFunction pointer, to mark arguments. Push(ImmWord(f)); which is in fact pushing two pointer-sized values.
Comment 2•12 years ago
|
||
Pushing pointer-sized values is fine; Push (or similar) just needs to be taught about the sign-extension semantics when pushing immediates. Wonder what the codegen looks like on x86.
Reporter | ||
Comment 3•12 years ago
|
||
Looks like pushl on x86-64 will in fact sign-extend 32-bit immediates to a 64-bit value. I'm building a 32-bit build at the moment to answer that last question. ;)
Assignee | ||
Comment 4•12 years ago
|
||
Reporter | ||
Comment 5•12 years ago
|
||
In a 32-bit build, this bit of asm looks like so: pushl $0x1 pushl $0x0
Assignee | ||
Updated•12 years ago
|
Attachment #688363 -
Flags: review?(jdemooij)
Comment 6•12 years ago
|
||
Comment on attachment 688363 [details] [diff] [review] optimize push(immword) to push(imm32) sometimes Review of attachment 688363 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonMacroAssembler.h @@ +491,5 @@ > exitCodePatch_ = PushWithPatch(ImmWord(-1)); > // Push VMFunction pointer, to mark arguments. > Push(ImmWord(f)); > } > + void enterFakeExitFrame(IonCode *codeVal = NULL) { Pre-existing, but can you add a comment to this method like this: // Note: use ImmWord, not ImmGCPtr, since codeVal is not a valid IonCode // pointer but a magic value like ION_FRAME_DOMGETTER.
Attachment #688363 -
Flags: review?(jdemooij) → review+
Comment 7•12 years ago
|
||
Comment on attachment 688363 [details] [diff] [review] optimize push(immword) to push(imm32) sometimes Review of attachment 688363 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/x64/Assembler-x64.h @@ +290,5 @@ > } > void push(const ImmWord ptr) { > + // We often end up with ImmWords that actually fit into int32. > + // Be aware of the sign extension behavior. > + if (ptr.value <= INT32_MAX) { This needs to check for ptr.value >= INT32_MIN as well, otherwise pointers whose numeric values are < INT32_MIN will be corrupted. Or is the pointer guaranteed to fit into a 32-bit address space? (Probably not a bad idea to check anyway.)
Updated•12 years ago
|
Whiteboard: [js:t]
Assignee | ||
Comment 8•12 years ago
|
||
ptr.value is an uintptr_t, so it can never be negative. So this should result in an unsigned compare.
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d148382e074c
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d148382e074c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in
before you can comment on or make changes to this bug.
Description
•