Skip to content
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

[interpreter] Fix registry to be per-thread #219

Open
wants to merge 2 commits into
base: upstream-rebuild
Choose a base branch
from

Conversation

rossberg
Copy link
Member

@rossberg rossberg commented Oct 19, 2023

Also:

  • Fix module registration in tests to make them pass again
  • Update run.py script to actually run threads tests
  • Re-add and extend original thread.wast test to check thread scripting features
  • Include either patterns from main that got lost on upstream-rebuild

Also, I noticed that the JS translation still can't handle the thread feature of test scripts. I opened #218.

@conrad-watt
Copy link
Collaborator

conrad-watt commented Oct 19, 2023

None of the tests are really able to use either - it would need to take values with full "result types" (i.e. lists of values rather than single values) to support moving the current result logic into the assertion (in general). Are you very attached to it?

@rossberg
Copy link
Member Author

I think it was adopted by relaxed SIMD tests, so I don't want to lose its correct integration with the threading implementation.

@@ -606,8 +628,8 @@ let of_command mods cmd =
of_assertion' mods act "run" [] None ^ "\n"
| Assertion ass ->
of_assertion mods ass ^ "\n"
| Thread _ -> failwith "JS translation of Thread is NYI"
| Wait _ -> failwith "JS translation of Wait is NYI"
| Thread _ -> "" (* TODO: failwith "JS translation of Thread is NYI" *)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry to have been slow to iterate on this PR. Is this behaviour preferable to having the failwith behaviour? My understanding is that this effectively causes garbled JS tests to be silently spat out instead of the translation explicitly failing.

Copy link
Member Author

Choose a reason for hiding this comment

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

The practical problem is that as is, this makes the test runner fail, which also breaks CI. The conceptual problem is that the current assumption is that every core test can be converted to JS. If we want to exempt some tests, we'll need additional infrastructure for that. But what would that imply for the role of the test suite?

Is there truly no way to transform the thread construct to JS faithfully?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is - it would have to be a follow-up PR though. I'm just trying to clarify if generating garbled tests would be a preferable behaviour in the meantime.

Copy link
Collaborator

@conrad-watt conrad-watt Oct 24, 2023

Choose a reason for hiding this comment

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

Could we at least turn the thread and wait constructs into some kind of dynamic "assert false" instead?

EDIT: maybe something like "assert_malformed <obviously malformed module>" with a comment that this is caused by a NYI construct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. The idea of opening an issue for it was to do it separately. Adding a "correct" temporary work-around probably is more work than is worthwhile.

Copy link
Member Author

Choose a reason for hiding this comment

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

(The thread construct is in a completely different category than the arguments of assertions, so we cannot simply wrap it into one.)

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