WIP: Indirect calls on null trapping #961

jayphelps · 2019-11-17T03:30:44Z

WIP: This PR makes indirect calls on null either trap with unreachable or, if exception-handling is enabled, throws a Wasm exception.

I'm not sure if this is the right approach right now, since AS currently fakes exceptions using an extern call to abort() (which requires people to provide a JS implementation as an import) then uses unreachable.

I started to do that at first, but worry that it means everyone now needs to include an abort() implementation. I considered disabling it if they use --noAssert but then again, it seems like this should be an exception and it seemed weird that --noAssert would disable that.

Then I noticed that AS actually already wraps Binaryen's exception-handling support even though it's not yet used anywhere, so I figured I'd give that a shot for fun and use this as a discussion point on whether this is instead the more correct direction?

One note: I'm using let message = this.ensureStaticString("null is not a function"); to generate an interned string for the exception, however it doesn't seem to be working. The string doesn't seem to show up in the Wasm file (no data section at all, in fact). Did I do something wrong? We solved the issue.

jayphelps · 2019-11-17T03:31:34Z

examples/game-of-life/build/untouched.wat

@@ -60,7 +60,7 @@
       global.get $assembly/config/BGR_DEAD
       i32.const 16777215
       i32.and
-      else       


I used the automated update feature and it did this. I'm guessing perhaps these files were generated by manual copy/paste or by hand previously maybe?

Looks like the output is from an earlier version of Binaryen. It used to emit such whitespace and I guess the example fixtures haven't been updated since then. Most likely nothing to worry about :)

jayphelps · 2019-11-17T03:32:23Z

src/compiler.ts

@@ -462,6 +462,36 @@ export class Compiler extends DiagnosticEmitter {
    return module;
  }

