Don't try to apply ForLoopUnroll to loops it can't handle

src/compiler/translator/Compiler.cpp

@@ -294,12 +294,14 @@ TIntermNode *TCompiler::compileTreeImpl(const char *const shaderStrings[],
         // Unroll for-loop markup needs to happen after validateLimitations pass.
         if (success && (compileOptions & SH_UNROLL_FOR_LOOP_WITH_INTEGER_INDEX))
         {
-            ForLoopUnrollMarker marker(ForLoopUnrollMarker::kIntegerIndex);
+            ForLoopUnrollMarker marker(ForLoopUnrollMarker::kIntegerIndex,
+                                       shouldRunLoopAndIndexingValidation(compileOptions));
             root->traverse(&marker);
         }
         if (success && (compileOptions & SH_UNROLL_FOR_LOOP_WITH_SAMPLER_ARRAY_INDEX))
         {
-            ForLoopUnrollMarker marker(ForLoopUnrollMarker::kSamplerArrayIndex);
+            ForLoopUnrollMarker marker(ForLoopUnrollMarker::kSamplerArrayIndex,
+                                       shouldRunLoopAndIndexingValidation(compileOptions));
             root->traverse(&marker);
             if (marker.samplerArrayIndexIsFloatLoopIndex())
             {
@@ -696,7 +698,7 @@ void TCompiler::rewriteCSSShader(TIntermNode* root)
 
 bool TCompiler::validateLimitations(TIntermNode* root)
 {
-    ValidateLimitations validate(shaderType, infoSink.info);
+    ValidateLimitations validate(shaderType, &infoSink.info);
     root->traverse(&validate);
     return validate.numErrors() == 0;
 }

src/compiler/translator/ForLoopUnroll.cpp

@@ -6,6 +6,9 @@
 
 #include "compiler/translator/ForLoopUnroll.h"
 
+#include "compiler/translator/ValidateLimitations.h"
+#include "angle_gl.h"
+
 bool ForLoopUnrollMarker::visitBinary(Visit, TIntermBinary *node)
 {
     if (mUnrollCondition != kSamplerArrayIndex)
@@ -38,11 +41,16 @@ bool ForLoopUnrollMarker::visitBinary(Visit, TIntermBinary *node)
 
 bool ForLoopUnrollMarker::visitLoop(Visit, TIntermLoop *node)
 {
-    if (mUnrollCondition == kIntegerIndex)
+    bool canBeUnrolled = mHasRunLoopValidation;
+    if (!mHasRunLoopValidation)
+    {
+        canBeUnrolled = ValidateLimitations::IsLimitedForLoop(node);
+    }
+    if (mUnrollCondition == kIntegerIndex && canBeUnrolled)
     {
         // Check if loop index type is integer.
-        // This is called after ValidateLimitations pass, so all the calls
-        // should be valid. See ValidateLimitations::validateForLoopInit().
+        // This is called after ValidateLimitations pass, so the loop has the limited form specified
+        // in ESSL 1.00 appendix A.
         TIntermSequence *declSeq = node->getInit()->getAsAggregate()->getSequence();
         TIntermSymbol *symbol = (*declSeq)[0]->getAsBinaryNode()->getLeft()->getAsSymbolNode();
         if (symbol->getBasicType() == EbtInt)
@@ -50,11 +58,18 @@ bool ForLoopUnrollMarker::visitLoop(Visit, TIntermLoop *node)
     }
 
     TIntermNode *body = node->getBody();
-    if (body != NULL)
+    if (body != nullptr)
     {
-        mLoopStack.push(node);
-        body->traverse(this);
-        mLoopStack.pop();
+        if (canBeUnrolled)
+        {
+            mLoopStack.push(node);
+            body->traverse(this);
+            mLoopStack.pop();
+        }
+        else
+        {
+            body->traverse(this);
+        }
     }
     // The loop is fully processed - no need to visit children.
     return false;

src/compiler/translator/ForLoopUnroll.h

@@ -24,11 +24,12 @@ class ForLoopUnrollMarker : public TIntermTraverser
         kSamplerArrayIndex
     };
 
-    ForLoopUnrollMarker(UnrollCondition condition)
+    ForLoopUnrollMarker(UnrollCondition condition, bool hasRunLoopValidation)
         : TIntermTraverser(true, false, false),
           mUnrollCondition(condition),
           mSamplerArrayIndexIsFloatLoopIndex(false),
-          mVisitSamplerArrayIndexNodeInsideLoop(false)
+          mVisitSamplerArrayIndexNodeInsideLoop(false),
+          mHasRunLoopValidation(hasRunLoopValidation)
     {
     }
 
@@ -46,6 +47,7 @@ class ForLoopUnrollMarker : public TIntermTraverser
     TLoopStack mLoopStack;
     bool mSamplerArrayIndexIsFloatLoopIndex;
     bool mVisitSamplerArrayIndexNodeInsideLoop;
+    bool mHasRunLoopValidation;
 };
 
 #endif // COMPILER_TRANSLATOR_FORLOOPUNROLL_H_

src/compiler/translator/ValidateLimitations.cpp

@@ -53,15 +53,37 @@ class ValidateConstIndexExpr : public TIntermTraverser
 
 }  // namespace anonymous
 
-ValidateLimitations::ValidateLimitations(sh::GLenum shaderType,
-                                         TInfoSinkBase &sink)
+ValidateLimitations::ValidateLimitations(sh::GLenum shaderType, TInfoSinkBase *sink)
     : TIntermTraverser(true, false, false),
       mShaderType(shaderType),
       mSink(sink),
-      mNumErrors(0)
+      mNumErrors(0),
+      mValidateIndexing(true),
+      mValidateInnerLoops(true)
 {
 }
 
+// static
+bool ValidateLimitations::IsLimitedForLoop(TIntermLoop *loop)
+{
+    // The shader type doesn't matter in this case.
+    ValidateLimitations validate(GL_FRAGMENT_SHADER, nullptr);
+    validate.mValidateIndexing   = false;
+    validate.mValidateInnerLoops = false;
+    if (!validate.validateLoopType(loop))
+        return false;
+    if (!validate.validateForLoopHeader(loop))
+        return false;
+    TIntermNode *body = loop->getBody();
+    if (body != nullptr)
+    {
+        validate.mLoopStack.push(loop);
+        body->traverse(&validate);
+        validate.mLoopStack.pop();
+    }
+    return (validate.mNumErrors == 0);
+}
+
 bool ValidateLimitations::visitBinary(Visit, TIntermBinary *node)
 {
     // Check if loop index is modified in the loop body.
@@ -72,10 +94,11 @@ bool ValidateLimitations::visitBinary(Visit, TIntermBinary *node)
     {
       case EOpIndexDirect:
       case EOpIndexIndirect:
-        validateIndexing(node);
-        break;
+          if (mValidateIndexing)
+              validateIndexing(node);
+          break;
       default:
-        break;
+          break;
     }
     return true;
 }
@@ -102,6 +125,9 @@ bool ValidateLimitations::visitAggregate(Visit, TIntermAggregate *node)
 
 bool ValidateLimitations::visitLoop(Visit, TIntermLoop *node)
 {
+    if (!mValidateInnerLoops)
+        return true;
+
     if (!validateLoopType(node))
         return false;
 
@@ -123,9 +149,12 @@ bool ValidateLimitations::visitLoop(Visit, TIntermLoop *node)
 void ValidateLimitations::error(TSourceLoc loc,
                                 const char *reason, const char *token)
 {
-    mSink.prefix(EPrefixError);
-    mSink.location(loc);
-    mSink << "'" << token << "' : " << reason << "\n";
+    if (mSink)
+    {
+        mSink->prefix(EPrefixError);
+        mSink->location(loc);
+        (*mSink) << "'" << token << "' : " << reason << "\n";
+    }
     ++mNumErrors;
 }
 

src/compiler/translator/ValidateLimitations.h

@@ -17,7 +17,7 @@ class TInfoSinkBase;
 class ValidateLimitations : public TIntermTraverser
 {
   public:
-    ValidateLimitations(sh::GLenum shaderType, TInfoSinkBase &sink);
+    ValidateLimitations(sh::GLenum shaderType, TInfoSinkBase *sink);
 
     int numErrors() const { return mNumErrors; }
 
@@ -26,6 +26,8 @@ class ValidateLimitations : public TIntermTraverser
     bool visitAggregate(Visit, TIntermAggregate *) override;
     bool visitLoop(Visit, TIntermLoop *) override;
 
+    static bool IsLimitedForLoop(TIntermLoop *node);
+
   private:
     void error(TSourceLoc loc, const char *reason, const char *token);
 
@@ -51,9 +53,11 @@ class ValidateLimitations : public TIntermTraverser
     bool validateIndexing(TIntermBinary *node);
 
     sh::GLenum mShaderType;
-    TInfoSinkBase &mSink;
+    TInfoSinkBase *mSink;
     int mNumErrors;
     TLoopStack mLoopStack;
+    bool mValidateIndexing;
+    bool mValidateInnerLoops;
 };
 
 #endif // COMPILER_TRANSLATOR_VALIDATELIMITATIONS_H_

src/tests/compiler_tests/MalformedShader_test.cpp

@@ -15,7 +15,7 @@
 class MalformedShaderTest : public testing::Test
 {
   public:
-    MalformedShaderTest() {}
+    MalformedShaderTest() : mExtraCompileOptions(0) {}
 
   protected:
     virtual void SetUp()
@@ -36,7 +36,8 @@ class MalformedShaderTest : public testing::Test
     bool compile(const std::string& shaderString)
     {
         const char *shaderStrings[] = { shaderString.c_str() };
-        bool compilationSuccess = mTranslator->compile(shaderStrings, 1, SH_INTERMEDIATE_TREE);
+        bool compilationSuccess =
+            mTranslator->compile(shaderStrings, 1, SH_INTERMEDIATE_TREE | mExtraCompileOptions);
         TInfoSink &infoSink = mTranslator->getInfoSink();
         mInfoLog = infoSink.info.c_str();
         return compilationSuccess;
@@ -50,6 +51,7 @@ class MalformedShaderTest : public testing::Test
   protected:
     std::string mInfoLog;
     TranslatorESSL *mTranslator;
+    int mExtraCompileOptions;
 };
 
 class MalformedVertexShaderTest : public MalformedShaderTest
@@ -100,6 +102,12 @@ class MalformedWebGL1ShaderTest : public MalformedShaderTest
     }
 };
 
+class UnrollForLoopsTest : public MalformedShaderTest
+{
+  public:
+    UnrollForLoopsTest() { mExtraCompileOptions = SH_UNROLL_FOR_LOOP_WITH_INTEGER_INDEX; }
+};
+
 // This is a test for a bug that used to exist in ANGLE:
 // Calling a function with all parameters missing should not succeed.
 TEST_F(MalformedShaderTest, FunctionParameterMismatch)
@@ -1389,3 +1397,42 @@ TEST_F(MalformedWebGL1ShaderTest, NonConstantLoopIndex)
         FAIL() << "Shader compilation succeeded, expecting failure " << mInfoLog;
     }
 }
+
+// Regression test for an old crash bug in ANGLE.
+// ForLoopUnroll used to crash when it encountered a while loop.
+TEST_F(UnrollForLoopsTest, WhileLoop)
+{
+    const std::string &shaderString =
+        "precision mediump float;\n"
+        "void main()\n"
+        "{\n"
+        "    while (true) {\n"
+        "        gl_FragColor = vec4(0.0);\n"
+        "        break;\n"
+        "    }\n"
+        "}\n";
+    if (!compile(shaderString))
+    {
+        FAIL() << "Shader compilation failed, expecting success " << mInfoLog;
+    }
+}
+
+// Regression test for an old crash bug in ANGLE.
+// ForLoopUnroll used to crash when it encountered a loop that didn't fit the ESSL 1.00
+// Appendix A limitations.
+TEST_F(UnrollForLoopsTest, UnlimitedForLoop)
+{
+    const std::string &shaderString =
+        "precision mediump float;\n"
+        "void main()\n"
+        "{\n"
+        "    for (;true;) {\n"
+        "        gl_FragColor = vec4(0.0);\n"
+        "        break;\n"
+        "    }\n"
+        "}\n";
+    if (!compile(shaderString))
+    {
+        FAIL() << "Shader compilation failed, expecting success " << mInfoLog;
+    }
+}