Should be an easy fix. My plan is to introduce a helper to WasmB3IRGenerator that just jumps to a particular location to throw an exception with some enum as an argument to the function call. The function call can switch on the enum to provide a nice error message.
Created attachment 297110 [details] WIP I just need to fill in the various exception checks now.
Created attachment 297118 [details] WIP RFC
Comment on attachment 297118 [details] WIP RFC View in context: https://bugs.webkit.org/attachment.cgi?id=297118&action=review Looks good. > JSTests/wasm/function-tests/exceptions.js:20 > + .Function("foo", 0 /*['i32', 'i32'] => 'i32'*/) Why not let these auto-populate in the Type section? > JSTests/wasm/function-tests/exceptions.js:23 > + .CallIndirect(1, 0) // calling function of type ['i32'] => 'i32' Can call indirect also take a type signature? > JSTests/wasm/function-tests/exceptions.js:57 > + assert.throws(() => foo(1 + i, i), WebAssembly.RuntimeError, "Out of bounds call_indirect"); Check that negative also fails. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:282 > + // The thing that jumps here will move ExceptionType into the argumentGPR1 and jump here. Can you use a named constant for that GPR so it's more obvious / can't go out of sync? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:297 > + switch (type) { Can we just make these a static const char * array next to the enum? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:308 > + message = "Can't make a call_indirect to a null table entry"; I'd drop "can't make a " from the message: the issue is "call_indirect to a null table entry". > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:311 > + message = "Tried to make a call_indirect to a signature that does not match"; Similar here: "call_indirect signature mismatch". > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:859 > + // FIXME: is this even possible? Do we need to zero-fill missing arguments, and drop extra ones?
Comment on attachment 297118 [details] WIP RFC View in context: https://bugs.webkit.org/attachment.cgi?id=297118&action=review >> JSTests/wasm/function-tests/exceptions.js:23 >> + .CallIndirect(1, 0) // calling function of type ['i32'] => 'i32' > > Can call indirect also take a type signature? We could make it do that but lets make that a separate patch >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:282 >> + // The thing that jumps here will move ExceptionType into the argumentGPR1 and jump here. > > Can you use a named constant for that GPR so it's more obvious / can't go out of sync? That constant would only be used in one place. I don't think it's worth it. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:297 >> + switch (type) { > > Can we just make these a static const char * array next to the enum? I'm not a big fan of this. I could just make all exception types a macro? >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:308 >> + message = "Can't make a call_indirect to a null table entry"; > > I'd drop "can't make a " from the message: the issue is "call_indirect to a null table entry". sounds good. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:311 >> + message = "Tried to make a call_indirect to a signature that does not match"; > > Similar here: "call_indirect signature mismatch". sounds good >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:859 >> + // FIXME: is this even possible? > > Do we need to zero-fill missing arguments, and drop extra ones? No idea. I'll leave this for another patch.
Comment on attachment 297118 [details] WIP RFC View in context: https://bugs.webkit.org/attachment.cgi?id=297118&action=review > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:747 > + check->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams&) { Doesn't this copy the entire B3IRGenerator for each exception generator? Maybe this should just capture the address of m_exceptionHandler? >>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:859 >>> + // FIXME: is this even possible? >> >> Do we need to zero-fill missing arguments, and drop extra ones? > > No idea. I'll leave this for another patch. I don't think this is possible anymore. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:923 > + return jsEntrypoint->codeRef(); Aren't you required to keep the byproducts alive along with the code ref?
Created attachment 297265 [details] patch
Created attachment 297266 [details] patch
Attachment 297266 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:328: Non-label code inside switch statements should be indented. [whitespace/indent] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 297266 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=297266&action=review It seems like emitExceptionHandler should only be called if it's actually needed. I think I would add a member, m_needsExceptionHandler that is set in emitExceptionCheck. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:296 > + jit.addLinkTask([=] (LinkBuffer& linkBuffer) { It's a little weird that you capture the entire B3IRGenerator here.
Comment on attachment 297266 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=297266&action=review >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:296 >> + jit.addLinkTask([=] (LinkBuffer& linkBuffer) { > > It's a little weird that you capture the entire B3IRGenerator here. What do you propose instead?
(In reply to comment #9) > Comment on attachment 297266 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297266&action=review > > It seems like emitExceptionHandler should only be called if it's actually > needed. I think I would add a member, m_needsExceptionHandler that is set in > emitExceptionCheck. Ok, sounds good.
Comment on attachment 297266 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=297266&action=review >>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:296 >>> + jit.addLinkTask([=] (LinkBuffer& linkBuffer) { >> >> It's a little weird that you capture the entire B3IRGenerator here. > > What do you propose instead? I would just capture m_exceptionHandler.
Could you explain in more detail what your goal is here? My principle concern is that it seems like you're trying to continue executing despite having generated incorrect code. The correct course of action is to terminate, so there needs to be a very compelling reason not to. If it's for debugging or similar, i'd rather you replace "call a function with an enum" with jit.move($enumValue, regT0) hit.breakpoint() or similar.
(In reply to comment #13) > Could you explain in more detail what your goal is here? My principle > concern is that it seems like you're trying to continue executing despite > having generated incorrect code. > > The correct course of action is to terminate, so there needs to be a very > compelling reason not to. > > If it's for debugging or similar, i'd rather you replace "call a function > with an enum" with > > jit.move($enumValue, regT0) > hit.breakpoint() > > or similar. This is not "trying to continue executing despite having generated incorrect code". There are operations in Wasm that *are* supposed to throw real JS exceptions. For example: - division by zero - out of bounds memory access - Trying to call an "imported" function with a signature mismatch. So these are things that need to be validated at runtime because they can't be validated at compile time. Therefore, we must validate such things and throw exceptions if the validation fails.
Created attachment 297302 [details] patch All Wasm functions now share the exact same exception thrower thunk. This should reduce code size by at least 20 bytes per function.
(In reply to comment #14) > (In reply to comment #13) > > Could you explain in more detail what your goal is here? My principle > > concern is that it seems like you're trying to continue executing despite > > having generated incorrect code. > > > > The correct course of action is to terminate, so there needs to be a very > > compelling reason not to. > > > > If it's for debugging or similar, i'd rather you replace "call a function > > with an enum" with > > > > jit.move($enumValue, regT0) > > hit.breakpoint() > > > > or similar. > > This is not "trying to continue executing despite having generated incorrect > code". > There are operations in Wasm that *are* supposed to throw real JS exceptions. > For example: > - division by zero > - out of bounds memory access > - Trying to call an "imported" function with a signature mismatch. > > So these are things that need to be validated at runtime because they can't > be validated at compile time. Therefore, we must validate such things and > throw exceptions if the validation fails. To elaborate a bit, when Keith began to implement Wasm, he didn't write the infrastructure needed to throw exceptions. I recently made our unwinder Wasm-aware, so we can now throw exceptions from (and through) Wasm code. Keith originally added breakpoints() for places that needed to throw exceptions. This patch just fills in some of the various breakpoints() with jumps to the thing that throws an exception.
Created attachment 297303 [details] patch rebased
Attachment 297303 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmExceptionType.h:53: Non-label code inside switch statements should be indented. [whitespace/indent] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 297303 [details] patch Looks good.
Comment on attachment 297303 [details] patch r=me.
Comment on attachment 297303 [details] patch Clearing flags on attachment: 297303 Committed r209928: <http://trac.webkit.org/changeset/209928>
All reviewed patches have been landed. Closing bug.
> To elaborate a bit, when Keith began to implement Wasm, he didn't write the > infrastructure needed to throw exceptions. I recently made our unwinder > Wasm-aware, so we can now throw exceptions from (and through) Wasm code. > Keith originally added breakpoints() for places that needed to throw > exceptions. This patch just fills in some of the various breakpoints() with > jumps to the thing that throws an exception. Ok, i understand now -- it was not clear to me that the existing breakpoints were (essentially) for unimplemented checks, so i interpreted the breakpoints as being the standard RELEASE_ASSERT of the jit work. That said i wonder if we could do something to make the existing jit asserts plant info into the crash trace (write error code to register or similar)
(In reply to comment #23) > > To elaborate a bit, when Keith began to implement Wasm, he didn't write the > > infrastructure needed to throw exceptions. I recently made our unwinder > > Wasm-aware, so we can now throw exceptions from (and through) Wasm code. > > Keith originally added breakpoints() for places that needed to throw > > exceptions. This patch just fills in some of the various breakpoints() with > > jumps to the thing that throws an exception. > > Ok, i understand now -- it was not clear to me that the existing breakpoints > were (essentially) for unimplemented checks, so i interpreted the > breakpoints as being the standard RELEASE_ASSERT of the jit work. > > That said i wonder if we could do something to make the existing jit asserts > plant info into the crash trace (write error code to register or similar) Yeah for things that are really crashes, we should use the JIT assert mechanism that's already in place instead of the breakpoint().