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

feat: module-level globals #78

Merged
merged 3 commits into from
Sep 11, 2024
Merged

feat: module-level globals #78

merged 3 commits into from
Sep 11, 2024

Conversation

george-cosma
Copy link
Collaborator

Pull Request Overview

This pull request adds module-level support for globals.
Note: not all constant expressions are implemented. The missing ones are:

  • ref.null
  • ref.func
  • global.get (need to properly implement imports first)

Testing Strategy

This pull request was tested by running cargo test + a lot of manually testing in wat2wasm. Details of my findings are in the comments

Help Wanted

I feel like the code is everywhere. A little advice as to how to organize all the new additions would be welcome!

Formatting

  • Ran cargo fmt
  • Ran cargo check
  • Ran cargo build
  • Ran cargo doc
  • Ran nix fmt

Github Issue

This pr depends on #77

Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 58.40708% with 47 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/execution/const_interpreter_loop.rs 36.36% 35 Missing ⚠️
src/validation/globals.rs 70.73% 8 Missing and 4 partials ⚠️
Files with missing lines Coverage Δ
src/core/reader/types/global.rs 25.00% <ø> (+25.00%) ⬆️
src/execution/mod.rs 96.07% <100.00%> (+4.75%) ⬆️
src/validation/code.rs 67.68% <ø> (+3.49%) ⬆️
src/validation/mod.rs 62.71% <100.00%> (+4.98%) ⬆️
src/validation/globals.rs 70.73% <70.73%> (ø)
src/execution/const_interpreter_loop.rs 36.36% <36.36%> (ø)

... and 2 files with indirect coverage changes

src/execution/interpreter_loop.rs Outdated Show resolved Hide resolved
src/execution/mod.rs Show resolved Hide resolved
src/execution/mod.rs Outdated Show resolved Hide resolved
src/validation/code.rs Outdated Show resolved Hide resolved
@george-cosma george-cosma force-pushed the feat/globals branch 2 times, most recently from 8523b85 to 2b61458 Compare August 29, 2024 11:39
@george-cosma
Copy link
Collaborator Author

As a note, besides what we discussed I also moved validate_value_stack to validation/mod.rs

Copy link
Collaborator

@florianhartung florianhartung left a comment

Choose a reason for hiding this comment

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

I'm not sure whether it is the right approach to have duplicated validations and implementations for all const expressions (see execution::const_interpreter_loop::run_const).

src/core/reader/types/global.rs Outdated Show resolved Hide resolved
@george-cosma
Copy link
Collaborator Author

I'm not sure whether it is the right approach to have duplicated validations and implementations for all const expressions (see execution::const_interpreter_loop::run_const).

While I agree that having both another validator and an execution loop for const expressions is a bit inelegant - tons of repeated code! - the alternative would be to integrate these expressions into the regular loops. While possible, this would take a lot of work and I'd wager that the end result would be less readable than what we have now. Just to name a few reasons, the const expression loop needs to return the length of its code (it isn't mentioned in the binary, like how it is for code blocks); a merged execution loop would still need to differentiate between regular execution and const execution, because we want to error out if we try to execute a non-const instruction in a const expression; etc.

florianhartung
florianhartung previously approved these changes Sep 9, 2024
The constant-expression proposal is also supported

Signed-off-by: George Cosma <[email protected]>
@george-cosma
Copy link
Collaborator Author

In the last commit, I've done a bit more rework so that all validation of globals, including init expressions is done entirely within the validation module.

nerodesu017
nerodesu017 previously approved these changes Sep 10, 2024
…moved global validation entirely to validation crate

Signed-off-by: George Cosma <[email protected]>
@wucke13 wucke13 added this pull request to the merge queue Sep 11, 2024
Merged via the queue into main with commit 9ee5291 Sep 11, 2024
12 checks passed
@wucke13 wucke13 deleted the feat/globals branch September 11, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants