Skip to content

Commit 4cbf9ff

Browse files
chrisethchriseth
authored
Merge pull request #10143 from ethereum/issue-10084
ControlFlowAnalyser: Also consider called functions in a flow
2 parents 21cd76d + 56ebea8 commit 4cbf9ff

33 files changed

Lines changed: 723 additions & 52 deletions

‎Changelog.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ Compiler Features:
1616

1717
Bugfixes:
1818
* AST: Do not output value of Yul literal if it is not a valid UTF-8 string.
19+
* Control Flow Graph: Take internal calls to functions that always revert into account for reporting unused or unassigned variables.
1920
* SMTChecker: Fix internal error on struct constructor with fixed bytes member initialized with string literal.
2021
* SMTChecker: Fix internal error on external calls from the constructor.
2122
* SMTChecker: Fix internal error on conversion from ``bytes`` to ``fixed bytes``.
@@ -28,6 +29,7 @@ AST Changes:
2829
* Add member `hexValue` for Yul string and hex literals.
2930

3031

32+
3133
### 0.8.4 (2021-04-21)
3234

3335
Important Bugfixes:

‎libsolidity/CMakeLists.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ set(sources
1010
analysis/ControlFlowBuilder.h
1111
analysis/ControlFlowGraph.cpp
1212
analysis/ControlFlowGraph.h
13+
analysis/ControlFlowRevertPruner.cpp
14+
analysis/ControlFlowRevertPruner.h
1315
analysis/DeclarationContainer.cpp
1416
analysis/DeclarationContainer.h
1517
analysis/DeclarationTypeChecker.cpp

‎libsolidity/analysis/ControlFlowAnalyzer.cpp‎

Lines changed: 44 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,28 +22,45 @@
2222
#include <libsolutil/Algorithms.h>
2323
#include <boost/range/algorithm/sort.hpp>
2424

25+
#include <functional>
26+
2527
using namespace std;
28+
using namespace std::placeholders;
2629
using namespace solidity::langutil;
2730
using namespace solidity::frontend;
2831

29-
bool ControlFlowAnalyzer::analyze(ASTNode const& _astRoot)
32+
33+
bool ControlFlowAnalyzer::run()
3034
{
31-
_astRoot.accept(*this);
35+
for (auto& [pair, flow]: m_cfg.allFunctionFlows())
36+
analyze(*pair.function, pair.contract, *flow);
37+
3238
return Error::containsOnlyWarnings(m_errorReporter.errors());
3339
}
3440

35-
bool ControlFlowAnalyzer::visit(FunctionDefinition const& _function)
41+
void ControlFlowAnalyzer::analyze(FunctionDefinition const& _function, ContractDefinition const* _contract, FunctionFlow const& _flow)
3642
{
37-
if (_function.isImplemented())
38-
{
39-
auto const& functionFlow = m_cfg.functionFlow(_function);
40-
checkUninitializedAccess(functionFlow.entry, functionFlow.exit, _function.body().statements().empty());
41-
checkUnreachable(functionFlow.entry, functionFlow.exit, functionFlow.revert, functionFlow.transactionReturn);
42-
}
43-
return false;
43+
if (!_function.isImplemented())
44+
return;
45+
46+
optional<string> mostDerivedContractName;
47+
48+
// The name of the most derived contract only required if it differs from
49+
// the functions contract
50+
if (_contract && _contract != _function.annotation().contract)
51+
mostDerivedContractName = _contract->name();
52+
53+
checkUninitializedAccess(
54+
_flow.entry,
55+
_flow.exit,
56+
_function.body().statements().empty(),
57+
mostDerivedContractName
58+
);
59+
checkUnreachable(_flow.entry, _flow.exit, _flow.revert, _flow.transactionReturn);
4460
}
4561

46-
void ControlFlowAnalyzer::checkUninitializedAccess(CFGNode const* _entry, CFGNode const* _exit, bool _emptyBody) const
62+
63+
void ControlFlowAnalyzer::checkUninitializedAccess(CFGNode const* _entry, CFGNode const* _exit, bool _emptyBody, optional<string> _contractName)
4764
{
4865
struct NodeInfo
4966
{
@@ -156,16 +173,27 @@ void ControlFlowAnalyzer::checkUninitializedAccess(CFGNode const* _entry, CFGNod
156173
" without prior assignment, which would lead to undefined behaviour."
157174
);
158175
else if (!_emptyBody && varDecl.name().empty())
176+
{
177+
if (!m_unassignedReturnVarsAlreadyWarnedFor.emplace(&varDecl).second)
178+
continue;
179+
159180
m_errorReporter.warning(
160181
6321_error,
161182
varDecl.location(),
162-
"Unnamed return variable can remain unassigned. Add an explicit return with value to all non-reverting code paths or name the variable."
183+
"Unnamed return variable can remain unassigned" +
184+
(
185+
_contractName.has_value() ?
186+
" when the function is called when \"" + _contractName.value() + "\" is the most derived contract." :
187+
"."
188+
) +
189+
" Add an explicit return with value to all non-reverting code paths or name the variable."
163190
);
191+
}
164192
}
165193
}
166194
}
167195

