Bailout code is a little bit-rotted with the introduction of new features such as spread calls and super calls. Do some cleanup before landing those features.

Comment on attachment 8852770 [details] Bug 1351951 - Cleanup InitFromBailout to fight bit-rot https://reviewboard.mozilla.org/r/124930/#review127580 Nice cleanup! ::: js/src/jit/BaselineBailouts.cpp:885 (Diff revision 1) > uint32_t inlined_args = 0; > if (op == JSOP_FUNCALL) > inlined_args = 2 + GET_ARGC(pc) - 1; > else if (op == JSOP_FUNAPPLY) > inlined_args = 2 + blFrame->numActualArgs(); > - else > + else // if (IsGetPropPC(pc) || IsSetPropPC(pc)) Nit: I know it's implied by the code before this, but the bailout code is tricky enough that I'd prefer the more explicit MOZ_ASSERT(IsGetPropPC(pc) || IsSetPropPC(pc)); (And add braces to all branches of ths if-else statement.) ::: js/src/jit/BaselineBailouts.cpp:1088 (Diff revision 1) > - uint32_t numCallArgs = isCall ? GET_ARGC(pc) : 0; > + uint32_t numUses = js::StackUses(script, pc); > + uint32_t numCallArgs = numUses - 2 - uint32_t(pushedNewTarget); Can we now move numCallArgs into the IsCallPC(pc) branch below, where we use it? I was wondering about this underflowing for JSOP_GETPROP but then I noticed we only use it for calls. (I guess for numUses that doesn't work because we do |pc = GetNextPc(pc);| after this. I think we should have used different/immutable pc pointers so we don't have to worry about the ordering, but that can wait.)

Thanks for the quick review. Good catch on the underflow confusion. There is another patch queue on the way for further optional cleanup. This covers the correctness bugs that will appear as soon as spread call lands. I also agree that pc should be immutable and will put together a patch for it.

Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6b183e0bc3e3 Cleanup InitFromBailout to fight bit-rot

Avoid inserting junk assembly in debug mode just to please assertions. Be more explicit about the stub frame handling. Optional cleanup for clarity and maintainability. Feel free to r- if you don't like the risk.

Cleanup GetStubReturnAddress handling. This code was fragile and annoying to add more landing sites (such as for spreadcalls). Another optional cleanup.

Comment on attachment 8853143 [details] [diff] [review] Bug-1351951-Add-ICStubCompiler-assumeStubFrame.patch Review of attachment 8853143 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense, definitely cleaner to have this in one place.

Comment on attachment 8853145 [details] [diff] [review] Bug-1351951-Cleanup-GetStubReturnAddress-handling.patch Review of attachment 8853145 [details] [diff] [review]: ----------------------------------------------------------------- Beautiful. ::: js/src/jit/BaselineIC.cpp @@ +1656,5 @@ > void > ICSetProp_Fallback::Compiler::postGenerateStubCode(MacroAssembler& masm, Handle<JitCode*> code) > { > + BailoutReturnStub kind = BailoutReturnStub::SetProp; > + void * address = code->raw() + bailoutReturnOffset_.offset(); Nit: s/void */void*/ ::: js/src/jit/Ion.cpp @@ +644,5 @@ > stubCodes_->sweep(); > cacheIRStubCodes_->sweep(); > > + // If the sweep removed a bailout Fallback stub, nullptr the corresponding return addr. > + for (auto &it : bailoutReturnStubInfo_) Nit: auto& it, add {} to the loop ::: js/src/jit/JitCompartment.h @@ +470,5 @@ > // Keep track of offset into various baseline stubs' code at return > // point from called script. > + struct BailoutReturnStubInfo > + { > + void * addr; Nit: void*. (Also personally I don't align the names of fields. Some people do and I don't care too much, but it can be a hassle when at some point we want to add a field with a longer type name and we have to fix them all up :)

Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb3516cde53 Add ICStubCompiler::assumeStubFrame https://hg.mozilla.org/integration/mozilla-inbound/rev/beeac308b684 Cleanup GetStubReturnAddress handling

https://hg.mozilla.org/mozilla-central/rev/6cb3516cde53 https://hg.mozilla.org/mozilla-central/rev/beeac308b684