Add build-time checks for ImpellerC's SkSL compiler by b-luk · Pull Request #180861 · flutter/flutter

b-luk · 2026-01-12T19:58:36Z

Adds build-time checks for ImpellerC's SkSL compiler in two places:

The CompilerSkSL class in spirv_sksl.cc generates the compiled SkSL from the user's input shader code. It has an existing detect_unsupported_resources method which analyzes the generated ParsedIR to detect various features that SkSL does not support (e.g. UBOs and SSBOs, push constant blocks, and user specified inputs). This PR adds additional checks to detect_unsupported_resources to detect array initialization using array initializers (e.g. float[](1.0, 2.0)), which is not supported by SkSL. This specifically addresses [Impeller] Compiler: GLSL array literals in shaders crash at runtime. #125630.
In the main Compiler class in compiler.cc, when compiling to SkSL, validate the compiled SkSL by using it to create a SkRuntimeEffect. Some invalid SkSL cases, like array assignment (float[2] float_array = another_float_array;) are hard to detect from the ParsedIR, so CompilerSkSL does not attempt to detect these invalid cases. The validation added in Compiler will catch all of these cases after SkSL compilation. The error messages produced from this validation are less informative than error messages from CompilerSkSL's checks. It surfaces the error produced by Skia's SkSL parser and outputs the entire generated SkSL file (which can be substantially different from the user's input shader code).

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

gemini-code-assist

Code Review

This pull request adds build-time checks for ImpellerC's SkSL compiler. The changes introduce validation in two key areas: first, by enhancing the CompilerSkSL class to detect unsupported array initializers directly from the ParsedIR, providing precise error messages. Second, it adds a post-compilation validation step in the Compiler class, which uses Skia's SkRuntimeEffect to catch a broader range of SkSL invalidities that are harder to detect from the IR, such as array assignments. The changes are accompanied by a comprehensive set of unit tests that cover both successful and failing compilation scenarios, ensuring the new checks work as expected. The implementation is solid and improves the robustness of the SkSL compilation process.

gaaclarke

The approach is looking good to me. The testing is looking good too. My only hangup is making sure we aren't being over prescriptive, I thought going into this it was only array initializers at file scope that was the problem.

gaaclarke · 2026-01-12T21:40:38Z

+      // this.
+      auto& block = id.get<SPIRBlock>();
+
+      // File ane line information for use in the error message.


Suggested change

// File ane line information for use in the error message.

// File and line information for use in the error message.

gaaclarke · 2026-01-12T21:41:57Z

+          // Check for OpStore instructions that store an array constant. This
+          // detects array initializations which use compile-time constants
+          // (e.g. `float[2] nums = float[](1.0, 2.0);`).
+          auto& store_object_id = ir.ids[ir.spirv[instruction.offset + 1]];


Please remove auto

https://google.github.io/styleguide/cppguide.html#Type_deduction

gaaclarke · 2026-01-12T21:43:11Z

+          // next OpLine instruction.
+          file = get<SPIRString>(ir.spirv[instruction.offset]).str;
+          line = ir.spirv[instruction.offset + 1];
+        } else if (instruction.op == OpStore) {


Is the problem limited to arrays at file scope? Will this check catch things in function scope?

This check catches array initialization at both file and function scope.

The problem isn't limited to array at file scope. The array initializer syntax isn't supported at all in SkSL.

Skia's doc for SkSL gives very little information about what syntax is or isn't supported, and no details on what GLSL version SkSL is based upon.

Older versions of GLSL do not support array initializers:

The GLSL 1.10 spec in section "4.1.9 Arrays" states: "There is no mechanism for initializing arrays at declaration time from within a shader."

The GLSL 1.20 spec in section "4.1.9 Arrays" is updated to show array initializer use.

I don't know what specific version of GLSL was used to base SkSL off of, so it's hard to figure out exactly what syntax is supported. From experimenting at https://shaders.skia.org/, all single-line array initialization gives an error. So I assume SkSL is based on an older GLSL version that does not support any array initializers.

gaaclarke · 2026-01-12T21:44:56Z

+
+  float more_nums[2];
+  // SkSL does not support array assignment.
+  more_nums = nums;


Same question as above, I thought the problem was just for initializers of uniforms, this is a problem too?

Yep. From experimenting at https://shaders.skia.org/, this is also unsupported.

gaaclarke · 2026-01-12T21:46:01Z

    return;
  }

