cranelift and stack limit checks · cranelift

Stream: cranelift

Topic: cranelift and stack limit checks

Alex Crichton (Apr 08 2020 at 19:29):

@Dan Gohman hey so I'm poking around (after yesterday) at more of the stack limit/interrupt stuff, and I was looking to tackle #900 while I was at it. I'm thinking of mirroring spidermonkey where each relevant function has a prologue which checks the stack limit and traps if we ran out. It looks like there's an ArgumentPurpose::StackLimit parameter which is already in cranelift which is roughly analagous to this, but it seems like it may not be quite right? Does that require that every caller of the function pass in the stack limit? I'd ideally prefer to avoid reserving another argument for our purposes (we already take 2 with the two vmctx arguments)

Improve wasm handling for stack overflow conditions · Issue #900 · bytecodealliance/wasmtime

Currently wasmtime handles stack overflow in wasm code through the use of the segfault signal handler, relying on wasm code to hit the guard page to trigger a segfault which we the longjmp to recov...

Alex Crichton (Apr 08 2020 at 19:30):

and fwiw you seemed active on both https://github.com/bytecodealliance/cranelift/pull/372 and https://github.com/bytecodealliance/cranelift/issues/349

Stack Limit as an Argument Purpose by pepyakin · Pull Request #372 · bytecodealliance/cranelift

Another approach to close #349

Stack limit checking · Issue #349 · bytecodealliance/cranelift

Cretonne is currently capable of detecting stack overflow when the stack is allocated with a guard page at the end, and it performs stack probes as needed when allocating large frames. Most popular...

Alex Crichton (Apr 08 2020 at 19:34):

or is there a way we could like synthesize an "argument" inside of each function?

Alex Crichton (Apr 08 2020 at 19:34):

so it's calculated from other arguments

Dan Gohman (Apr 08 2020 at 19:36):

right now, you're right, StackLimit is an argument that needs to be passed in

Alex Crichton (Apr 08 2020 at 19:36):

bah ok, so this isn't quite what I want then

Dan Gohman (Apr 08 2020 at 19:38):

take a look at insert_common_prologue in cranelift/codegen/src/isa/x86/abi.rs

Dan Gohman (Apr 08 2020 at 19:38):

You probably want to reuse insert_stack_check, but you just need another way of telling it where to find the limit

Alex Crichton (Apr 08 2020 at 19:38):

kk, I'll see if I can whip up something

Alex Crichton (Apr 08 2020 at 20:06):

error[E0061]: this function takes 12 parameters but 11 parameters were supplied

Alex Crichton (Apr 08 2020 at 20:06):

cry

Alex Crichton (Apr 08 2020 at 21:35):

@Dan Gohman so I'm getting can't encode v17 = ifcmp_sp.i32 v16 as a panic message which is called from here and panics here

bytecodealliance/wasmtime

Standalone JIT-style runtime for WebAssembly, using Cranelift - bytecodealliance/wasmtime

bytecodealliance/wasmtime

Standalone JIT-style runtime for WebAssembly, using Cranelift - bytecodealliance/wasmtime

Alex Crichton (Apr 08 2020 at 21:35):

it seems like this previous StackLimit stuff just wasn't ever fully tested before? do you know if that's the case?

Alex Crichton (Apr 08 2020 at 21:36):

I see a text test or two but I don't know why that doesn't panic but what I'm doing panics (unless something is specific to actually encoding instructions)

Dan Gohman (Apr 08 2020 at 21:37):

cranelift/filetests/filetests/isa/x86/prologue-epilogue.clif is a test which exercises a stack_limit argument

Alex Crichton (Apr 08 2020 at 21:37):

right yeah

Alex Crichton (Apr 08 2020 at 21:37):

is that testing the actual x86 encoding/emission

Alex Crichton (Apr 08 2020 at 21:37):

which means that I'm just doing something wrong?

Dan Gohman (Apr 08 2020 at 21:37):

but, I wouldn't be too surprised if it doesn't work if you use it differently

Alex Crichton (Apr 08 2020 at 21:37):

hm ok, I know very little about cranelift and all this...

Dan Gohman (Apr 08 2020 at 21:38):

oh, you probably want to do ifcmp_sp.i64, not i32, on x86-64

Dan Gohman (Apr 08 2020 at 21:38):

It probably doesn't know how to encode a 32-bit version of that

Alex Crichton (Apr 08 2020 at 21:38):

is there a separate method for that?

Alex Crichton (Apr 08 2020 at 21:39):

oh wait I think I had a type error in the IR

Dan Gohman (Apr 08 2020 at 21:39):

It's inferred from the operand.

Alex Crichton (Apr 08 2020 at 21:39):

yeah that made me realize I was doing a 32-bit load when I wanted a 64-bit one

Alex Crichton (Apr 08 2020 at 21:39):

which may be why the encoding failed

Alex Crichton (Apr 08 2020 at 21:39):

oh hey!

Alex Crichton (Apr 08 2020 at 21:39):

now it works!

Alex Crichton (Apr 08 2020 at 21:39):

sorta at least

Alex Crichton (Apr 08 2020 at 21:40):

ok cool thanks

Alex Crichton (Apr 08 2020 at 21:40):

for being a rubber duck :)

Alex Crichton (Apr 08 2020 at 21:40):

and pointing out bit-nes

Alex Crichton (Apr 08 2020 at 21:41):

@Dan Gohman is there a way to dump generated code to stderr?

Alex Crichton (Apr 08 2020 at 21:41):

or otherwise debug what's happening?

Dan Gohman (Apr 08 2020 at 21:43):

Not from wasmtime right now.

Dan Gohman (Apr 08 2020 at 21:43):

Cranelift has some utilities in cranelift/src/disasm.rs which can use capstone to disassemble the output, but they're not yet wired up in convenient ways

Dan Gohman (Apr 08 2020 at 21:43):

outside of clif-util, at least

Yury Delendik (Apr 08 2020 at 21:44):

or use lldb :P

Dan Gohman (Apr 08 2020 at 21:46):

that too :smile:

Alex Crichton (Apr 08 2020 at 21:46):

ok no worries, I've now got every single wasm function to immediately trap

Alex Crichton (Apr 08 2020 at 21:46):

which is good, and it seems to be passing the verifier too

Benjamin Bouvier (Apr 09 2020 at 10:55):

Fwiw, Spidermonkey doesn't pass the stack check limit as an argument; stack checks are generated in Cranelift IR directly.

Last updated: Nov 22 2024 at 17:03 UTC