1118826 - Refactor JitFrameIterator::baselineScriptAndPc

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

JitFrameIterator::baselineScriptAndPc has become more and more complicated over time. Right now it tries the following steps to determine the bytecode pc:

- frame->getUnwoundScopeOverridePc()
- frame->getDebugModeOSRInfo()->pc
- prologueEntryAddr / postDebugPrologueAddr => script->code()
- maybeICEntryFromReturnAddress
- pcForReturnAddress

Especially pcForReturnAddress is confusing, because it uses the script's bytecode pc <-> native pc mapping, and multiple native code addresses can map to the same bytecode pc. This has lead to a number of bugs, see bug 1115844 and others.

The attached patch simplifies this to just:

- frame->getOverridePc, if any
- icEntryFromReturnAddress

So whenever a frame has a native code address that does not have a corresponding ICEntry, we have to save the pc in the frame. This happens only in a few cases: FinishBailoutToBaseline, generator throw/close and debugger stuff. This "override pc" must be cleared whenever we return to JIT code. The patch emits a check for this in debug builds after each Baseline callVM.

The patch also cleans up some other code and I think it's an improvement over what we have now.

Shu, asking you to review this because of the debugger changes, let me know if you can think of cases this patch doesn't handle. There's also an XXX comment in BaselineDebugModeOSR.cpp.

Comment 1

[Jan de Mooij [:jandem]]

I verified this also fixes the problem in bug 1115844. I'll land the patch there for now, because it's simple and we probably want to backport it, but we can undo those changes after this lands.

Comment 2

[Shu-yu Guo [:shu]]

Sorry for the delay, I'll get to the review tomorrow.

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 8545321 [details] [diff] [review]
Patch

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

Awesome simplification!

Let me know if removing the DebugModeOSR parts turns out crashy.

::: js/src/jit/BaselineBailouts.cpp
@@ +1057,5 @@
>                  //
>                  // Note that we never resume into this address, it is set for
>                  // the sake of frame iterators giving the correct answer.
>                  jsbytecode *throwPC = script->offsetToPC(iter.pcOffset());
>                  nativeCodeForPC = baselineScript->nativeCodeForPC(script, throwPC);

I don't think we need to set the native code address here anymore; perhaps set a sentinel value? (see comment below in DebugModeOSR.cpp)