+  addNullPointerExceptionHandling(module: Module) {
+    if (this.options.hasFeature(Feature.EXCEPTION_HANDLING)) {
+      let message = this.ensureStaticString("null is not a function");


~~This returns an index that is emitted but for some reason the string isn't interned in a data section AFAICT. Anyone see what I did wrong?~~

Also, bikeshedding welcome on the error message, as always.

jayphelps · 2019-11-17T03:32:38Z

src/compiler.ts

+        [NativeType.I32]
+      );
+
+      module.addEvent("NullPointerException", 0, exceptionType);


For that real Java~~Script~~ feeling

btw this string is only used as a .wat debugging symbol it isn't used at runtime. Wasm exceptions don't (currently) have names like JS exceptions do. They're all WebAssembly.Runtime instances with a message field of just wasm exception from the JS side of things.

dcodeIO · 2019-11-17T05:46:20Z

I'm not sure if this is the right approach right now, since AS currently fakes exceptions using an extern call to abort() (requires its available) then uses unreachable.

Yeah, there haven't been any attempts to implement exception handling yet. Mostly because - last time I checked - there wasn't a working implementation in V8 to test against. If there is one now, we could actually attempt to implement try-catch behind a flag now to level the field.

I started to do that at first, but worry that it means everyone now needs to include an abort() implementation. I considered disabling it if they use --noAssert but then again, it seems like this should be an exception and it seemed weird that --noAssert would disable that.

AS has those two dependencies on optional env.abort and env.trace that the loader provides. As such, as soon as there is an assert(...) anywhere in the code being compiled, like it is common in the standard library, and --noAssert is not given, which is the default, one will need that env.abort import anyway, so using it should be just fine for the time being.

Then I noticed that AS actually already wraps Binaryen's exception-handling support even though it's not yet used anywhere, so I figured I'd give that a shot for fun and use this as a discussion point on whether this is instead the more correct direction?

My general feeling there is that we should first tackle exception handling in general before using it at other places, because otherwise it is very likely that one will end up with both experimental exceptions and the then-unnecessary abort import. So, for this PR it seems fine to skip exception handling for now and make this another occasion to replace with a proper exception later, like so:

Without --enable exception-handling
- Use unreachable if --noAssert
- Use abort otherwise
With --enable exception handling (later)
- Use throw everywhere, don't use abort anymore

One note: I'm using let message = this.ensureStaticString("null is not a function"); to generate an interned string for the exception, however it doesn't seem to be working. The string doesn't seem to show up in the Wasm file (no data section at all, in fact). Did I do something wrong?

Hmm, this should work. My guess is that the exception-handling feature is not yet wired up properly, in turn leading to the respective code never executing. One candidate might be tests/features.json missing it.

Overall I think it's fine to wait with exception handling related code until there is initial try / catch plus the necessary bits on what would otherwise be an abort, as mentioned earlier.

jayphelps · 2019-11-17T05:56:16Z

Hmm, this should work. My guess is that the exception-handling feature is not yet wired up properly, in turn leading to the respective code never executing. One candidate might be tests/features.json missing it.

Edit: we solved this. Needed to move addNullPointerExceptionHandling() to before module.setMemory.

FWIW I verified the tests I included are in fact running and it does throw a wasm exception. You can see it did the correct codgen for that function too, the module just doesn't include the data segment for some reason.

Without --enable exception-handling

Use unreachable if --noAssert

Use abort otherwise

With --enable exception handling (later)

Use throw everywhere, don't use abort anymore

Overall I think it's fine to wait with exception handling related code until there is initial try / catch plus the necessary bits on what would otherwise be an abort, as mentioned earlier.

Okiedoke I'll see what I can do! Thanks!

dcodeIO · 2019-11-17T06:00:22Z

src/compiler.ts

+        this.ensureFunctionType(null, Type.void),
+        null,
+        module.block(null, [
+          module.throw("NullPointerException", [module.i32(message)])


Here, message already is an i32.const (or i64.const once WASM64 is a thing), so this makes an i32.const(someRandomPtrToBinaryenMemory). Wondering if this is related to the string not showing up in static data.

Nice catch! That's indeed a bug, although, it didn't seem to solve the issue when I regenerate the files. Only change was:

- i32.const 5460624 + i32.const 24

Which that number makes much more sense for a data segment index (5460624 in hindsight should have been an obvious give away that something was wrong)

Ohh I think I found the issue. I think it's too late, the module.setMemory call is made above, before the call to addNullPointerExceptionHandling, so the module misses out on any changes to the internal this.memorySegments storage.

Checking...

Yep that was the issue. It needs to happen before the setMemory call.

…ption-handlig is enabled, throws a Wasm exception

MaxGraey · 2020-05-16T17:25:00Z

@jayphelps thanks for PR. Currently we get rid of null trapping function and just reserve table in zero slot for this.

Closing this PR as part of 2020 vacuum as it is outdated. My expectation is that I'll eventually cannibalize the remains of this PR when making a new one.

jayphelps commented Nov 17, 2019

View reviewed changes

dcodeIO reviewed Nov 17, 2019

View reviewed changes

Indirect calls on null now either trap with unreachable or, if exce…

fef1379

…ption-handlig is enabled, throws a Wasm exception

jayphelps force-pushed the null-functions branch from 73047b2 to fef1379 Compare November 17, 2019 06:13

dcodeIO mentioned this pull request Nov 18, 2019

Try-catch experiment #963

Closed

MaxGraey closed this May 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Indirect calls on null trapping #961

WIP: Indirect calls on null trapping #961

jayphelps commented Nov 17, 2019 •

edited

Loading

jayphelps Nov 17, 2019

dcodeIO Nov 17, 2019

jayphelps Nov 17, 2019 •

edited

Loading

jayphelps Nov 17, 2019 •

edited

Loading

jayphelps Nov 17, 2019

dcodeIO commented Nov 17, 2019

jayphelps commented Nov 17, 2019 •

edited

Loading

dcodeIO Nov 17, 2019 •

edited

Loading

jayphelps Nov 17, 2019

jayphelps Nov 17, 2019

jayphelps Nov 17, 2019 •

edited

Loading

MaxGraey commented May 16, 2020

WIP: Indirect calls on null trapping #961

WIP: Indirect calls on null trapping #961

Conversation

jayphelps commented Nov 17, 2019 • edited Loading

jayphelps Nov 17, 2019

Choose a reason for hiding this comment

dcodeIO Nov 17, 2019

Choose a reason for hiding this comment

jayphelps Nov 17, 2019 • edited Loading

Choose a reason for hiding this comment

jayphelps Nov 17, 2019 • edited Loading

Choose a reason for hiding this comment

jayphelps Nov 17, 2019

Choose a reason for hiding this comment

dcodeIO commented Nov 17, 2019

jayphelps commented Nov 17, 2019 • edited Loading

dcodeIO Nov 17, 2019 • edited Loading

Choose a reason for hiding this comment

jayphelps Nov 17, 2019

Choose a reason for hiding this comment

jayphelps Nov 17, 2019

Choose a reason for hiding this comment

jayphelps Nov 17, 2019 • edited Loading

Choose a reason for hiding this comment

MaxGraey commented May 16, 2020

jayphelps commented Nov 17, 2019 •

edited

Loading

jayphelps Nov 17, 2019 •

edited

Loading

jayphelps Nov 17, 2019 •

edited

Loading

jayphelps commented Nov 17, 2019 •

edited

Loading

dcodeIO Nov 17, 2019 •

edited

Loading

jayphelps Nov 17, 2019 •

edited

Loading