Bug 165834 – WebAssembly: WasmB3IRGenerator should throw exceptions instead of crash

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

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.

[Saam Barati]

Created attachment 297110 [details]
WIP

I just need to fill in the various exception checks now.

[Saam Barati]

Created attachment 297118 [details]
WIP RFC

[JF Bastien]

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?

[Saam Barati]

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.

[Keith Miller]

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?

[Saam Barati]

Created attachment 297265 [details]
patch

[Saam Barati]

Created attachment 297266 [details]
patch

[WebKit Commit Bot]

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.

[Keith Miller]

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.

[Saam Barati]

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?

[Saam Barati]

(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.

[Keith Miller]

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.

[Oliver Hunt]

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.

[Saam Barati]

(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.

[Saam Barati]

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.

[Saam Barati]

(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.

[Saam Barati]

Created attachment 297303 [details]
patch

rebased

[WebKit Commit Bot]

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.

[JF Bastien]

Comment on attachment 297303 [details]
patch

Looks good.

[Keith Miller]

Comment on attachment 297303 [details]
patch

r=me.

[WebKit Commit Bot]

Comment on attachment 297303 [details]
patch

Clearing flags on attachment: 297303

Committed r209928: <http://trac.webkit.org/changeset/209928>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Oliver Hunt]

> 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)

[Saam Barati]

(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().