781979 - IonMonkey: Provide line number information to SPS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[u443197]

This has been done with JaegerMonkey in bug 778979, now it should be done with IonMonkey

Comment 1

[u443197]

Attachment: Part 1: Move SPSInstrumentation to a template and rework the interface

Comment 2

[u443197]

Attachment: Part 2: Bring IonMonkey back up to speed

Comment 3

[u443197]

Turns out SPS is dying while stack walking because %rbp is getting trashed, hence the dependence on 736299. I tested out SPS with stack walking disabled, and assertions aren't being tripped left and right (or at all!).

Comment 4

[u443197]

Attachment: Part 3: Don't die on erroneous frame pointers when stack walking

Comment 5

[u443197]

After discussion with BenWa and looking at a few examples, we decided it's probably best to actually respect the frame pointer instead of just hoping sps can deal with it.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 652230 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed

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

1/ Decouple the MASM from the profiling, at best add an assertion which ensure that we are profiling calls correctly.
2/ Replace LCallBoundary by an SPSCallBoundary class which use a constructor and a destructor to update the profiler stack around a call.
3/ Ensure that you are only using the call boundary once per call.
3/ VM functions calls are not covered by the current mechanism and the handleException of the VM wrapper will not pop all frames from the profiler stack.

Nice approach and nice work, but still this plenty of corner cases to fix before being acceptable.

::: js/src/ion/IonMacroAssembler.h
@@ +31,1 @@
>   

nit: remove trailing whitespace.