+  if (sl_compiler.GetType() == CompilerBackend::Type::kSkSL) {


This isn't technically needed right? This is just a backstop for other issues? This sounds good to me.

It's a backstop to detect any issues which we don't have explicit checks for in spirv_sksl.cc. The array_assignment.frag test case is one such undetected issue which this backstop catches.

Without this, we have the existing behavior of the invalid SkSL triggering a runtime error when the fragment program is loaded. With this backstop, the invalid SkSL triggers an error at build time instead of runtime.

gaaclarke

lgtm, thanks b-luk!

…flutter#180861)

Adds build-time checks for ImpellerC's SkSL compiler in two places: - The `CompilerSkSL` class in `spirv_sksl.cc` generates the compiled SkSL from the user's input shader code. It has an existing `detect_unsupported_resources` method which analyzes the generated `ParsedIR` to detect various features that SkSL does not support (e.g. UBOs and SSBOs, push constant blocks, and user specified inputs). This PR adds additional checks to `detect_unsupported_resources` to detect array initialization using array initializers (e.g. `float[](1.0, 2.0)`), which is not supported by SkSL. This specifically addresses flutter#125630. - In the main `Compiler` class in `compiler.cc`, when compiling to SkSL, validate the compiled SkSL by using it to create a `SkRuntimeEffect`. Some invalid SkSL cases, like array assignment (`float[2] float_array = another_float_array;`) are hard to detect from the `ParsedIR`, so `CompilerSkSL` does not attempt to detect these invalid cases. The validation added in `Compiler` will catch all of these cases after SkSL compilation. The error messages produced from this validation are less informative than error messages from `CompilerSkSL`'s checks. It surfaces the error produced by Skia's SkSL parser and outputs the entire generated SkSL file (which can be substantially different from the user's input shader code). Fixes flutter#125630. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

sksl compiler checks

9530332

github-actions Bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Jan 12, 2026

b-luk mentioned this pull request Jan 12, 2026

[Impeller] Compiler: GLSL array literals in shaders crash at runtime. #125630

Closed

2 tasks

b-luk requested a review from gaaclarke January 12, 2026 20:09

gemini-code-assist Bot reviewed Jan 12, 2026

View reviewed changes

gaaclarke reviewed Jan 12, 2026

View reviewed changes

b-luk mentioned this pull request Jan 12, 2026

[Web Engine] Change FragmentProgram SkSL shader runtime validation to be build/compile-time #180868

Open

address gaaclarke comments

c66f6a9

b-luk requested a review from gaaclarke January 12, 2026 22:58

gaaclarke approved these changes Jan 12, 2026

View reviewed changes

b-luk added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 12, 2026

auto-submit Bot added this pull request to the merge queue Jan 13, 2026

Merged via the queue into flutter:master with commit 3dda832 Jan 13, 2026
181 checks passed

flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 13, 2026

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2026

3dda832 Add build-time checks for ImpellerC's SkSL compiler (flutter/…

9594d3b

…flutter#180861)

engine-flutter-autoroll mentioned this pull request Jan 14, 2026

Roll Flutter from 793b0b89e82e to 9aea438bfcb9 (22 revisions) flutter/packages#10789

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2026

3dda832 Add build-time checks for ImpellerC's SkSL compiler (flutter/…

9d54245

…flutter#180861)

engine-flutter-autoroll mentioned this pull request Jan 14, 2026

Roll Flutter from 793b0b89e82e to bc63dfe662d6 (24 revisions) flutter/packages#10790

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2026

3dda832 Add build-time checks for ImpellerC's SkSL compiler (flutter/…

ed794d7

…flutter#180861)

engine-flutter-autoroll mentioned this pull request Jan 14, 2026

Roll Flutter from 793b0b89e82e to 5e417dc81274 (25 revisions) flutter/packages#10791

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2026

3dda832 Add build-time checks for ImpellerC's SkSL compiler (flutter/…

a177b82

…flutter#180861)

engine-flutter-autoroll mentioned this pull request Jan 14, 2026

Roll Flutter from 793b0b89e82e to 5e417dc81274 (25 revisions) flutter/packages#10795

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2026

3dda832 Add build-time checks for ImpellerC's SkSL compiler (flutter/…

f5607e3

…flutter#180861)

engine-flutter-autoroll mentioned this pull request Jan 14, 2026

Manual roll Flutter from 793b0b89e82e to 53e9db28740b (29 revisions) flutter/packages#10796

Closed

b-luk deleted the sksl-compiler-checks branch January 14, 2026 22:56

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 15, 2026

3dda832 Add build-time checks for ImpellerC's SkSL compiler (flutter/…

db1ebfe

…flutter#180861)

engine-flutter-autoroll mentioned this pull request Jan 15, 2026

Manual roll Flutter from 793b0b89e82e to d316469e1d13 (13 revisions) flutter/packages#10797

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 15, 2026

3dda832 Add build-time checks for ImpellerC's SkSL compiler (flutter/…

5615169

…flutter#180861)

engine-flutter-autoroll mentioned this pull request Jan 15, 2026

Manual roll Flutter from 793b0b89e82e to 5a2067baa3d1 (12 revisions) flutter/packages#10798

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 15, 2026

3dda832 Add build-time checks for ImpellerC's SkSL compiler (flutter/…

1ceed34

…flutter#180861)

engine-flutter-autoroll mentioned this pull request Jan 15, 2026

Manual roll Flutter from 793b0b89e82e to b45a73b1dc3e (8 revisions) flutter/packages#10799

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 15, 2026

3dda832 Add build-time checks for ImpellerC's SkSL compiler (flutter/…

87de937

…flutter#180861)

engine-flutter-autoroll mentioned this pull request Jan 15, 2026

Manual roll Flutter from 793b0b89e82e to 3dda832e0523 (1 revision) flutter/packages#10800

Closed

zanderso mentioned this pull request Feb 13, 2026

SkSL compilation errors are generated when they're irrelevant #182400

Closed

	// File ane line information for use in the error message.
	// File and line information for use in the error message.

Uh oh!

Conversation

b-luk commented Jan 12, 2026

Pre-launch Checklist

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants