feat(compiler): Stub panic with unreachable to prevent bad codegen#2382
feat(compiler): Stub panic with unreachable to prevent bad codegen#2382spotandjake wants to merge 1 commit into
Conversation
| resolve_global(~env, panic_with_exception_name), | ||
| gensym_label("runtime_panic_stub"), | ||
| List.fold_left( | ||
| (acc, arg) => [Expression.Drop.make(wasm_mod, arg), ...acc], |
There was a problem hiding this comment.
This is a little weird to me. This is only the exception being passed to the panic handler, so there's no sense in keeping it around. We should be able to just emit unreachable and have it be a little cleaner.
There was a problem hiding this comment.
That's fair. I made the change.
The reason I chose to drop the arguments initially was suppose that any of the arguments had side effects like local.tee, we might be setting a value and using it later, in which case not emitting the wasm would cause issues. Reviewing the implementation of compile_imm, this won't happen right now.
dfa54d6 to
82b0284
Compare
ospencer
left a comment
ospencer
left a comment
There was a problem hiding this comment.
The very next instruction is unreachable, so side effects like a local tee wouldn't matter. The thing to look out here for would be if we didn't do SSA, since an argument could be something like a function call. But not something for us to worry about!
35e071d to
7354757
Compare
This pr stubs panic with unreachable when we are compiling in `@runtimeMode`. The reason for this change is that when working in `@runtimeMode` we don't have the panic module available however if you want to use something like `arr[x]` you are out of luck as it has a dependency on panic, this results in bad wasm codegen. My solution to this is we just fail with `unreachable` the downside is we don't get a nice error but if someone is working in the runtime I feel like thats a reasonable sacrifice, to be able to write the higher level code in the first place. Closes: grain-lang#2379 fix: skip test in js mode
82b0284 to
ae37229
Compare
2 participants
This pr stubs panic with unreachable when we are compiling in
@runtimeMode.Because we use
@runtimeModeitself to compile the panic module, we can't allow a circular dependency, so we don't import panic when compiling in runtime mode.This had the side effect that if someone tried to use a higher-level expression such as
arr[x], we would generate invalid WASM. In order to avoid this, I figure we can just fail withunreachableinstead of properly panicking. The downside is we don't get a nice error, but I figure that is a perfectly reasonable tradeoff in the context of the runtime.Closes: #2379