@@ +482,5 @@
> +    void callWithABI(void *fun, Result result = GENERAL) {
> +        // There aren't any guaranteed non-live registers here, so if we need to
> +        // use one for instrumentation it must be pushed/poppped around the
> +        // instrumentation.
> +        if (sps) {

In general I would prefer to avoid such instrumentation in the macro assembler, but doing otherwise sounds more error prone because we can easily forget to add the call boundary.

Another solution to prevent such instrumentation while not forgetting to add the call boundary would be to add an assertion to ensure that any profiled call is under a call scope.

@@ +485,5 @@
> +        // instrumentation.
> +        if (sps) {
> +            reserveStack(STACK_SLOT_SIZE);
> +            storePtr(CallTempReg0, Address(StackPointer, 0));
> +            sps->leave(*pc_, *this, CallTempReg0);

nit: See comment below.  You can do such instrumentation by adding a class with a constructor and destructor to produce this pieces of assembly.  Also you might want to assert that no other Call scope is around a callWithABI.

@@ +490,5 @@
> +            loadPtr(Address(StackPointer, 0), CallTempReg0);
> +            freeStack(STACK_SLOT_SIZE);
> +        }
> +
> +        archCallWithABI(fun, result);

nit:  Don't rename the symbols, just use  MacroAssemblerSpecific::callWithABI.

@@ +495,5 @@
> +
> +        if (sps) {
> +            reserveStack(STACK_SLOT_SIZE);
> +            storePtr(CallTempReg0, Address(StackPointer, 0));
> +            sps->reenter(*this, CallTempReg0);

nit: After a call you can use any register which is not used for returning a value from the call.  Which means not ReturnReg, and not JS_ReturnReg*.

@@ +506,5 @@
> +        if (sps) {
> +            // We're going to leave this function, so there's no need to emit
> +            // reentry instrumentation.
> +            sps->skipNextReenter();
> +            IonInstrumentation::CallScope cs(sps, *pc_, *this, CallTempReg2);

CallTempReg2 ?
Is this code removing all frames of the current Ion activation ?

@@ +510,5 @@
> +            IonInstrumentation::CallScope cs(sps, *pc_, *this, CallTempReg2);
> +            archHandleException();
> +        } else {
> +            archHandleException();
> +        }

nit:

    …
    IonInstrumentation::CallScope cs(sps, *pc_, *this, CallTempReg2);
}
MacroAssemblerSpecific::handleException()

::: js/src/ion/Lowering.cpp
@@ +225,5 @@
>      uint32 argslot = getArgumentSlotForCall();
>      JSFunction *target = call->getSingleTarget();
>  
> +    if (gen->instrumentedProfiling() &&
> +        !add(new LCallBoundary(tempFixed(CallTempReg0), true), call))

We usually produce only one LIR node for each MIR node.  I don't think you need to handle this with a LIR instruction.  A call boundary can be instrumented with a simple class which create the leave at the construction and the reenter at the destruction, such as:

{
  CallBoundary(sps, &temp, call);

  … calling code …
}

@@ +232,3 @@
>      if (target && target->isNative()) {
>          if (call->isDOMFunction()) {
>              LCallDOMNative *lir = new LCallDOMNative(argslot,

This LIR is using callWithABI to implement it's function call, by creating the CallBoundaries instead of using a CallBoundary class, you will create 2 call boundaries where only one is needed.

@@ +274,5 @@
>      freeArguments(call->numStackArgs());
> +
> +    if (gen->instrumentedProfiling() &&
> +        !add(new LCallBoundary(tempFixed(CallTempReg0), false), call))
> +        return false;

nit: same here.

::: js/src/ion/MIR.h
@@ +282,5 @@
>      static void PrintOpcodeName(FILE *fp, Opcode op);
>      virtual void printOpcode(FILE *fp);
>  
> +    void setTrackedPc(jsbytecode *pc) {
> +        trackedPc_ = pc;

Why removing the check which ensure that we do not define the trackedPc multiple times ?

::: js/src/ion/x86/MacroAssembler-x86.cpp
@@ +139,1 @@
>      

nit: remove trailing whitespace.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 652230 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed

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

Additional Re-review:
- correction: CallVM are handled correctly with the CallScope function.
- Modifications made in ion::HandleException (IonFrames.cpp) to pop SPS stack, seen on Alex screen, are not present in this patch.

::: js/src/ion/shared/CodeGenerator-shared.cpp
@@ +347,5 @@
>      IonCode *wrapper = ion->generateVMWrapper(cx, fun);
>      if (!wrapper)
>          return false;
>  
> +    IonInstrumentation::CallScope cs(&sps, lastPC, masm, CallTempReg2);

This line register the last registered PC into the last SPS entry for all CallVM.  So call to VM function are handled as opposed as my previous comment.  The only detail is that this callVM function might be call inside an instruction such as visitCallGeneric which is already wrapped.

Comment 8

[u443197]

Attachment: Part 2: Bring IonMonkey back up to speed (v2)

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #6)

> Another solution to prevent such instrumentation while not forgetting to add
> the call boundary would be to add an assertion to ensure that any profiled
> call is under a call scope.

The assertion in place, and work has been done to make sure it doesn't trip.
This is for both callWithABI and handleException.

> nit:  Don't rename the symbols, just use 
> MacroAssemblerSpecific::callWithABI.

I decided against this because then each specific assembler would have to have a constructor which takes the instrumentation as an argument which seemed like unnecessary duplication.

> We usually produce only one LIR node for each MIR node.  I don't think you
> need to handle this with a LIR instruction.  A call boundary can be
> instrumented with a simple class which create the leave at the construction
> and the reenter at the destruction, such as:
> 
> {
>   CallBoundary(sps, &temp, call);
> 
>   … calling code …
> }

The LCallBoundary class no longer exists.

> Why removing the check which ensure that we do not define the trackedPc
> multiple times ?

When LICM moved code around, I had to always make sure that the listed pc for an instruction always fell within range of the current script's code. This check prevented this when code was moved, so I removed it.


Overall, this patch has a lot more 'IonInstrumentation::CallScope' objects all over the place, and I was running into some crashes when running in the browser of the pc index being wildly out of bounds (turned out to be OOL instructions). I put in some extra work to make sure that the 'pc' of an OOL instruction is sane.

This also has the fix nbp and I talked about with MPolyInlineDispatch nodes with a fallback by adding a new basic block and some extra phis.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 653873 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed (v2)

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

Sorry, I did not noticed before, but CallScope is a nice C++ trick but as briefly commented here it is error prone because the register given at the creation of the callScope is used implicitly when the callScope is destructed and this does not show up in the code generator.  As you told me in person, this sounded to be fine in the test suite which means that errors I caught by reviewing are effort were not catch by the test suite, and I cannot accept such things knowing that this is not obvious to track.

So you should instrument the masm in IonMacroAssembler, even if I don't like this solution, by overwrite the function and calling the parent class methods with the explicit name of the parent class.  Keep the code minimal in the overwritten version, such as:

  void callWithABI(…) {
    SampleProfiler::leaveJS(&sps, pc);
    MacroAssemblerSpecific::callWithABI(…);
    SampleProfiler::reenterJS(&sps, pc, unusedAfterCallReg);
  };

::: js/src/ion/IonMacroAssembler.h
@@ +477,5 @@
> +
> +    // Emits a call to a C/C++ function, resolving all argument moves.
> +    void callWithABI(void *fun, Result result = GENERAL) {
> +        JS_ASSERT_IF(sps && sps->enabled(), sps->left());
> +        archCallWithABI(fun, result);

nit: Do not use the arch prefix, use MacroAssemblerSpecific:: instead.

::: js/src/ion/shared/CodeGenerator-shared.cpp
@@ +358,5 @@
>      IonCode *wrapper = ion->generateVMWrapper(cx, fun);
>      if (!wrapper)
>          return false;
>  
> +    IonInstrumentation::CallScope cs(&sps, lastPC, masm, CallTempReg2);

Please use/create another static const which is by definition differnt than the JS_ReturnReg / ReturnReg

@@ +427,5 @@
> +    // Normally saveVolatile would be used, but the instrumentation needs to get
> +    // some register so instead just keep track of the relevant registers
> +    RegisterSet regs = RegisterSet::Volatile();
> +    regs.maybeTake(dest);
> +    masm.PushRegsInMask(regs);

It seems like you just want to make saveVolatile return one of the unused registers by calling getAny inside the saveVolatile.

@@ +439,1 @@
>      masm.storeCallResult(dest);

move storeCallResult inside the previous scope because the C return register is a volatile register which might be the register returned by .getAny().

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +1277,1 @@
>          masm.storeCallResult(output);

same here.

::: js/src/ion/x86/MacroAssembler-x86.cpp
@@ +139,1 @@
>      

nit: trailing whitespace.

Comment 10

[u443197]

Attachment: Part 2: Bring IonMonkey back up to speed (v3)

I might have overlooked a thing here or there, but this is the bulk of the changes minus future minor tweaks. It failed 32-bit try last night before I added some extra push logic in reenterAfterCall(), and I'm running that through try again.

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 654209 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed (v3)

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

Apply nits and go to land.

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +210,5 @@
>  IonCompartment::generateArgumentsRectifier(JSContext *cx)
>  {
> +    // Don't use rbp for this function because if profiling is enabled it will
> +    // cause stack walkers to get very confused.
> +

nit: // Do not erase the frame pointer in this function.

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +212,5 @@
>  IonCompartment::generateArgumentsRectifier(JSContext *cx)
>  {
> +    // Don't use ebp for this function because if profiling is enabled it will
> +    // cause stack walkers to get very confused.
> +

nit: Remove this comment since you are using it your-self.  Then you might want to rename ebp to FramePointer to ensure no-body erase it.

@@ +270,5 @@
>          masm.j(Assembler::NonZero, &copyLoopTop);
>      }
>  
>      // Construct descriptor.
> +    masm.movl(ebp, ebx);

masm.lea(Address(ebp, 4), ebx); // to account for the frame pointer in the frame size.

Comment 12

[u443197]

Attachment: Part 2: Bring IonMonkey back up to speed (actual v3)

Accidentally uploaded the complete wrong patch previously, but I will also take those into account!

Comment 13

[Nicolas B. Pierron [:nbp]]

Comment on attachment 654269 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed (actual v3)

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

This patch sounds good, except that modifications made to HandleException are still lacking in this patch — hg queues issues ?
Apply nits, include IonFrames.cpp modifications in this patch, ask :djvj to review IonBuilder.cpp and ask me again to review this patch with HandleException modifications.

::: js/src/ion/IonMacroAssembler.h
@@ +495,5 @@
> +        if (sps)
> +            sps->skipNextReenter();
> +        leaveBeforeCall();
> +        MacroAssemblerSpecific::handleException();
> +        reenterAfterCall();

Is the reenter needed after all, because this instruction will still produce some assembly.

@@ +559,5 @@
> +            return;
> +        GeneralRegisterSet regs(Registers::TempMask);
> +        Register r = regs.getAny();
> +        reserveStack(sizeof(void*));
> +        storePtr(r, Address(StackPointer, 0));

nit: see below.

@@ +562,5 @@
> +        reserveStack(sizeof(void*));
> +        storePtr(r, Address(StackPointer, 0));
> +        sps->leave(*pc_, *this, r);
> +        loadPtr(Address(StackPointer, 0), r);
> +        freeStack(sizeof(void*));

nit: see below.

@@ +574,5 @@
> +        if (regs.empty()) {
> +            regs = GeneralRegisterSet(Registers::TempMask);
> +            Register r = regs.getAny();
> +            reserveStack(sizeof(void*));
> +            storePtr(r, Address(StackPointer, 0));

nit: use push instead of reserveStack & storePtr.

@@ +577,5 @@
> +            reserveStack(sizeof(void*));
> +            storePtr(r, Address(StackPointer, 0));
> +            sps->reenter(*this, r);
> +            loadPtr(Address(StackPointer, 0), r);
> +            freeStack(sizeof(void*));

nit: and pop instead of loadPtr & freeStack.

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #13)
> Comment on attachment 654269 [details] [diff] [review]
> Part 2: Bring IonMonkey back up to speed (actual v3)
> 
> Review of attachment 654269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch sounds good, except that modifications made to HandleException
> are still lacking in this patch — hg queues issues ?
> Apply nits, include IonFrames.cpp modifications in this patch, ask :djvj to
> review IonBuilder.cpp and ask me again to review this patch with
> HandleException modifications.

Ok, my mistake, HandleException modifications are already in trunk.

Comment 15

[Nicolas B. Pierron [:nbp]]

Comment on attachment 654269 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed (actual v3)

djvj, I'm forwarding the review of the inlining logic (ionBuilder.cpp), in case I miss something.

Comment 16

[Kannan Vijayan [:djvj]]

Comment on attachment 654269 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed (actual v3)

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

The IonBuilder.cpp changes look good.  Good work.

Comment 17

[u443197]

http://hg.mozilla.org/projects/ionmonkey/rev/2d190481b4a4
http://hg.mozilla.org/projects/ionmonkey/rev/01c69de69dc3

Comment 18

[Guilherme Lima]

Resolved Fixed, not Worksforme, right?

Comment 19

[u443197]

I apparently no longer have the option of FIXED, so I chose the next best one. Not sure why I lost it?