When making IC which are attaching stubs with calls, we need to ensure that the the frame size of the parent script is correctly extracted and set to the macro assembler, and, as well that the all liveRegs are correctly pushed before manipulating the stack. I suggest we add the constructor MacroAssembler::MacroAssembler(JSContext *, IonScript *) to always initialized the FramePushed size before hand, even if we are not making any call. And add icSaveLive & icRestoreLive function to assert that the spilled registers are made at the correct stack location (at the FrameSize of the IonScript). And Make the buildOOLFakeExitFrame take a phantom type returned by icSaveLive, to ensure that it has been called before. Doing these suggestions, will prevent writing any code which might lead to a SEGV, which are easily missed during the review process.

I would like to take it up.

I make MacroAssembler::MacroAssembler(JSContext *, IonScript *) and icSaveLive as below. (in /js/src/ion/IonMacroAssembler.h) MacroAssembler(JSContext *cx,IonScript *ion) { setFramePushed(ion->frameSize()); } bool icSaveLive(IonScript *ion) const { return framePushed()==ion->frameSize(); } I don't know the role of icRestoreLive and icSaveLive. You say icSaveLive return a phantom type. So, do I change buildOOLFakeExitFrame like this? template<class something,class Tag> bool buildOOLFakeExitFrame(void *fakeReturnAddr) { uint32_t descriptor = MakeFrameDescriptor(framePushed(), IonFrame_OptimizedJS) Push(Imm32(descriptor)) Push(ImmWord(fakeReturnAddr)) return true }

