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

Fix Multiple Statevar Initialization Bugs #487

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

Conversation

jstuder-gh
Copy link
Contributor

@jstuder-gh jstuder-gh commented Jul 21, 2018

This PR is one part of a series spanning the MoarVM, NQP, and Rakudo repositories.
MoarVM
Rakudo

This series of PRs modifies the state init check to instead keep a record of the statevars that have been 'HLL init-ed' and have that value determine whether to perform the assignment or not. If the statevar is not assigned to on the first invocation, it is shown as needing assignment the next time it is encountered despite what the MVM_FRAME_FLAG_STATE_INIT value is. If the statevar has been assigned to already and is encountered again within the same frame invocation (as the C-style loop example below), it is aware the that assignment has occurred and does not repeat it.

I had submitted a PR similar to this in the past, but this one has been rebased to a much newer HEAD and changed enough to the point that it feels appropriate to open a new PR.

Specific modifications made are:

  • Modified the p6stateinit extop to take a symbol as an argument. Within the function, lookup that symbol within the frame's lexical registry and determine whether the 'HLL assignment' has taken place. If so, do not assign again.

    • The value that persists whether an 'HLL assignment' has taken place is stored in the CodeRef object (alongside the statevar values themselves), which is unique to each closure. This 'isHllInit' value is a variable-length bit array, with each lexical represented by one bit.
  • Added a new extop, p6stateinitbulk, that will take a list of symbols and check multiple in one extop execution (for use in destructuring state assignments)

  • Implemented for both the MoarVM and JVM backends.

Issue being addressed:

In the past, we have determined whether to perform an assignment to a state variable by determining whether or not it was the first invocation of a closure (or clone of a frame). In MoarVM, an MVM_FRAME_FLAG_STATE_INIT value is set on the first frame of a given closure.

This works most of the time, but there are some instances where this is producing undesired results (see RT #102994)

sub f($x) {
    return if $x;
    state $y = 5;
    $y;
} 

f(1);
say f(0);
# OUTPUT:   (Any)
# EXPECTED: 5

Since during the first invocation of this closure (when MVM_FRAME_FLAG_STATE_INIT is set) the statevar is not assigned to, on the next invocation the assignment is skipped and the unassigned value (Any) is returned.

In the case of 'once' with a C-style loop conditional (see GH issue 1684), this method also produces some unexpected results.

loop (my $i = 0; $i < once { print $i; 10 }; ++$i ) { }
# OUTPUT:   012345678910
# EXPECTED: 0

In this case, the 'once' appears in the e2 expression in the C-style loop. The statevar init check occurs within the loop conditional and is executed multiple times within the same initial closure invocation. Because of this, MVM_FRAME_FLAG_STATE_INIT is set and the code within the block (containing print) is executed for each iteration.

Add a value that indicates whether the statevar associated with the
coderef has been assigned a value by the HLL (flag set via extop on HLL
side).

This change is being made to coincide with a Rakudo development
regarding statevar initialization.

See [RT#102994](https://rt.perl.org/Public/Bug/Display.html?id=102994)
It would be really inefficient to use an array to record whether the hll
statevar is init when only one bit is needed (per lexical), especially
when there are a large amount of lexicals. Record in a BitSet instead.
@usev6
Copy link
Contributor

usev6 commented May 21, 2020

@jstuder-gh and another of your open PR that has conflicts after I merged the s/perl6/raku/ rename (#630). Again, it's hopefully easy to solve the conflicts, since the changes just need to be copied to the new locations of the changed files (src/vm/jvm/runtime/org/nqp/....

I don't feel qualified to comment on the changes you made (esp. since this is related to other pull requests).

@coke coke changed the base branch from master to main April 19, 2023 13:41
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