::: js/src/jit/BaselineCompiler.cpp
@@ -3699,5 @@
>      } else {
>          MOZ_ASSERT(resumeKind == GeneratorObject::THROW || resumeKind == GeneratorObject::CLOSE);
>  
>          // Update the frame's frameSize field.
> -        Register scratch3 = regs.takeAny();

Nice, less registers needed.

::: js/src/jit/BaselineDebugModeOSR.cpp
@@ +443,5 @@
> +                // Store the PC in the BaselineFrame so frame iterators continue
> +                // to work. This PC will be cleared when HandleException
> +                // returns.
> +                //
> +                // XXX: what should we do with previous code ^? Still necessary?

I'm think this code is okay to be removed. JitFrameIterator doesn't uses the cached returnAddressToFp_ for anything other than pc lookup, and we're guaranteed to have an override pc here. Perhaps we can set a sentinel return address (0x0?) and see if any tests fail?

The setOverridePc shouldn't be needed here either. In the Baseline->Baseline case, DebugModeOSR doesn't mess with the frames. So the override pc already set on the frame should still be the same. In the Ion->Baseline case, FinishBailoutToBaseline should never clear the override pc in the case of bailing-in-place for DebugModeOSR, since that goes straight back into HandleExceptionBaseline. Asserting that the baseline frame already has the same override pc would be better.

Moreover, the call to pcForNativeAddress under CollectJitStackScripts can be changed in favor of reading the override pc directly.

::: js/src/jit/JitFrames.cpp
@@ +240,3 @@
>      uint8_t *retAddr = returnAddressToFp();
> +    ICEntry &icEntry = script->baselineScript()->icEntryFromReturnAddress(retAddr);
> +    *pcRes = icEntry.pc(script);

such clean, very simple, wow

::: js/src/jit/shared/BaselineCompiler-shared.cpp
@@ +105,5 @@
> +    masm.branchTest32(Assembler::Zero, frame.addressOfFlags(),
> +                      Imm32(BaselineFrame::HAS_OVERRIDE_PC), &ok);
> +    masm.assumeUnreachable("BaselineFrame shouldn't override pc when executing JIT code!");
> +    masm.bind(&ok);
> +#endif

Do you think it'd be useful to also assert that the override pc is not set _before_ calling into the VM? In case future changes to the bailout code mess up invariants.

Comment 4

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c213d9d53886

Thanks for the careful review.

(In reply to Shu-yu Guo [:shu] from comment #3)
> I don't think we need to set the native code address here anymore; perhaps
> set a sentinel value? (see comment below in DebugModeOSR.cpp)

Nice, I changed it to nullptr.

> I'm think this code is okay to be removed. JitFrameIterator doesn't uses the
> cached returnAddressToFp_ for anything other than pc lookup, and we're
> guaranteed to have an override pc here. Perhaps we can set a sentinel return
> address (0x0?) and see if any tests fail?

Changed this to nullptr as well.

> The setOverridePc shouldn't be needed here either. In the Baseline->Baseline
> case, DebugModeOSR doesn't mess with the frames. So the override pc already
> set on the frame should still be the same.

Good point, I removed the setOverridePc call and we now assert it's the same pc.

> Moreover, the call to pcForNativeAddress under CollectJitStackScripts can be
> changed in favor of reading the override pc directly.

Ah, also a nice simplification. With that there's one pcForNativeAddress call left, in JitcodeMap. Maybe we should rename this function to approximatePcForNativeAddress or something, to indicate it's not very reliable for ops that generate no code.. Will think about it.

> Do you think it'd be useful to also assert that the override pc is not set
> _before_ calling into the VM? In case future changes to the bailout code
> mess up invariants.

Done.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Remove more code

This patch removes the pcForReturnOffset, pcForReturnAddress and pcForNativeOffset methods from BaselineScript.

pcForNativeAddress is renamed to approximatePcForNativeAddress, to indicate it's OK to use for profiling etc but shouldn't be used for anything that requires an exact pc.

The patch also removes some nops from BaselineCompiler that we no longer need I think.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c213d9d53886

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8547105 [details] [diff] [review]
Part 2 - Remove more code

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

::: js/src/jit/JitcodeMap.cpp
@@ +78,5 @@
>      MOZ_ASSERT(containsPointer(ptr));
>      MOZ_ASSERT(script_->hasBaselineScript());
>  
> +    uint8_t *addr = reinterpret_cast<uint8_t*>(ptr);
> +    jsbytecode *pc = script_->baselineScript()->approximatePcForNativeAddress(script_, addr);

ISTM this still has the problem of distinguishing return-from-IC-addresses from start-of-op addresses, which could be pretty bad for profiling (especially for non-leaf frames, which we always want to associate with the return-from-IC address).

Perhaps we should consult ICEntry return addresses as well in that function and have that take higher priority than the PCMappingEntries (like in the old baselineScriptAndPc). It's too bad that that's part of the very complexity you were cleaning up here, but at least now it would be isolated to a profiler-specific function.

I'm going to NI Kannan here for his opinion as well.

Comment 8

[Shu-yu Guo [:shu]]

See comment 7.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Shu-yu Guo [:shu] from comment #7)
> ISTM this still has the problem of distinguishing return-from-IC-addresses
> from start-of-op addresses, which could be pretty bad for profiling
> (especially for non-leaf frames, which we always want to associate with the
> return-from-IC address).

Can you give an example of the non-leaf frame case where this would be a problem?

> Perhaps we should consult ICEntry return addresses as well in that function
> and have that take higher priority than the PCMappingEntries (like in the
> old baselineScriptAndPc). It's too bad that that's part of the very
> complexity you were cleaning up here, but at least now it would be isolated
> to a profiler-specific function.

Yeah, I really don't want to bring back the maybeICEntryFromReturnAddress stuff (part 1 removed that function...)

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8547105 [details] [diff] [review]
Part 2 - Remove more code

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

Agreed on IRC we can land this now and file a followup bug for profiler usage.

Comment 11

[Shu-yu Guo [:shu]]

Clearing NI for djvj. Jan and I talked it out on IRC, turns out there's no problem since in Baseline the return-from-IC address for calls will never be the same as the start-of-op address for the next op, due to fully syncing the stack around the call.

Comment 12

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f7a1f6e23c3

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f7a1f6e23c3

Comment 14

[Jan de Mooij [:jandem]]

Setting milestone to mozilla37 because the first part is the main one and it landed in that cycle.