D3D: Fix overflow in varying packing.

src/libANGLE/renderer/d3d/ProgramD3D.cpp

@@ -1490,8 +1490,8 @@ LinkResult ProgramD3D::link(const gl::ContextState &data, gl::InfoLog &infoLog)
 
     // Map the varyings to the register file
     VaryingPacking varyingPacking(data.getCaps().maxVaryingVectors);
-    if (!varyingPacking.packVaryings(infoLog, packedVaryings,
-                                     mState.getTransformFeedbackVaryingNames()))
+    if (!varyingPacking.packUserVaryings(infoLog, packedVaryings,
+                                         mState.getTransformFeedbackVaryingNames()))
     {
         return false;
     }
@@ -1501,7 +1501,7 @@ LinkResult ProgramD3D::link(const gl::ContextState &data, gl::InfoLog &infoLog)
     metadata.updatePackingBuiltins(SHADER_VERTEX, &varyingPacking);
     metadata.updatePackingBuiltins(SHADER_PIXEL, &varyingPacking);
 
-    if (static_cast<GLuint>(varyingPacking.getRegisterCount()) > data.getCaps().maxVaryingVectors)
+    if (!varyingPacking.validateBuiltins())
     {
         infoLog << "No varying registers left to support gl_FragCoord/gl_PointCoord";
         return false;

src/libANGLE/renderer/d3d/VaryingPacking_unittest.cpp

+//
+// Copyright 2016 The ANGLE Project Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// VaryingPacking_unittest.cpp:
+//   Tests for ANGLE's internal varying packing algorithm.
+//
+
+#include "libANGLE/renderer/d3d/hlsl/VaryingPacking.h"
+
+#include <gtest/gtest.h>
+
+#include "libANGLE/Program.h"
+
+namespace
+{
+
+class VaryingPackingTest : public ::testing::TestWithParam<GLuint>
+{
+  protected:
+    VaryingPackingTest() {}
+
+    bool testVaryingPacking(const std::vector<sh::Varying> &shVaryings,
+                            rx::VaryingPacking *varyingPacking)
+    {
+        std::vector<rx::PackedVarying> packedVaryings;
+        for (const auto &shVarying : shVaryings)
+        {
+            packedVaryings.push_back(rx::PackedVarying(shVarying, shVarying.interpolation));
+        }
+
+        gl::InfoLog infoLog;
+        std::vector<std::string> transformFeedbackVaryings;
+
+        if (!varyingPacking->packUserVaryings(infoLog, packedVaryings, transformFeedbackVaryings))
+            return false;
+
+        return varyingPacking->validateBuiltins();
+    }
+
+    bool packVaryings(GLuint maxVaryings, const std::vector<sh::Varying> &shVaryings)
+    {
+        rx::VaryingPacking varyingPacking(maxVaryings);
+        return testVaryingPacking(shVaryings, &varyingPacking);
+    }
+
+    const int kMaxVaryings = GetParam();
+};
+
+std::vector<sh::Varying> MakeVaryings(GLenum type, size_t count, size_t arraySize)
+{
+    std::vector<sh::Varying> varyings;
+
+    for (size_t index = 0; index < count; ++index)
+    {
+        std::stringstream strstr;
+        strstr << type << index;
+
+        sh::Varying varying;
+        varying.type          = type;
+        varying.precision     = GL_MEDIUM_FLOAT;
+        varying.name          = strstr.str();
+        varying.mappedName    = strstr.str();
+        varying.arraySize     = static_cast<unsigned int>(arraySize);
+        varying.staticUse     = true;
+        varying.interpolation = sh::INTERPOLATION_FLAT;
+        varying.isInvariant   = false;
+
+        varyings.push_back(varying);
+    }
+
+    return varyings;
+}
+
+void AddVaryings(std::vector<sh::Varying> *varyings, GLenum type, size_t count, size_t arraySize)
+{
+    const auto &newVaryings = MakeVaryings(type, count, arraySize);
+    varyings->insert(varyings->end(), newVaryings.begin(), newVaryings.end());
+}
+
+// Test that a single varying can't overflow the packing.
+TEST_P(VaryingPackingTest, OneVaryingLargerThanMax)
+{
+    ASSERT_FALSE(packVaryings(1, MakeVaryings(GL_FLOAT_MAT4, 1, 0)));
+}
+
+// Tests that using FragCoord as a user varying will eat up a register.
+TEST_P(VaryingPackingTest, MaxVaryingVec4PlusFragCoord)
+{
+    const std::string &userSemantic = rx::GetVaryingSemantic(4, false);
+
+    rx::VaryingPacking varyingPacking(kMaxVaryings);
+    unsigned int reservedSemanticIndex = varyingPacking.getMaxSemanticIndex();
+
+    varyingPacking.builtins(rx::SHADER_PIXEL)
+        .glFragCoord.enable(userSemantic, reservedSemanticIndex);
+
+    const auto &varyings = MakeVaryings(GL_FLOAT_VEC4, kMaxVaryings, 0);
+
+    ASSERT_FALSE(testVaryingPacking(varyings, &varyingPacking));
+}
+
+// Tests that using PointCoord as a user varying will eat up a register.
+TEST_P(VaryingPackingTest, MaxVaryingVec4PlusPointCoord)
+{
+    const std::string &userSemantic = rx::GetVaryingSemantic(4, false);
+
+    rx::VaryingPacking varyingPacking(kMaxVaryings);
+    unsigned int reservedSemanticIndex = varyingPacking.getMaxSemanticIndex();
+
+    varyingPacking.builtins(rx::SHADER_PIXEL)
+        .glPointCoord.enable(userSemantic, reservedSemanticIndex);
+
+    const auto &varyings = MakeVaryings(GL_FLOAT_VEC4, kMaxVaryings, 0);
+
+    ASSERT_FALSE(testVaryingPacking(varyings, &varyingPacking));
+}
+
+// This will overflow the available varying space.
+TEST_P(VaryingPackingTest, MaxPlusOneVaryingVec3)
+{
+    ASSERT_FALSE(packVaryings(kMaxVaryings, MakeVaryings(GL_FLOAT_VEC3, kMaxVaryings + 1, 0)));
+}
+
+// This will overflow the available varying space.
+TEST_P(VaryingPackingTest, MaxPlusOneVaryingVec3Array)
+{
+    ASSERT_FALSE(packVaryings(kMaxVaryings, MakeVaryings(GL_FLOAT_VEC3, kMaxVaryings / 2 + 1, 2)));
+}
+
+// This will overflow the available varying space.
+TEST_P(VaryingPackingTest, MaxVaryingVec3AndOneVec2)
+{
+    std::vector<sh::Varying> varyings = MakeVaryings(GL_FLOAT_VEC3, kMaxVaryings, 0);
+    AddVaryings(&varyings, GL_FLOAT_VEC2, 1, 0);
+    ASSERT_FALSE(packVaryings(kMaxVaryings, varyings));
+}
+
+// This should work since two vec2s are packed in a single register.
+TEST_P(VaryingPackingTest, MaxPlusOneVaryingVec2)
+{
+    ASSERT_TRUE(packVaryings(kMaxVaryings, MakeVaryings(GL_FLOAT_VEC2, kMaxVaryings + 1, 0)));
+}
+
+// Same for this one as above.
+TEST_P(VaryingPackingTest, TwiceMaxVaryingVec2)
+{
+    ASSERT_TRUE(packVaryings(kMaxVaryings, MakeVaryings(GL_FLOAT_VEC2, kMaxVaryings * 2, 0)));
+}
+
+// This should not work since it overflows available varying space.
+TEST_P(VaryingPackingTest, TooManyVaryingVec2)
+{
+    ASSERT_FALSE(packVaryings(kMaxVaryings, MakeVaryings(GL_FLOAT_VEC2, kMaxVaryings * 2 + 1, 0)));
+}
+
+// This should work according to the example GL packing rules - the float varyings are slotted
+// into the end of the vec3 varying arrays.
+TEST_P(VaryingPackingTest, MaxVaryingVec3ArrayAndFloatArrays)
+{
+    std::vector<sh::Varying> varyings = MakeVaryings(GL_FLOAT_VEC3, kMaxVaryings / 2, 2);
+    AddVaryings(&varyings, GL_FLOAT, kMaxVaryings / 2, 2);
+    ASSERT_TRUE(packVaryings(kMaxVaryings, varyings));
+}
+
+// This should not work - it has one too many float arrays.
+TEST_P(VaryingPackingTest, MaxVaryingVec3ArrayAndMaxPlusOneFloatArray)
+{
+    std::vector<sh::Varying> varyings = MakeVaryings(GL_FLOAT_VEC3, kMaxVaryings / 2, 2);
+    AddVaryings(&varyings, GL_FLOAT, kMaxVaryings / 2 + 1, 2);
+    ASSERT_FALSE(packVaryings(kMaxVaryings, varyings));
+}
+
+// Makes separate tests for different values of kMaxVaryings.
+INSTANTIATE_TEST_CASE_P(, VaryingPackingTest, ::testing::Values(1, 4, 8));
+
+}  // anonymous namespace

src/libANGLE/renderer/d3d/hlsl/VaryingPacking.cpp

@@ -53,9 +53,6 @@ VaryingPacking::VaryingPacking(GLuint maxVaryingVectors)
 // Returns false if unsuccessful.
 bool VaryingPacking::packVarying(const PackedVarying &packedVarying)
 {
-    unsigned int varyingRows    = 0;
-    unsigned int varyingColumns = 0;
-
     const auto &varying = *packedVarying.varying;
 
     // "Non - square matrices of type matCxR consume the same space as a square matrix of type matN
@@ -63,15 +60,21 @@ bool VaryingPacking::packVarying(const PackedVarying &packedVarying)
     // Here we are a bit more conservative and allow packing non-square matrices more tightly.
     // Make sure we use transposed matrix types to count registers correctly.
     ASSERT(!varying.isStruct());
-    GLenum transposedType = gl::TransposeMatrixType(varying.type);
-    varyingRows           = gl::VariableRowCount(transposedType);
-    varyingColumns        = gl::VariableColumnCount(transposedType);
+    GLenum transposedType       = gl::TransposeMatrixType(varying.type);
+    unsigned int varyingRows    = gl::VariableRowCount(transposedType);
+    unsigned int varyingColumns = gl::VariableColumnCount(transposedType);
 
     // "Arrays of size N are assumed to take N times the size of the base type"
     varyingRows *= varying.elementCount();
 
     unsigned int maxVaryingVectors = static_cast<unsigned int>(mRegisterMap.size());
 
+    // Fail if we are packing a single over-large varying.
+    if (varyingRows > maxVaryingVectors)
+    {
+        return false;
+    }
+
     // "For 2, 3 and 4 component variables packing is started using the 1st column of the 1st row.
     // Variables are then allocated to successive rows, aligning them to the 1st column."
     if (varyingColumns >= 2 && varyingColumns <= 4)
@@ -226,9 +229,9 @@ void VaryingPacking::insert(unsigned int registerRow,
 }
 
 // See comment on packVarying.
-bool VaryingPacking::packVaryings(gl::InfoLog &infoLog,
-                                  const std::vector<PackedVarying> &packedVaryings,
-                                  const std::vector<std::string> &transformFeedbackVaryings)
+bool VaryingPacking::packUserVaryings(gl::InfoLog &infoLog,
+                                      const std::vector<PackedVarying> &packedVaryings,
+                                      const std::vector<std::string> &transformFeedbackVaryings)
 {
     std::set<std::string> uniqueVaryingNames;
 
@@ -312,6 +315,11 @@ bool VaryingPacking::packVaryings(gl::InfoLog &infoLog,
     return true;
 }
 
+bool VaryingPacking::validateBuiltins() const
+{
+    return (static_cast<size_t>(getRegisterCount()) <= mRegisterMap.size());
+}
+
 unsigned int VaryingPacking::getRegisterCount() const
 {
     unsigned int count = 0;

src/libANGLE/renderer/d3d/hlsl/VaryingPacking.h

@@ -108,9 +108,13 @@ class VaryingPacking final : angle::NonCopyable
   public:
     VaryingPacking(GLuint maxVaryingVectors);
 
-    bool packVaryings(gl::InfoLog &infoLog,
-                      const std::vector<PackedVarying> &packedVaryings,
-                      const std::vector<std::string> &transformFeedbackVaryings);
+    bool packUserVaryings(gl::InfoLog &infoLog,
+                          const std::vector<PackedVarying> &packedVaryings,
+                          const std::vector<std::string> &transformFeedbackVaryings);
+
+    // Some built-in varyings require emulation that eats up available registers. This method
+    // checks that we're within the register limits of the implementation.
+    bool validateBuiltins() const;
 
     struct Register
     {

src/tests/angle_unittests.gypi

@@ -100,6 +100,7 @@
         # TODO(jmadill): should probably call this windows sources
         'angle_unittests_hlsl_sources':
         [
+            '<(angle_path)/src/libANGLE/renderer/d3d/VaryingPacking_unittest.cpp',
             '<(angle_path)/src/tests/compiler_tests/UnrollFlatten_test.cpp',
         ],
     },

src/tests/gl_tests/GLSLTest.cpp

@@ -1246,30 +1246,6 @@ TEST_P(GLSLTest, MaxMinusTwoVaryingVec4PlusThreeSpecialVariables)
     VaryingTestBase(0, 0, 0, 0, 0, 0, maxVaryings - 2, 0, true, true, true, true);
 }
 
-// Disabled because drivers are allowed to successfully compile shaders that have more than the
-// maximum number of varyings. (http://anglebug.com/1296)
-TEST_P(GLSLTest, DISABLED_MaxVaryingVec4PlusFragCoord)
-{
-    GLint maxVaryings = 0;
-    glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
-
-    // Generate shader code that uses gl_FragCoord, a special fragment shader variables.
-    // This test should fail, since we are really using (maxVaryings + 1) varyings.
-    VaryingTestBase(0, 0, 0, 0, 0, 0, maxVaryings, 0, true, false, false, false);
-}
-
-// Disabled because drivers are allowed to successfully compile shaders that have more than the
-// maximum number of varyings. (http://anglebug.com/1296)
-TEST_P(GLSLTest, DISABLED_MaxVaryingVec4PlusPointCoord)
-{
-    GLint maxVaryings = 0;
-    glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
-
-    // Generate shader code that uses gl_FragCoord, a special fragment shader variables.
-    // This test should fail, since we are really using (maxVaryings + 1) varyings.
-    VaryingTestBase(0, 0, 0, 0, 0, 0, maxVaryings, 0, false, true, false, false);
-}
-
 TEST_P(GLSLTest, MaxVaryingVec3)
 {
     GLint maxVaryings = 0;
@@ -1380,56 +1356,6 @@ TEST_P(GLSLTest, MaxVaryingVec2Arrays)
     VaryingTestBase(0, 0, 0, maxVaryings, 0, 0, 0, 0, false, false, false, true);
 }
 
-// Disabled because drivers are allowed to successfully compile shaders that have more than the
-// maximum number of varyings. (http://anglebug.com/1296)
-TEST_P(GLSLTest, DISABLED_MaxPlusOneVaryingVec3)
-{
-    GLint maxVaryings = 0;
-    glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
-
-    VaryingTestBase(0, 0, 0, 0, maxVaryings + 1, 0, 0, 0, false, false, false, false);
-}
-
-// Disabled because drivers are allowed to successfully compile shaders that have more than the
-// maximum number of varyings. (http://anglebug.com/1296)
-TEST_P(GLSLTest, DISABLED_MaxPlusOneVaryingVec3Array)
-{
-    GLint maxVaryings = 0;
-    glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
-
-    VaryingTestBase(0, 0, 0, 0, 0, maxVaryings / 2 + 1, 0, 0, false, false, false, false);
-}
-
-// Disabled because drivers are allowed to successfully compile shaders that have more than the
-// maximum number of varyings. (http://anglebug.com/1296)
-TEST_P(GLSLTest, DISABLED_MaxVaryingVec3AndOneVec2)
-{
-    GLint maxVaryings = 0;
-    glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
-
-    VaryingTestBase(0, 0, 1, 0, maxVaryings, 0, 0, 0, false, false, false, false);
-}
-
-// Disabled because drivers are allowed to successfully compile shaders that have more than the
-// maximum number of varyings. (http://anglebug.com/1296)
-TEST_P(GLSLTest, DISABLED_MaxPlusOneVaryingVec2)
-{
-    GLint maxVaryings = 0;
-    glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
-
-    VaryingTestBase(0, 0, 2 * maxVaryings + 1, 0, 0, 0, 0, 0, false, false, false, false);
-}
-
-// Disabled because drivers are allowed to successfully compile shaders that have more than the
-// maximum number of varyings. (http://anglebug.com/1296)
-TEST_P(GLSLTest, DISABLED_MaxVaryingVec3ArrayAndMaxPlusOneFloatArray)
-{
-    GLint maxVaryings = 0;
-    glGetIntegerv(GL_MAX_VARYING_VECTORS, &maxVaryings);
-
-    VaryingTestBase(0, maxVaryings / 2 + 1, 0, 0, 0, 0, 0, maxVaryings / 2, false, false, false, false);
-}
-
 // Verify shader source with a fixed length that is less than the null-terminated length will compile.
 TEST_P(GLSLTest, FixedShaderLength)
 {