From 48749146da6bd335928126855053ea0e3b66aee4 Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 18 Oct 2018 14:45:11 +0200 Subject: Fix a bug in CSE where a variable that was already out of scope was used. --- libyul/optimiser/CommonSubexpressionEliminator.cpp | 5 ++-- libyul/optimiser/DataFlowAnalyzer.cpp | 35 ++++++++++++++-------- libyul/optimiser/DataFlowAnalyzer.h | 8 ++++- libyul/optimiser/Rematerialiser.cpp | 9 ++---- 4 files changed, 34 insertions(+), 23 deletions(-) (limited to 'libyul') diff --git a/libyul/optimiser/CommonSubexpressionEliminator.cpp b/libyul/optimiser/CommonSubexpressionEliminator.cpp index 23d15cad..51737097 100644 --- a/libyul/optimiser/CommonSubexpressionEliminator.cpp +++ b/libyul/optimiser/CommonSubexpressionEliminator.cpp @@ -50,8 +50,8 @@ void CommonSubexpressionEliminator::visit(Expression& _e) if (m_value.at(name)->type() == typeid(Identifier)) { string const& value = boost::get(*m_value.at(name)).name; - if (inScope(value)) - _e = Identifier{locationOf(_e), value}; + assertThrow(inScope(value), OptimizerException, ""); + _e = Identifier{locationOf(_e), value}; } } } @@ -61,6 +61,7 @@ void CommonSubexpressionEliminator::visit(Expression& _e) for (auto const& var: m_value) { assertThrow(var.second, OptimizerException, ""); + assertThrow(inScope(var.first), OptimizerException, ""); if (SyntacticalEqualityChecker::equal(_e, *var.second)) { _e = Identifier{locationOf(_e), var.first}; diff --git a/libyul/optimiser/DataFlowAnalyzer.cpp b/libyul/optimiser/DataFlowAnalyzer.cpp index ca1e5153..7ac42c30 100644 --- a/libyul/optimiser/DataFlowAnalyzer.cpp +++ b/libyul/optimiser/DataFlowAnalyzer.cpp @@ -84,19 +84,19 @@ void DataFlowAnalyzer::operator()(Switch& _switch) void DataFlowAnalyzer::operator()(FunctionDefinition& _fun) { - m_variableScopes.emplace_back(true); + pushScope(true); for (auto const& parameter: _fun.parameters) m_variableScopes.back().variables.insert(parameter.name); for (auto const& var: _fun.returnVariables) m_variableScopes.back().variables.insert(var.name); ASTModifier::operator()(_fun); - m_variableScopes.pop_back(); + popScope(); } void DataFlowAnalyzer::operator()(ForLoop& _for) { // Special scope handling of the pre block. - m_variableScopes.emplace_back(false); + pushScope(false); for (auto& statement: _for.pre.statements) visit(statement); @@ -110,16 +110,15 @@ void DataFlowAnalyzer::operator()(ForLoop& _for) (*this)(_for.post); clearValues(assignments.names()); - - m_variableScopes.pop_back(); + popScope(); } void DataFlowAnalyzer::operator()(Block& _block) { size_t numScopes = m_variableScopes.size(); - m_variableScopes.emplace_back(false); + pushScope(false); ASTModifier::operator()(_block); - m_variableScopes.pop_back(); + popScope(); assertThrow(numScopes == m_variableScopes.size(), OptimizerException, ""); } @@ -148,7 +147,18 @@ void DataFlowAnalyzer::handleAssignment(set const& _variables, Expressio } } -void DataFlowAnalyzer::clearValues(set const& _variables) +void DataFlowAnalyzer::pushScope(bool _functionScope) +{ + m_variableScopes.emplace_back(_functionScope); +} + +void DataFlowAnalyzer::popScope() +{ + clearValues(std::move(m_variableScopes.back().variables)); + m_variableScopes.pop_back(); +} + +void DataFlowAnalyzer::clearValues(set _variables) { // All variables that reference variables to be cleared also have to be // cleared, but not recursively, since only the value of the original @@ -163,16 +173,15 @@ void DataFlowAnalyzer::clearValues(set const& _variables) // This cannot be easily tested since the substitutions will be done // one by one on the fly, and the last line will just be add(1, 1) - set variables = _variables; // Clear variables that reference variables to be cleared. - for (auto const& name: variables) + for (auto const& name: _variables) for (auto const& ref: m_referencedBy[name]) - variables.insert(ref); + _variables.insert(ref); // Clear the value and update the reference relation. - for (auto const& name: variables) + for (auto const& name: _variables) m_value.erase(name); - for (auto const& name: variables) + for (auto const& name: _variables) { for (auto const& ref: m_references[name]) m_referencedBy[ref].erase(name); diff --git a/libyul/optimiser/DataFlowAnalyzer.h b/libyul/optimiser/DataFlowAnalyzer.h index f998eadf..95671467 100644 --- a/libyul/optimiser/DataFlowAnalyzer.h +++ b/libyul/optimiser/DataFlowAnalyzer.h @@ -56,9 +56,15 @@ protected: /// Registers the assignment. void handleAssignment(std::set const& _names, Expression* _value); + /// Creates a new inner scope. + void pushScope(bool _functionScope); + + /// Removes the innermost scope and clears all variables in it. + void popScope(); + /// Clears information about the values assigned to the given variables, /// for example at points where control flow is merged. - void clearValues(std::set const& _names); + void clearValues(std::set _names); /// Returns true iff the variable is in scope. bool inScope(std::string const& _variableName) const; diff --git a/libyul/optimiser/Rematerialiser.cpp b/libyul/optimiser/Rematerialiser.cpp index dd6653ea..a99db0b6 100644 --- a/libyul/optimiser/Rematerialiser.cpp +++ b/libyul/optimiser/Rematerialiser.cpp @@ -38,16 +38,11 @@ void Rematerialiser::visit(Expression& _e) if (m_value.count(identifier.name)) { string name = identifier.name; - bool expressionValid = true; for (auto const& ref: m_references[name]) - if (!inScope(ref)) - { - expressionValid = false; - break; - } + assertThrow(inScope(ref), OptimizerException, ""); assertThrow(m_value.at(name), OptimizerException, ""); auto const& value = *m_value.at(name); - if (expressionValid && CodeSize::codeSize(value) <= 7) + if (CodeSize::codeSize(value) <= 7) _e = (ASTCopier{}).translate(value); } } -- cgit v1.2.3