From 6a60c2f9ead58eb9040e47e3e2ada01488648901 Mon Sep 17 00:00:00 2001 From: John Kessenich Date: Thu, 8 Dec 2016 21:01:59 -0700 Subject: [PATCH] Linker: Walk the call graph to report an error on missing bodies. --- SPIRV/GlslangToSpv.cpp | 4 +- Test/baseResults/100scope.vert.out | 2 + Test/baseResults/110scope.vert.out | 4 + Test/baseResults/300scope.vert.out | 2 + Test/baseResults/430scope.vert.out | 2 + Test/baseResults/missingBodies.vert.out | 117 +++++++++++++++++++++++++ Test/missingBodies.vert | 24 +++++ glslang/Include/revision.h | 4 +- glslang/MachineIndependent/linkValidate.cpp | 70 ++++++++++++++- glslang/MachineIndependent/localintermediate.h | 6 +- gtests/Link.FromFile.cpp | 1 + 11 files changed, 228 insertions(+), 8 deletions(-) create mode 100755 Test/baseResults/missingBodies.vert.out create mode 100644 Test/missingBodies.vert diff --git a/SPIRV/GlslangToSpv.cpp b/SPIRV/GlslangToSpv.cpp index 7abd04d..15af947 100755 --- a/SPIRV/GlslangToSpv.cpp +++ b/SPIRV/GlslangToSpv.cpp @@ -1352,7 +1352,7 @@ bool TGlslangToSpvTraverser::visitAggregate(glslang::TVisit visit, glslang::TInt // anything else gets there, so visit out of order, doing them all now. makeGlobalInitializers(node->getAsAggregate()->getSequence()); - // Initializers are done, don't want to visit again, but functions link objects need to be processed, + // Initializers are done, don't want to visit again, but functions and link objects need to be processed, // so do them manually. visitFunctions(node->getAsAggregate()->getSequence()); @@ -2634,7 +2634,7 @@ void TGlslangToSpvTraverser::visitFunctions(const glslang::TIntermSequence& glsl { for (int f = 0; f < (int)glslFunctions.size(); ++f) { glslang::TIntermAggregate* node = glslFunctions[f]->getAsAggregate(); - if (node && (node->getOp() == glslang::EOpFunction || node->getOp() == glslang ::EOpLinkerObjects)) + if (node && (node->getOp() == glslang::EOpFunction || node->getOp() == glslang::EOpLinkerObjects)) node->traverse(this); } } diff --git a/Test/baseResults/100scope.vert.out b/Test/baseResults/100scope.vert.out index 2b542b0..44132bd 100644 --- a/Test/baseResults/100scope.vert.out +++ b/Test/baseResults/100scope.vert.out @@ -128,6 +128,8 @@ ERROR: node is still EOpNull! Linked vertex stage: +ERROR: Linking vertex stage: No function definition (body) found: + g( Shader version: 100 ERROR: node is still EOpNull! diff --git a/Test/baseResults/110scope.vert.out b/Test/baseResults/110scope.vert.out index dd53250..e71bba3 100644 --- a/Test/baseResults/110scope.vert.out +++ b/Test/baseResults/110scope.vert.out @@ -130,6 +130,10 @@ ERROR: node is still EOpNull! Linked vertex stage: +ERROR: Linking vertex stage: No function definition (body) found: + sin(f1; +ERROR: Linking vertex stage: No function definition (body) found: + g( Shader version: 110 ERROR: node is still EOpNull! diff --git a/Test/baseResults/300scope.vert.out b/Test/baseResults/300scope.vert.out index 2a9a945..d2428e0 100644 --- a/Test/baseResults/300scope.vert.out +++ b/Test/baseResults/300scope.vert.out @@ -131,6 +131,8 @@ ERROR: node is still EOpNull! Linked vertex stage: +ERROR: Linking vertex stage: No function definition (body) found: + g( Shader version: 300 ERROR: node is still EOpNull! diff --git a/Test/baseResults/430scope.vert.out b/Test/baseResults/430scope.vert.out index 0a097a2..f1b9594 100644 --- a/Test/baseResults/430scope.vert.out +++ b/Test/baseResults/430scope.vert.out @@ -127,6 +127,8 @@ ERROR: node is still EOpNull! Linked vertex stage: +ERROR: Linking vertex stage: No function definition (body) found: + g( Shader version: 430 ERROR: node is still EOpNull! diff --git a/Test/baseResults/missingBodies.vert.out b/Test/baseResults/missingBodies.vert.out new file mode 100755 index 0000000..7cf4734 --- /dev/null +++ b/Test/baseResults/missingBodies.vert.out @@ -0,0 +1,117 @@ +missingBodies.vert +Warning, version 450 is not yet complete; most version-specific features are present, but some are missing. + +Shader version: 450 +0:? Sequence +0:4 Function Definition: foo( (global void) +0:4 Function Parameters: +0:4 Sequence +0:4 Function Call: bar( (global void) +0:8 Function Definition: C(i1;i1; (global void) +0:8 Function Parameters: +0:8 '' (in int) +0:8 '' (in int) +0:10 Function Definition: A( (global void) +0:10 Function Parameters: +0:10 Sequence +0:10 Function Call: B( (global void) +0:10 Function Call: C(i1; (global void) +0:10 Constant: +0:10 1 (const int) +0:10 Function Call: C(b1; (global void) +0:10 Constant: +0:10 true (const bool) +0:10 Function Call: C(i1;i1; (global void) +0:10 Constant: +0:10 1 (const int) +0:10 Constant: +0:10 2 (const int) +0:12 Function Definition: main( (global void) +0:12 Function Parameters: +0:14 Sequence +0:14 Function Call: foo( (global void) +0:15 Function Call: C(b1; (global void) +0:15 Constant: +0:15 true (const bool) +0:20 Sequence +0:20 move second child to first child (temp int) +0:20 'f1' (global int) +0:20 Function Call: ret1( (global int) +0:22 Function Definition: ret2( (global int) +0:22 Function Parameters: +0:22 Sequence +0:22 Branch: Return with expression +0:22 Constant: +0:22 3 (const int) +0:24 Sequence +0:24 move second child to first child (temp int) +0:24 'f2' (global int) +0:24 Function Call: ret2( (global int) +0:? Linker Objects +0:? 'f1' (global int) +0:? 'f2' (global int) +0:? 'gl_VertexID' (gl_VertexId int VertexId) +0:? 'gl_InstanceID' (gl_InstanceId int InstanceId) + + +Linked vertex stage: + +ERROR: Linking vertex stage: No function definition (body) found: + ret1( +ERROR: Linking vertex stage: No function definition (body) found: + C(b1; +ERROR: Linking vertex stage: No function definition (body) found: + bar( + +Shader version: 450 +0:? Sequence +0:4 Function Definition: foo( (global void) +0:4 Function Parameters: +0:4 Sequence +0:4 Function Call: bar( (global void) +0:8 Function Definition: C(i1;i1; (global void) +0:8 Function Parameters: +0:8 '' (in int) +0:8 '' (in int) +0:10 Function Definition: A( (global void) +0:10 Function Parameters: +0:10 Sequence +0:10 Function Call: B( (global void) +0:10 Function Call: C(i1; (global void) +0:10 Constant: +0:10 1 (const int) +0:10 Function Call: C(b1; (global void) +0:10 Constant: +0:10 true (const bool) +0:10 Function Call: C(i1;i1; (global void) +0:10 Constant: +0:10 1 (const int) +0:10 Constant: +0:10 2 (const int) +0:12 Function Definition: main( (global void) +0:12 Function Parameters: +0:14 Sequence +0:14 Function Call: foo( (global void) +0:15 Function Call: C(b1; (global void) +0:15 Constant: +0:15 true (const bool) +0:20 Sequence +0:20 move second child to first child (temp int) +0:20 'f1' (global int) +0:20 Function Call: ret1( (global int) +0:22 Function Definition: ret2( (global int) +0:22 Function Parameters: +0:22 Sequence +0:22 Branch: Return with expression +0:22 Constant: +0:22 3 (const int) +0:24 Sequence +0:24 move second child to first child (temp int) +0:24 'f2' (global int) +0:24 Function Call: ret2( (global int) +0:? Linker Objects +0:? 'f1' (global int) +0:? 'f2' (global int) +0:? 'gl_VertexID' (gl_VertexId int VertexId) +0:? 'gl_InstanceID' (gl_InstanceId int InstanceId) + diff --git a/Test/missingBodies.vert b/Test/missingBodies.vert new file mode 100644 index 0000000..04b7181 --- /dev/null +++ b/Test/missingBodies.vert @@ -0,0 +1,24 @@ +#version 450 + +void bar(); +void foo() { bar(); } + +void B(); +void C(int); +void C(int, int) { } +void C(bool); +void A() { B(); C(1); C(true); C(1, 2); } + +void main() +{ + foo(); + C(true); +} + +int ret1(); + +int f1 = ret1(); + +int ret2() { return 3; } + +int f2 = ret2(); diff --git a/glslang/Include/revision.h b/glslang/Include/revision.h index c53a14d..62856cc 100644 --- a/glslang/Include/revision.h +++ b/glslang/Include/revision.h @@ -2,5 +2,5 @@ // For the version, it uses the latest git tag followed by the number of commits. // For the date, it uses the current date (when then script is run). -#define GLSLANG_REVISION "Overload400-PrecQual.1676" -#define GLSLANG_DATE "05-Dec-2016" +#define GLSLANG_REVISION "Overload400-PrecQual.1686" +#define GLSLANG_DATE "08-Dec-2016" diff --git a/glslang/MachineIndependent/linkValidate.cpp b/glslang/MachineIndependent/linkValidate.cpp index 050fdae..8e14bbb 100644 --- a/glslang/MachineIndependent/linkValidate.cpp +++ b/glslang/MachineIndependent/linkValidate.cpp @@ -94,7 +94,7 @@ void TIntermediate::merge(TInfoSink& infoSink, TIntermediate& unit) callGraph.insert(callGraph.end(), unit.callGraph.begin(), unit.callGraph.end()); if (originUpperLeft != unit.originUpperLeft || pixelCenterInteger != unit.pixelCenterInteger) - error(infoSink, "gl_FragCoord redeclarations must match across shaders\n"); + error(infoSink, "gl_FragCoord redeclarations must match across shaders"); if (! earlyFragmentTests) earlyFragmentTests = unit.earlyFragmentTests; @@ -390,8 +390,9 @@ void TIntermediate::finalCheck(TInfoSink& infoSink) if (numPushConstants > 1) error(infoSink, "Only one push_constant block is allowed per stage"); - // recursion checking + // recursion and missing body checking checkCallGraphCycles(infoSink); + checkCallGraphBodies(infoSink); // overlap/alias/missing I/O, etc. inOutLocationCheck(infoSink); @@ -502,7 +503,7 @@ void TIntermediate::finalCheck(TInfoSink& infoSink) // void TIntermediate::checkCallGraphCycles(TInfoSink& infoSink) { - // Reset everything, once. + // Clear fields we'll use for this. for (TGraph::iterator call = callGraph.begin(); call != callGraph.end(); ++call) { call->visited = false; call->currentPath = false; @@ -578,6 +579,69 @@ void TIntermediate::checkCallGraphCycles(TInfoSink& infoSink) } // +// See which functions are reachable from the entry point and which have bodies. +// Reachable ones with missing bodies are errors. +// +void TIntermediate::checkCallGraphBodies(TInfoSink& infoSink) +{ + // Clear fields we'll use for this. + for (TGraph::iterator call = callGraph.begin(); call != callGraph.end(); ++call) { + call->visited = false; + call->calleeBodyPosition = -1; + } + + // The top level of the AST includes function definitions (bodies). + // Compare these to function calls in the call graph. + // We'll end up knowing which have bodies, and if so, + // how to map the call-graph node to the location in the AST. + TIntermSequence &functionSequence = getTreeRoot()->getAsAggregate()->getSequence(); + for (int f = 0; f < (int)functionSequence.size(); ++f) { + glslang::TIntermAggregate* node = functionSequence[f]->getAsAggregate(); + if (node && (node->getOp() == glslang::EOpFunction)) { + for (TGraph::iterator call = callGraph.begin(); call != callGraph.end(); ++call) { + if (call->callee == node->getName()) + call->calleeBodyPosition = f; + } + } + } + + // Start call-graph traversal by visiting the entry point nodes. + for (TGraph::iterator call = callGraph.begin(); call != callGraph.end(); ++call) { + if (call->caller.compare(getEntryPointMangledName().c_str()) == 0) + call->visited = true; + } + + // Propagate 'visited' through the call-graph to every part of the graph it + // can reach (seeded with the entry-point setting above). + bool changed; + do { + changed = false; + for (auto call1 = callGraph.begin(); call1 != callGraph.end(); ++call1) { + if (call1->visited) { + for (TGraph::iterator call2 = callGraph.begin(); call2 != callGraph.end(); ++call2) { + if (! call2->visited) { + if (call1->callee == call2->caller) { + changed = true; + call2->visited = true; + } + } + } + } + } + } while (changed); + + // Any call-graph node set to visited but without a callee body is an error. + for (TGraph::iterator call = callGraph.begin(); call != callGraph.end(); ++call) { + if (call->visited) { + if (call->calleeBodyPosition == -1) { + error(infoSink, "No function definition (body) found: "); + infoSink.info << " " << call->callee << "\n"; + } + } + } +} + +// // Satisfy rules for location qualifiers on inputs and outputs // void TIntermediate::inOutLocationCheck(TInfoSink& infoSink) diff --git a/glslang/MachineIndependent/localintermediate.h b/glslang/MachineIndependent/localintermediate.h index 028de06..a16383f 100644 --- a/glslang/MachineIndependent/localintermediate.h +++ b/glslang/MachineIndependent/localintermediate.h @@ -67,7 +67,9 @@ struct TVectorFields { // by TIntermediate. // -// Used for detecting recursion: A "call" is a pair: . +// Used for call-graph algorithms for detecting recursion, missing bodies, and dead bodies. +// A "call" is a pair: . +// There can be duplicates. General assumption is the list is small. struct TCall { TCall(const TString& pCaller, const TString& pCallee) : caller(pCaller), callee(pCallee) { } TString caller; @@ -75,6 +77,7 @@ struct TCall { bool visited; bool currentPath; bool errorGiven; + int calleeBodyPosition; }; // A generic 1-D range. @@ -393,6 +396,7 @@ protected: void mergeImplicitArraySizes(TType&, const TType&); void mergeErrorCheck(TInfoSink&, const TIntermSymbol&, const TIntermSymbol&, bool crossStage); void checkCallGraphCycles(TInfoSink&); + void checkCallGraphBodies(TInfoSink&); void inOutLocationCheck(TInfoSink&); TIntermSequence& findLinkerObjects() const; bool userOutputUsed() const; diff --git a/gtests/Link.FromFile.cpp b/gtests/Link.FromFile.cpp index 2651a1e..ab845bf 100644 --- a/gtests/Link.FromFile.cpp +++ b/gtests/Link.FromFile.cpp @@ -99,6 +99,7 @@ INSTANTIATE_TEST_CASE_P( {"150.tesc", "150.tese", "400.tesc", "400.tese", "410.tesc", "420.tesc", "420.tese"}, {"max_vertices_0.geom"}, {"es-link1.frag", "es-link2.frag"}, + {"missingBodies.vert"} })), ); // clang-format on -- 2.7.4