Merge PushActiveVMFrame and SetVMFrameRegs

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla12
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
As far as I can tell, SetVMFrameRegs is always called immediately after PushActiveVMFrame, and only then. So they can be merged. (This also eliminates a state where a StackSegment's regs points to garbage until it gets fixed at the end of EnterMethodJIT, which is only observable from a signal handler.)
(Assignee)

Comment 1

6 years ago
Created attachment 581143 [details] [diff] [review]
Merge PushActiveVMFrame, SetVMFrameRegs

Patch delayed by fire alarm :)
Attachment #581143 - Flags: review?(bhackett1024)
Comment on attachment 581143 [details] [diff] [review]
Merge PushActiveVMFrame, SetVMFrameRegs

Review of attachment 581143 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/methodjit/MethodJIT.cpp
@@ +154,5 @@
>  extern "C" void JS_FASTCALL
>  PopActiveVMFrame(VMFrame &f)
>  {
>      f.entryfp->script()->compartment()->jaegerCompartment()->popActiveFrame();
> +    f.cx->stack.repointRegs(f.oldregs);

Can you add back SetVMFrameRegs but make it a no-op?  There are a bunch of other trampolines in js/src/methodjit .asm and .s files for Tier 2 platforms which will be broken by this change, but if SetVMFrameRegs is still there then they will work without needing to be changed.  Alternatively you can fix the trampolines in the other files, but I don't know how straightforward that is (I try to avoid changing the trampolines unless absolutely necessary because of the headache of updating all of them).
Attachment #581143 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 3

6 years ago
Created attachment 581322 [details]
Mer
(Assignee)

Comment 4

6 years ago
Created attachment 581325 [details] [diff] [review]
Merge PushActiveVMFrame, SetVMFrameRegs

Oh, sorry, that's what I missed. I just grepped for *.h *.cpp. Forgot about *.s and *.asm.

Here's an updated patch carrying over the r+, but with updates for the other architectures. My apologies in advance if I broke any of them, but this particular change seemed pretty straightforward.

(stupid bugzilla/firefox autocomplete UX failure caused me to submit this prematurely)
Assignee: general → sphink
Attachment #581143 - Attachment is obsolete: true
Attachment #581322 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #581325 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/136c73b6457c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.