Translator: Validate consistent variable references

src/compiler/translator/Compiler.cpp

@@ -547,6 +547,12 @@ bool TCompiler::validateAST(TIntermNode *root)
     {
         bool valid = ValidateAST(root, &mDiagnostics, mValidateASTOptions);
 
+#if defined(ANGLE_ENABLE_ASSERTS)
+        if (!valid)
+        {
+            fprintf(stderr, "AST validation error(s):\n%s\n", mInfoSink.info.c_str());
+        }
+#endif
         // In debug, assert validation.  In release, validation errors will be returned back to the
         // application as internal ANGLE errors.
         ASSERT(valid);
@@ -561,6 +567,10 @@ bool TCompiler::checkAndSimplifyAST(TIntermBlock *root,
                                     ShCompileOptions compileOptions)
 {
     mValidateASTOptions = {};
+    if (!validateAST(root))
+    {
+        return false;
+    }
 
     // Disallow expressions deemed too complex.
     if ((compileOptions & SH_LIMIT_EXPRESSION_COMPLEXITY) != 0 && !limitExpressionComplexity(root))

src/compiler/translator/Compiler.h

@@ -214,6 +214,9 @@ class TCompiler : public TShHandleBase
     std::vector<sh::InterfaceBlock> mUniformBlocks;
     std::vector<sh::InterfaceBlock> mShaderStorageBlocks;
 
+    // Track what should be validated given passes currently applied.
+    ValidateASTOptions mValidateASTOptions;
+
     // Specialization constant usage bits
     SpecConstUsageBits mSpecConstUsageBits;
 
@@ -330,9 +333,6 @@ class TCompiler : public TShHandleBase
 
     TPragma mPragma;
 
-    // Track what should be validated given passes currently applied.
-    ValidateASTOptions mValidateASTOptions;
-
     ShCompileOptions mCompileOptions;
 };
 

src/compiler/translator/IntermNode.cpp

@@ -413,6 +413,9 @@ TIntermBlock::TIntermBlock(const TIntermBlock &node)
     {
         mStatements.push_back(node->deepCopy());
     }
+
+    ASSERT(!node.mIsTreeRoot);
+    mIsTreeRoot = false;
 }
 
 size_t TIntermBlock::getChildCount() const

src/compiler/translator/IntermNode.h

@@ -673,7 +673,7 @@ class TIntermAggregate : public TIntermOperator, public TIntermAggregateBase
 class TIntermBlock : public TIntermNode, public TIntermAggregateBase
 {
   public:
-    TIntermBlock() : TIntermNode() {}
+    TIntermBlock() : TIntermNode(), mIsTreeRoot(false) {}
     ~TIntermBlock() override {}
 
     TIntermBlock *getAsBlock() override { return this; }
@@ -694,9 +694,16 @@ class TIntermBlock : public TIntermNode, public TIntermAggregateBase
 
     TIntermBlock *deepCopy() const override { return new TIntermBlock(*this); }
 
+    void setIsTreeRoot() { mIsTreeRoot = true; }
+    bool isTreeRoot() const { return mIsTreeRoot; }
+
   protected:
     TIntermSequence mStatements;
 
+    // Used to distinguish the tree root from the other blocks.  When validating the AST, some
+    // validations are not applicable if not run on the entire tree and are thus skipped.
+    bool mIsTreeRoot;
+
   private:
     TIntermBlock(const TIntermBlock &);
 };

src/compiler/translator/ParseContext.cpp

@@ -399,6 +399,12 @@ void TParseContext::outOfRangeError(bool isError,
     }
 }
 
+void TParseContext::setTreeRoot(TIntermBlock *treeRoot)
+{
+    mTreeRoot = treeRoot;
+    mTreeRoot->setIsTreeRoot();
+}
+
 //
 // Same error message for all places assignments don't work.
 //

src/compiler/translator/ParseContext.h

@@ -64,7 +64,7 @@ class TParseContext : angle::NonCopyable
                          const char *token);
 
     TIntermBlock *getTreeRoot() const { return mTreeRoot; }
