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.

Sign up for GitHub

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

Jump to bottom

structured ctrl #101

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

structured ctrl #101

wants to merge 6 commits into from

Conversation

cemonem
Copy link

@cemonem cemonem commented Dec 4, 2024 •
edited
Loading

Pull Request Overview

  • implementations for block...end, br, br_if, br_table, return, loop...end, if..else...end instructions.
  • ctrl stack and type stack are separated during validation to avoid linear look up for ctrl labels.
  • sidetable generation per function.
  • integration of sidetable and sidetable pointer in the interpreter loop for all ctrl structures.
  • execution tests for structured_ctrl.
  • assertion of types in the type stack during validation might mutate the stack. Any error during validation may leave the stack in a modified, invalid state.
  • validation tests for cases where assertion of types that do mutate the stack, and this change is required.
  • preparation for the type polymorphic select instruction (Polymorphic type NumOrVecType for select instruction on the type stack)

TODO

  • Per module sidetables instead of per function sidetables.
  • Backpatching sidetable entries without extra memory / without vectors in the CtrlStackEntries (using the linked list trick in wizard)

This pull request still needs...

Formatting

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

@cemonem cemonem force-pushed the dev/cemonem-structured-ctrl2 branch 2 times, most recently from 903fced to f68c38d Compare December 4, 2024 15:21
@cemonem cemonem changed the title structured ctrl: block..br n...end structured ctrl: block..br(if) n/return...end Dec 6, 2024
@codecov Codecov
Copy link

codecov bot commented Dec 6, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 89.44282% with 72 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/validation/code.rs 80.00% 9 Missing and 28 partials ⚠️
src/validation/validation_stack.rs 93.35% 13 Missing and 7 partials ⚠️
src/core/error.rs 0.00% 6 Missing ⚠️
src/core/reader/types/values.rs 64.28% 2 Missing and 3 partials ⚠️
src/core/reader/types/mod.rs 94.11% 0 Missing and 3 partials ⚠️
src/validation/mod.rs 95.23% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/core/reader/mod.rs 98.53% <100.00%> (+0.02%) ⬆️
src/core/reader/types/element.rs 82.29% <100.00%> (ø)
src/execution/interpreter_loop.rs 91.95% <100.00%> (+0.55%) ⬆️
src/execution/mod.rs 90.52% <100.00%> (+0.04%) ⬆️
src/execution/store.rs 81.81% <ø> (ø)
src/execution/value_stack.rs 80.59% <100.00%> (-2.22%) ⬇️
src/validation/mod.rs 79.05% <95.23%> (+0.58%) ⬆️
src/core/reader/types/mod.rs 57.26% <94.11%> (+9.89%) ⬆️
src/core/reader/types/values.rs 83.33% <64.28%> (-2.67%) ⬇️
src/core/error.rs 2.81% <0.00%> (-0.05%) ⬇️
... and 2 more

... and 1 file with indirect coverage changes

@cemonem cemonem force-pushed the dev/cemonem-structured-ctrl2 branch from dff4ddf to 032a136 Compare December 6, 2024 17:33
george-cosma
george-cosma requested changes Dec 9, 2024
View reviewed changes
Copy link
Collaborator

@george-cosma george-cosma left a comment

Choose a reason for hiding this comment

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

My review is not yet done, I still have to check the validation stack changes

src/core/reader/types/opcode.rs Show resolved Hide resolved
src/core/reader/types/values.rs Show resolved Hide resolved
src/core/reader/mod.rs Outdated Show resolved Hide resolved
src/core/reader/types/mod.rs Outdated Show resolved Hide resolved
src/core/reader/types/mod.rs Outdated Show resolved Hide resolved
src/validation/mod.rs Outdated Show resolved Hide resolved
src/execution/interpreter_loop.rs Show resolved Hide resolved
src/execution/interpreter_loop.rs Outdated Show resolved Hide resolved
src/execution/interpreter_loop.rs Show resolved Hide resolved
@cemonem cemonem changed the title structured ctrl: block..br(if) n/return...end structured ctrl: block..br(if) n/return...end, if...else...end, loop...end, return Dec 9, 2024
@cemonem cemonem changed the title structured ctrl: block..br(if) n/return...end, if...else...end, loop...end, return structured ctrl: block..br(if) n/return...end, if...else...end, loop...end Dec 9, 2024
florianhartung
florianhartung requested changes Dec 9, 2024
View reviewed changes
src/core/reader/types/mod.rs Outdated Show resolved Hide resolved
@cemonem cemonem changed the title structured ctrl: block..br(if) n/return...end, if...else...end, loop...end structured ctrl Dec 10, 2024
@cemonem cemonem force-pushed the dev/cemonem-structured-ctrl2 branch 3 times, most recently from 7a39a5a to db296f6 Compare December 17, 2024 10:19
@wucke13 wucke13 mentioned this pull request Jan 8, 2025
Closed
5 tasks
wucke13 and others added 6 commits January 9, 2025 09:34
@wucke13
feat: introduce sidetable
d2d1dca 
Signed-off-by: wucke13 <[email protected]>
Signed-off-by: Florian Hartung <[email protected]>
@wucke13
feat: add validation code for block types
5c261c1 
Signed-off-by: wucke13 <[email protected]>
Signed-off-by: Florian Hartung <[email protected]>
@florianhartung
test: add various block tests
ba84131 
Signed-off-by: Florian Hartung <[email protected]>
feat!: structured ctrl flow with per function sidetable
f9468ba 
Signed-off-by: Cem Onem <[email protected]>
fix: implement if...end without else
edc75c8 
Signed-off-by: Cem Onem <[email protected]>
fix: local.tee validation
623a393 
Signed-off-by: Cem Onem <[email protected]>
@cemonem cemonem force-pushed the dev/cemonem-structured-ctrl2 branch from 93de1ce to 623a393 Compare January 9, 2025 10:25
@wucke13 wucke13 enabled auto-merge January 9, 2025 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@florianhartung florianhartung florianhartung requested changes

@george-cosma george-cosma george-cosma requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
core execution priority-high priority-medium tests validation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cemonem @george-cosma @florianhartung @wucke13