168-
void ControlFlowAnalyzer::checkUnreachable(CFGNode const* _entry, CFGNode const* _exit, CFGNode const* _revert, CFGNode const* _transactionReturn) const
196+
void ControlFlowAnalyzer::checkUnreachable(CFGNode const* _entry, CFGNode const* _exit, CFGNode const* _revert, CFGNode const* _transactionReturn)
169197
{
170198
// collect all nodes reachable from the entry point
171199
std::set<CFGNode const*> reachable = util::BreadthFirstSearch<CFGNode const*>{{_entry}}.run(
@@ -193,6 +221,8 @@ void ControlFlowAnalyzer::checkUnreachable(CFGNode const* _entry, CFGNode const*
193221
// Extend the location, as long as the next location overlaps (unreachable is sorted).
194222
for (; it != unreachable.end() && it->start <= location.end; ++it)
195223
location.end = std::max(location.end, it->end);
196-
m_errorReporter.warning(5740_error, location, "Unreachable code.");
224+
225+
if (m_unreachableLocationsAlreadyWarnedFor.emplace(location).second)
226+
m_errorReporter.warning(5740_error, location, "Unreachable code.");
197227
}
198228
}

‎libsolidity/analysis/ControlFlowAnalyzer.h‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,30 +19,38 @@
1919
#pragma once
2020

2121
#include <libsolidity/analysis/ControlFlowGraph.h>
22+
#include <liblangutil/ErrorReporter.h>
2223
#include <set>
2324

2425
namespace solidity::frontend
2526
{
2627

27-
class ControlFlowAnalyzer: private ASTConstVisitor
28+
class ControlFlowAnalyzer
2829
{
2930
public:
3031
explicit ControlFlowAnalyzer(CFG const& _cfg, langutil::ErrorReporter& _errorReporter):
3132
m_cfg(_cfg), m_errorReporter(_errorReporter) {}
3233

33-
bool analyze(ASTNode const& _astRoot);
34-
35-
bool visit(FunctionDefinition const& _function) override;
34+
bool run();
3635

3736
private:
37+
void analyze(FunctionDefinition const& _function, ContractDefinition const* _contract, FunctionFlow const& _flow);
3838
/// Checks for uninitialized variable accesses in the control flow between @param _entry and @param _exit.
39-
void checkUninitializedAccess(CFGNode const* _entry, CFGNode const* _exit, bool _emptyBody) const;
39+
/// @param _entry entry node
40+
/// @param _exit exit node
41+
/// @param _emptyBody whether the body of the function is empty (true) or not (false)
42+
/// @param _contractName name of the most derived contract, should be empty
43+
/// if the function is also defined in it
44+
void checkUninitializedAccess(CFGNode const* _entry, CFGNode const* _exit, bool _emptyBody, std::optional<std::string> _contractName = {});
4045
/// Checks for unreachable code, i.e. code ending in @param _exit, @param _revert or @param _transactionReturn
4146
/// that can not be reached from @param _entry.
42-
void checkUnreachable(CFGNode const* _entry, CFGNode const* _exit, CFGNode const* _revert, CFGNode const* _transactionReturn) const;
47+
void checkUnreachable(CFGNode const* _entry, CFGNode const* _exit, CFGNode const* _revert, CFGNode const* _transactionReturn);
4348

4449
CFG const& m_cfg;
4550
langutil::ErrorReporter& m_errorReporter;
51+
52+
std::set<langutil::SourceLocation> m_unreachableLocationsAlreadyWarnedFor;
53+
std::set<VariableDeclaration const*> m_unassignedReturnVarsAlreadyWarnedFor;
4654
};
4755

4856
}

‎libsolidity/analysis/ControlFlowBuilder.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,11 @@ bool ControlFlowBuilder::visit(FunctionCall const& _functionCall)
287287
m_currentNode = nextNode;
288288
return false;
289289
}
290+
case FunctionType::Kind::Internal:
291+
{
292+
m_currentNode->functionCalls.emplace_back(&_functionCall);
293+
break;
294+
}
290295
default:
291296
break;
292297
}

‎libsolidity/analysis/ControlFlowGraph.cpp‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,15 +34,25 @@ bool CFG::constructFlow(ASTNode const& _astRoot)
3434

