threads: improve tests #1719
threads: improve tests #1719
Conversation
|
I would like to add two more Binaryen tests here but can't yet:
|
| /// Pop a reference type from the operand stack, checking if it matches | ||
| /// `expected` or the shared version of `expected`. This function will panic | ||
| /// if `expected` is shared or a concrete type. | ||
| fn pop_maybe_shared_ref(&mut self, expected: RefType) -> Result<Option<RefType>> { | ||
| assert!(!self.resources.is_shared_ref_type(expected)); | ||
| let actual = self._calculate_operand_type(Some(expected.into()))?; | ||
| let actual_ref = match self._as_ref_type(actual)? { | ||
| Some(rt) => rt, | ||
| None => return Ok(None), | ||
| }; | ||
| // Change our expectation based on whether we're dealing with an actual | ||
| // shared or unshared type. | ||
| let expected = if self.resources.is_shared_ref_type(actual_ref) { | ||
| expected | ||
| .shared() | ||
| .expect("this only expects abstract heap types") | ||
| } else { | ||
| expected | ||
| }; | ||
| self._check_operand_type(expected.into(), actual)?; | ||
| Ok(Some(actual_ref)) | ||
| } |
There was a problem hiding this comment.
This method feels like roughly the right signature but I'm not sure it requires the other refactorings in this PR. Some changes I might recommend:
- Don't add
is_shared_ref_typeand insteaddebug_assert!here thatexpectedis an abstract heap type with thesharedflag unset - Use
pop_refinternally here which doesn't take anexpectedargument - Given a popped ref test if it's a subtype of the given
expectedeither with thesharedflag set or unset. - Return
Result<Option<(RefType, bool)>>so callers don't have to recalculateshared-vs-not.
I think that should satisfy most use cases and avoid the need for larger refactorings?
There was a problem hiding this comment.
Sure, let's try that. I think we need to keep is_shared_ref_type or something like it, though, because we need need to be able to figure out the sharedness of an actual concrete type. We can debug_assert! away the checks for expected but, because actual can actually be concrete, we need to be able to resolve the index to the underlying type somehow to inspect its sharedness.
There was a problem hiding this comment.
A side note: at some point in earlier PRs I had a RefType::is_shared(&self) -> Option<bool> that would check sharedness only for the abstract types but I made do without it until now — even if I re-added it, it wouldn't be enough here because we still have to check sharedness for concrete types.
This change simply renames the shared-everything-threads tests to use more conventional names within the repository.
@tlively has been working on Binaryen support for shared-everything-threads and has upstreamed several [tests] that are useful here. This change incorporates some "bottom type" checks for arrays and structs as a `shared-ref_eq.wast` test checking `ref.eq` behavior for shared types (renamed to `ref-equivalence.wast` here). To make these added tests pass, validation for `ref.eq` now allows it to check both shared and ushared `eqref` types the the `ref.i31_shared` returns the correct type. [tests]: https://github.com/WebAssembly/binaryen/blob/main/test/spec [shared-ref_eq.wast]: https://github.com/WebAssembly/binaryen/blob/main/test/spec/shared-ref_eq.wast
Certain instructions accept both shared and unshared references: `ref.eq`, `i31.get_s`, `i31.get_u`, `array.len`, `any.convert_extern`, and `extern.convert_any`. Validation needs to handle these special cases to pass Binaryen's `polymorphism.wast` test. This change refactors the `pop_operand` family of functions with several helpers to add `pop_maybe_shared_ref`; `pop_maybe_shared_ref` lets the validator pass an unshared `RefType` expectation but adapts it to being shared if that is what is on the stack.
This change undoes previous work to rationalize the duplication in `_pop_operand`, `pop_ref`, and now `pop_maybe_shared_ref`. As suggested by @alexcrichton, this attempt keeps the status quo by reusing `pop_ref` and doing a bit more work after the fact (re-checking the subtype relationship).
This validator's existing logic assumed that the bottom and heap bottom types matched any reference type (see `_pop_operand`). This change surfaces that logic to `visit_ref_eq` as well: if `pop_maybe_shared_ref` returns `None` (a bottom or heap bottom) for either value to `ref.eq`, we skip checking the shared-ness match, assuming like we do in `_pop_operand` that a bottom or heap bottom matches any reference type (we've already checked that the types are subtypes of `eqref`).
|
Ok, I refactored and rewrote parts of this to address the feedback. I kept the |
This was an idea I had after bytecodealliance#1719 landed to statically ensure that the asserts won't trip.
This was an idea I had after #1719 landed to statically ensure that the asserts won't trip.
These changes improve shared-everything-threads tests; see commit messages for more details.