(In reply to iseki.m.aa from comment #2) > I make MacroAssembler::MacroAssembler(JSContext *, IonScript *) and > icSaveLive as below. (in /js/src/ion/IonMacroAssembler.h) > > bool icSaveLive(IonScript *ion) const { > return framePushed()==ion->frameSize(); > } > > I don't know the role of icRestoreLive and icSaveLive. This is supposed to do the equivalent of CodeGeneratorShared::saveLive that we currently have in the IonCache code, which is pushing all registers live registers. > You say icSaveLive return a phantom type. > So, do I change buildOOLFakeExitFrame like this? > > template<class something,class Tag> > bool buildOOLFakeExitFrame(void *fakeReturnAddr) { There is no need for templates for this use case. We just want a type to serve as a guarantee that icSaveLive has been called. so we want to have icSaveLive return a dummy type which prevent any unwanted construction outside of icSaveLive. The class does not even have to contain anything and it can be marked as final.

I create MacroAssembler::MacroAssembler(JSContext *, IonScript *) and icSaveLive(RegisterSet &liveRegs) & icRestoreLive(RegisterSet &liveRegs). I can't still understand how to change buildOOLFakeExitFrame.

Comment on attachment 795905 [details] [diff] [review] WIP Review of attachment 795905 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonMacroAssembler.h @@ +106,4 @@ > #endif > } > > + MacroAssembler(JSContext *cx,IonScript *ion) style-nit: we add a space after the comma. @@ +573,4 @@ > return exitCodePatch_.offset() != 0; > } > > + void icSaveLive(RegisterSet &liveRegs) { You want a type which can only be constructed from the MacroAssembler through this function *only*. The trick is to make a class which has a private constructor and a public copy constructor, such as you can only build it from a friend function/class but you can still copy it around to give it as argument of buildOOLFakeExitFrame & isRestoreLive. class MacroAssembler { class Foo { friend class MacroAssembler; protected: Foo() {} public: Foo(const Foo &) {} }; Foo create() { return Foo(); } void consume(const Foo &) { } }; This small code should ensure that you cannot call "consume" before you called "create" because you have no way (in a cast-less C++) to build an instance of Foo without using the create function.

(In reply to Nicolas B. Pierron [:nbp] from comment #5) > class Foo { > friend class MacroAssembler; > protected: > Foo() {} > public: > Foo(const Foo &) {} > }; You might notice that is similar to Singletons, except that we do not want it to be static and we want to be able to copy it without being able to forge a new one.

Sorry, I can't still understand well. icSaveLive needs to return a phantom type to prevent pushing "descriptor" and "fakeReturnAddr" in buildOOLFakeExitFrame. I can't understand a phantom type.

(In reply to iseki.m.aa from comment #7) > Created attachment 797585 [details] [diff] [review] > WIP > > Sorry, I can't still understand well. > > icSaveLive needs to return a phantom type to prevent pushing "descriptor" > and "fakeReturnAddr" in buildOOLFakeExitFrame. > I can't understand a phantom type. In this case, "phantom type" is just a name for a type that is impossible to construct except as a return value from icSaveLive; Nicolas gives an example of one in comment 5.

(In reply to iseki.m.aa from comment #7) > icSaveLive needs to return a phantom type to prevent pushing "descriptor" > and "fakeReturnAddr" in buildOOLFakeExitFrame. The goal is not to prevent pushing anything. The goal is to ensure that nobody forgot to call icSaveLive before calling buildOOLFakeExitFrame. Having a class which can only be constructed by isSaveLive is one way to provide this guarantee at compile time. "Phantom types" is the name given to types which are only represented at compile time and which have literally no cost at runtime.

I change icSaveLive. void* icSaveLive(RegisterSet &liveRegs) { PushRegsInMask(liveRegs); return this; } buildOOLFakeExit is below (only ARM) bool MacroAssemblerARMCompat::buildOOLFakeExitFrame(const MacroAssembler &masm) { return true; } Is icSaveLive called ? returnAddr = masm.icSaveLive(liveRegs)

I understand phantom type.

Comment on attachment 799526 [details] [diff] [review] Bug900285.patch Review of attachment 799526 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.cpp @@ +753,4 @@ > masm.move32(Imm32(0), argUintNReg); > masm.movePtr(StackPointer, argVpReg); > > + if (!masm.saveBuildOOLFakeExitFrame(returnAddr, masm.icSaveLive(liveRegs))) If we were able to do so we would have merged the function. Sadly we cannot as we expect to spill everything at the top of the stack and that an IC can still manipulate anything after as being some free-space. ::: js/src/ion/IonMacroAssembler.h @@ +743,5 @@ > + class AfterICSaveLive { > + friend class MacroAssembler; > + public: > + AfterICSaveLive() > + { } This should not be public, only the copy constructor should be public.

I make bool variable instead of the class which return in icSaveLive.

I'd like to know about IonCache algorithm. Is any document or book which help me understanding ?

I'd like to know about IonCache algorithm. Is any document or book which help me understanding ?

Comment on attachment 801221 [details] [diff] [review] Bug900285.patch Review of attachment 801221 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonMacroAssembler.h @@ +747,5 @@ > void printf(const char *output); > void printf(const char *output, Register value); > + > + void setIsIcSaveLive() { > + isSaveLive = true; I do not call that a phantom type as this add overhead at runtime. @@ +762,5 @@ > + } > + > + bool saveBuildOOLFakeExitFrame(void *fakeReturnAddr) { > + icSaveLive(); > + return buildOOLFakeExitFrame(fakeReturnAddr); This is wrong, as mentioned in comment 12.

(In reply to iseki.m.aa from comment #15) > I'd like to know about IonCache algorithm. > Is any document or book which help me understanding ? There is a research paper on Self which explains the concept of ICs, but I do not think they would be of any use for solving this issue.

(In reply to Nicolas B. Pierron [:nbp] from comment #17) > There is a research paper on Self which explains the concept of ICs, but I > do not think they would be of any use for solving this issue. Thanks. I just would like to understand ICs.

(In reply to iseki.m.aa from comment #19) > Thanks. > I just would like to understand ICs. Here's a blog post on SpiderMonkey's ICs: http://blog.cdleary.com/2010/09/picing-on-javascript-for-fun-and-profit/ Note that it's about 3 years old, so I'm sure much of it is outdated. It does nicely outline the whys and hows of ICs.

(In reply to Till Schneidereit [:till] from comment #20) > (In reply to iseki.m.aa from comment #19) > > Thanks. > > I just would like to understand ICs. > > Here's a blog post on SpiderMonkey's ICs: > http://blog.cdleary.com/2010/09/picing-on-javascript-for-fun-and-profit/ > > Note that it's about 3 years old, so I'm sure much of it is outdated. It > does nicely outline the whys and hows of ICs. Thanks :)

Comment on attachment 804829 [details] [diff] [review] Bug-900285 Review of attachment 804829 [details] [diff] [review]: ----------------------------------------------------------------- Next time follow the rule describe on the following page[1]. This way one person can be notified when you attach a new patch. Sorry for the delay. [1] https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch ::: js/src/ion/IonCaches.cpp @@ +755,4 @@ > masm.move32(Imm32(0), argUintNReg); > masm.movePtr(StackPointer, argVpReg); > > + if (!masm.saveBuildOOLFakeExitFrame(returnAddr,aic)) nit: Add a space after the comma. ::: js/src/ion/IonMacroAssembler.h @@ +755,5 @@ > + } > + > + bool saveBuildOOLFakeExitFrame(void *fakeReturnAddr, AfterICSaveLive &aic) { > + return buildOOLFakeExitFrame(fakeReturnAddr); > + } Why not just adding one extra argument to buildOOLFakeExitFrame instead of a moving it under the protect section? All the use cases of it are in IonCaches.cpp, so there is no need to make a special wrapper for it.

(In reply to Nicolas B. Pierron [:nbp] from comment #22) > ::: js/src/ion/IonMacroAssembler.h > @@ +755,5 @@ > > + } > > + > > + bool saveBuildOOLFakeExitFrame(void *fakeReturnAddr, AfterICSaveLive &aic) { > > + return buildOOLFakeExitFrame(fakeReturnAddr); > > + } > > Why not just adding one extra argument to buildOOLFakeExitFrame instead of a > moving it under the protect section? > All the use cases of it are in IonCaches.cpp, so there is no need to make a > special wrapper for it. AfterICSaveLive is nest class of MacroAssembler. buildOOLFakeExitFrame is defined before MacroAssembler. If I add one extra argument to buildOOLFakeExitFrame , I can't compile because of Incomplete type 'AfterICSaveLive' named in nested name specifier. I don't know how to avoid this error. So I make saveBuildOOLFakeExitFrame.

(In reply to iseki.m.aa from comment #24) > (In reply to Nicolas B. Pierron [:nbp] from comment #22) > > ::: js/src/ion/IonMacroAssembler.h > > @@ +755,5 @@ > > > + } > > > + > > > + bool saveBuildOOLFakeExitFrame(void *fakeReturnAddr, AfterICSaveLive &aic) { > > > + return buildOOLFakeExitFrame(fakeReturnAddr); > > > + } > > > > Why not just adding one extra argument to buildOOLFakeExitFrame instead of a > > moving it under the protect section? > > All the use cases of it are in IonCaches.cpp, so there is no need to make a > > special wrapper for it. > > AfterICSaveLive is nest class of MacroAssembler. > buildOOLFakeExitFrame is defined before MacroAssembler. > If I add one extra argument to buildOOLFakeExitFrame , I can't compile > because of Incomplete type 'AfterICSaveLive' named in nested name specifier. > I don't know how to avoid this error. A part from the modification (no need to add that as part of the patch). One way to work around would be to declare AfterICSaveLive class in the parent class, and define it later in the same file as icSaveLive as it need to have it defined to be able to return an instance of this class. Notice that C++ distinguish define & declare in the following way: // In the header file // define MacroAssemblerX86Shared class MacroAssemblerX86Shared { // declare AfterICSaveLive class AfterICSaveLive; // declare buildOOLFakeExitFrame bool buildOOLFakeExitFrame(void *fakeReturnAddr, AfterICSaveLive &aic); }; // define AfterICSaveLive class MacroAssemblerX86Shared::AfterICSaveLive { … class body … }; // define buildOOLFakeExitFrame bool MacroAssemblerX86Shared::buildOOLFakeExitFrame(void *fakeReturnAddr, AfterICSaveLive &aic) { … function body … } > So I make saveBuildOOLFakeExitFrame. Ok, just rename it to icBuildOOLFakeExitFrame, This would be a better name for this patch as "save" is related a list of register, and the context of this fakeExitFrame, is to be used under an IC stub.

Comment on attachment 812931 [details] [diff] [review] Bug-900285 Review of attachment 812931 [details] [diff] [review]: ----------------------------------------------------------------- Ok, this patch is almost good to go. But to get it landed, you need to follow the rules[1] such as the patch can apply directly on top of the tree. You will also need to update your patch with the latest changes which have been made on these files. [1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F ::: js/src/ion/IonMacroAssembler.h @@ +757,5 @@ > + bool icBuildOOLFakeExitFrame(void *fakeReturnAddr, AfterICSaveLive &aic) { > + return buildOOLFakeExitFrame(fakeReturnAddr); > + } > + > + void icRestoreLive(RegisterSet &liveRegs) { One last thing, add "AfterICSaveLive &" as argument of this function.

Comment on attachment 813703 [details] [diff] [review] Bug900285.patch Review of attachment 813703 [details] [diff] [review]: ----------------------------------------------------------------- Ok, I tried your patch locally and gcc told me the following error message. I fixed it and your patch apply and compile. I will check that it works locally and submit it for you. Good Work. ::: js/src/jit/IonMacroAssembler.h @@ +1331,5 @@ > } > + > + public: > + class AfterICSaveLive { > + friend MacroAssembler; js/src/jit/IonMacroAssembler.h:1335:9: error: a class-key must be used when declaring a friend

You will be able to find your patch in the nightly versions of Firefox at https://hg.mozilla.org/integration/mozilla-inbound/rev/33bb2c20c28a Also you can follow the progress of our continuous integrations system (tbpl) at https://tbpl.mozilla.org/?tree=Mozilla-Inbound&showall=1&rev=33bb2c20c28a Thanks. :)

(In reply to Nicolas B. Pierron [:nbp] from comment #30) > You will be able to find your patch in the nightly versions of Firefox at > > https://hg.mozilla.org/integration/mozilla-inbound/rev/33bb2c20c28a > > Also you can follow the progress of our continuous integrations system > (tbpl) at > > https://tbpl.mozilla.org/?tree=Mozilla-Inbound&showall=1&rev=33bb2c20c28a > > Thanks. :) Thanks. :)