Last Comment Bug 710055 - Merge PushActiveVMFrame and SetVMFrameRegs
: Merge PushActiveVMFrame and SetVMFrameRegs
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
-- normal (vote)
: mozilla12
Assigned To: Steve Fink [:sfink] [:s:]
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2011-12-12 16:21 PST by Steve Fink [:sfink] [:s:]
Modified: 2011-12-30 05:00 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Merge PushActiveVMFrame, SetVMFrameRegs (5.24 KB, patch)
2011-12-12 18:40 PST, Steve Fink [:sfink] [:s:]
bhackett1024: review+
Details | Diff | Splinter Review
Mer (9.86 KB, text/plain)
2011-12-13 10:34 PST, Steve Fink [:sfink] [:s:]
no flags Details
Merge PushActiveVMFrame, SetVMFrameRegs (9.86 KB, patch)
2011-12-13 10:38 PST, Steve Fink [:sfink] [:s:]
sphink: review+
Details | Diff | Splinter Review

Description User image Steve Fink [:sfink] [:s:] 2011-12-12 16:21:41 PST
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.)
Comment 1 User image Steve Fink [:sfink] [:s:] 2011-12-12 18:40:56 PST
Created attachment 581143 [details] [diff] [review]
Merge PushActiveVMFrame, SetVMFrameRegs

Patch delayed by fire alarm :)
Comment 2 User image Brian Hackett (:bhackett) 2011-12-13 08:08:26 PST
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();
> +>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).
Comment 3 User image Steve Fink [:sfink] [:s:] 2011-12-13 10:34:15 PST
Created attachment 581322 [details]
Comment 4 User image Steve Fink [:sfink] [:s:] 2011-12-13 10:38:45 PST
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)
Comment 5 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-30 05:00:32 PST

Note You need to log in before you can comment on or make changes to this bug.