-
Notifications
You must be signed in to change notification settings - Fork 371
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(gnovm): handle loop variables #2429
Open
jaekwon
wants to merge
64
commits into
master
Choose a base branch
from
dev/jae/loopescape
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
+3,344
−354
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
jaekwon
force-pushed
the
dev/jae/loopescape
branch
from
June 26, 2024 01:07
c4a72f3
to
6b440e0
Compare
jaekwon
requested review from
ajnavarro,
a team,
gfanton,
thehowl,
mvertes and
zivkovicmilos
as code owners
June 26, 2024 01:07
github-actions
bot
added
📦 🌐 tendermint v2
Issues or PRs tm2 related
📦 ⛰️ gno.land
Issues or PRs gno.land package related
labels
Jun 26, 2024
7 tasks
deelawn
added a commit
that referenced
this pull request
Jul 31, 2024
I noticed this while reviewing #2429. The previous conditional could never evaluate to true: `if (1<<16 - 1) < sb.NumNames {`. `sb.NumNames` is a `uint16` and the value it is being compared to, `1<<16 - 1` is the max uint16 value, so it is impossible the the number of names to be greater than this value. Instead, we should check to see if the current number of names is the maximum value and panic if this is the case. <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [x] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details>
piux2
reviewed
Aug 10, 2024
ltzmaxwell
approved these changes
Aug 16, 2024
thehowl
reviewed
Aug 28, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicks for now; will need some more time to go through it all.
deelawn
approved these changes
Sep 9, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution seems to work well. Please see the comments and questions I left.
Co-authored-by: Morgan <[email protected]>
Co-authored-by: Morgan <[email protected]>
Co-authored-by: Morgan <[email protected]>
Co-authored-by: Morgan <[email protected]>
Co-authored-by: Morgan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
in focus
Core team is prioritizing this work
📦 🌐 tendermint v2
Issues or PRs tm2 related
📦 ⛰️ gno.land
Issues or PRs gno.land package related
📦 🤖 gnovm
Issues or PRs gnovm related
🧾 package/realm
Tag used for new Realms or Packages.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem Definition
The problem originates from the issue described in #1135. While the full scope of the issue is broader, it fundamentally relates to the concept of loop variable escapes block where it's defined.
e.g. 1:
e.g. 2:
e.g. 3:
Solution ideas
identify escaped vars in preprocess:
Detect situations where a loop variable is defined within a loop block(including
for/range
loops or loops constructed usinggoto
statements), and escapes the block where it's defined.runtime allocation:
Allocate a new heap item for the loop variable in each iteration to ensure each iteration operates with its unique variable instance.
NOTE1: this is consistent with Go's Strategy:
"Each iteration has its own separate declared variable (or variables) [Go 1.22]. The variable used by the first iteration is declared by the init statement. The variable used by each subsequent iteration is declared implicitly before executing the post statement and initialized to the value of the previous iteration's variable at that moment."
NOTE2: the
loopvar
feature of Go 1.22 is not supported in this version, and will be supported in next version.not supporting capture
i
defined in for/range clause;Implementation Details
Preprocess Stage(Multi-Phase Preprocessor):
Phase 1:
initStaticBlocks
: Establish a cascading scope structure wherepredefine
is conducted. In this phase Name expressions are initially marked asNameExprTypeDefine
, which may later be upgraded toNameExprTypeHeapDefine
if it is determined that they escape the loop block. This phase also supports other processes as a prerequisite#2077.Phase 2:
preprocess1
: This represents the original preprocessing phase(not going into details).Phase 3:
findGotoLoopDefines
: By traversing the AST, any name expression defined in a loop block (for/range, goto) with the attributeNameExprTypeDefine
is promoted toNameExprTypeHeapDefine
. This is used in later phase.Phase 4:
findLoopUses1
: Identify the usage ofNameExprTypeHeapDefine
name expressions. If a name expression is used in a function literal or is referrnced(e.g. &a), and it was previously defined asNameExprTypeHeapDefine
, theused
name expression is then given the attributeNameExprTypeHeapUse
. This step finalizes whether a name expression will be allocated on the heap and used from heap.Closures
represent a particular scenario in this context. Each closure, defined by a funcLitExpr that captures variables, is associated with a HeapCaptures list. This list consists of NameExprs, which are utilized at runtime to obtain the actual variable values for each iteration. Correspondingly, within the funcLitExpr block, a list of placeholder values are defined. These placeholders are populated during the doOpFuncLit phase and subsequently utilized in thedoOpCall
to ensure that each iteration uses the correct data.Phase 5:
findLoopUses2
: Convert non-loop uses of loop-defined names toNameExprTypeHeapUse
. Also, demoteNameExprTypeHeapDefine
back toNameExprTypeDefine
if no actual usage is found. Also , as the last phase, attributes no longer needed will be cleaned up after this phase.Runtime Stage:
Variable Allocation:
NameExprTypeHeapDefine
triggers the allocation of a newheapItemValue
for it, which will be used by anyNameExprTypeHeapUse
.Function Literal Handling:
doOpFuncLit
, retrieve theHeapCapture
values (previously allocated heap item values) and fill in the placeholder values within thefuncLitExpr
block.doOpCall
), theplaceHolder
values(fv.Captures) are used to update the execution context, ensuring accurate and consistent results across iterations.