Stream: git-wasmtime

Topic: wasmtime / PR #1923 wasmtime: Implement `table.get` and `...

Wasmtime GitHub notifications bot (Jun 25 2020 at 18:42):

fitzgen requested sunfishcode for a review on PR #1923.

Wasmtime GitHub notifications bot (Jun 25 2020 at 18:42):

fitzgen opened PR #1923 from table-get-and-table-set to master:

These instructions have fast, inline JIT paths for the common cases, and only
call out to host VM functions for the slow paths. This required some changes to
cranelift-wasm's FuncEnvironment: instead of taking a FuncCursor to insert
an instruction sequence within the current basic block,
FuncEnvironment::translate_table_{get,set} now take a &mut FunctionBuilder
so that they can create whole new basic blocks. This is necessary for
implementing GC read/write barriers that involve branching (e.g. checking for
null, or whether a store buffer is at capacity).

Furthermore, it required that the load, load_complex, and store
instructions handle loading and storing through an r{32,64} rather than just
i{32,64} addresses. This involved making r{32,64} types acceptable
instantiations of the iAddr type variable, plus a few new instruction
encodings.

Part of #929

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jun 25 2020 at 18:48):

alexcrichton edited PR #1923 from table-get-and-table-set to main:

These instructions have fast, inline JIT paths for the common cases, and only
call out to host VM functions for the slow paths. This required some changes to
cranelift-wasm's FuncEnvironment: instead of taking a FuncCursor to insert
an instruction sequence within the current basic block,
FuncEnvironment::translate_table_{get,set} now take a &mut FunctionBuilder
so that they can create whole new basic blocks. This is necessary for
implementing GC read/write barriers that involve branching (e.g. checking for
null, or whether a store buffer is at capacity).

Furthermore, it required that the load, load_complex, and store
instructions handle loading and storing through an r{32,64} rather than just
i{32,64} addresses. This involved making r{32,64} types acceptable
instantiations of the iAddr type variable, plus a few new instruction
encodings.

Part of #929

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jun 25 2020 at 19:29):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 19:29):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 19:29):

alexcrichton created PR Review Comment:

This seems like it may not be right, we don't have a *mut VMExternRef here I thought but rather *mut VMExternData? Should this be VMExternData::drop_and_dealloc?

Wasmtime GitHub notifications bot (Jun 25 2020 at 19:29):

alexcrichton created PR Review Comment:

I think technically this is memory unsafe since the destructor can run arbitrary code which can poke at the table. Perhaps the third statement could be let current_elem = replace(&mut table[index], value), and I think that'd fix any issues here?

Wasmtime GitHub notifications bot (Jun 25 2020 at 19:29):

alexcrichton created PR Review Comment:

(also does this mean that a test should be added for this too?

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:14):

fitzgen created PR Review Comment:

This isn't a *mut VMExternData (which is indeed what a VMExternRef is represented with) but actually a pointer to a VMExternRef. The table_addr instruction evaluates to a pointer to the table element, not the table element itself, and it is the result of the table_addr that we pass as an argument to the call to this function.

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:14):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:17):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:17):

fitzgen created PR Review Comment:

(also does this mean that a test should be added for this too?

I am 99% sure the spec tests exercise this path, but I can double check, and it might make sense to explicitly ensure that we test this, since it requires ref_count == 1 on the current table element that will be overridden.

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:19):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:19):

alexcrichton created PR Review Comment:

Ah sorry I assuming the pseudocode a bit too literally, if that's the case then this is all good!

(although I think this still runs afoul of the issue where the dtor tries to touch the table, so we may not be able to implement it as this for long)

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:20):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:20):

fitzgen created PR Review Comment:

This whole thing is sort of an inline destructor, the table[index] = ... bit of pseudocode isn't intended to convey that we are running destructors on the old table[index] within this assignment.

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:43):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:43):

fitzgen created PR Review Comment:

(although I think this still runs afoul of the issue where the dtor tries to touch the table, so we may not be able to implement it as this for long)

Hm this is a good point!

Normally, we would be safe because the table elements are in a RefCell, but the JIT code doesn't set the RefCell's borrow flags :-/

The only way I see to avoid this would be to either

• Make a custom version of RefCell where the JIT knows how to poke at the borrow flags (not great to add this extra overhead if we can avoid it, plus duplicating a bunch of std code)

• Do some kind of deferred destruction, where we queue up ready-to-die references and drain them at known safe points (opens the door for even more confusing/unexpected drop times).

Of the two, I think deferred destruction is probably the more promising approach.

Wasmtime GitHub notifications bot (Jun 25 2020 at 21:05):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 21:05):

alexcrichton created PR Review Comment:

We talked a bit on Zulip about how I think by swapping pointers we can sidestep the issue here, but this also makes me think that we should stop using RefCell in Table internals, instead switching to UnsafeCell because the JIT doesn't know about it.

Wasmtime GitHub notifications bot (Jun 25 2020 at 23:50):

fitzgen updated PR #1923 from table-get-and-table-set to main:

These instructions have fast, inline JIT paths for the common cases, and only
call out to host VM functions for the slow paths. This required some changes to
cranelift-wasm's FuncEnvironment: instead of taking a FuncCursor to insert
an instruction sequence within the current basic block,
FuncEnvironment::translate_table_{get,set} now take a &mut FunctionBuilder
so that they can create whole new basic blocks. This is necessary for
implementing GC read/write barriers that involve branching (e.g. checking for
null, or whether a store buffer is at capacity).

