Merge pull request #538 from steve-lunarg/iomap-binding-range-err

Check for out-of-range bindings during IO mapping.

StandAlone/StandAlone.cpp

@@ -584,6 +584,12 @@ void CompileAndLinkShaderUnits(std::vector<ShaderCompUnit> compUnits)
     if (! (Options & EOptionOutputPreprocessed) && ! program.link(messages))
         LinkFailed = true;
 
+    // Map IO
+    if (Options & EOptionSpv) {
+        if (!program.mapIO())
+            LinkFailed = true;
+    }
+    
     // Report
     if (! (Options & EOptionSuppressInfolog) &&
         ! (Options & EOptionMemoryLeakMode)) {
@@ -591,10 +597,6 @@ void CompileAndLinkShaderUnits(std::vector<ShaderCompUnit> compUnits)
         PutsIfNonEmpty(program.getInfoDebugLog());
     }
 
-    // Map IO
-    if (Options & EOptionSpv)
-        program.mapIO();
-    
     // Reflect
     if (Options & EOptionDumpReflection) {
         program.buildReflection();

Test/baseResults/spv.register.autoassign.rangetest.frag.out

@@ -0,0 +1,12 @@
+spv.register.autoassign.rangetest.frag
+
+Linked fragment stage:
+
+INTERNAL ERROR: mapped binding out of range: g_tScene
+INTERNAL ERROR: mapped binding out of range: g_tSamp
+INTERNAL ERROR: mapped binding out of range: g_tScene
+INTERNAL ERROR: mapped binding out of range: g_tSamp
+INTERNAL ERROR: mapped binding out of range: g_tSamp
+INTERNAL ERROR: mapped binding out of range: g_tScene
+
+SPIR-V is not generated for failed compile or link

Test/spv.register.autoassign.rangetest.frag

@@ -0,0 +1,15 @@
+
+SamplerState g_tSamp : register(s5);
+
+Texture2D g_tScene[2] : register(t5);
+
+struct PS_OUTPUT
+{
+    float4 Color : SV_Target0;
+};
+
+void main(out PS_OUTPUT psout)
+{
+    psout.Color = g_tScene[0].Sample(g_tSamp, 0.3) +
+                  g_tScene[1].Sample(g_tSamp, 0.3);
+}

glslang/MachineIndependent/ShaderLang.cpp

@@ -1724,7 +1724,7 @@ bool TProgram::mapIO()
 
     for (int s = 0; s < EShLangCount; ++s) {
         if (intermediate[s]) {
-            if (! ioMapper->addStage((EShLanguage)s, *intermediate[s]))
+            if (! ioMapper->addStage((EShLanguage)s, *intermediate[s], *infoSink))
                 return false;
         }
     }

glslang/MachineIndependent/iomapper.cpp

@@ -34,6 +34,7 @@
 //
 
 #include "../Include/Common.h"
+#include "../Include/InfoSink.h"
 #include "iomapper.h"
 #include "LiveTraverser.h"
 #include "localintermediate.h"
@@ -150,11 +151,30 @@ protected:
 class TIoMappingTraverser : public TBindingTraverser {
 public:
     TIoMappingTraverser(TIntermediate& i, TBindingMap& bindingMap, TUsedBindings& usedBindings,
-                        bool traverseDeadCode) :
+                        TInfoSink& infoSink, bool traverseDeadCode) :
-        TBindingTraverser(i, bindingMap, usedBindings, traverseDeadCode)
+        TBindingTraverser(i, bindingMap, usedBindings, traverseDeadCode),
+        infoSink(infoSink),
+        assignError(false)
     { }
 
+    bool success() const { return !assignError; }
+
 protected:
+    unsigned checkBindingRange(const TIntermSymbol& base, unsigned binding)
+    {
+        if (binding >= TQualifier::layoutBindingEnd) {
+            TString err = "mapped binding out of range: ";
+            err += base.getName();
+
+            infoSink.info.message(EPrefixInternalError, err.c_str());
+            assignError = true;
+            
+            return 0;
+        }
+
+        return binding;
+    }
+
     void addUniform(TIntermSymbol& base) override
     {
         // Skip things we don't intend to bind.
@@ -165,7 +185,7 @@ protected:
 
         // Apply existing binding, if we were given one or already made one up.
         if (existingBinding != -1) {
-            base.getWritableType().getQualifier().layoutBinding = existingBinding;
+            base.getWritableType().getQualifier().layoutBinding = checkBindingRange(base, existingBinding);
             return;
         }
 
@@ -174,7 +194,7 @@ protected:
             const int freeBinding = getFreeBinding(base.getType(), getBindingBase(base.getType()));
 
             markBinding(base, freeBinding);
-            base.getWritableType().getQualifier().layoutBinding = freeBinding;
+            base.getWritableType().getQualifier().layoutBinding = checkBindingRange(base, freeBinding);
         }
     }
 
@@ -195,13 +215,17 @@ protected:
 
         return nextBinding;
     }
+
+private:
+    bool assignError;  // true if there was an error assigning the bindings
+    TInfoSink& infoSink;
 };
 
 // Map I/O variables to provided offsets, and make bindings for
 // unbound but live variables.
 //
 // Returns false if the input is too malformed to do this.
-bool TIoMapper::addStage(EShLanguage, TIntermediate& intermediate)
+bool TIoMapper::addStage(EShLanguage, TIntermediate& intermediate, TInfoSink& infoSink)
 {
     // Trivial return if there is nothing to do.
     if (intermediate.getShiftSamplerBinding() == 0 &&
@@ -223,7 +247,7 @@ bool TIoMapper::addStage(EShLanguage, TIntermediate& intermediate)
 
     TBindingTraverser it_binding_all(intermediate, bindingMap, usedBindings, true);
     TBindingTraverser it_binding_live(intermediate, bindingMap, usedBindings, false);
-    TIoMappingTraverser it_iomap(intermediate, bindingMap, usedBindings, true);
+    TIoMappingTraverser it_iomap(intermediate, bindingMap, usedBindings, infoSink, true);
 
     // Traverse all (live+dead) code to find explicit bindings, so we can avoid those.
     root->traverse(&it_binding_all);
@@ -240,7 +264,7 @@ bool TIoMapper::addStage(EShLanguage, TIntermediate& intermediate)
     // Bind everything that needs a binding and doesn't have one.
     root->traverse(&it_iomap);
 
-    return true;
+    return it_iomap.success();
 }
 
 } // end namespace glslang

glslang/MachineIndependent/iomapper.h

@@ -42,6 +42,8 @@
 // A reflection database and its interface, consistent with the OpenGL API reflection queries.
 //
 
+class TInfoSink;
+
 namespace glslang {
 
 class TIntermediate;
@@ -53,7 +55,7 @@ public:
     virtual ~TIoMapper() {}
 
     // grow the reflection stage by stage
-    bool addStage(EShLanguage, TIntermediate&);
+    bool addStage(EShLanguage, TIntermediate&, TInfoSink&);
 };
 
 } // end namespace glslang

gtests/Spv.FromFile.cpp

@@ -287,6 +287,10 @@ INSTANTIATE_TEST_CASE_P(
         { "spv.register.noautoassign.frag", "main_ep", 5, 10, 15, false, false },
         { "spv.register.autoassign-2.frag", "main", 5, 10, 15, true, true },
         { "spv.buffer.autoassign.frag", "main", 5, 10, 15, true, true },
+        { "spv.register.autoassign.rangetest.frag", "main", 
+                glslang::TQualifier::layoutBindingEnd-2,
+                glslang::TQualifier::layoutBindingEnd+5,
+                20, true, false },
     }),
     FileNameAsCustomTestSuffixIoMap
 );