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

[CP] Fix Dart2js stack overflow in value range analysis #54494

Closed
biggs0125 opened this issue Jan 2, 2024 · 4 comments
Closed

[CP] Fix Dart2js stack overflow in value range analysis #54494

biggs0125 opened this issue Jan 2, 2024 · 4 comments
Assignees
Labels
area-infrastructure Use area-infrastructure for SDK infrastructure issues, like continuous integration bot changes. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable

Comments

@biggs0125
Copy link

Commit(s) to merge

6a9b550

Target

stable

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/344241

Issue Description

Dart2JS is crashing with a stack overflow in some scenarios when a variable is declared before a loop and updated within the loop. This logic was improved recently but some logic was missed when dispatching operators within the value range analyzer.

What is the fix

The fix is to correctly delegate operations within Dart2JS's value range analysis.

Why cherry-pick

This can cause the compiler to crash on fairly trivial code. For example (from original bug):

void main() {
  int counter = 0;
  for (int i = 0; i < 5; i++) {
      counter += counter;
  }
}

Risk

low

Issue link(s)

#54453

Extra Info

No response

@biggs0125 biggs0125 added the cherry-pick-review Issue that need cherry pick triage to approve label Jan 2, 2024
@sigmundch
Copy link
Member

I'm very much in favor of cherry-picking. This regression was introduced in 3.2 and causes the compiler to crash. The fix is small and low-risk.

@itsjustkevin itsjustkevin added cherry-pick-approved Label for approved cherrypick request merge-to-stable labels Jan 3, 2024
@mit-mit mit-mit added the area-infrastructure Use area-infrastructure for SDK infrastructure issues, like continuous integration bot changes. label Jan 3, 2024
@itsjustkevin
Copy link
Contributor

Cherry-pick approved feel free to merge @biggs0125.

copybara-service bot pushed a commit that referenced this issue Jan 5, 2024
The MarkerValue class was added fairly recently: 404edd9

Bug: #54453
Change-Id: I10d327a24425cecb2f0b44c0ea04bafa4cbf0698
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/343800
Cherry-pick-request: #54494
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/344241
Commit-Queue: Nate Biggs <natebiggs@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
@parlough
Copy link
Member

Fix landed in 719a36e which was part of today's release of Dart 3.2.5 :)

Thanks all!

@Zeeshan-H

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Use area-infrastructure for SDK infrastructure issues, like continuous integration bot changes. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable
Projects
None yet
Development

No branches or pull requests

9 participants