Skip to content

Add a compiler warning for fragment shaders containing both texture samplers and early returns. - #191969

Merged
auto-submit[bot] merged 9 commits into
flutter:masterfrom
walley892:windows-compiler-warning
Sep 15, 2026
Merged

auto-submit[bot] merged 9 commits into
flutter:masterfrom
walley892:windows-compiler-warning

Conversation

@walley892

@walley892 walley892 commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

What it says on the tin.

Fragment shaders with both texture samplers and early returns can hit a bug in the ANGLE->DX11 pipeline that can cause crashes on Windows #190809. This PR adds a check in impellerc for this condition and emits a warning when a fragment shader uses both early returns and texture samplers. The error is also emitted when using flutter run -v

@walley892 walley892 added the CICD Run CI/CD label Aug 28, 2026
@walley892

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a warning mechanism in the Impeller shader compiler to alert users when a fragment shader uses texture samplers alongside early return statements, which can cause compilation errors on Windows. The warning is propagated through the compiler, logged in the Flutter tools, and verified with new tests. Feedback on the changes highlights several robustness and correctness issues in the helper function CheckHasEarlyReturnsInMain—including missing unbraced returns, false positives in comments, and substring matching—and suggests a more robust parsing implementation.

Comment thread engine/src/flutter/impeller/compiler/compiler.cc Outdated
@github-actions github-actions Bot added tool Affects the "flutter" command-line tool. See also t: labels. engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Aug 28, 2026
@walley892 walley892 added CICD Run CI/CD and removed CICD Run CI/CD labels Aug 28, 2026
@walley892 walley892 added CICD Run CI/CD and removed CICD Run CI/CD labels Aug 28, 2026
@walley892
walley892 requested a review from gaaclarke August 31, 2026 18:33
@walley892
walley892 marked this pull request as ready for review August 31, 2026 18:33

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds a warning mechanism to the Impeller shader compiler to detect fragment shaders that use texture samplers alongside early return statements, which can cause compilation errors on Windows. The feedback suggests stripping comments from the shader source before parsing to prevent false positives in early return detection, and adding null checks to avoid undefined behavior when constructing strings from the source mapping.

Comment thread engine/src/flutter/impeller/compiler/compiler.cc Outdated
Comment thread engine/src/flutter/impeller/compiler/compiler.cc
@walley892 walley892 added CICD Run CI/CD and removed CICD Run CI/CD labels Aug 31, 2026
Comment thread engine/src/flutter/impeller/compiler/compiler.cc Outdated
Comment thread engine/src/flutter/impeller/compiler/compiler.cc Outdated
Comment thread engine/src/flutter/impeller/compiler/compiler.cc Outdated
Comment thread engine/src/flutter/impeller/compiler/compiler.cc Outdated
Comment thread engine/src/flutter/impeller/compiler/compiler.cc Outdated
Comment thread engine/src/flutter/impeller/compiler/compiler.cc Outdated
@walley892
walley892 requested a review from gaaclarke September 2, 2026 18:06
@walley892 walley892 added CICD Run CI/CD and removed CICD Run CI/CD labels Sep 2, 2026
Comment thread engine/src/flutter/impeller/compiler/compiler.cc Outdated
Comment thread engine/src/flutter/impeller/compiler/compiler.cc Outdated
@walley892
walley892 requested a review from gaaclarke September 3, 2026 17:57

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm =)

If we get rid of the derivatives, will this still be necessary?

@walley892

Copy link
Copy Markdown
Contributor Author

If we get rid of the derivatives, will this still be necessary?

Yerrr. This is for user-written fragment shaders, none of ours hit this error.

@walley892 walley892 removed the CICD Run CI/CD label Sep 3, 2026
@walley892 walley892 added the CICD Run CI/CD label Sep 3, 2026
@gaaclarke

Copy link
Copy Markdown
Member

Yea, I was thinking about the other problem we ran into where DX11 seemed to crash with multiple returns and derivative functions. This issue's reproduction didn't have derivatives at all.

The derivatives + early returns made sense to me since they require the other warps to be running to sample from. I have no idea what the deal is here.

@walley892 walley892 added CICD Run CI/CD and removed CICD Run CI/CD labels Sep 9, 2026
@walley892 walley892 added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 15, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Sep 15, 2026
Merged via the queue into flutter:master with commit 728b40b Sep 15, 2026
23 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 15, 2026
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Sep 17, 2026
…12890)

Manual roll requested by [email protected]

flutter/flutter@27fec0e...8737f11

2026-09-15 [email protected] [flutter_tools] Migrate BuildBundleCommand to modular dependency injection (flutter/flutter#192462)
2026-09-15 [email protected] Migrate SDK constraints for record_use and data_asset integration test packages to ^3.13.0-0 (flutter/flutter#192728)
2026-09-15 [email protected] [tool] Defer app.restart until the app has finished starting (flutter/flutter#192650)
2026-09-15 [email protected] Suggest the -d flag after interactive device selection (flutter/flutter#188941)
2026-09-15 [email protected] Roll Packages from 9caa77c to bebbb57 (9 revisions) (flutter/flutter#192809)
2026-09-15 [email protected] Add a compiler warning for fragment shaders containing both texture samplers and early returns. (flutter/flutter#191969)
2026-09-15 [email protected] feat(flutter_tools): migrate Android build commands to typed options (flutter/flutter#192524)
2026-09-15 [email protected] Roll Fuchsia Linux SDK from 7R1l5MhSZZUr1Q6D1... to XCxrICzX1DWvyfqhA... (flutter/flutter#192789)
2026-09-15 [email protected] Roll Dart SDK from 7253dff72c96 to c870e8afe036 (1 revision) (flutter/flutter#192791)

If this roll has caused a breakage, revert this CL and set the roller
to dry run mode using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD e: impeller Impeller rendering backend issues and features requests engine flutter/engine related. See also e: labels. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants