Merge pull request #1063 from LoopDawg/remapper-error-cleanup

Remapper: make remapper robust against non-exiting error handlers
2017-09-23 06:32:02 -06:00 · 2017-09-23 06:32:02 -06:00 · 9cf5dfbdc7
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@ -54,7 +54,7 @@ endif(WIN32)

 if(${CMAKE_CXX_COMPILER_ID} MATCHES "GNU")
    add_compile_options(-Wall -Wmaybe-uninitialized -Wuninitialized -Wunused -Wunused-local-typedefs
-                        -Wunused-parameter -Wunused-value  -Wunused-variable -Wunused-but-set-parameter -Wunused-but-set-variable)
+                        -Wunused-parameter -Wunused-value  -Wunused-variable -Wunused-but-set-parameter -Wunused-but-set-variable -fno-exceptions)
    add_compile_options(-Wno-reorder)  # disable this from -Wall, since it happens all over.
 elseif(${CMAKE_CXX_COMPILER_ID} MATCHES "Clang")
    add_compile_options(-Wall -Wuninitialized -Wunused -Wunused-local-typedefs
--- a/SPIRV/SPVRemapper.cpp
+++ b/SPIRV/SPVRemapper.cpp
@ -135,6 +135,9 @@ namespace spv {
        const unsigned typeStart = idPos(id);
        const spv::Op  opCode    = asOpCode(typeStart);

+        if (errorLatch)
+            return 0;
+
        switch (opCode) {
        case spv::OpTypeInt:   // fall through...
        case spv::OpTypeFloat: return (spv[typeStart+2]+31)/32;
@ -148,8 +151,10 @@ namespace spv {
    unsigned spirvbin_t::idTypeSizeInWords(spv::Id id) const
    {
        const auto tid_it = idTypeSizeMap.find(id);
-        if (tid_it == idTypeSizeMap.end())
+        if (tid_it == idTypeSizeMap.end()) {
            error("type size for ID not found");
+            return 0;
+        }

        return tid_it->second;
    }
@ -253,19 +258,31 @@ namespace spv {
    {
        assert(id != spv::NoResult && newId != spv::NoResult);

+        if (id > bound()) {
+            error(std::string("ID out of range: ") + std::to_string(id));
+            return spirvbin_t::unused;
+        }
+
        if (id >= idMapL.size())
            idMapL.resize(id+1, unused);

        if (newId != unmapped && newId != unused) {
-            if (isOldIdUnused(id))
+            if (isOldIdUnused(id)) {
                error(std::string("ID unused in module: ") + std::to_string(id));
+                return spirvbin_t::unused;
+            }

-            if (!isOldIdUnmapped(id))
+            if (!isOldIdUnmapped(id)) {
                error(std::string("ID already mapped: ") + std::to_string(id) + " -> "
-                + std::to_string(localId(id)));
+                        + std::to_string(localId(id)));

-            if (isNewIdMapped(newId))
+                return spirvbin_t::unused;
+            }
+
+            if (isNewIdMapped(newId)) {
                error(std::string("ID already used in module: ") + std::to_string(newId));
+                return spirvbin_t::unused;
+            }

            msg(4, 4, std::string("map: ") + std::to_string(id) + " -> " + std::to_string(newId));
            setMapped(newId);
@ -299,6 +316,10 @@ namespace spv {
        process(inst_fn_nop, // ignore instructions
            [this](spv::Id& id) {
                id = localId(id);
+
+                if (errorLatch)
+                    return;
+
                assert(id != unused && id != unmapped);
            }
        );
@ -317,14 +338,22 @@ namespace spv {
                continue;

            // Find a new mapping for any used but unmapped IDs
-            if (isOldIdUnmapped(id))
+            if (isOldIdUnmapped(id)) {
                localId(id, unusedId = nextUnusedId(unusedId));
+                if (errorLatch)
+                    return;
+            }

-            if (isOldIdUnmapped(id))
+            if (isOldIdUnmapped(id)) {
                error(std::string("old ID not mapped: ") + std::to_string(id));
+                return;
+            }

            // Track max bound
            maxBound = std::max(maxBound, localId(id) + 1);
+
+            if (errorLatch)
+                return;
        }

        bound(maxBound); // reset header ID bound to as big as it now needs to be
@ -406,6 +435,9 @@ namespace spv {
                    if (typeId != spv::NoResult) {
                        const unsigned idTypeSize = typeSizeInWords(typeId);

+                        if (errorLatch)
+                            return false;
+
                        if (idTypeSize != 0)
                            idTypeSizeMap[resultId] = idTypeSize;
                    }
@ -421,17 +453,26 @@ namespace spv {
                } else if (opCode == spv::Op::OpEntryPoint) {
                    entryPoint = asId(start + 2);
                } else if (opCode == spv::Op::OpFunction) {
-                    if (fnStart != 0)
+                    if (fnStart != 0) {
                        error("nested function found");
+                        return false;
+                    }
+
                    fnStart = start;
                    fnRes   = asId(start + 2);
                } else if (opCode == spv::Op::OpFunctionEnd) {
                    assert(fnRes != spv::NoResult);
-                    if (fnStart == 0)
+                    if (fnStart == 0) {
                        error("function end without function start");
+                        return false;
+                    }
+
                    fnPos[fnRes] = range_t(fnStart, start + asWordCount(start));
                    fnStart = 0;
                } else if (isConstOp(opCode)) {
+                    if (errorLatch)
+                        return false;
+
                    assert(asId(start + 2) != spv::NoResult);
                    typeConstPos.insert(start);
                } else if (isTypeOp(opCode)) {
@ -451,18 +492,24 @@ namespace spv {
    {
        msg(2, 2, std::string("validating: "));

-        if (spv.size() < header_size)
+        if (spv.size() < header_size) {
            error("file too short: ");
+            return;
+        }

-        if (magic() != spv::MagicNumber)
+        if (magic() != spv::MagicNumber) {
            error("bad magic number");
+            return;
+        }

        // field 1 = version
        // field 2 = generator magic
        // field 3 = result <id> bound

-        if (schemaNum() != 0)
+        if (schemaNum() != 0) {
            error("bad schema, must be 0");
+            return;
+        }
    }

    int spirvbin_t::processInstruction(unsigned word, instfn_t instFn, idfn_t idFn)
@ -472,8 +519,10 @@ namespace spv {
        const int      nextInst  = word++ + wordCount;
        spv::Op  opCode    = asOpCode(instructionStart);

-        if (nextInst > int(spv.size()))
+        if (nextInst > int(spv.size())) {
            error("spir instruction terminated too early");
+            return -1;
+        }

        // Base for computing number of operands; will be updated as more is learned
        unsigned numOperands = wordCount - 1;
@ -555,6 +604,9 @@ namespace spv {
                    const unsigned literalSize = idTypeSizeInWords(idBuffer[literalSizePos]);
                    const unsigned numLiteralIdPairs = (nextInst-word) / (1+literalSize);

+                    if (errorLatch)
+                        return -1;
+
                    for (unsigned arg=0; arg<numLiteralIdPairs; ++arg) {
                        word += literalSize;  // literal
                        idFn(asId(word++));   // label
@ -631,9 +683,13 @@ namespace spv {
        // basic parsing and InstructionDesc table borrowed from SpvDisassemble.cpp...
        unsigned nextInst = unsigned(spv.size());

-        for (unsigned word = begin; word < end; word = nextInst)
+        for (unsigned word = begin; word < end; word = nextInst) {
            nextInst = processInstruction(word, instFn, idFn);

+            if (errorLatch)
+                return *this;
+        }
+
        return *this;
    }

@ -648,8 +704,11 @@ namespace spv {
            for (const char c : name.first)
                hashval = hashval * 1009 + c;

-            if (isOldIdUnmapped(name.second))
+            if (isOldIdUnmapped(name.second)) {
                localId(name.second, nextUnusedId(hashval % softTypeIdLimit + firstMappedID));
+                if (errorLatch)
+                    return;
+            }
        }
    }

@ -671,6 +730,9 @@ namespace spv {
            [&](spv::Op, unsigned start) { instPos.push_back(start); return true; },
            op_fn_nop);

+        if (errorLatch)
+            return;
+
        // Window size for context-sensitive canonicalization values
        // Empirical best size from a single data set.  TODO: Would be a good tunable.
        // We essentially perform a little convolution around each instruction,
@ -706,8 +768,12 @@ namespace spv {
                        hashval = hashval * 30103 + asOpCodeHash(instPos[i]); // 30103 = semiarbitrary prime
                    }

-                    if (isOldIdUnmapped(resId))
+                    if (isOldIdUnmapped(resId)) {
                        localId(resId, nextUnusedId(hashval % softTypeIdLimit + firstMappedID));
+                        if (errorLatch)
+                            return;
+                    }
+
                }
            }
        }
@ -800,6 +866,9 @@ namespace spv {
            [&](spv::Id& id) { if (idMap.find(id) != idMap.end()) id = idMap[id]; }
        );

+        if (errorLatch)
+            return;
+
        // EXPERIMENTAL: Implicit output stores
        fnLocalVars.clear();
        idMap.clear();
@ -820,11 +889,17 @@ namespace spv {
            },
            op_fn_nop);

+        if (errorLatch)
+            return;
+
        process(
            inst_fn_nop,
            [&](spv::Id& id) { if (idMap.find(id) != idMap.end()) id = idMap[id]; }
        );

+        if (errorLatch)
+            return;
+
        strip();          // strip out data we decided to eliminate
    }

@ -924,6 +999,9 @@ namespace spv {
            }
        );

+        if (errorLatch)
+            return;
+
        process(
            [&](spv::Op opCode, unsigned start) {
                if (opCode == spv::OpLoad && fnLocalVars.count(asId(start+3)) > 0)
@ -932,6 +1010,9 @@ namespace spv {
            },
            op_fn_nop);

+        if (errorLatch)
+            return;
+
        // Chase replacements to their origins, in case there is a chain such as:
        //   2 = store 1
        //   3 = load 2
@ -965,6 +1046,9 @@ namespace spv {
            }
        );

+        if (errorLatch)
+            return;
+
        strip();          // strip out data we decided to eliminate
    }

@ -1008,6 +1092,9 @@ namespace spv {
                        fn->second.first,
                        fn->second.second);

+                    if (errorLatch)
+                        return;
+
                    fn = fnPos.erase(fn);
                } else ++fn;
            }
@ -1040,6 +1127,9 @@ namespace spv {
            [&](spv::Id& id) { if (varUseCount[id]) ++varUseCount[id]; }
        );

+        if (errorLatch)
+            return;
+
        // Remove single-use function variables + associated decorations and names
        process(
            [&](spv::Op opCode, unsigned start) {
@ -1081,6 +1171,9 @@ namespace spv {
                    [&](spv::Id& id) { if (isType[id]) ++typeUseCount[id]; }
                    );

+            if (errorLatch)
+                return;
+
            // Remove single reference types
            for (const auto typeStart : typeConstPos) {
                const spv::Id typeId = asTypeConstId(typeStart);
@ -1090,6 +1183,9 @@ namespace spv {
                    stripInst(typeStart);
                }
            }
+
+            if (errorLatch)
+                return;
        }
    }

@ -1168,8 +1264,10 @@ namespace spv {
    unsigned spirvbin_t::idPos(spv::Id id) const
    {
        const auto tid_it = idPosR.find(id);
-        if (tid_it == idPosR.end())
+        if (tid_it == idPosR.end()) {
            error("ID not found");
+            return 0;
+        }

        return tid_it->second;
    }
@ -1268,8 +1366,14 @@ namespace spv {
            const spv::Id       resId     = asTypeConstId(typeStart);
            const std::uint32_t hashval   = hashType(typeStart);

-            if (isOldIdUnmapped(resId))
+            if (errorLatch)
+                return;
+
+            if (isOldIdUnmapped(resId)) {
                localId(resId, nextUnusedId(hashval % softTypeIdLimit + firstMappedID));
+                if (errorLatch)
+                    return;
+            }
        }
    }

@ -1315,24 +1419,49 @@ namespace spv {
        msg(3, 4, std::string("ID bound: ") + std::to_string(bound()));

        if (options & STRIP)         stripDebug();
+        if (errorLatch) return;
+
        strip();        // strip out data we decided to eliminate
+        if (errorLatch) return;
+
        if (options & OPT_LOADSTORE) optLoadStore();
+        if (errorLatch) return;
+
        if (options & OPT_FWD_LS)    forwardLoadStores();
+        if (errorLatch) return;
+
        if (options & DCE_FUNCS)     dceFuncs();
+        if (errorLatch) return;
+
        if (options & DCE_VARS)      dceVars();
+        if (errorLatch) return;
+
        if (options & DCE_TYPES)     dceTypes();
+        if (errorLatch) return;

        strip();         // strip out data we decided to eliminate
+        if (errorLatch) return;
+
        stripDeadRefs(); // remove references to things we DCEed
+        if (errorLatch) return;
+
        // after the last strip, we must clean any debug info referring to now-deleted data

        if (options & MAP_TYPES)     mapTypeConst();
+        if (errorLatch) return;
+
        if (options & MAP_NAMES)     mapNames();
+        if (errorLatch) return;
+
        if (options & MAP_FUNCS)     mapFnBodies();
+        if (errorLatch) return;

        if (options & MAP_ALL) {
            mapRemainder(); // map any unmapped IDs
+            if (errorLatch) return;
+
            applyMap();     // Now remap each shader to the new IDs we've come up with
+            if (errorLatch) return;
        }
    }

--- a/SPIRV/SPVRemapper.h
+++ b/SPIRV/SPVRemapper.h
@ -39,6 +39,7 @@
 #include <string>
 #include <vector>
 #include <cstdlib>
+#include <exception>

 namespace spv {

@ -111,7 +112,9 @@ namespace spv {
 class spirvbin_t : public spirvbin_base_t
 {
 public:
-   spirvbin_t(int verbose = 0) : entryPoint(spv::NoResult), largestNewId(0), verbose(verbose) { }
+   spirvbin_t(int verbose = 0) : entryPoint(spv::NoResult), largestNewId(0), verbose(verbose), errorLatch(false)
+   { }
+
   virtual ~spirvbin_t() { }

   // remap on an existing binary in memory
@ -165,7 +168,7 @@ private:
   typedef std::unordered_map<spv::Id, unsigned> typesize_map_t;

   // handle error
-   void error(const std::string& txt) const { errorHandler(txt); }
+   void error(const std::string& txt) const { errorLatch = true; errorHandler(txt); }

   bool     isConstOp(spv::Op opCode)      const;
   bool     isTypeOp(spv::Op opCode)       const;
@ -286,6 +289,11 @@ private:
   std::uint32_t options;
   int           verbose;     // verbosity level

+   // Error latch: this is set if the error handler is ever executed.  It would be better to
+   // use a try/catch block and throw, but that's not desired for certain environments, so
+   // this is the alternative.
+   mutable bool errorLatch;
+
   static errorfn_t errorHandler;
   static logfn_t   logHandler;
 };
--- a/Test/baseResults/remap.invalid-spirv-1.out
+++ b/Test/baseResults/remap.invalid-spirv-1.out
@ -0,0 +1 @@
+ID out of range: 4160749568
--- a/Test/baseResults/remap.invalid-spirv-2.out
+++ b/Test/baseResults/remap.invalid-spirv-2.out
@ -0,0 +1 @@
+ID not found
--- a/Test/remap.invalid-spirv-1.spv
+++ b/Test/remap.invalid-spirv-1.spv
--- a/Test/remap.invalid-spirv-2.spv
+++ b/Test/remap.invalid-spirv-2.spv
--- a/Test/runtests
+++ b/Test/runtests
@ -3,6 +3,7 @@
 TARGETDIR=localResults
 BASEDIR=baseResults
 EXE=../build/install/bin/glslangValidator
+REMAPEXE=../build/install/bin/spirv-remap
 HASERROR=0
 mkdir -p localResults

@ -141,6 +142,7 @@ diff -b $BASEDIR/hlsl.dashI.vert.out $TARGETDIR/hlsl.dashI.vert.out || HASERROR=
 #
 # Testing -D and -U
 #
+echo "Testing -D and -U"
 $EXE -DUNDEFED -UIN_SHADER -DFOO=200 -i -l -UUNDEFED -DMUL=FOO*2 glsl.-D-U.frag > $TARGETDIR/glsl.-D-U.frag.out
 diff -b $BASEDIR/glsl.-D-U.frag.out $TARGETDIR/glsl.-D-U.frag.out || HASERROR=1
 $EXE -D -e main -V -i -DUNDEFED -UIN_SHADER -DFOO=200 -UUNDEFED hlsl.-D-U.frag > $TARGETDIR/hlsl.-D-U.frag.out
@ -149,6 +151,7 @@ diff -b $BASEDIR/hlsl.-D-U.frag.out $TARGETDIR/hlsl.-D-U.frag.out || HASERROR=1
 #
 # Test --client and --target-env
 #
+echo "Testing --client and --target-env"
 $EXE --client vulkan100      spv.targetVulkan.vert || HASERROR=1
 $EXE --client opengl100      spv.targetOpenGL.vert || HASERROR=1
 $EXE --target-env vulkan1.0  spv.targetVulkan.vert || HASERROR=1
@ -159,6 +162,7 @@ $EXE -G100                   spv.targetOpenGL.vert || HASERROR=1
 #
 # Testing GLSL entry point rename
 #
+echo "Testing GLSL entry point rename"
 $EXE -H -e foo --source-entrypoint main glsl.entryPointRename.vert > $TARGETDIR/glsl.entryPointRename.vert.out
 diff -b $BASEDIR/glsl.entryPointRename.vert.out $TARGETDIR/glsl.entryPointRename.vert.out || HASERROR=1
 $EXE -H -e foo --source-entrypoint bar glsl.entryPointRename.vert > $TARGETDIR/glsl.entryPointRename.vert.bad.out
@ -166,6 +170,15 @@ diff -b $BASEDIR/glsl.entryPointRename.vert.bad.out $TARGETDIR/glsl.entryPointRe
 $EXE -H -e foo --source-entrypoint main glsl.entryPointRename2.vert > $TARGETDIR/glsl.entryPointRename2.vert.out
 diff -b $BASEDIR/glsl.entryPointRename2.vert.out $TARGETDIR/glsl.entryPointRename2.vert.out || HASERROR=1

+#
+# Testing remapper error handling
+#
+echo "Testing remapper error handling"
+$REMAPEXE --do-everything -i remap.invalid-spirv-1.spv -o $TARGETDIR > $TARGETDIR/remap.invalid-spirv-1.out && HASERROR=1
+diff -b $BASEDIR/remap.invalid-spirv-1.out $TARGETDIR/remap.invalid-spirv-1.out || HASERROR=1
+$REMAPEXE --do-everything -i remap.invalid-spirv-2.spv -o $TARGETDIR > $TARGETDIR/remap.invalid-spirv-2.out && HASERROR=1
+diff -b $BASEDIR/remap.invalid-spirv-2.out $TARGETDIR/remap.invalid-spirv-2.out || HASERROR=1
+
 #
 # Final checking
 #