About cpu profiler DCHECK failed.

[Yahan Lu]

Hi all~

Cpu profiler could excute GetStackSample  and run stack StackFrameIterator.

But in riscv64/mips archs, Push operation is not atomic and consists of several instructions. For example:

  void Push(Register src1, Register src2) {

    Sub64(sp, sp, Operand(2 * kSystemPointerSize));

    Sd(src1, MemOperand(sp, 1 * kSystemPointerSize));

    Sd(src2, MemOperand(sp, 0 * kSystemPointerSize));

  }

If cpu profiler run GetStackSample after Sub64 but before Sd src1, then the value between sp and fp is undefined. So it causes a error:

 

#

# Fatal error in ../../src/execution/frames.h, line 184

# Debug check failed: static_cast<uintptr_t>(type) < Type::NUMBER_OF_TYPES (70049115717448 vs. 23).

#

#

#

#FailureMessage Object: 0x7ffcf54a26c0

The concrete example occurs in BaselineCompiler::Prologue():

After run EnterFrame(StackFrame::BASELINE);

Builtin kBaselineOutOfLinePrologue will Push(callee_context, callee_js_function); 

If cpu profiler run GetStackSample in Push(callee_context, callee_js_function) but before Sd(callee_context, sp + 8), will cause Debug check failed: static_cast<uintptr_t>(type) < Type::NUMBER_OF_TYPES.

Details:

sp: 0x7f177f207e08

fp:0x7f177f207e18 size: 16 

pc:0x7f178b874040

lr:0x7f177708309c

DD: 0x7f177f207e28 : 0x68c7101119

DD: 0x7f177f207e20 : 0x7f17770844f8

DD: 0x7f177f207e18 : 0x7f177f207e90

DD: 0x7f177f207e10 : 0x7f177f207e90

DD: 0x7f177f207e08 : 0x1c

DD: 0x7f177f207e00 : 0x68c7101de9

DD: 0x7f177f207df8 : 0x68c7101139

DD: 0x7f177f207df0 : 0xb02

DD: 0x7f177f207de8 : 0x68c711fc91

[Leszek Swirski]

Could we flip the order of operations here (first write the values into the red zone, and only then change the stack pointer) to make the push pseudo-atomic?

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/999d5008-6480-4cd3-905e-b91387e804e1n%40googlegroups.com.

[Yahan Lu]

If i  modify Push 

void Push(Register src1, Register src2) {

    Sd(src1, MemOperand(sp, -1 * kSystemPointerSize));

    Sd(src2, MemOperand(sp,  -2 * kSystemPointerSize));

    Sub64(sp, sp, Operand(2 * kSystemPointerSize));

  }

This error will not appear again.

But the new Push does not conform to the assembly manual.

[Leszek Swirski]

But the new Push does not conform to the assembly manual.

Is this an issue? It feels like a very pragmatic solution. 

[Yahan Lu]

May be not a issue. Just  not consistent with GCC LLVM. 

Thanks 😁

[Jiivy Qiu]

AFAIK, not only RISC-V has the red zone problem (mean in the prologue "first reduce sp then store register to stack"), MIPS/MIPS64/PPC/S390/LOONG as well as ARM64 all have the same assemble sequence. 

------------------------------------------------------------------------  How mips64 PUSH like ------------------------------------------------------------------------  

  // Push two registers. Pushes leftmost register first (to highest address).

  void Push(Register src1, Register src2) {

    Dsubu(sp, sp, Operand(2 * kPointerSize));

    Sd(src1, MemOperand(sp, 1 * kPointerSize));

    Sd(src2, MemOperand(sp, 0 * kPointerSize));

  }

------------------------------------------------------------------------ mips is quite the same,  sd is replaced by sw , so does the Loong ISA ------------------------------------------------------------------------ 

------------------------------------------------------------------------  How s390 PUSH like ------------------------------------------------------------------------- 

  // Push two registers.  Pushes leftmost register first (to highest address).

  void Push(Register src1, Register src2) {

    lay(sp, MemOperand(sp, -kSystemPointerSize * 2)); 

    StoreU64(src1, MemOperand(sp, kSystemPointerSize));

    StoreU64(src2, MemOperand(sp, 0)); 

  }

------------------------------------------------------------------------ How ARM64 PUSH like ------------------------------------------------------------------------ 

void TurboAssembler::PushHelper(int count, int size, const CPURegister& src0,

                                const CPURegister& src1,

                                const CPURegister& src2,

                                const CPURegister& src3) {

  // Ensure that we don't unintentially modify scratch or debug registers.

  InstructionAccurateScope scope(this);

  DCHECK(AreSameSizeAndType(src0, src1, src2, src3));

  DCHECK(size == src0.SizeInBytes());

  // When pushing multiple registers, the store order is chosen such that

  // Push(a, b) is equivalent to Push(a) followed by Push(b).

  switch (count) {

    case 1:

      DCHECK(src1.IsNone() && src2.IsNone() && src3.IsNone());

      str(src0, MemOperand(sp, -1 * size, PreIndex));

      break;

    case 2:

      DCHECK(src2.IsNone() && src3.IsNone());

      stp(src1, src0, MemOperand(sp, -2 * size, PreIndex));

      break;

    case 3:

      DCHECK(src3.IsNone());

      stp(src2, src1, MemOperand(sp, -3 * size, PreIndex));

      str(src0, MemOperand(sp, 2 * size));

      break;

    case 4:..

-------

In the above Push code from mips64/s390 and arm64, we can see they all have the red zone.  So it's perhaps not unique for riscv. 

So maybe we should take a look into the SafeStackFrameIterator and try to skip the corrupt stack frame if the sampled position is just at the red zone? 

[Leszek Swirski]

That's also a reasonable option, thanks for pointing that out

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/d9f35430-105b-423b-86de-5ddf1b863bcdn%40googlegroups.com.

[Brice Dobry]

I agree with Qiuji. The fact that all of those other targets, including ARM, also decrement SP first tells me that we should not unilaterally make this change in RISC-V.