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

Rename mm to moduleinst #99

Merged
merged 4 commits into from
Jun 5, 2024
Merged

Rename mm to moduleinst #99

merged 4 commits into from
Jun 5, 2024

Conversation

rossberg
Copy link
Collaborator

@rossberg rossberg commented Jun 4, 2024

The seemingly innocent alpha-renaming of a local variable in this PR causes a stack overflow in the interpreter:

File "test-interpreter/dune", line 1, characters 0-285:
 1 | (mdx
 2 |   (libraries spectec)
 3 |   (deps
....
12 |   )
13 |   (files TEST.md)
14 | )
Fatal error: exception Stack overflow

@f52985, @ShinWonho, could one of you please have a look?

@rossberg rossberg requested review from f52985 and ShinWonho June 4, 2024 09:53
@ShinWonho
Copy link

ShinWonho commented Jun 5, 2024

The problem was that we hardcoded the return instruction of module instantiation, and the variable "mm" is also hardcoded. I think this is one of the part that is hard to automate, so I simply renamed the variables.

@rossberg
Copy link
Collaborator Author

rossberg commented Jun 5, 2024

Thanks! So, if I understand the change correctly, this only concerns the variable name in $instantiate?

Do you think we could have some suitable checks and error messages for assumptions like this? Just silently running with misbehaving code when a user fails to match undocumented backend assumptions is not the best user experience. ;)

@ShinWonho
Copy link

ShinWonho commented Jun 5, 2024

Yes, it was the case in $instantiation, but now I find that we can remove the hardcoded variable name here. (Now you can freely use "mm", "moduleinst", or anything else) However, for checks and error messages, it is not a trivial things. I have several things in mind, but maybe I need to discuss it with Donjun and other people at KAIST.

@rossberg rossberg merged commit edbe4a7 into main Jun 5, 2024
5 checks passed
@rossberg rossberg deleted the bug.mm branch June 5, 2024 07:19
rossberg pushed a commit that referenced this pull request Nov 7, 2024
Interpreter:

- Fixed evaluation of v128 load/store instructions to work with i64
- Reworked bulk operation execution to still reduce to well-typed instructions for i32
- Added missing size check to table allocation
- Various minor refactorings and clean-ups

Tests:

- Added tests for size check in i64 table and memory type limits

Split out from WebAssembly#1839
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