Improve support for memory64/shared-everything-threads in components #2104
Conversation
Previously the validator in wasm-tools was updated to conservatively reject all aliases of exports of core memories/tables that were 64-bit or shared. This is a coarse approximation of the actual rules though which today disallow using 64-bit memories in the canonical ABI. Instead this commit updates with a few minor changes to improve the situation here, namely: * Aliasing core exports is now allowed for any table and memory type. * Usage of memories in canonical ABI positions now verifies that the memory is of the expected type, using the standard sub-typing. * Usage of tables in `canon thread.spawn_indirect` now performs a type-check via a similar method to ensure the table is of the right type. This adjusts the behavior of a few tests here and there as well to account for some slight differences in behavior with these proposals.
| if a.shared != b.shared { | ||
| bail!(offset, "mismatch in the shared flag for tables") | ||
| } |
There was a problem hiding this comment.
I'll note that this check was actually entirely missing from the table-type-subtyping here (oh dear!)
| @@ -3092,6 +3069,39 @@ impl<'a> SubtypeCx<'a> { | |||
| } | |||
| } | |||
|
|
|||
| pub(crate) fn table_type(a: &TableType, b: &TableType, offset: usize) -> Result<()> { | |||
| if a.element_type != b.element_type { | |||
There was a problem hiding this comment.
I'll also note that this isn't fully GC-compatible because this should actually be a subtype check I think (or something like that)
There was a problem hiding this comment.
I believe though that this is of the more general problem of "components and gc are not well integrated yet"
| bail!(offset, "expected a table of shared `funcref`"); | ||
| } | ||
|
|
||
| SubtypeCx::table_type( |
There was a problem hiding this comment.
I learn something new every day...
|
|
||
| (core module $B (import "" "" (table shared 1 2 (ref null (shared func))))) | ||
| (core instance (instantiate $B (with "" (instance (export "" (table $m)))))) | ||
| ) |
There was a problem hiding this comment.
You want to keep these tests in not-accepted.wast? Or rename this file? Not a big deal if we leave things here, I suppose, just noting the file name.
There was a problem hiding this comment.
I tend to pay far less attention to filenames than I should unfortunately... I think I'll go ahead and merge this PR, but I think it'd be best to clean up some organization from the old tests/local/* test suite which is no longer necessary in the future
Previously the validator in wasm-tools was updated to conservatively reject all aliases of exports of core memories/tables that were 64-bit or shared. This is a coarse approximation of the actual rules though which today disallow using 64-bit memories in the canonical ABI. Instead this commit updates with a few minor changes to improve the situation here, namely:
canon thread.spawn_indirectnow performs a type-check via a similar method to ensure the table is of the right type.This adjusts the behavior of a few tests here and there as well to account for some slight differences in behavior with these proposals.