Furthermore, it required that the load, load_complex, and store
instructions handle loading and storing through an r{32,64} rather than just
i{32,64} addresses. This involved making r{32,64} types acceptable
instantiations of the iAddr type variable, plus a few new instruction
encodings.

Part of #929

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jun 26 2020 at 19:51):

fitzgen updated PR #1923 from table-get-and-table-set to main:

These instructions have fast, inline JIT paths for the common cases, and only
call out to host VM functions for the slow paths. This required some changes to
cranelift-wasm's FuncEnvironment: instead of taking a FuncCursor to insert
an instruction sequence within the current basic block,
FuncEnvironment::translate_table_{get,set} now take a &mut FunctionBuilder
so that they can create whole new basic blocks. This is necessary for
implementing GC read/write barriers that involve branching (e.g. checking for
null, or whether a store buffer is at capacity).

Furthermore, it required that the load, load_complex, and store
instructions handle loading and storing through an r{32,64} rather than just
i{32,64} addresses. This involved making r{32,64} types acceptable
instantiations of the iAddr type variable, plus a few new instruction
encodings.

Part of #929

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jun 26 2020 at 20:04):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 26 2020 at 20:04):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 26 2020 at 20:04):

alexcrichton created PR Review Comment:

Technically this comment applies to let current_elem above, right?

Wasmtime GitHub notifications bot (Jun 26 2020 at 20:04):

alexcrichton created PR Review Comment:

If GC/wasm happens multiple times, will this property always hold though? This seems like it'd need to both set next == end but also some sort of flag saying "always take the slow path, never reset next/end"

Wasmtime GitHub notifications bot (Jun 26 2020 at 20:04):

alexcrichton created PR Review Comment:

Oh wow nice find :+1:

Wasmtime GitHub notifications bot (Jun 26 2020 at 20:04):

alexcrichton created PR Review Comment:

FITZGEN!!!

Wasmtime GitHub notifications bot (Jun 26 2020 at 20:15):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Jun 26 2020 at 20:15):

fitzgen created PR Review Comment:

If you try to insert into the activations table, and next == end, then you do a gc (which will be re-entrant in this case, and exit early) and then insert into the over-approximized hash set. So, yes this relies on no one but the GC resetting next/end, but that property does hold right now.

Wasmtime GitHub notifications bot (Jun 26 2020 at 20:17):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Jun 26 2020 at 20:17):

fitzgen created PR Review Comment:

Woops, yeah I moved things around a bit.

Wasmtime GitHub notifications bot (Jun 26 2020 at 20:19):

fitzgen updated PR #1923 from table-get-and-table-set to main:

These instructions have fast, inline JIT paths for the common cases, and only
call out to host VM functions for the slow paths. This required some changes to
cranelift-wasm's FuncEnvironment: instead of taking a FuncCursor to insert
an instruction sequence within the current basic block,
FuncEnvironment::translate_table_{get,set} now take a &mut FunctionBuilder
so that they can create whole new basic blocks. This is necessary for
implementing GC read/write barriers that involve branching (e.g. checking for
null, or whether a store buffer is at capacity).

Furthermore, it required that the load, load_complex, and store
instructions handle loading and storing through an r{32,64} rather than just
i{32,64} addresses. This involved making r{32,64} types acceptable
instantiations of the iAddr type variable, plus a few new instruction
encodings.

Part of #929

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jun 26 2020 at 20:23):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 26 2020 at 20:23):

alexcrichton created PR Review Comment:

Oh right sorry I thought this happened somewhere in try_insert or similar, but it actually happens in this function! In that case the recursive guard on gc should be good enough :+1:

Wasmtime GitHub notifications bot (Jun 26 2020 at 23:25):

fitzgen updated PR #1923 from table-get-and-table-set to main:

These instructions have fast, inline JIT paths for the common cases, and only
call out to host VM functions for the slow paths. This required some changes to
cranelift-wasm's FuncEnvironment: instead of taking a FuncCursor to insert
an instruction sequence within the current basic block,
FuncEnvironment::translate_table_{get,set} now take a &mut FunctionBuilder
so that they can create whole new basic blocks. This is necessary for
implementing GC read/write barriers that involve branching (e.g. checking for
null, or whether a store buffer is at capacity).

Furthermore, it required that the load, load_complex, and store
instructions handle loading and storing through an r{32,64} rather than just
i{32,64} addresses. This involved making r{32,64} types acceptable
instantiations of the iAddr type variable, plus a few new instruction
encodings.

Part of #929

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jun 30 2020 at 18:25):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Jun 30 2020 at 18:25):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:02):

fitzgen updated PR #1923 from table-get-and-table-set to main:

These instructions have fast, inline JIT paths for the common cases, and only
call out to host VM functions for the slow paths. This required some changes to
cranelift-wasm's FuncEnvironment: instead of taking a FuncCursor to insert
an instruction sequence within the current basic block,
FuncEnvironment::translate_table_{get,set} now take a &mut FunctionBuilder
so that they can create whole new basic blocks. This is necessary for
implementing GC read/write barriers that involve branching (e.g. checking for
null, or whether a store buffer is at capacity).

Furthermore, it required that the load, load_complex, and store
instructions handle loading and storing through an r{32,64} rather than just
i{32,64} addresses. This involved making r{32,64} types acceptable
instantiations of the iAddr type variable, plus a few new instruction
encodings.

Part of #929

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:02):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:02):

fitzgen created PR Review Comment:

Good idea -- done!

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:52):

fitzgen merged PR #1923.

Last updated: Nov 22 2024 at 16:03 UTC