-    void setTreeRoot(TIntermBlock *treeRoot) { mTreeRoot = treeRoot; }
+    void setTreeRoot(TIntermBlock *treeRoot);
 
     bool getFragmentPrecisionHigh() const
     {

src/compiler/translator/TranslatorMetal.cpp

@@ -284,6 +284,10 @@ ANGLE_NO_DISCARD bool TranslatorMetal::insertSampleMaskWritingLogic(
     TIntermBlock *root,
     const DriverUniformMetal *driverUniforms)
 {
+    // This transformation leaves the tree in an inconsistent state by using a variable that's
+    // defined in text, outside of the knowledge of the AST.
+    mValidateASTOptions.validateVariableReferences = false;
+
     TSymbolTable *symbolTable = &getSymbolTable();
 
     // Insert coverageMaskEnabled specialization constant and sample_mask writing function.
@@ -337,6 +341,10 @@ ANGLE_NO_DISCARD bool TranslatorMetal::insertSampleMaskWritingLogic(
 ANGLE_NO_DISCARD bool TranslatorMetal::insertRasterizerDiscardLogic(TInfoSinkBase &sink,
                                                                     TIntermBlock *root)
 {
+    // This transformation leaves the tree in an inconsistent state by using a variable that's
+    // defined in text, outside of the knowledge of the AST.
+    mValidateASTOptions.validateVariableReferences = false;
+
     TSymbolTable *symbolTable = &getSymbolTable();
 
     // Insert rasterizationDisabled specialization constant.

src/compiler/translator/TranslatorVulkan.cpp

@@ -890,6 +890,9 @@ bool TranslatorVulkan::translateImpl(TInfoSinkBase &sink,
 
     if (defaultUniformCount > 0)
     {
+        // This transformation leaves the tree in an inconsistent state.  TODO: anglebug.com/4889
+        mValidateASTOptions.validateVariableReferences = false;
+
         sink << "\nlayout(set=0, binding=" << outputGLSL->nextUnusedBinding()
              << ", std140) uniform " << kDefaultUniformNames[packedShaderType] << "\n{\n";
 

src/compiler/translator/ValidateAST.cpp

@@ -7,7 +7,9 @@
 #include "compiler/translator/ValidateAST.h"
 
 #include "compiler/translator/Diagnostics.h"
+#include "compiler/translator/Symbol.h"
 #include "compiler/translator/tree_util/IntermTraverse.h"
+#include "compiler/translator/tree_util/SpecializationConstant.h"
 
 namespace sh
 {
@@ -48,6 +50,10 @@ class ValidateAST : public TIntermTraverser
     // Visit as a generic node
     void visitNode(Visit visit, TIntermNode *node);
 
+    void scope(Visit visit);
+    bool isVariableDeclared(const TVariable *variable);
+    bool variableNeedsDeclaration(const TVariable *variable);
+
     void expectNonNullChildren(Visit visit, TIntermNode *node, size_t least_count);
 
     bool validateInternal();
@@ -59,10 +65,15 @@ class ValidateAST : public TIntermTraverser
     std::map<TIntermNode *, TIntermNode *> mParent;
     bool mSingleParentFailed = false;
 
-    // For validateNullNodes
+    // For validateVariableReferences:
+    std::vector<std::set<const TVariable *>> mDeclaredVariables;
+    std::set<ImmutableString> mNamelessInterfaceBlockFields;
+    bool mVariableReferencesFailed = false;
+
+    // For validateNullNodes:
     bool mNullNodesFailed = false;
 
-    // For validateMultiDeclarations
+    // For validateMultiDeclarations:
     bool mMultiDeclarationsFailed = false;
 };
 
@@ -80,6 +91,14 @@ ValidateAST::ValidateAST(TIntermNode *root,
                          const ValidateASTOptions &options)
     : TIntermTraverser(true, false, true, nullptr), mOptions(options), mDiagnostics(diagnostics)
 {
+    bool isTreeRoot = root->getAsBlock() && root->getAsBlock()->isTreeRoot();
+
+    // Some validations are not applicable unless run on the entire tree.
+    if (!isTreeRoot)
+    {
+        mOptions.validateVariableReferences = false;
+    }
+
     if (mOptions.validateSingleParent)
     {
         mParent[root] = nullptr;
@@ -111,6 +130,55 @@ void ValidateAST::visitNode(Visit visit, TIntermNode *node)
     }
 }
 
+void ValidateAST::scope(Visit visit)
+{
+    if (mOptions.validateVariableReferences)
+    {
+        if (visit == PreVisit)
+        {
+            mDeclaredVariables.push_back({});
+        }
+        else if (visit == PostVisit)
+        {
+            mDeclaredVariables.pop_back();
+        }
+    }
+}
+
+bool ValidateAST::isVariableDeclared(const TVariable *variable)
+{
+    ASSERT(mOptions.validateVariableReferences);
+
+    for (const std::set<const TVariable *> &scopeVariables : mDeclaredVariables)
+    {
+        if (scopeVariables.count(variable) > 0)
+        {
+            return true;
+        }
+    }
+
+    return false;
+}
+
+bool ValidateAST::variableNeedsDeclaration(const TVariable *variable)
+{
+    // Don't expect declaration for built-in variables.
+    if (variable->name().beginsWith("gl_"))
+    {
+        return false;
+    }
+
+    // Additionally, don't expect declaration for Vulkan specialization constants.  There is no
+    // representation for them in the AST.
+    if (variable->symbolType() == SymbolType::AngleInternal &&
+        SpecConst::IsSpecConstName(variable->name()))
+    {
+        return false;
+    }
+
+    return true;
+}
+
 void ValidateAST::expectNonNullChildren(Visit visit, TIntermNode *node, size_t least_count)
 {
     if (visit == PreVisit && mOptions.validateNullNodes)
@@ -136,6 +204,36 @@ void ValidateAST::expectNonNullChildren(Visit visit, TIntermNode *node, size_t l
 void ValidateAST::visitSymbol(TIntermSymbol *node)
 {
     visitNode(PreVisit, node);
+
+    const TVariable *variable = &node->variable();
+    const TType &type         = node->getType();
+
+    if (mOptions.validateVariableReferences && variableNeedsDeclaration(variable))
+    {
+        // If it's a reference to a field of a nameless interface block, match it by name.
+        if (type.getInterfaceBlock() && !type.isInterfaceBlock())
+        {
+            if (mNamelessInterfaceBlockFields.count(node->getName()) == 0)
+            {
+                mDiagnostics->error(node->getLine(),
+                                    "Found reference to undeclared nameless interface block field "
+                                    "<validateVariableReferences>",
+                                    node->getName().data());
+                mVariableReferencesFailed = true;
+            }
+        }
+        else
+        {
+            if (!isVariableDeclared(variable))
+            {
+                mDiagnostics->error(node->getLine(),
+                                    "Found reference to undeclared or inconsistently redeclared "
+                                    "variable <validateVariableReferences>",
+                                    node->getName().data());
+                mVariableReferencesFailed = true;
+            }
+        }
+    }
 }
 
 void ValidateAST::visitConstantUnion(TIntermConstantUnion *node)
@@ -193,6 +291,31 @@ void ValidateAST::visitFunctionPrototype(TIntermFunctionPrototype *node)
 bool ValidateAST::visitFunctionDefinition(Visit visit, TIntermFunctionDefinition *node)
 {
     visitNode(visit, node);
+    scope(visit);
+
+    if (mOptions.validateVariableReferences && visit == PreVisit)
+    {
+        const TFunction *function = node->getFunction();
+
+        size_t paramCount = function->getParamCount();
+        for (size_t paramIndex = 0; paramIndex < paramCount; ++paramIndex)
+        {
+            const TVariable *variable = function->getParam(paramIndex);
+
+            if (isVariableDeclared(variable))
+            {
+                mDiagnostics->error(node->getLine(),
+                                    "Found two declarations of the same function argument "
+                                    "<validateVariableReferences>",
+                                    variable->name().data());
+                mVariableReferencesFailed = true;
+                break;
+            }
+
+            mDeclaredVariables.back().insert(variable);
+        }
+    }
+
     return true;
 }
 
@@ -206,6 +329,7 @@ bool ValidateAST::visitAggregate(Visit visit, TIntermAggregate *node)
 bool ValidateAST::visitBlock(Visit visit, TIntermBlock *node)
 {
     visitNode(visit, node);
+    scope(visit);
     expectNonNullChildren(visit, node, 0);
     return true;
 }
@@ -222,11 +346,61 @@ bool ValidateAST::visitDeclaration(Visit visit, TIntermDeclaration *node)
     visitNode(visit, node);
     expectNonNullChildren(visit, node, 0);
 
-    if (mOptions.validateMultiDeclarations && node->getSequence()->size() > 1)
+    const TIntermSequence &sequence = *(node->getSequence());
+
+    if (mOptions.validateMultiDeclarations && sequence.size() > 1)
     {
         mMultiDeclarationsFailed = true;
     }
 
+    if (mOptions.validateVariableReferences && visit == PreVisit)
+    {
+        for (TIntermNode *instance : sequence)
+        {
+            TIntermSymbol *symbol = instance->getAsSymbolNode();
+            if (symbol == nullptr)
+            {
+                TIntermBinary *init = instance->getAsBinaryNode();
+                ASSERT(init && init->getOp() == EOpInitialize);
+                symbol = init->getLeft()->getAsSymbolNode();
+            }
+            ASSERT(symbol);
+
+            const TVariable *variable = &symbol->variable();
+
+            if (isVariableDeclared(variable))
+            {
+                mDiagnostics->error(
+                    node->getLine(),
+                    "Found two declarations of the same variable <validateVariableReferences>",
+                    variable->name().data());
+                mVariableReferencesFailed = true;
+                break;
+            }
+
+            mDeclaredVariables.back().insert(variable);
+
+            if (variable->symbolType() == SymbolType::Empty &&
+                variable->getType().getInterfaceBlock())
+            {
+                // Nameless interface blocks can only be declared at the top level.  Their fields
+                // are not identified by TVariables, so resort to name-matching.  Conflict in names
+                // should have already generated a compile error.
+                ASSERT(mDeclaredVariables.size() == 1);
+
+                const TFieldList &fieldList = variable->getType().getInterfaceBlock()->fields();
+
+                for (size_t memberIndex = 0; memberIndex < fieldList.size(); ++memberIndex)
+                {
+                    TField *field = fieldList[memberIndex];
+                    ASSERT(mNamelessInterfaceBlockFields.count(field->name()) == 0);
+
+                    mNamelessInterfaceBlockFields.insert(field->name());
+                }
+            }
+        }
+    }
+
     return true;
 }
 
@@ -249,7 +423,8 @@ void ValidateAST::visitPreprocessorDirective(TIntermPreprocessorDirective *node)
 
 bool ValidateAST::validateInternal()
 {
-    return !mSingleParentFailed && !mNullNodesFailed && !mMultiDeclarationsFailed;
+    return !mSingleParentFailed && !mVariableReferencesFailed && !mNullNodesFailed &&
+           !mMultiDeclarationsFailed;
 }
 
 }  // anonymous namespace

src/compiler/translator/ValidateAST.h

@@ -23,6 +23,8 @@ struct ValidateASTOptions
 
     // Check that every node always has only one parent,
     bool validateSingleParent = true;
+    // Check that all symbols reference TVariables that have been declared.
+    bool validateVariableReferences = true;
     // Check that all EOpCallFunctionInAST have their corresponding function definitions in the AST,
     // with matching symbol ids. There should also be at least a prototype declaration before the
     // function is called.

src/compiler/translator/tree_ops/vulkan/MonomorphizeUnsupportedFunctionsInVulkanGLSL.cpp

@@ -196,10 +196,12 @@ const TFunction *MonomorphizeFunction(TSymbolTable *symbolTable,
         if (nextReplacedArg >= replacedArguments->size() ||
             paramIndex != (*replacedArguments)[nextReplacedArg].argumentIndex)
         {
+            TVariable *substituteArgument =
+                new TVariable(symbolTable, originalParam->name(), &originalParam->getType(),
+                              originalParam->symbolType());
             // Not replaced, add an identical parameter.
-            substituteFunction->addParameter(new TVariable(symbolTable, originalParam->name(),
-                                                           &originalParam->getType(),
-                                                           originalParam->symbolType()));
+            substituteFunction->addParameter(substituteArgument);
+            (*argumentMapOut)[originalParam] = new TIntermSymbol(substituteArgument);
         }
         else
         {

src/compiler/translator/tree_ops/vulkan/NameEmbeddedUniformStructs.cpp

@@ -10,6 +10,7 @@
 
 #include "compiler/translator/SymbolTable.h"
 #include "compiler/translator/tree_util/IntermTraverse.h"
+#include "compiler/translator/tree_util/ReplaceVariable.h"
 
 namespace sh
 {
@@ -51,6 +52,15 @@ class Traverser : public TIntermTraverser
         return false;
     }
 
+    void visitSymbol(TIntermSymbol *symbol) override
+    {
+        const TVariable *variable = &symbol->variable();
+        if (mVariableMap.count(variable) > 0)
+        {
+            queueReplacement(mVariableMap[variable]->deepCopy(), OriginalNode::IS_DROPPED);
+        }
+    }
+
   private:
     void doReplacement(TIntermDeclaration *decl,
                        TIntermTyped *declarator,
@@ -85,11 +95,15 @@ class Traverser : public TIntermTraverser
             namedDecl->appendDeclarator(newSymbol);
 
             newSequence.push_back(namedDecl);
+
+            mVariableMap[&asSymbol->variable()] = new TIntermSymbol(newVar);
         }
 
         mMultiReplacements.emplace_back(getParentNode()->getAsBlock(), decl,
                                         std::move(newSequence));
     }
+
+    VariableReplacementMap mVariableMap;
 };
 }  // anonymous namespace
 

src/compiler/translator/tree_util/SpecializationConstant.cpp

@@ -471,4 +471,11 @@ TIntermBinary *SpecConst::getHalfRenderArea()
 
     return rotatedHalfRenderArea;
 }
+
+bool SpecConst::IsSpecConstName(const ImmutableString &name)
+{
+    return name == kLineRasterEmulationSpecConstVarName ||
+           name == kSurfaceRotationSpecConstVarName || name == kDrawableWidthSpecConstVarName ||
+           name == kDrawableHeightSpecConstVarName;
+}
 }  // namespace sh

src/compiler/translator/tree_util/SpecializationConstant.h

@@ -46,6 +46,8 @@ class SpecConst
     void outputLayoutString(TInfoSinkBase &sink) const;
     SpecConstUsageBits getSpecConstUsageBits() const { return mUsageBits; }
 
+    static bool IsSpecConstName(const ImmutableString &name);
+
   private:
     TIntermSymbol *getFlipRotation();
     TIntermTyped *getNegFlipY();

src/tests/angle_deqp_tests_main.cpp

@@ -17,6 +17,13 @@ namespace angle
 void InitTestHarness(int *argc, char **argv);
 }  // namespace angle
 
+// If we ever move to a text-based expectations format, we should move this list in that file.
+namespace
+{
+const char *kSlowTests[] = {
+    "dEQP.KHR_GLES31/core_arrays_of_arrays_ConstructorsAndUnsizedDeclConstructors1"};
+}  // namespace
+
 int main(int argc, char **argv)
 {
 #if defined(ANGLE_PLATFORM_MACOS)
@@ -27,5 +34,6 @@ int main(int argc, char **argv)
 
     angle::InitTestHarness(&argc, argv);
     angle::TestSuite testSuite(&argc, argv);
+    testSuite.registerSlowTests(kSlowTests, ArraySize(kSlowTests));
     return testSuite.run();
 }