Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@zanderso
Copy link
Member

Fixes flutter/flutter#111379. Identifiers with double-underscore in them are reserved in SkSL, but it looks like this is only enforced when targeting an Android emulator.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@chinmaygarde chinmaygarde changed the title [impellerc] Don't emit double underscores in SkSL backend [Impeller] Don't emit double underscores in SkSL backend. Sep 13, 2022
@chinmaygarde chinmaygarde self-requested a review September 13, 2022 20:14
@chinmaygarde
Copy link
Contributor

chinmaygarde commented Sep 13, 2022

Identifiers with double-underscore in them are reserved in SkSL.

This isn't a reservation in SKSL. Section 3.6 of the GLSL spec has this restriction. "In
addition, all identifiers containing two consecutive underscores (__) are reserved as possible future keywords."

SKSL is leaving the identifier as-is when transpiling to GLSL. This may lead to a situation where a valid SKSL shader doesn't generate valid GLSL. Perhaps this is also a Skia bug. But yeah, fixing it here sounds good as we'll side-step this issue in ImpellerC.

EDIT: Oof, what a mess. The 4.60 spec clarifies: "Defining or undefining such a name in a shader does not itself result in an error, but may result in unintended behaviors that stem from having multiple definitions of the same name." I think the Android emulator is incorrect in treating this as an error.

@zanderso
Copy link
Member Author

Ahh. Thanks for clarifying and bringing it up with the Skia team @chinmaygarde.

@zanderso zanderso merged commit 6642cfd into flutter:main Sep 13, 2022
@zanderso zanderso deleted the rm-double-underscore branch September 13, 2022 20:54
@johnstiles-google
Copy link
Contributor

This is an oversight in SkSL. We should reject this user code. I'll get in a fix.

@johnstiles-google
Copy link
Contributor

@johnstiles-google

This comment was marked as outdated.

@johnstiles-google
Copy link
Contributor

johnstiles-google commented Sep 14, 2022

I have an SkSL fix on deck at http://review.skia.org/580587

SkSL will continue to allow identifiers with multiple underscores. The GLSL backend will perform mangling as needed to make sure that we emit valid GLSL identifiers even if the original identifier contained __.

It should be safe for Impeller to ignore this issue going forward, as SkSL will paper over it for you.

@johnstiles-google
Copy link
Contributor

The SkSL mangling fix has been submitted.

Oleh-Sv pushed a commit to Oleh-Sv/engine that referenced this pull request Sep 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flutter shader ERROR: 0:12: '__main_S1_c0_v' : identifiers containing two consecutive underscores (__) are reserved

3 participants