Skip to content

Passing Instance through an Object Literal Causes a runtime error on exit #1272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
torch2424 opened this issue May 14, 2020 · 10 comments
Closed

Comments

@torch2424
Copy link
Contributor

Hello!

So I noticed another thing weird with Object literals. If I try to pass a class into my init object, to construct my other class. Everything works fine, but when everything is done, the runtime will throw a memory error.

Here is the test case code:

import {Console} from "as-wasi";

class OtherClass {
  stuff: i32;

  constructor(newStuff: i32) {
    this.stuff = newStuff;
  }
}

class MyClassInit {
  otherClass: OtherClass;
}

class MyClass {
  _myParam: i32;
  _otherClass: OtherClass;

  constructor(param: i32, init: MyClassInit) {
    this._myParam = param;
    this._otherClass = init.otherClass;
  }

  toString(): string {
    return this._otherClass.stuff.toString();
  }
}

export function _start(): void {

  // Make our Other class
  let otherClass = new OtherClass(24);

  // Pass to my class as an object literal
  let myClass = new MyClass(2424, {
    otherClass: otherClass
  });

  // This will all work
  Console.log("Logging myClass.toString() ...\n");
  Console.log(myClass.toString());
  Console.log("\n");

  // Will Error on Exit
  // ~lib/rt/pure.ts:122:13: error: null
}

Here is a reproducable test case: https://github.com/torch2424/as-playground/tree/master/object-literal-pass-instance