3535
bool CFG::visit(FunctionDefinition const& _function)
3636
{
37-
if (_function.isImplemented())
38-
m_functionControlFlow[&_function] = ControlFlowBuilder::createFunctionFlow(m_nodeContainer, _function);
37+
if (_function.isImplemented() && _function.isFree())
38+
m_functionControlFlow[{nullptr, &_function}] = ControlFlowBuilder::createFunctionFlow(m_nodeContainer, _function);
3939
return false;
4040
}
4141

42-
FunctionFlow const& CFG::functionFlow(FunctionDefinition const& _function) const
42+
bool CFG::visit(ContractDefinition const& _contract)
4343
{
44-
solAssert(m_functionControlFlow.count(&_function), "");
45-
return *m_functionControlFlow.find(&_function)->second;
44+
for (ContractDefinition const* contract: _contract.annotation().linearizedBaseContracts)
45+
for (FunctionDefinition const* function: contract->definedFunctions())
46+
if (function->isImplemented())
47+
m_functionControlFlow[{&_contract, function}] =
48+
ControlFlowBuilder::createFunctionFlow(m_nodeContainer, *function);
49+
50+
return true;
51+
}
52+
53+
FunctionFlow const& CFG::functionFlow(FunctionDefinition const& _function, ContractDefinition const* _contract) const
54+
{
55+
return *m_functionControlFlow.at({_contract, &_function});
4656
}
4757

4858
CFGNode* CFG::NodeContainer::newNode()

‎libsolidity/analysis/ControlFlowGraph.h‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,8 @@ struct CFGNode
9898
std::vector<CFGNode*> entries;
9999
/// Exit nodes. All CFG nodes to which control flow may continue after this node.
100100
std::vector<CFGNode*> exits;
101+
/// Function calls done by this node
102+
std::vector<FunctionCall const*> functionCalls;
101103

102104
/// Variable occurrences in the node.
103105
std::vector<VariableOccurrence> variableOccurrences;
@@ -118,7 +120,7 @@ struct FunctionFlow
118120
/// (e.g. all return statements of the function).
119121
CFGNode* exit = nullptr;
120122
/// Revert node. Control flow of the function in case of revert.
121-
/// This node is empty does not have any exits, but may have multiple entries
123+
/// This node is empty and does not have any exits, but may have multiple entries
122124
/// (e.g. all assert, require, revert and throw statements).
123125
CFGNode* revert = nullptr;
124126
/// Transaction return node. Destination node for inline assembly "return" calls.
@@ -130,13 +132,39 @@ struct FunctionFlow
130132
class CFG: private ASTConstVisitor
131133
{
132134
public:
135+
struct FunctionContractTuple
136+
{
137+
ContractDefinition const* contract = nullptr;
138+
FunctionDefinition const* function = nullptr;
139+
140+
// Use AST ids for comparison to keep a deterministic order in the
141+
// containers using this struct
142+
bool operator<(FunctionContractTuple const& _other) const
143+
{
144+
if (contract && _other.contract)
145+
if (contract->id() != _other.contract->id())
146+
return contract->id() < _other.contract->id();
147+
148+
return function->id() < _other.function->id();
149+
}
150+
};
133151
explicit CFG(langutil::ErrorReporter& _errorReporter): m_errorReporter(_errorReporter) {}
134152

135153
bool constructFlow(ASTNode const& _astRoot);
136154

137155
bool visit(FunctionDefinition const& _function) override;
156+
bool visit(ContractDefinition const& _contract) override;
157+
158+
/// Get the function flow for the given function, using `_contract` as the
159+
/// most derived contract
160+
/// @param _function function to find the function flow for
161+
/// @param _contract most derived contract or nullptr for free functions
162+
FunctionFlow const& functionFlow(FunctionDefinition const& _function, ContractDefinition const* _contract = nullptr) const;
138163

139-
FunctionFlow const& functionFlow(FunctionDefinition const& _function) const;
164+
std::map<FunctionContractTuple, std::unique_ptr<FunctionFlow>> const& allFunctionFlows() const
165+
{
166+
return m_functionControlFlow;
167+
}
140168

141169
class NodeContainer
142170
{
@@ -153,7 +181,7 @@ class CFG: private ASTConstVisitor
153181
/// are owned by the CFG class and stored in this container.
154182
NodeContainer m_nodeContainer;
155183

156-
std::map<FunctionDefinition const*, std::unique_ptr<FunctionFlow>> m_functionControlFlow;
184+
std::map<FunctionContractTuple, std::unique_ptr<FunctionFlow>> m_functionControlFlow;
157185
};
158186

159187
}

0 commit comments

Comments
 (0)