Commit f51fdd2e by Olli Etuaho Committed by Commit Bot

Ensure that all functions have a body node in the AST

Some traversers that insert code to main() assume that the main() function has a non-null body node in place. This assumption was previously wrong, since functions could be missing the body node in case the function body was empty. Fix possible invalid dereferencing of missing function body nodes by always adding an empty sequence node to represent the body of functions that have an empty body in the ESSL source. This also enables simplifying some tree traversers that used to take the possibility of missing function body nodes into account. Also fix AddDefaultReturnStatements to check the last statement inside the function body for a return statement, instead of checking the first statement. BUG=angleproject:1539 TEST=angle_unittests, angle_end2end_tests Change-Id: I2fbd18c78653fa2f1a96dbd9a619accc4874030d Reviewed-on: https://chromium-review.googlesource.com/392046Reviewed-by: 's avatarCorentin Wallez <cwallez@chromium.org> Reviewed-by: 's avatarJamie Madill <jmadill@chromium.org> Commit-Queue: Olli Etuaho <oetuaho@nvidia.com>
parent c9e6026c
...@@ -39,13 +39,9 @@ class AddDefaultReturnStatementsTraverser : private TIntermTraverser ...@@ -39,13 +39,9 @@ class AddDefaultReturnStatementsTraverser : private TIntermTraverser
return false; return false;
} }
TIntermAggregate *lastNode = node->getSequence()->back()->getAsAggregate(); TIntermAggregate *bodyNode = node->getSequence()->back()->getAsAggregate();
if (lastNode == nullptr) ASSERT(bodyNode);
{ TIntermBranch *returnNode = bodyNode->getSequence()->back()->getAsBranchNode();
return true;
}
TIntermBranch *returnNode = lastNode->getSequence()->front()->getAsBranchNode();
if (returnNode != nullptr && returnNode->getFlowOp() == EOpReturn) if (returnNode != nullptr && returnNode->getFlowOp() == EOpReturn)
{ {
return false; return false;
...@@ -62,8 +58,8 @@ class AddDefaultReturnStatementsTraverser : private TIntermTraverser ...@@ -62,8 +58,8 @@ class AddDefaultReturnStatementsTraverser : private TIntermTraverser
TIntermBranch *branch = TIntermBranch *branch =
new TIntermBranch(EOpReturn, TIntermTyped::CreateZero(returnType)); new TIntermBranch(EOpReturn, TIntermTyped::CreateZero(returnType));
TIntermAggregate *lastNode = node->getSequence()->back()->getAsAggregate(); TIntermAggregate *bodyNode = node->getSequence()->back()->getAsAggregate();
lastNode->getSequence()->push_back(branch); bodyNode->getSequence()->push_back(branch);
return false; return false;
} }
......
...@@ -84,13 +84,10 @@ bool GLFragColorBroadcastTraverser::visitAggregate(Visit visit, TIntermAggregate ...@@ -84,13 +84,10 @@ bool GLFragColorBroadcastTraverser::visitAggregate(Visit visit, TIntermAggregate
if (node->getName() == "main(") if (node->getName() == "main(")
{ {
TIntermSequence *sequence = node->getSequence(); TIntermSequence *sequence = node->getSequence();
ASSERT((sequence->size() == 1) || (sequence->size() == 2)); ASSERT(sequence->size() == 2);
if (sequence->size() == 2) TIntermAggregate *body = (*sequence)[1]->getAsAggregate();
{ ASSERT(body);
TIntermAggregate *body = (*sequence)[1]->getAsAggregate(); mMainSequence = body->getSequence();
ASSERT(body);
mMainSequence = body->getSequence();
}
} }
break; break;
default: default:
......
...@@ -54,17 +54,8 @@ bool VariableInitializer::visitAggregate(Visit visit, TIntermAggregate *node) ...@@ -54,17 +54,8 @@ bool VariableInitializer::visitAggregate(Visit visit, TIntermAggregate *node)
if (node->getName() == "main(") if (node->getName() == "main(")
{ {
TIntermSequence *sequence = node->getSequence(); TIntermSequence *sequence = node->getSequence();
ASSERT((sequence->size() == 1) || (sequence->size() == 2)); ASSERT(sequence->size() == 2);
TIntermAggregate *body = NULL; TIntermAggregate *body = (*sequence)[1]->getAsAggregate();
if (sequence->size() == 1)
{
body = new TIntermAggregate(EOpSequence);
sequence->push_back(body);
}
else
{
body = (*sequence)[1]->getAsAggregate();
}
ASSERT(body); ASSERT(body);
insertInitCode(body->getSequence()); insertInitCode(body->getSequence());
mCodeInserted = true; mCodeInserted = true;
......
...@@ -818,22 +818,19 @@ bool TOutputGLSLBase::visitAggregate(Visit visit, TIntermAggregate *node) ...@@ -818,22 +818,19 @@ bool TOutputGLSLBase::visitAggregate(Visit visit, TIntermAggregate *node)
out << " " << hashFunctionNameIfNeeded(node->getNameObj()); out << " " << hashFunctionNameIfNeeded(node->getNameObj());
incrementDepth(node); incrementDepth(node);
// Function definition node contains one or two children nodes // Function definition node contains two child nodes representing the function parameters
// representing function parameters and function body. The latter // and the function body.
// is not present in case of empty function bodies.
const TIntermSequence &sequence = *(node->getSequence()); const TIntermSequence &sequence = *(node->getSequence());
ASSERT((sequence.size() == 1) || (sequence.size() == 2)); ASSERT(sequence.size() == 2);
TIntermSequence::const_iterator seqIter = sequence.begin();
// Traverse function parameters. // Traverse function parameters.
TIntermAggregate *params = (*seqIter)->getAsAggregate(); TIntermAggregate *params = sequence[0]->getAsAggregate();
ASSERT(params != NULL); ASSERT(params != NULL);
ASSERT(params->getOp() == EOpParameters); ASSERT(params->getOp() == EOpParameters);
params->traverse(this); params->traverse(this);
// Traverse function body. // Traverse function body.
TIntermAggregate *body = ++seqIter != sequence.end() ? TIntermAggregate *body = sequence[1]->getAsAggregate();
(*seqIter)->getAsAggregate() : NULL;
visitCodeBlock(body); visitCodeBlock(body);
decrementDepth(); decrementDepth();
......
...@@ -1618,19 +1618,13 @@ bool OutputHLSL::visitAggregate(Visit visit, TIntermAggregate *node) ...@@ -1618,19 +1618,13 @@ bool OutputHLSL::visitAggregate(Visit visit, TIntermAggregate *node)
out << ")\n"; out << ")\n";
if (sequence->size() > 1) mInsideFunction = true;
{ ASSERT(sequence->size() == 2);
mInsideFunction = true; TIntermNode *body = (*sequence)[1];
TIntermNode *body = (*sequence)[1]; // The function body node will output braces.
// The function body node will output braces. ASSERT(IsSequence(body));
ASSERT(IsSequence(body)); body->traverse(this);
body->traverse(this); mInsideFunction = false;
mInsideFunction = false;
}
else
{
out << "{}\n";
}
mCurrentFunctionMetadata = nullptr; mCurrentFunctionMetadata = nullptr;
......
...@@ -2063,22 +2063,31 @@ TIntermAggregate *TParseContext::addFunctionDefinition(const TFunction &function ...@@ -2063,22 +2063,31 @@ TIntermAggregate *TParseContext::addFunctionDefinition(const TFunction &function
TIntermAggregate *functionBody, TIntermAggregate *functionBody,
const TSourceLoc &location) const TSourceLoc &location)
{ {
//?? Check that all paths return a value if return type != void ? // Check that non-void functions have at least one return statement.
// May be best done as post process phase on intermediate code
if (mCurrentFunctionType->getBasicType() != EbtVoid && !mFunctionReturnsValue) if (mCurrentFunctionType->getBasicType() != EbtVoid && !mFunctionReturnsValue)
{ {
error(location, "function does not return a value:", "", function.getName().c_str()); error(location, "function does not return a value:", "", function.getName().c_str());
} }
TIntermAggregate *aggregate = TIntermAggregate *functionNode = new TIntermAggregate(EOpFunction);
intermediate.growAggregate(functionPrototype, functionBody, location); functionNode->setLine(location);
intermediate.setAggregateOperator(aggregate, EOpFunction, location);
aggregate->setName(function.getMangledName().c_str()); ASSERT(functionPrototype != nullptr);
aggregate->setType(function.getReturnType()); functionNode->getSequence()->push_back(functionPrototype);
aggregate->setFunctionId(function.getUniqueId());
if (functionBody == nullptr)
{
functionBody = new TIntermAggregate(EOpSequence);
functionBody->setLine(location);
}
functionNode->getSequence()->push_back(functionBody);
functionNode->setName(function.getMangledName().c_str());
functionNode->setType(function.getReturnType());
functionNode->setFunctionId(function.getUniqueId());
symbolTable.pop(); symbolTable.pop();
return aggregate; return functionNode;
} }
void TParseContext::parseFunctionPrototype(const TSourceLoc &location, void TParseContext::parseFunctionPrototype(const TSourceLoc &location,
......
...@@ -2568,4 +2568,18 @@ TEST_F(MalformedShaderTest, ShiftBy32) ...@@ -2568,4 +2568,18 @@ TEST_F(MalformedShaderTest, ShiftBy32)
{ {
FAIL() << "Shader compilation succeeded, expecting failure " << mInfoLog; FAIL() << "Shader compilation succeeded, expecting failure " << mInfoLog;
} }
} }
\ No newline at end of file
// Test that deferring global variable init works with an empty main().
TEST_F(MalformedShaderTest, DeferGlobalVariableInitWithEmptyMain)
{
const std::string &shaderString =
"precision mediump float;\n"
"uniform float u;\n"
"float foo = u;\n"
"void main() {}\n";
if (!compile(shaderString))
{
FAIL() << "Shader compilation failed, expecting success " << mInfoLog;
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment