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.
On Wednesday, August 25, 2021 at 8:28:14 AM UTC-4 [email protected] wrote: > That's also a reasonable option, thanks for pointing that out > > On Wed, Aug 25, 2021 at 2:27 PM Jiivy Qiu <[email protected]> wrote: > >> 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? >> >> >> 在2021年8月25日星期三 UTC+8 下午7:00:39<[email protected]> 写道： >> >>> 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? >>> >>> On Wed, Aug 25, 2021 at 12:58 PM Yahan Lu <[email protected]> wrote: >>> >>>> 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() >>>> <https://source.chromium.org/chromium/chromium/src/+/main:v8/src/baseline/riscv64/baseline-compiler-riscv64-inl.h;l=16?q=BaselineCompiler::Prologue()&ss=chromium%2Fchromium%2Fsrc:v8%2F> >>>> : >>>> >>>> After run EnterFrame(StackFrame::BASELINE); >>>> Builtin kBaselineOutOfLinePrologue will >>>> Push(callee_context, callee_js_function) >>>> <https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/riscv64/builtins-riscv64.cc;l=1135> >>>> ; >>>> >>>> If cpu profiler run GetStackSample in >>>> Push(callee_context, callee_js_function) >>>> <https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/riscv64/builtins-riscv64.cc;l=1135> >>>> 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 >>>> >>>> >>>> >>>> >>>> -- >>>> -- >>>> v8-dev mailing list >>>> [email protected] >>>> 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 [email protected]. >>>> To view this discussion on the web visit >>>> https://groups.google.com/d/msgid/v8-dev/999d5008-6480-4cd3-905e-b91387e804e1n%40googlegroups.com >>>> >>>> <https://groups.google.com/d/msgid/v8-dev/999d5008-6480-4cd3-905e-b91387e804e1n%40googlegroups.com?utm_medium=email&utm_source=footer> >>>> . >>>> >>> -- >> -- >> v8-dev mailing list >> [email protected] >> 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 [email protected]. >> > To view this discussion on the web visit >> https://groups.google.com/d/msgid/v8-dev/d9f35430-105b-423b-86de-5ddf1b863bcdn%40googlegroups.com >> >> <https://groups.google.com/d/msgid/v8-dev/d9f35430-105b-423b-86de-5ddf1b863bcdn%40googlegroups.com?utm_medium=email&utm_source=footer> >> . >> > -- -- v8-dev mailing list [email protected] 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/5e5fc3d2-01c1-42b9-8157-2660ecb3c5d4n%40googlegroups.com.
