This work is to make way for proper exception handling and GC'ing while Ion code is on the stack. The plan is to give ion::ThunkToInterpreter a proper exit frame so it looks like a normal call to C++. This will also introduce some cleaner constructors for frames and bailouts. Split off from bug 695075.

(In reply to Nicolas B. Pierron [:pierron] from bug 695075 comment #4) > > +class IonEntryFrameLayout > > +{ > > class IonEntryFrameLayout : public IonCommonFrameLayout Whoops, thanks. > > + double fpregs_[FloatRegisters::Total]; > > + uintptr_t regs_[Registers::Total]; > > I would prefer to have a name for this couple, such as ProcessContext / > RegistersContext / MachineState and provide a RegistersContextRef / > MachineStateRef class in place of the current MachineState. > > Thus we could have the corresponding macro assembler instructions for making > context. I like the idea, though we don't have a clear definition for a context yet. For example, in some situations, we may only want to save double registers. but: > > JS_STATIC_ASSERT(sizeof(BailoutStack) == > > sizeof(uintptr_t) + > > sizeof(double) * 8 + > > sizeof(uintptr_t) * 8 + > > 8 is a nice inlining of Registers::Total & FloatRegisters::Total values. ;) > sizeof(RegistersContext) is a good substitute. Hrm - yeah, this is gross. We should really be using NumAllocatableRegisters and only pushing registers in that set. I'll clean this up. > 1: Thus I wonder why you reinterpret data in a larger type than what it can > be. Instead of a void **esp, you should expect a "BailoutStack *". This > would make the prototype of the function cleaner for readers. Good idea.

This patch changes implicit Value outparams to be explicit

This moves ABI calls to be specific to x86/x64 macro assemblers. ARM will come next. This makes it possible to perform ABI calls from within arch-specific assemblers.

Make ABI class arch-specific, ARM changes.

Comment on attachment 572063 [details] [diff] [review] part 3: ARM changes Review of attachment 572063 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/arm/MacroAssembler-arm.h @@ +332,5 @@ > void breakpoint(); > > + // Setup a call to C/C++ code, given the number of general arguments it > + // takes. Note that this only supports cdecl. > + // As Jacob Bramley has pointed out in the past, cdecl doesn't exist on arm. The whole ABI is called "eabi", which is probably good enough to distinguish the calling convention @@ +353,5 @@ > + void setABIArg(uint32 arg, const MoveOperand &from); > + void setABIArg(uint32 arg, const Register &reg); > + > + // Emits a call to a C/C++ function, resolving all argument moves. > + void callWithABI(void *fun); I'd actually put all of this code into Compat. My idea was that eventually, we can mark the entire body of MacroAssemblerARM as protected, and have all code that isn't in the arm/ directory to only have access to the compat assembler, not the underlying assembler. However, I don't know if this is a good direction for the code to head in ::: js/src/ion/arm/MoveEmitter-arm.cpp @@ +48,1 @@ > : inCycle_(false), In general, everything in the arm directory should be able to suffice using MacroAssemblerARM rather than MacroAssemblerARMCompat, but I don't want to unnecessarily restrict future code.

untested, builds on x86. this patch introduces a number of refactorings: * Frames are now opaque, hidden behind a new IonFrameIterator, which must start iterating from a pointer to an exit frame. * When leaving JIT code to potentially re-enter the VM, an exit frame must be created and a pointer to its header stored in ThreadData. * returnError trampolines are now gone. Instead, calls within an exit frame must check for failure and then call an exception handler. The exception handler returns a new stack pointer. * BailoutEnvironment has been refactored into FrameRecovery and MachineState, which can be shared across architectures. * Architecture-specific bailout code is now much simpler, you just need to supply a MachineState and FrameRecovery. * Bailouts now replace the deoptimized JS frame with an Exit frame, and call the interpreter from that (as if it were called from generateCWrapper).

x86 changes - x64 and ARM in the next two patches

This patch changes ABI calls to use the same ExitFrame mechanism as bailouts. For the most part this just simplifies things.

Hopefully the last patch. This one makes Ion re-entrant again, by saving/restoring ionTop. It also makes ABI calls no longer bake in JSContext, which isn't safe (yet).

Comment on attachment 571869 [details] [diff] [review] part 1: use explicit outparams in ABI calls Review of attachment 571869 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonMacroAssembler.cpp @@ +105,5 @@ > { > JS_ASSERT(inCall_); > > + // Position all arguments. > + { The extra indentation level can be removed. @@ +110,3 @@ > enoughMemory_ &= moveResolver_.resolve(); > if (!enoughMemory_) > return; If we don't have enough memory to perform necessary moves, then the result is nonsense. Should callWithABI() be made fallible? ::: js/src/ion/x64/Trampoline-x64.cpp @@ +436,5 @@ > + Register outReg = InvalidReg; > + if (f.outParam == VMFunction::OutParam_Value) { > + outReg = regs.takeAny(); > + masm.reserveStack(sizeof(Value)); > + masm.movl(rsp, outReg); Great! @@ +445,2 @@ > > + // Set the implicit context parameter. nit: // Initialize and set the implicit context parameter. Then we can eliminate the latter comment and the whitespace for no less clarity. @@ +463,5 @@ > > + // Load the outparam and free any allocated stack. > + if (f.outParam == VMFunction::OutParam_Value) { > + masm.loadValue(Operand(esp, 0), JSReturnOperand); > + masm.freeStack(sizeof(Value)); =] ::: js/src/ion/x86/Assembler-x86.h @@ -66,5 @@ > static const Register JSReturnReg_Data = edx; > static const Register StackPointer = esp; > static const Register ReturnReg = eax; > -static const Register JSCReturnReg_Type = eax; > -static const Register JSCReturnReg_Data = edx; JSCReturnReg_Type, JSCReturnReg_Data, and JSCReturnOperand also exist in Assembler-x64.h. Since this patch removes both uses of JSCReturnOperand in Trampoline-x64.cpp, these definitions can be removed for x64 as well.

Comment on attachment 571870 [details] [diff] [review] part 2: make ABI calls arch-specific Review of attachment 571870 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/x86/MacroAssembler-x86.cpp @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- nit: shouldn't tab-width be '4'? This looks like it affects a bunch of files.

Comment on attachment 572381 [details] [diff] [review] part 4: introduce exit frames for bailouts, refactoring Review of attachment 572381 [details] [diff] [review]: ----------------------------------------------------------------- This patch is /extremely/ pleasant. A bunch of annoying nits follow. ::: js/src/ion/IonFrames.cpp @@ +48,5 @@ > +using namespace js; > +using namespace js::ion; > + > +FrameRecovery::FrameRecovery(uint8 *fp, uint8 *sp, const MachineState &machine) > + : fp_((IonJSFrameLayout *)fp), Since IonJSFrameLayouts have certain known types, can we assert in the body below that fp actually points to an IonJSFrameLayout? ::: js/src/ion/IonFrames.h @@ +175,5 @@ > + MachineState(uintptr_t *regs, double *fpregs) > + : regs_(regs), fpregs_(fpregs) > + { } > + > + double readDoubleReg(FloatRegister reg) const { readFloatReg()? They're called FloatRegisters everywhere else. Maybe they should be called something different. @@ +223,5 @@ > + uintptr_t readSlot(uint32 offset) const { > + JS_ASSERT((offset % STACK_SLOT_SIZE) == 0); > + return *(uintptr_t *)(sp_ + offset); > + } > + double readDoubleSlot(uint32 offset) const { readFloatSlot()? Or rename FloatRegister, as above. @@ +287,5 @@ > return (JSScript*)(uintptr_t(token) & ~uintptr_t(1)); > } > > } > } // namespace ion // namespace js ::: js/src/ion/shared/IonFrames-x86-shared.h @@ +63,5 @@ > + return descriptor_ >> FRAMETYPE_BITS; > + } > +}; > + > +class IonEntryFrameLayout This appears to be empty. Perhaps a relic of refactoring? Certainly it should at least extend IonCommonFrameLayout. @@ +68,5 @@ > +{ > + private: > +}; > + > +class IonJSFrameLayout : public IonCommonFrameLayout It would be useful to occasionally assert that, based on the descriptor, the right class is mapped to the right stack location. @@ +78,5 @@ > + void *calleeToken() const { > + return calleeToken_; > + } > + > + static size_t offsetOfCalleeToken() { nit: static functions typically start with a capital. @@ +101,5 @@ > uintptr_t snapshotOffset; > }; > > } > } // namespace ion // namespace js ::: js/src/ion/x86/Architecture-x86.h @@ +83,5 @@ > static const Code StackPointer = JSC::X86Registers::esp; > static const Code Invalid = JSC::X86Registers::invalid_reg; > > static const uint32 Total = 8; > + static const uint32 Allocatable = 7; Oh dear. ::: js/src/ion/x86/Assembler-x86.h @@ +237,5 @@ > } > + void movl(ImmWord imm, Register dest) { > + masm.movl_i32r(imm.value, dest.code()); > + } > + void mov(Imm32 imm, Register dest) { We should probably standardize on either "Register dest" or "const Register &dest", since in practice it makes no difference, but having multiple styles is annoying. I prefer the former. ::: js/src/ion/x86/Bailouts-x86.cpp @@ +107,3 @@ > > + if (bailout->frameClass() == FrameSizeClass::None()) > + return FrameRecovery::FromSnapshot(fp, sp, bailout->machine(), bailout->snapshotOffset()); This is exceptionally pretty. ::: js/src/ion/x86/MacroAssembler-x86.cpp @@ +144,5 @@ > inCall_ = false; > } > > +void > +MacroAssemblerX86::handleException() This is pleasant. ::: js/src/ion/x86/Trampoline-x86.cpp @@ +123,2 @@ > > // Save the stack size so we can remove arguments and alignment after the Above this line, the callee token is still pushed. But IonEntryFrameLayout doesn't have a calleeToken. One of these is wrong: I suspect IonEntryFrameLayout. @@ +238,5 @@ > > // Construct sizeDescriptor. > masm.subl(esp, ebp); > + masm.shll(Imm32(FRAMETYPE_BITS), ebp); > + masm.orl(Imm32(IonFrame_Rectifier), ebp); Should we make constructing a sizeDescriptor a function in the MacroAssembler? It might be nice to not have the logic splayed out in many locations. @@ +254,5 @@ > masm.call(eax); > > // Remove the rectifier frame. > masm.pop(ebp); // ebp <- sizeDescriptor with FrameType. > + masm.shrl(Imm32(FRAMETYPE_BITS), ebp); // ebp <- sizeDescriptor. Just "descriptor" now, I believe =] @@ +281,5 @@ > // Push the bailout table number. > masm.push(Imm32(frameClass)); > > + // Get the stack pointer into a register. This is the first argument to > + // ion:Bailout. nit: "// Stack pointer is the first argument to ion::Bailout()." @@ +290,5 @@ > masm.setABIArg(0, eax); > masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, Bailout)); > > + // Common size of all the stuff we've pushed since creating the external > + // frame. nit: -we've +frame <== fits on one line. ::: js/src/jscntxt.h @@ +216,5 @@ > PendingProxyOperation *pendingProxyOperation; > > ConservativeGCThreadData conservativeGC; > > + uint8 *ionTop; Comment on usage? It is not obvious that there is only one ionTop, given the possibility of nested ion/interp/ion/interp frames.

Comment on attachment 572396 [details] [diff] [review] part 5: x64 changes Review of attachment 572396 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/x64/Bailouts-x64.cpp @@ +46,5 @@ > > using namespace js; > using namespace js::ion; > > +#if defined(_WIN32) _WIN64? @@ +78,5 @@ > + > +} // namespace ion > +} // namespace js > + > +#if defined(_WIN32) _WIN64? ::: js/src/ion/x64/Trampoline-x64.cpp @@ +134,5 @@ > Push the number of bytes we've pushed so far on the stack and call > *****************************************************************/ > masm.subq(rsp, r14); > + masm.shlq(Imm32(FRAMETYPE_BITS), r14); > + masm.orl(Imm32(IonFrame_Entry), r14); masm.makeDescriptor(r14, IonFrame_Entry)? @@ +282,5 @@ > masm.reserveStack(FloatRegisters::Total * sizeof(double)); > for (uint32 i = 0; i < FloatRegisters::Total; i++) > masm.movsd(FloatRegister::FromCode(i), Operand(rsp, i * sizeof(double))); > > + masm.breakpoint(); Trace/breakpoint trap (core dumped) @@ +333,5 @@ > + // Store to ThreadData::ionTop. Note that rax still holds the return value > + // from ion::Bailout. > + > + masm.movq(ImmWord(JS_THREAD_DATA(cx)), rdx); > + masm.movq(rsp, Operand(rdx, offsetof(ThreadData, ionTop))); Is (JS_THREAD_DATA(cx) + offsetof(ThreadData, ionTop)) is already a compile-time constant? We could move it directly into %rdx. (Likewise for x86.)

Comment on attachment 572561 [details] [diff] [review] part 6: make ABI calls use ExitFrames Review of attachment 572561 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/shared/MacroAssembler-x86-shared.h @@ +121,5 @@ > + // Save an exit frame (which must be aligned to the stack pointer) to > + // ThreadData::ionTop. > + void linkExitFrame(Register scratch) { > + mov(ImmWord(JS_THREAD_DATA(GetIonContext()->cx)), scratch); > + mov(StackPointer, Operand(scratch, offsetof(ThreadData, ionTop))); This can be made more efficient without the use of a scratch register: "JS_THREAD_DATA(...) + offsetof(ThreadData, ionTop)" is a compile-time constant. ::: js/src/ion/x64/Assembler-x64.h @@ +370,5 @@ > JS_NOT_REACHED("unexpected operand kind"); > } > } > > + void mov(ImmWord word, const Register &dest) { "const ImmWord &word"? We should canonicalize on a passing style.

(In reply to Sean Stangl from comment #15) > ::: js/src/ion/shared/MacroAssembler-x86-shared.h > @@ +121,5 @@ > > + // Save an exit frame (which must be aligned to the stack pointer) to > > + // ThreadData::ionTop. > > + void linkExitFrame(Register scratch) { > > + mov(ImmWord(JS_THREAD_DATA(GetIonContext()->cx)), scratch); > > + mov(StackPointer, Operand(scratch, offsetof(ThreadData, ionTop))); > > This can be made more efficient without the use of a scratch register: > "JS_THREAD_DATA(...) + offsetof(ThreadData, ionTop)" is a compile-time > constant. Oops, I missed that this was in x86-shared. It can be written with a single move on x86, whereas x64 already has the ScratchReg.

Comment on attachment 572711 [details] [diff] [review] part 7: properly stack exit frames Review of attachment 572711 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/x64/MacroAssembler-x64.h @@ +448,5 @@ > + // Save an exit frame (which must be aligned to the stack pointer) to > + // ThreadData::ionTop. > + void linkExitFrame(Register scratch) { > + mov(ImmWord(JS_THREAD_DATA(GetIonContext()->cx)), scratch); > + mov(StackPointer, Operand(scratch, offsetof(ThreadData, ionTop))); Can be rewritten without a specific "scratch" register -- x86 doesn't need one, and x64 already has ScratchReg. Also the offsetof() can be moved into the first mov. ::: js/src/ion/x64/Trampoline-x64.cpp @@ +426,5 @@ > masm.setupUnalignedABICall(f.argc(), temp); > > // Initialize the context. > + masm.movq(ImmWord(JS_THREAD_DATA(cx)), ArgReg0); > + masm.movq(Operand(ArgReg0, offsetof(ThreadData, ionTop)), ArgReg0); ionJSContext masm.movq(ImmWord(((uint8*)JS_THREAD_DATA(cx)) + offestof(ThreadData, ionTop)), ScratchReg); masm.movq(Operand(ScratchReg), ArgReg0); I would expect this to be faster than the alternative. ::: js/src/ion/x86/MacroAssembler-x86.h @@ +397,5 @@ > void handleException(); > + > + // Save an exit frame (which must be aligned to the stack pointer) to > + // ThreadData::ionTop. > + void linkExitFrame(Register scratch) { Scratch register is unused. ::: js/src/ion/x86/Trampoline-x86.cpp @@ +445,5 @@ > masm.setupUnalignedABICall(f.argc(), temp); > > // Initialize the context. > Register cxreg = regs.takeAny(); > + masm.movl(Operand(&JS_THREAD_DATA(cx)->ionTop), cxreg); ionJSContext

> If we don't have enough memory to perform necessary moves, then the result > is nonsense. Should callWithABI() be made fallible? This sets enoughMemory=false, which the linker will see when it checks MacroAssembler::oom(). > > +FrameRecovery::FrameRecovery(uint8 *fp, uint8 *sp, const MachineState &machine) > > + : fp_((IonJSFrameLayout *)fp), > > Since IonJSFrameLayouts have certain known types, can we assert in the body > below that fp actually points to an IonJSFrameLayout? Hrm, I think we can only assert on the previous frame type, not the current one. > readFloatReg()? They're called FloatRegisters everywhere else. Maybe they > should be called something different. readFloatReg() is fine. I had used "Double" for parity with readDoubleSlot(). > readFloatSlot()? Or rename FloatRegister, as above. Erg. I'll save this for another day. I think DoubleRegister would be best for clarity. "FloatRegister" isn't meant to mean "32-bit float" but "Floating-Point Register". > nit: static functions typically start with a capital. Luke and njn have been making exceptions for the offsetOf ones, so they look sort of like the macro. > We should probably standardize on either "Register dest" or "const Register > &dest", since in practice it makes no difference, but having multiple styles > is annoying. I prefer the former. Yeah, "Register dest" is way better. Let's just delete the const and & incrementally. > > // Save the stack size so we can remove arguments and alignment after the > > Above this line, the callee token is still pushed. But IonEntryFrameLayout > doesn't have a calleeToken. The callee is pushed as part of the outgoing JS frame - the "entry frame" really doesn't exist, I think. It's just a stopping point. I'll delete the class if it's totally unused, I think it is. > Should we make constructing a sizeDescriptor a function in the > MacroAssembler? It might be nice to not have the logic splayed out in many > locations. Good idea - done.

> > +#if defined(_WIN32) > > _WIN64? _WIN32 is set on _WIN64 as well > > + masm.breakpoint(); > > Trace/breakpoint trap (core dumped) You can tell I do lots of testing :) > Is (JS_THREAD_DATA(cx) + offsetof(ThreadData, ionTop)) is already a > compile-time constant? We could move it directly into %rdx. (Likewise for > x86.) Yeah it is, I like having the pointer though. It's easier to debug. > Can be rewritten without a specific "scratch" register Done. > > // Initialize the context. > > + masm.movq(ImmWord(JS_THREAD_DATA(cx)), ArgReg0); > > + masm.movq(Operand(ArgReg0, offsetof(ThreadData, ionTop)), ArgReg0); > > ionJSContext Whoops, good catch.

http://hg.mozilla.org/projects/ionmonkey/rev/8284e6365e99 http://hg.mozilla.org/projects/ionmonkey/rev/27dda98f1ece http://hg.mozilla.org/projects/ionmonkey/rev/b54f9c27c458 http://hg.mozilla.org/projects/ionmonkey/rev/6aefeb68c04d http://hg.mozilla.org/projects/ionmonkey/rev/6778cd6941a1 http://hg.mozilla.org/projects/ionmonkey/rev/2251703b072e http://hg.mozilla.org/projects/ionmonkey/rev/29c5f1b61413 Filed bug 701984 for ARM