Repository navigation
Add a compiler warning for fragment shaders containing both texture samplers and early returns. - #191969
Conversation
…amplers and early returns.
|
/gemini review |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
gaaclarke
left a comment
There was a problem hiding this comment.
lgtm =)
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. |
|
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. |
…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
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