[move-vm][closures] Interpreter #15680
Conversation
[PR 5/n vm closures] This closes the loop, putting the pieces together to implement the closure logic in the interpreter loop. They are some paranoid checks still missing which are marked in the code.
⏱️ 11m total CI duration on this PR
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
|
||
| let ty_args = function.ty_args(); | ||
| if RTTCheck::should_perform_checks() { | ||
| if RTTCheck::should_perform_checks() && is_captured { |
There was a problem hiding this comment.
Change if RTTCheck::should_perform_checks() && is_captured { to if RTTCheck::should_perform_checks() && !is_captured { since type checking should be performed on stack operands, not captured arguments which were already verified
Spotted by Graphite Reviewer (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| let num_param_tys = function.param_tys().len(); | ||
|
|
||
| for i in 0..num_param_tys { | ||
| locals.store_loc( | ||
| num_param_tys - i - 1, | ||
| self.operand_stack.pop()?, | ||
| loader.vm_config().check_invariant_in_swap_loc, | ||
| )?; | ||
| for i in (0..num_param_tys).rev() { | ||
| let is_captured = mask.is_captured(i); | ||
| let value = if is_captured { | ||
| captured.pop().ok_or_else(|| { | ||
| PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR) | ||
| .with_message("inconsistent closure mask".to_string()) | ||
| })? | ||
| } else { | ||
| self.operand_stack.pop()? | ||
| }; | ||
| locals.store_loc(i, value, loader.vm_config().check_invariant_in_swap_loc)?; |
There was a problem hiding this comment.
Fix argument ordering mismatch by using captured.remove(0) instead of pop() to maintain consistent argument ordering between regular and captured arguments. This ensures arguments are processed in the same order regardless of whether they are captured or not.
Spotted by Graphite Reviewer (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
wrwg
requested review from
georgemitenkov,
runtian-zhou,
vgao1996,
vineethk and
ziaptos
Description
[PR 5/n vm closures]
This closes the loop, putting the pieces together to implement the closure logic in the interpreter loop.
They are some paranoid checks still missing which are marked in the code.
How Has This Been Tested?
Testing postponed until e2e tests are possible
Type of Change
Which Components or Systems Does This Change Impact?