nqp icon indicating copy to clipboard operation
nqp copied to clipboard

Fix Multiple Statevar Initialization Bugs

Open jstuder-gh opened this issue 5 years ago • 1 comments

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.

jstuder-gh avatar Jul 21 '18 19:07 jstuder-gh

@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).

usev6 avatar May 21 '20 19:05 usev6