Disallow most modifiers in runtime effects

Bug: skia:11301
Change-Id: Ib644235ef35f9fb37a1373ada6a6896005dc4451
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/558537
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>

gn/sksl_tests.gni

@@ -749,6 +749,7 @@ sksl_rte_error_tests = [
   "/sksl/runtime_errors/IllegalArrayOps.rts",
   "/sksl/runtime_errors/IllegalIndexing.rts",
   "/sksl/runtime_errors/IllegalLayoutFlags.rts",
+  "/sksl/runtime_errors/IllegalModifiers.rts",
   "/sksl/runtime_errors/IllegalOperators.rts",
   "/sksl/runtime_errors/IllegalPrecisionQualifiers.rts",
   "/sksl/runtime_errors/IllegalShaderSampling.rts",

resources/sksl/errors/BadModifiers.sksl

@@ -6,6 +6,7 @@ void func2(const in out uniform flat noperspective sk_has_side_effects
 const in out uniform flat noperspective sk_has_side_effects inline noinline float var;
 
 /*%%*
+'sk_has_side_effects' is not permitted here
 'const' is not permitted here
 'in' is not permitted here
 'out' is not permitted here
@@ -13,14 +14,15 @@ const in out uniform flat noperspective sk_has_side_effects inline noinline floa
 'flat' is not permitted here
 'noperspective' is not permitted here
 functions cannot be both 'inline' and 'noinline'
+'sk_has_side_effects' is not permitted here
 'uniform' is not permitted here
 'flat' is not permitted here
 'noperspective' is not permitted here
 'sk_has_side_effects' is not permitted here
 'inline' is not permitted here
 'noinline' is not permitted here
-'in uniform' variables not permitted
 'sk_has_side_effects' is not permitted here
+'in uniform' variables not permitted
 'inline' is not permitted here
 'noinline' is not permitted here
 'const' variables must be initialized

resources/sksl/runtime_errors/IllegalModifiers.rts

@@ -0,0 +1,40 @@
+flat                float _flat;
+noperspective       float _noperspective;
+in                  float _in;
+out                 float _out;
+threadgroup         float _threadgroup;
+$es3                float _es3;
+sk_has_side_effects float _sk_has_side_effects;
+inline              float _inline;
+noinline            float _noinline;
+
+flat                void flat_fn() {}
+noperspective       void noperspective_fn() {}
+in                  void in_fn() {}
+out                 void out_fn() {}
+threadgroup         void threadgroup_fn() {}
+$es3                void es3_fn() {}
+sk_has_side_effects void sk_has_side_effect_fn() {}
+
+float4 main(float2 xy) {
+    return float4(1);
+}
+
+/*%%*
+'flat' is not permitted here
+'noperspective' is not permitted here
+'in' is not permitted here
+'out' is not permitted here
+'threadgroup' is not permitted here
+'$es3' is not permitted here
+'sk_has_side_effects' is not permitted here
+'inline' is not permitted here
+'noinline' is not permitted here
+'flat' is not permitted here
+'noperspective' is not permitted here
+'in' is not permitted here
+'out' is not permitted here
+'threadgroup' is not permitted here
+'$es3' is not permitted here
+'sk_has_side_effects' is not permitted here
+*%%*/

resources/sksl/runtime_errors/IllegalShaderUse.rts

@@ -33,8 +33,8 @@ half4  dangling_eval()     { s1.eval; }
 
 /*%%*
 variables of type 'shader' must be uniform
-'in' variables not permitted in runtime effects
 variables of type 'shader' must be uniform
+'in' is not permitted here
 opaque type 'shader' is not permitted in a struct
 variables of type 'S' may not be uniform
 opaque type 'shader' may not be used in an array

src/sksl/SkSLDSLParser.cpp

@@ -940,8 +940,14 @@ DSLModifiers DSLParser::modifiers() {
         if (!tokenFlag) {
             break;
         }
+        Token modifier = this->nextToken();
+        // We have to check for this (internal) modifier here. It's automatically added to user
+        // functions before the IR is built, so testing for it in Convert gives false positives.
+        if (tokenFlag == Modifiers::kHasSideEffects_Flag && !ThreadContext::IsModule()) {
+            this->error(modifier, "'sk_has_side_effects' is not permitted here");
+        }
         flags |= tokenFlag;
-        end = this->position(this->nextToken()).endOffset();
+        end = this->position(modifier).endOffset();
     }
     return DSLModifiers(std::move(layout), flags, Position::Range(start, end));
 }

src/sksl/ir/SkSLVarDeclarations.cpp

@@ -179,11 +179,6 @@ void VarDeclaration::ErrorCheck(const Context& context,
     if ((modifiers.fFlags & Modifiers::kUniform_Flag)) {
         check_valid_uniform_type(pos, baseType, context);
     }
-    if (ProgramConfig::IsRuntimeEffect(context.fConfig->fKind)) {
-        if (modifiers.fFlags & Modifiers::kIn_Flag) {
-            context.fErrors->error(pos, "'in' variables not permitted in runtime effects");
-        }
-    }
     if (baseType->isEffectChild() && !(modifiers.fFlags & Modifiers::kUniform_Flag)) {
         context.fErrors->error(pos,
                 "variables of type '" + baseType->displayName() + "' must be uniform");
@@ -213,14 +208,25 @@ void VarDeclaration::ErrorCheck(const Context& context,
     int permitted = Modifiers::kConst_Flag | Modifiers::kHighp_Flag | Modifiers::kMediump_Flag |
                     Modifiers::kLowp_Flag;
     if (storage == Variable::Storage::kGlobal) {
-        permitted |= Modifiers::kIn_Flag | Modifiers::kOut_Flag;
         if (!ProgramConfig::IsCompute(context.fConfig->fKind)) {
-            permitted |= Modifiers::kUniform_Flag | Modifiers::kFlat_Flag |
+            permitted |= Modifiers::kUniform_Flag;
-                    Modifiers::kNoPerspective_Flag;
+        }
-        } else if (!baseType->isOpaque()) {
+
-            permitted |= Modifiers::kThreadgroup_Flag;
+        // No other modifiers are allowed in runtime effects
+        if (!ProgramConfig::IsRuntimeEffect(context.fConfig->fKind)) {
+            permitted |= Modifiers::kIn_Flag | Modifiers::kOut_Flag;
+            if (!ProgramConfig::IsCompute(context.fConfig->fKind)) {
+                permitted |= Modifiers::kFlat_Flag | Modifiers::kNoPerspective_Flag;
+            } else if (!baseType->isOpaque()) {
+                permitted |= Modifiers::kThreadgroup_Flag;
+            }
         }
     }
+    // This modifier isn't actually allowed on variables, at all. However, it's restricted to only
+    // appear in module code by the parser. We "allow" it here, to avoid double-reporting errors.
+    // This means that module code could put it on a variable (to no effect). We'll live with that.
+    permitted |= Modifiers::kHasSideEffects_Flag;
+
     // TODO(skbug.com/11301): Migrate above checks into building a mask of permitted layout flags
 
     int permittedLayoutFlags = ~0;

tests/sksl/errors/BadModifiers.glsl

@@ -1,5 +1,8 @@
 ### Compilation failed:
 
+error: 1: 'sk_has_side_effects' is not permitted here
+const in out uniform flat noperspective sk_has_side_effects inline noinline void func1() {}
+                                        ^^^^^^^^^^^^^^^^^^^
 error: 1: 'const' is not permitted here
 const in out uniform flat noperspective sk_has_side_effects inline noinline void func1() {}
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -21,6 +24,9 @@ const in out uniform flat noperspective sk_has_side_effects inline noinline void
 error: 1: functions cannot be both 'inline' and 'noinline'
 const in out uniform flat noperspective sk_has_side_effects inline noinline void func1() {}
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+error: 3: 'sk_has_side_effects' is not permitted here
+void func2(const in out uniform flat noperspective sk_has_side_effects
+                                                   ^^^^^^^^^^^^^^^^^^^
 error: 3: 'uniform' is not permitted here
 void func2(const in out uniform flat noperspective sk_has_side_effects
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^...
@@ -39,12 +45,12 @@ void func2(const in out uniform flat noperspective sk_has_side_effects
 error: 3: 'noinline' is not permitted here
 void func2(const in out uniform flat noperspective sk_has_side_effects
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^...
+error: 6: 'sk_has_side_effects' is not permitted here
+const in out uniform flat noperspective sk_has_side_effects inline noinline float var;
+                                        ^^^^^^^^^^^^^^^^^^^
 error: 6: 'in uniform' variables not permitted
 const in out uniform flat noperspective sk_has_side_effects inline noinline float var;
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-error: 6: 'sk_has_side_effects' is not permitted here
-const in out uniform flat noperspective sk_has_side_effects inline noinline float var;
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 error: 6: 'inline' is not permitted here
 const in out uniform flat noperspective sk_has_side_effects inline noinline float var;
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -54,4 +60,4 @@ const in out uniform flat noperspective sk_has_side_effects inline noinline floa
 error: 6: 'const' variables must be initialized
 const in out uniform flat noperspective sk_has_side_effects inline noinline float var;
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-18 errors
+20 errors

tests/sksl/runtime_errors/IllegalModifiers.skvm

@@ -0,0 +1,51 @@
+### Compilation failed:
+
+error: 1: 'flat' is not permitted here
+flat                float _flat;
+^^^^
+error: 2: 'noperspective' is not permitted here
+noperspective       float _noperspective;
+^^^^^^^^^^^^^
+error: 3: 'in' is not permitted here
+in                  float _in;
+^^
+error: 4: 'out' is not permitted here
+out                 float _out;
+^^^
+error: 5: 'threadgroup' is not permitted here
+threadgroup         float _threadgroup;
+^^^^^^^^^^^
+error: 6: '$es3' is not permitted here
+$es3                float _es3;
+^^^^
+error: 7: 'sk_has_side_effects' is not permitted here
+sk_has_side_effects float _sk_has_side_effects;
+^^^^^^^^^^^^^^^^^^^
+error: 8: 'inline' is not permitted here
+inline              float _inline;
+^^^^^^
+error: 9: 'noinline' is not permitted here
+noinline            float _noinline;
+^^^^^^^^
+error: 11: 'flat' is not permitted here
+flat                void flat_fn() {}
+^^^^
+error: 12: 'noperspective' is not permitted here
+noperspective       void noperspective_fn() {}
+^^^^^^^^^^^^^
+error: 13: 'in' is not permitted here
+in                  void in_fn() {}
+^^
+error: 14: 'out' is not permitted here
+out                 void out_fn() {}
+^^^
+error: 15: 'threadgroup' is not permitted here
+threadgroup         void threadgroup_fn() {}
+^^^^^^^^^^^
+error: 16: '$es3' is not permitted here
+$es3                void es3_fn() {}
+^^^^
+error: 17: 'sk_has_side_effects' is not permitted here
+sk_has_side_effects void sk_has_side_effect_fn() {}
+^^^^^^^^^^^^^^^^^^^
+16 errors

tests/sksl/runtime_errors/IllegalShaderUse.skvm

@@ -3,12 +3,12 @@
 error: 10: variables of type 'shader' must be uniform
 shader s3;
 ^^^^^^^^^
-error: 11: 'in' variables not permitted in runtime effects
-in shader s4;
-^^^^^^^^^^^^
 error: 11: variables of type 'shader' must be uniform
 in shader s4;
 ^^^^^^^^^^^^
+error: 11: 'in' is not permitted here
+in shader s4;
+^^
 error: 13: opaque type 'shader' is not permitted in a struct
 struct S { shader sh; };
            ^^^^^^^^^