cc @dcodeIO or @MaxGraey , if you can point me to where this would be fixed in the compiler (would this object literal be fixed in the same place as #1229 ), I can make this fix 😄

Screenshot

Screenshot from 2020-05-14 10-33-16

@dcodeIO
Copy link
Member

dcodeIO commented May 14, 2020

Translated your example to this test case:

class OtherClass {
  stuff: i32;

  constructor(newStuff: i32) {
    this.stuff = newStuff;
  }
}

class MyClassInit {
  otherClass: OtherClass;
}

class MyClass {
  _myParam: i32;
  _otherClass: OtherClass;

  constructor(param: i32, init: MyClassInit) {
    this._myParam = param;
    this._otherClass = init.otherClass;
  }

  toString(): string {
    return this._otherClass.stuff.toString();
  }
}

function start(): void {

  // Make our Other class
  let otherClass = new OtherClass(24);

  // Pass to my class as an object literal
  let myClass = new MyClass(2424, {
    otherClass: otherClass
  });

  trace("Logging myClass.toString() ...");
  trace(myClass.toString());
}
start();

Running this on master, with rtrace enabled, doesn't show any problems. Might this have been fixed since the last release?

@MaxGraey
Copy link
Member

Hmm, I don't see any issues in browser (--debug flag turned on):
https://webassembly.studio/?f=xioif2ne2k

It could be problem with wasmtime or as-wasi and Console

@MaxGraey
Copy link
Member

@dcodeIO I guess need test with node.js and as-wasi for reproduce full picture.
Also could you check this function?

@MaxGraey
Copy link
Member

MaxGraey commented May 14, 2020

@dcodeIO
Copy link
Member

dcodeIO commented May 14, 2020

  writeStringLn(s: string): void {
    let s_utf8_buf = String.UTF8.encode(s); // __retain
    let s_utf8_len: usize = s_utf8_buf.byteLength;
    let iov = changetype<ArrayBufferView>(mem256).dataStart;
    store<u32>(iov, changetype<usize>(s_utf8_buf)); // mem256 = [s_utf8_buf]
    store<u32>(iov, s_utf8_len, sizeof<usize>()); // mem256 = [s_utf8_buf, s_utf8_len]
    let lf = changetype<ArrayBufferView>(mem64).dataStart;
    store<u8>(lf, 10); // mem64 = [10]
    store<u32>(iov, lf, sizeof<usize>() * 2); // mem256 = [s_utf8_buf, s_utf8_len, lf]
    store<u32>(iov,  1, sizeof<usize>() * 3); // mem256 = [s_utf8_buf, s_utf8_len, lf, 1]
    let written_ptr = changetype<ArrayBufferView>(mem64).dataStart;
    fd_write(this.rawfd, iov, 2, written_ptr);
    // ^ writes to mem64 at 0 (undefined behavior)
    // __release(s_utf8_buf)
  }

Looks fine from a runtime perspective, with the UB depending on the implementation of fd_write but even then not affecting the runtime.

@torch2424
Copy link
Contributor Author

Oh Wow that was fast haha! 😄 Thanks y'all!

@dcodeIO

Translated your example to this test case: ... Running this on master, with rtrace enabled, doesn't show any problems. Might this have been fixed since the last release?

So I tried installing the latest master with my example and it didn't work 😢

Screenshot from 2020-05-14 16-07-30

@MaxGraey

btw you use pretty old assemblyscript version in your playground: https://github.com/torch2424/as-playground/blob/master/object-literal-pass-instance/package-lock.json#L12

Two minors is old? 😂 But thanks for catching that! I updated to 0.9.4 and still no dice 😢

It could be problem with wasmtime or as-wasi and Console

So I replaced out as-wasi and the console logging and it didn't work:

Screenshot from 2020-05-14 16-05-04

So It may be an issue with Wasmtime? Though, I originally found this bug running AS on Lucet 🤔

@dcodeIO

Looks fine from a runtime perspective, with the UB depending on the implementation of fd_write but even then not affecting the runtime.

I think your responding to Max here, but just in case: I don't think there's anything wrong with the Logging. Because I can totally add some more logs after my testing logs, or do whatever I need to do, and it's fine. I think we may have some dangling memory or something weird after the _start ends? 🤔

@dcodeIO
Copy link
Member

dcodeIO commented May 14, 2020

In my test, what might have happened is that since I'm not importing as-wasi, static memory contents differ in a way that the bug does not show. Will try to replicate it more closely.

@dcodeIO
Copy link
Member

dcodeIO commented May 14, 2020

Tried to compile your exact snippet using as-wasi from Git and AS master, but that doesn't compile due to

ERROR AS231: A class with a constructor explicitly returning something else than 'this' must be '@final'.

 export class Descriptor {
              ~~~~~~~~~~
 in ~lib/as-wasi/as-wasi.ts(91,14)

Are you sure you tried with master? I think you should have seen that error as well then.

@dcodeIO
Copy link
Member

dcodeIO commented May 14, 2020

Updating the snippet to AS 0.10.0 (incl. adding @final to Descriptor)

import "wasi";
import {Console} from "as-wasi";

class OtherClass {
  stuff: i32;

  constructor(newStuff: i32) {
    this.stuff = newStuff;
  }
}

class MyClassInit {
  otherClass: OtherClass;
}

class MyClass {
  _myParam: i32;
  _otherClass: OtherClass;

  constructor(param: i32, init: MyClassInit) {
    this._myParam = param;
    this._otherClass = init.otherClass;
  }

  toString(): string {
    return this._otherClass.stuff.toString();
  }
}

// Make our Other class
let otherClass = new OtherClass(24);

// Pass to my class as an object literal
let myClass = new MyClass(2424, {
  otherClass: otherClass
});

Console.log("Logging myClass.toString() ...\n");
Console.log(myClass.toString());
Console.log("\n");

compiling with

asc assembly/index.ts -b build/optimized.wasm -t build/optimized.wat --runtime half --explicitStart

and then running it in wasmtime v0.16.0

wasmtime build/optimized.wasm

seems to work:

Logging myClass.toString() ...

24

@torch2424
Copy link
Contributor Author

@dcodeIO

Tried to compile your exact snippet using as-wasi from Git and AS master, but that doesn't compile due to

Oh huh :o I installed from git using like the syntax of:

npm install --save https://git.assemblyscript ....

Maybe that doesn't install master for you? 🤔

Updating the snippet to AS 0.10.0 (incl. adding @Final to Descriptor)

Just tested this, and yep, 0.10.0 fixes this issue! 😄 👍 Thanks very much for making the release that fixed this by proxy haha! 😄 😂

Closing this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants