Validate gl_FragData and gl_FragColor access after parsing

src/compiler/translator/Compiler.cpp

@@ -45,6 +45,7 @@
 #include "compiler/translator/tree_ops/UnfoldShortCircuitAST.h"
 #include "compiler/translator/tree_ops/UseInterfaceBlockFields.h"
 #include "compiler/translator/tree_ops/VectorizeVectorScalarArithmetic.h"
+#include "compiler/translator/tree_util/BuiltIn_autogen.h"
 #include "compiler/translator/tree_util/IntermNodePatternMatcher.h"
 #include "compiler/translator/util.h"
 #include "third_party/compiler/ArrayBoundsClamper.h"
@@ -224,6 +225,51 @@ int MapSpecToShaderVersion(ShShaderSpec spec)
     }
 }
 
+bool ValidateFragColorAndFragData(GLenum shaderType,
+                                  int shaderVersion,
+                                  const TSymbolTable &symbolTable,
+                                  TDiagnostics *diagnostics)
+{
+    if (shaderVersion > 100 || shaderType != GL_FRAGMENT_SHADER)
+    {
+        return true;
+    }
+
+    bool usesFragColor = false;
+    bool usesFragData  = false;
+    // This validation is a bit stricter than the spec - it's only an error to write to
+    // both FragData and FragColor. But because it's better not to have reads from undefined
+    // variables, we always return an error if they are both referenced, rather than only if they
+    // are written.
+    if (symbolTable.isStaticallyUsed(*BuiltInVariable::gl_FragColor()) ||
+        symbolTable.isStaticallyUsed(*BuiltInVariable::gl_SecondaryFragColorEXT()))
+    {
+        usesFragColor = true;
+    }
+    // Extension variables may not always be initialized (saves some time at symbol table init).
+    bool secondaryFragDataUsed =
+        symbolTable.gl_SecondaryFragDataEXT() != nullptr &&
+        symbolTable.isStaticallyUsed(*symbolTable.gl_SecondaryFragDataEXT());
+    if (symbolTable.isStaticallyUsed(*symbolTable.gl_FragData()) || secondaryFragDataUsed)
+    {
+        usesFragData = true;
+    }
+    if (usesFragColor && usesFragData)
+    {
+        const char *errorMessage = "cannot use both gl_FragData and gl_FragColor";
+        if (symbolTable.isStaticallyUsed(*BuiltInVariable::gl_SecondaryFragColorEXT()) ||
+            secondaryFragDataUsed)
+        {
+            errorMessage =
+                "cannot use both output variable sets (gl_FragData, gl_SecondaryFragDataEXT)"
+                " and (gl_FragColor, gl_SecondaryFragColorEXT)";
+        }
+        diagnostics->globalError(errorMessage);
+        return false;
+    }
+    return true;
+}
+
 }  // namespace
 
 TShHandleBase::TShHandleBase()
@@ -450,6 +496,11 @@ bool TCompiler::checkAndSimplifyAST(TIntermBlock *root,
         return false;
     }
 
+    if (!ValidateFragColorAndFragData(shaderType, shaderVersion, symbolTable, &mDiagnostics))
+    {
+        return false;
+    }
+
     // Fold expressions that could not be folded before validation that was done as a part of
     // parsing.
     FoldExpressions(root, &mDiagnostics);

src/compiler/translator/ParseContext.cpp

@@ -196,9 +196,6 @@ TParseContext::TParseContext(TSymbolTable &symt,
                         resources.WEBGL_debug_shader_precision == 1),
       mPreprocessor(mDiagnostics, &mDirectiveHandler, pp::PreprocessorSettings()),
       mScanner(nullptr),
-      mUsesFragData(false),
-      mUsesFragColor(false),
-      mUsesSecondaryOutputs(false),
       mMinProgramTexelOffset(resources.MinProgramTexelOffset),
       mMaxProgramTexelOffset(resources.MaxProgramTexelOffset),
       mMinProgramTextureGatherOffset(resources.MinProgramTextureGatherOffset),
@@ -1830,40 +1827,9 @@ const TVariable *TParseContext::getNamedVariable(const TSourceLoc &location,
         checkCanUseExtension(location, variable->extension());
     }
 
-    // Reject shaders using both gl_FragData and gl_FragColor
-    TQualifier qualifier = variable->getType().getQualifier();
-    if (qualifier == EvqFragData || qualifier == EvqSecondaryFragDataEXT)
-    {
-        mUsesFragData = true;
-    }
-    else if (qualifier == EvqFragColor || qualifier == EvqSecondaryFragColorEXT)
-    {
-        mUsesFragColor = true;
-    }
-    if (qualifier == EvqSecondaryFragDataEXT || qualifier == EvqSecondaryFragColorEXT)
-    {
-        mUsesSecondaryOutputs = true;
-    }
-
-    // This validation is not quite correct - it's only an error to write to
-    // both FragData and FragColor. For simplicity, and because users shouldn't
-    // be rewarded for reading from undefined varaibles, return an error
-    // if they are both referenced, rather than assigned.
-    if (mUsesFragData && mUsesFragColor)
-    {
-        const char *errorMessage = "cannot use both gl_FragData and gl_FragColor";
-        if (mUsesSecondaryOutputs)
-        {
-            errorMessage =
-                "cannot use both output variable sets (gl_FragData, gl_SecondaryFragDataEXT)"
-                " and (gl_FragColor, gl_SecondaryFragColorEXT)";
-        }
-        error(location, errorMessage, name);
-    }
-
     // GLSL ES 3.1 Revision 4, 7.1.3 Compute Shader Special Variables
     if (getShaderType() == GL_COMPUTE_SHADER && !mComputeShaderLocalSizeDeclared &&
-        qualifier == EvqWorkGroupSize)
+        variable->getType().getQualifier() == EvqWorkGroupSize)
     {
         error(location,
               "It is an error to use gl_WorkGroupSize before declaring the local group size",

src/compiler/translator/ParseContext.h

@@ -606,10 +606,6 @@ class TParseContext : angle::NonCopyable
     TDirectiveHandler mDirectiveHandler;
     pp::Preprocessor mPreprocessor;
     void *mScanner;
-    bool mUsesFragData;  // track if we are using both gl_FragData and gl_FragColor
-    bool mUsesFragColor;
-    bool mUsesSecondaryOutputs;  // Track if we are using either gl_SecondaryFragData or
-                                 // gl_Secondary FragColor or both.
     int mMinProgramTexelOffset;
     int mMaxProgramTexelOffset;
 

src/compiler/translator/SymbolTable.cpp

@@ -158,6 +158,16 @@ TVariable *TSymbolTable::getGlInVariableWithArraySize() const
     return mGlInVariableWithArraySize;
 }
 
+const TVariable *TSymbolTable::gl_FragData() const
+{
+    return mVar_gl_FragData;
+}
+
+const TVariable *TSymbolTable::gl_SecondaryFragDataEXT() const
+{
+    return mVar_gl_SecondaryFragDataEXT;
+}
+
 void TSymbolTable::markStaticWrite(const TVariable &variable)
 {
     int id    = variable.uniqueId().get();

src/compiler/translator/SymbolTable.h

@@ -94,6 +94,9 @@ class TSymbolTable : angle::NonCopyable, TSymbolTableBase
     bool setGlInArraySize(unsigned int inputArraySize);
     TVariable *getGlInVariableWithArraySize() const;
 
+    const TVariable *gl_FragData() const;
+    const TVariable *gl_SecondaryFragDataEXT() const;
+
     void markStaticRead(const TVariable &variable);
     void markStaticWrite(const TVariable &variable);
 

src/tests/compiler_tests/ShaderValidation_test.cpp

@@ -5919,3 +5919,23 @@ TEST_F(FragmentShaderValidationTest, HandleLoopInnerIfStatementAlwaysTriviallyPr
         FAIL() << "Shader compilation failed, expecting success:\n" << mInfoLog;
     }
 }
+
+// Test that declaring both gl_FragColor and gl_FragData invariant is not an error. The GLSL ES 1.00
+// spec only disallows writing to both of them. ANGLE extends this validation to also cover reads,
+// but it makes sense not to treat declaring them both invariant as an error.
+TEST_F(FragmentShaderValidationTest, DeclareBothBuiltInFragmentOutputsInvariant)
+{
+    const std::string &shaderString =
+        R"(
+        invariant gl_FragColor;
+        invariant gl_FragData;
+        precision mediump float;
+        void main()
+        {
+            gl_FragColor = vec4(0.0);
+        })";
+    if (!compile(shaderString))
+    {
+        FAIL() << "Shader compilation failed, expecting success:\n" << mInfoLog;
+    }
+}