[wasm] Fix stack shrinking during tail calls on growable stacks [v8/v8 : main]
Francis McCabe (Gerrit)
Francis McCabe added 17 comments![]( "Open in Gerrit")
Made a few improvements and implemented on missing architectures.
call. This deallocates the secondary segment and ensures stackFrancis McCabeThis doesn't actually deallocate it, we keep it around so that we can reuse it in case we grow again. Shrinking just switches back to the previous segment.
Acknowledged
grew the stack), we must shrink the stack before executing the tail
call. This deallocates the secondary segment and ensures stackFrancis McCabeCan you explain why? I.e. because otherwise, the tail callee overwrites the frame type slot containing WASM_SEGMENT_START with a new WASM frame type. So when the function returns, it sees the WASM frame type and does not correctly switch back to the previous segment (doesn't update the stack limit, doesn't copy the stack returns, etc.). I think it can also confuse the stack walker.
Acknowledged
if (v8_flags.wasm_growable_stacks) {Francis McCabePlease add a high-level comment to explain why we need to shrink before the tail call.
Acknowledged
// Check if we need to shrink stack (marker at sp - 24)
__ Ldr(scratch, MemOperand(sp, -24));Francis McCabePlease consider using `StandardFrameConstants::kContextOrFrameTypeOffset` (fp-relative) to avoid using a magic number.
Acknowledged
__ PushCPURegList(gp_regs);Francis McCabeThere is a DCHECK inside this call:
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/codegen/arm64/macro-assembler-arm64.cc;drc=0dfa33ccbd59150e54c81dee86e42a4df6232a83;l=65Which I'm hitting when I try to run regress-520827581.js on arm64.
I think we need to use a padding reg here to ensure that we push an even number of them.
After adding this padding reg, I played around with regress-520827581.js, and I noticed that it crashes if I add `--no-wasm-inlining`.
Can you confirm these two issues locally?
Acknowledged
// Call wasm_shrink_stackFrancis McCabeSelf-explanatory, please remove.
Acknowledged
// Temporarily restore sp to Segment 1 stack to pop registersFrancis McCabeFull stop at the end of comments (here and in several other places).
Acknowledged
// Copy return address (saved lr in x20) to Segment 0 return address
// slot? Wait, on arm64 we don't need to write lr to stack, but we must
// restore it to lr register!Francis McCabeThis comment sounds like an AI talking to itself, can we rephrase it?
Acknowledged
__ Mov(lr, x20);Francis McCabe`x20` is uninitialized here. Is there supposed to be a `__ Mov(x20, lr)` before the C call?
Acknowledged
__ movq(r15, Operand(rsp, 0));
__ movq(r12, rsp);Francis McCabeWhat if `r12` and `r15` hold arguments or something? Shouldn't we preserve them and restore them from the stack, like `x19` and `x20` on arm64?
Acknowledged
__ jmp(&done_prepare);Francis McCabeWhy not just fall through?
At this point, if the stack has shrunk, both rbp and rsp have been calculated based on shrinking.
if (v8_flags.wasm_growable_stacks) {Francis McCabeSame for Liftoff: let's add a comment to explain why we need to shrink here.
Acknowledged
// Flags: --experimental-wasm-wasmfx
// Flags: --allow-natives-syntax --fuzzing --future --wasm-staging
// Flags: --wasm-stack-switching-stack-size=291 --experimental-wasm-growable-stacks
// Flags: --stress-wasm-stack-switching --experimental-wasm-wide-arithmeticFrancis McCabeFYI: we are dropping the `--experimental` prefix from wasm feature flags. We have aliases at the moment so users can migrate smoothly. But we should use `--wasm-<feature>` from now on.
Acknowledged
// Flags: --allow-natives-syntax --experimental-wasm-growable-stacks --expose-gc --trace-wasm-stack-switchingFrancis McCabeThe tracing flag is not needed for the test and will just slow it down.
Acknowledged
// Flags: --allow-natives-syntax --experimental-wasm-growable-stacks --expose-gc --trace-wasm-stack-switchingFrancis McCabesame here: `--wasm-growable-stacks`
Acknowledged
.addLocals(kWasmI32, 10000)Francis McCabeWe need something more reliable for Turboshaft, the locals will be eliminated if they are unused.
We have several tests now.
Related details
- Ryan Diaz
- Thibaud Michaud
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud added 16 comments![]( "Open in Gerrit")
I didn't finish the review. I mostly reviewed the x64 implementation so far, but I'm sending you my first round of comments so you can work on it asynchronously.
Bug: 520400061nit: let's use the canonical bug ID, 450597581.
#if V8_ENABLE_WEBASSEMBLY
if (v8_flags.wasm_growable_stacks &&
linkage()->GetIncomingDescriptor()->IsAnyWasmFunctionCall()) {
return;
}
#endif // V8_ENABLE_WEBASSEMBLYWhy do we skip this? Please add a comment.
int depth = 2;
#if V8_ENABLE_WEBASSEMBLY
ArchOpcode arch_opcode = ArchOpcodeField::decode(instr->opcode());
if (arch_opcode == kArchTailCallWasm ||
arch_opcode == kArchTailCallWasmIndirect) {
depth = 4;
}
#endifThis looks error prone, it uses magic numbers and needs to be kept in sync with the instruction selector. See also comments below, the whole input encoding looks pretty fragile. Not really this change's fault, to be clear, but maybe we can limit the damage.
We could push the tail call inputs first in instruction-selector.cc so that they don't affect the existing (reverse) indices. This would just move the complexity of computing the correct indices to the tail-call inputs, but at least this reduces the risk of breaking existing code.
I don't have anything much better to suggest. Maybe this needs a larger refactoring that is out of scope for this change. But if you can think of any DCHECK or anything to improve the situation while you are here, please don't hesitate.
if (v8_flags.wasm_growable_stacks) {Francis McCabePlease add a high-level comment to explain why we need to shrink before the tail call.
Acknowledged
The comment just says that we need to shrink, it still doesn't explain *why*.
return InputCount() - 3;Doesn't this also need to be updated? This is pushed before the new optional tail call inputs in instruction-selector.cc, so its index is also shifted.
return InputCount() - 5;In instruction-selector.cc, the new tail call inputs are only pushed if the callee is a wasm function. Don't we need something similar here to keep the indices in sync?
// Save return address of w1's caller (from rsp + 16 because we pushed 2
// words) and Segment 1 rsp.
__ movq(r15, Operand(rsp, 2 * kSystemPointerSize));I don't understand: at [rsp+16], we will find whatever was at the top of the current frame before pushing r12 and r15, not the return address?
The return address should be at an FP-relative offset.
// Save GP caller-saved registers.
__ pushq(rax);
__ pushq(rcx);
__ pushq(rdx);
__ pushq(rsi);
__ pushq(rdi);
__ pushq(r8);
__ pushq(r9);
__ pushq(r10);
__ pushq(r11);
// Save FP registers.
__ subq(rsp, Immediate(128));
for (int idx = 0; idx < 8; ++idx) {
__ movdqu(Operand(rsp, idx * 16), XMMRegister::from_code(idx + 1));
}There's a `PushCallerSaved`/`PopCallerSaved` helper in the x64 macro assembler, can you use it here?
__ jmp(&done_prepare);Francis McCabeWhy not just fall through?
At this point, if the stack has shrunk, both rbp and rsp have been calculated based on shrinking.
Yeah, it makes sense after your latest changes.
In the old version, the label was bound immediately after the jump so it didn't actually skip anything.
if (v8_flags.wasm_growable_stacks) {Francis McCabeSame for Liftoff: let's add a comment to explain why we need to shrink here.
Acknowledged
Same here, the comment still doesn't explain the *why*.
I insist on it because it's not obvious at all.
// stack segments. Checking isolate->IsOnCentralStack() instead of
// active_stack->jmpbuf()->parent == nullptr ensures that growable stacks work
// correctly even when invoked without stack switching.This comment is a bit confusing:
- It refers to old code (`...->parent == nullptr`) that won't be visible to future readers,
- "ensures that growable stacks work correctly (...) without stack switching". We should not even be using growable stacks without stack switching. So saying that they "work correctly" is odd, and the check we do here is irrelevant.
The actual difference between the old and the new check is that `...->parent == nullptr` checks that we are on the root of the stack chain, while `IsOnCentralStack()` would also return true if we are in a wasm stack that temporarily switched to the central stack for an external call.
Both should hold, so we can keep the new (stronger) check. And we should even turn it into a `CHECK`, we should never even call that function from the central stack.
let res = await wrapper(101);Thibaud MichaudThis magic number didn't work for me, and it is probably very platform and tier dependent. Given the complexity of the change, I think it's worth having more robust tests.
I'm still not convinced by this test. It expects the stack growth to happen at a specific stack depth that will probably be different depending on the tier, platform, etc.
I sent you a test that reliably crashes by doing a tail-call at each stack depth. It looks like you have added it as "variant3" below. Does regress-520827581.js cover a different code path? If yes, can we make it more robust in a similar way? If not, let's remove it.
Same question for variant 1 and 2.
// Flags: --wasm-growable-stacks --no-liftoff --no-wasm-inlining --experimental-wasm-wasmfx --allow-natives-syntaxRunning with and without Liftoff is already covered by the test variants on the CI. Best to drop this flag to avoid conflicting flags and increase coverage. Same for the other tests.
f_grow.addLocals(kWasmF64, 600);Given that you have `--no-liftoff` above, aren't these locals optimized away?
As mentioned above, the runner should run it with and without Liftoff, so maybe it helps with Liftoff. But I'm wondering how you have been running it locally, and if these locals made any difference.
Same question for variant 2.
let depth = 60;60 is not enough on my config to hit the stack growth.
I set it back to 200 like in my original test, and it still segfaults after the change. Maybe because of the return address issue I mentioned in the comment above. Can you reproduce it?
Related details
- Francis McCabe
- Ryan Diaz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud added 14 comments![]( "Open in Gerrit")
Another round of comments before I log out for the week-end. Mostly reviewed ia32 this time, but some comments apply to all platforms.
__ ldr(scratch, MemOperand(sp, (idx + 1) * kSystemPointerSize));Where does this `+ 1` come from? I would expect the stack parameters to be directly under `sp`.
__ str(scratch, MemOperand(fp, (idx + 2 - stack_param_delta) *And if I understand the meaning of this `+ 2` correctly, I would suggest the following to make it clearer:
```suggestion
__ str(scratch, MemOperand(fp, kCallerSPOffset + (idx - stack_param_delta) *
```
(also on other platforms).
__ ldr(r1, MemOperand(fp, 0));Doesn't that clobber r1? What if it holds a register parameter?
See also comment below about testing tail-call parameters.
__ mov(Operand(eax, (1 - stack_param_delta) * kSystemPointerSize), edi);```suggestion
__ mov(Operand(eax, kCallerPCOffset - stack_param_delta * kSystemPointerSize), edi);
```
__ mov(edi, Operand(esi, (idx + 3) * kSystemPointerSize));Why `+ 3`? We push `esi` and `edi` before saving the `sp` so I would only expect `+ 2`.
Operand(eax, (idx + 2 - stack_param_delta) * kSystemPointerSize),```suggestion
Operand(eax, kCallerSPOffset + (idx - stack_param_delta) * kSystemPointerSize),
```
__ lea(esp, Operand(eax, (1 - stack_param_delta) * kSystemPointerSize));```suggestion
__ lea(esp, Operand(eax, kCallerPCOffset - stack_param_delta * kSystemPointerSize));
```
__ bind(&no_shrink);Should we be jumping to `done_prepare` here like on other platforms? The label is unused at the moment.
if (frame_access_state()->has_frame()) {Don't we already assume that the function has a frame from the very start of this code, by checking the frame type marker, and then by using the caller FP slot, etc.?
I think we need to check that at the very beginning. And if the code has no frame, then it should be safe to assume that we are not at a segment start, and we can skip the whole thing. A function with no frame does not have a stack check, so it cannot have grown the stack.
Operand(ebp, (1 - stack_param_delta) * kSystemPointerSize));```suggestion
Operand(ebp, kCallerPCOffset - stack_param_delta * kSystemPointerSize));
```
LiftoffRegList regs_to_save = cache_state()->used_registers;This is the only platform and tier where we save/restore these additional registers on top of kGpParamRegisters. Is there a reason for it?
const sig1 = builder.addType(makeSig([kWasmI32, kWasmF64, kWasmI32], returns1));Related to the comment above about a clobbered register on arm: this test has parameters, but it doesn't seem to use them. It would be nice to have at least one test that uses parameters to check that they don't get clobbered during the new tail-call sequence. This could also be added to variant3, which already checks multi-return stack parameters.
f_grow.addLocals(kWasmF64, 600);Given that you have `--no-liftoff` above, aren't these locals optimized away?
As mentioned above, the runner should run it with and without Liftoff, so maybe it helps with Liftoff. But I'm wondering how you have been running it locally, and if these locals made any difference.
Same question for variant 2.
BTW: maybe we can force a large frame on Turboshaft by using many parameters instead of many locals.
Until recently, this wouldn't have worked because the stack checks didn't take outgoing stack parameters into account. But this has been fixed now, so if a function has many parameters, this can easily trigger a stack growth for the *caller*.
Related details
- Francis McCabe
- Ryan Diaz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |