Skip to content

Enable the gc/global.wast spec test#1296

Closed
fitzgen wants to merge 1 commit into
bytecodealliance:mainfrom
fitzgen:gc-table-init-and-globals
Closed

Enable the gc/global.wast spec test#1296
fitzgen wants to merge 1 commit into
bytecodealliance:mainfrom
fitzgen:gc-table-init-and-globals

Conversation

@fitzgen

@fitzgen fitzgen commented Nov 16, 2023

Copy link
Copy Markdown
Member

This involved queuing up table init expressions for later validation because:

  • Tables (and their init expressions) come before globals definitions in the binary format,
  • constant expressions can contain global.gets of defined globals now,
  • and therefore a table init can validly reference a global before it is defined.

This is not ideal, but is fine, since we have some other end-of-module checking already in validate_end. However, that alone isn't good enough. Because constant expressions can contain ref.func $f instructions, which need to insert $f into the "referenced function" set, we need mutable access to the Arc<Module> when processing constant expressions. So we need to drain the unchecked table init expressions before processing each code body, when we still have unique access to the Arc<Module>. But not all Wasm modules have code sections, so at the same time, we still need to do the checking inside validate_end. And when there was a code section, the queue of table init expressions will have already been drained by the time we are in validate_end, so it is okay that we no longer have unique access to the Arc<Module> in that case.

This involved queuing up table init expressions for later validation because:

* Tables (and their init expressions) come before globals are defined in the
  binary format,
* constant expressions can contain `global.get`s of defined globals now,
* and therefore a table init can validly reference a global before it is
  defined.

This is not ideal, but is fine, since we have some other end-of-module checking
already in `validate_end`. However, that alone isn't good enough. Because
constant expressions can contain `ref.func $f` instructions, which need to
insert `$f` into the "referenced function" set, we need mutable access to the
`Arc<Module>` when processing constant expressions. So we need to drain the
unchecked table init expressions *before* processing each code body, when we
still have unique access to the `Arc<Module>`. But not all Wasm modules have
code sections, so at the same time, we still need to do the checking inside
`validate_end`. And when there was a code section, the queue of table init
expressions will have already been drained by the time we are in `validate_end`,
so it is okay that we no longer have unique access to the `Arc<Module>`.
@fitzgen fitzgen requested a review from alexcrichton November 16, 2023 20:35

@alexcrichton alexcrichton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh I'm a bit surprised that bulk-memory went with a data-count section and gc went this direction. To confirm this is what other engines are doing?

Also can this new validation behavior be gated behind the gc feature?

@fitzgen

fitzgen commented Nov 17, 2023

Copy link
Copy Markdown
Member Author

Aha, I guess that our copy of the test suite is a bit out of date, because they fixed this upstream so that the contex that table initializers are evaluated within do not have defined globals present anymore:

@fitzgen

fitzgen commented Nov 17, 2023

Copy link
Copy Markdown
Member Author

Just FYI, I started trying to update the testsuite and plumb that through to this repo, but I probably won't finish today and I go on vacation for three weeks starting tomorrow.

@fitzgen

fitzgen commented Nov 17, 2023

Copy link
Copy Markdown
Member Author

WebAssembly/testsuite#71

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

Successfully merging this pull request may close these issues.

2 participants