Control flow side effects of user defined functions #12082

chriseth · 2021-10-04T10:11:20Z

No description provided.

chriseth · 2021-10-04T16:26:56Z

This is working now apart from recursive function calls. I'm actually wondering if it would be better to finish implementing ControlFlowBuilder on the full Yul AST and then doing reachability analysis there.

chriseth · 2021-10-04T16:27:55Z

Ok actually I think ControlFlowBuilder cannot be really used, but essentially something that actually builds a graph.

chriseth · 2021-10-05T15:38:42Z

libyul/backends/evm/ControlFlowGraphBuilder.cpp

 	//       not only for builtins.
 	if (auto const* funCall = get_if<FunctionCall>(&_exprStmt.expression))
 		if (BuiltinFunction const* builtin = m_dialect.builtin(funCall->functionName.name))
-			if (builtin->controlFlowSideEffects.terminates)


@ekpyron the above comment can now be implemented.

stackenbotten · 2021-10-05T17:00:30Z

There was an error when running chk_coding_style for commit d5a397a680fdefabefe23289dfdb9ed0084f1a01:

Error: Trailing whitespace found:
test/libyul/controlFlowSideEffects/recursion.yul:32:// c: 
test/libyul/controlFlowSideEffects/recursion.yul:33:// d: 
test/libyul/controlFlowSideEffects/recursion.yul:35:// x: 
test/libyul/controlFlowSideEffects/recursion.yul:36:// y: 
test/libyul/controlFlowSideEffects/recursion.yul:37:// z:

Please check that your changes are working as intended.

hrkrshnn · 2021-10-07T12:36:15Z

libyul/ControlFlowSideEffects.h

+#include <stack>
+#include <optional>


All these includes are unnecessary, aren't they? Even set.

hrkrshnn · 2021-10-07T12:50:42Z

libyul/ControlFlowSideEffectsCollector.cpp

+{
+	walkVector(_functionCall.arguments | ranges::views::reverse);
+	newConnectedNode();
+	m_currentNode->functionCall = {_functionCall.functionName.name};


Suggested change

m_currentNode->functionCall = {_functionCall.functionName.name};

m_currentNode->functionCall = _functionCall.functionName.name;

The braces are a bit misleading for the optional type.

hrkrshnn · 2021-10-07T14:17:26Z

libyul/ControlFlowSideEffectsCollector.cpp

+	ScopedSaveAndRestore breakNode(m_break, nullptr);
+	ScopedSaveAndRestore continueNode(m_continue, nullptr);


Are these necessary?

Maybe not if we assume the hoister, but better safe than sorry.

Yeah, okay. We could assert that m_continue and m_break are nullptr if we really want to be extra safe. Same with m_leave and m_break.

I think we should either assert or save and restore. What would you prefer?

I would prefer the assert.

hrkrshnn · 2021-10-07T14:18:44Z

libyul/ControlFlowSideEffectsCollector.h

+struct ControlFlowNode
+{
+	std::vector<ControlFlowNode const*> successors;
+	std::optional<YulString> functionCall;
+};
+
+struct FunctionFlow
+{
+	ControlFlowNode const* entry;
+	ControlFlowNode const* exit;
+};


This could use some documentation.

hrkrshnn · 2021-10-07T15:24:15Z

libyul/ControlFlowSideEffects.h

+	/// If true, the function contains at least one reachable branch that reverts.
+	bool canRevert = false;
+	/// If true, the function has a regular outgoing control-flow.
+	bool canContinue = true;


I think this can be replaced by willTerminate and that sounds more appropriate.

I'm not sure. These three bools are essentially edges to special nodes in the control-flow graph, and I would prefer not to invert the meaning of one of them.

Also canContinue can be false due to the function being always recursive.

Also canContinue can be false due to the function being always recursive.

This is not currently done, is it?

https://github.com/ethereum/solidity/pull/12082/files#diff-206aa4eb462f2802b4a49004b969fa9ac56299142ff16692fc99eb8bfec42acbR182

hrkrshnn · 2021-10-07T15:34:42Z

libyul/ControlFlowSideEffectsCollector.cpp

+	while (it != end)
+	{
+		ControlFlowNode const* node = *it;
+		if (!node->functionCall || exitKnownReachable(*node->functionCall))
+		{
+			m_pendingNodes[_functionName].erase(it);
+			return node;
+		}
+		++it;
+	}


This can be replaced by std::find, followed by an erase.

Like this? Would you prefer it?

auto it = m_pendingNodes[_functionName].begin(); auto end = m_pendingNodes[_functionName].end(); it = find_if(it, end, [](ControlFlowNode const* node) { return !node->functionCall || exitKnownReachable(*node->functionCall); } if (it == end) return nullptr; else { ControlFlowNode const* node = *it; m_pendingNodes[_functionName].erase(it); return node; }

Yes :) But it did look better when I thought about the change.

libyul/ControlFlowSideEffectsCollector.cpp

libyul/backends/evm/EVMDialect.cpp

hrkrshnn · 2021-10-13T14:19:10Z

libyul/ControlFlowSideEffectsCollector.cpp

+	ScopedSaveAndRestore breakNode(m_break, nullptr);
+	ScopedSaveAndRestore continueNode(m_continue, nullptr);


Yeah, okay. We could assert that m_continue and m_break are nullptr if we really want to be extra safe. Same with m_leave and m_break.

hrkrshnn · 2021-10-13T14:22:14Z

libyul/ControlFlowSideEffectsCollector.cpp

+	flow.exit = newNode();
+	m_currentNode = newNode();
+	flow.entry = m_currentNode;
+	newConnectedNode();


I think this might be redundant.

I think you are right.

libyul/ControlFlowSideEffectsCollector.cpp

hrkrshnn · 2021-10-13T15:13:39Z

libyul/ControlFlowSideEffectsCollector.cpp

+void ControlFlowBuilder::operator()(Leave const&)
+{
+	m_currentNode->successors.emplace_back(m_leave);
+	m_currentNode = newNode();


I wonder if this is really needed. Effectively, edges coming from here doesn't matter, I think.

I think we discussed this offline but if you don't disconnect the node, then function f() { leave revert(0, 0) } will have can revert. With dead code removal this might not matter, but I think it is still good to have.

libyul/ControlFlowSideEffectsCollector.cpp

hrkrshnn · 2021-10-14T10:22:56Z

libyul/ControlFlowSideEffectsCollector.cpp

+	while (progress)
+	{
+		progress = false;
+		for (auto const& fun: m_pendingNodes)


This may lead to undefined behaviour. The call procesFunction(...) can update m_pendingNodes in two different ways:

nextProcessableNode(...) can remove the current iterator; wouldn't that lead to undefined behaviour when the current one gets incremented?

recordReachabilityAndQueue can add to the beginning of m_pendingNodes. Why is it guaranteed that this node will get processed?

Would it be more appropriate to do something like while(!m_pendingNodes.empty()) { node = m_pendingNodes.front(); ...}

Also, otherwise could change to m_pendingNodes | range::views::keys)

I'm not sure I follow, I think this might be a misunderstanding on your part: m_pendingNodes is a map by function name - so there is one queue per function name. As long as we don't add or remove a function, the iterator here is not invalidated. I can split recordReachabilityAndQueue into two functions:

one that can also add a new function

one that asserts that the function is already there
and use the first one above in line 171 and the second one in line 232

Thanks for the change. Was a misunderstanding.

hrkrshnn · 2021-10-14T11:32:28Z

libyul/ControlFlowSideEffectsCollector.cpp

+			if (m_functionCalls.count(_function))
+				for (YulString callee: m_functionCalls.at(_function))


We could replace the .count followed by .at by valueOrDefault from CommonData.h

hrkrshnn · 2021-10-14T11:40:05Z

libyul/ControlFlowSideEffectsCollector.h

+	std::map<YulString, ControlFlowSideEffects> m_functionSideEffects;
+	std::map<YulString, std::list<ControlFlowNode const*>> m_pendingNodes;
+	std::map<YulString, std::set<ControlFlowNode const*>> m_processedNodes;
+	std::map<YulString, std::set<YulString>> m_functionCalls;


This could use documentation. Wasn't obvious that this is a set of reachable function calls inside a function.

hrkrshnn · 2021-10-14T15:41:29Z

Some commits are left for squashing, can be merged afterwards.

chriseth force-pushed the controlFlowSideEffectsUserDefined branch from d908dd9 to 6a6ed4e Compare October 5, 2021 15:32

argotorg deleted a comment from stackenbotten Oct 5, 2021

chriseth commented Oct 5, 2021

View reviewed changes

chriseth marked this pull request as ready for review October 5, 2021 16:47

chriseth force-pushed the controlFlowSideEffectsUserDefined branch from 6a6ed4e to d5a397a Compare October 5, 2021 16:59

chriseth force-pushed the controlFlowSideEffectsUserDefined branch from d5a397a to 6220621 Compare October 5, 2021 17:02

chriseth requested review from hrkrshnn and leonardoalt October 5, 2021 17:03

chriseth force-pushed the controlFlowSideEffectsUserDefined branch from 6220621 to a1756ae Compare October 6, 2021 15:07

hrkrshnn reviewed Oct 7, 2021

View reviewed changes

chriseth force-pushed the controlFlowSideEffectsUserDefined branch 6 times, most recently from 179ca43 to 35b5a68 Compare October 13, 2021 15:39

hrkrshnn reviewed Oct 13, 2021

View reviewed changes

hrkrshnn reviewed Oct 14, 2021

View reviewed changes

chriseth force-pushed the controlFlowSideEffectsUserDefined branch from c2bd1d2 to dc4e951 Compare October 14, 2021 14:59

hrkrshnn previously approved these changes Oct 14, 2021

View reviewed changes

chriseth added 2 commits October 14, 2021 17:46

Control flow side-effects for user-defined functions.

9417d67

Tests

2c2269d

chriseth dismissed hrkrshnn’s stale review via 2c2269d October 14, 2021 15:46

chriseth force-pushed the controlFlowSideEffectsUserDefined branch from 95b68f0 to 2c2269d Compare October 14, 2021 15:46

hrkrshnn approved these changes Oct 14, 2021

View reviewed changes

chriseth enabled auto-merge October 14, 2021 16:12

chriseth merged commit 1e630fc into develop Oct 14, 2021

chriseth deleted the controlFlowSideEffectsUserDefined branch October 14, 2021 16:28

	m_currentNode->functionCall = {_functionCall.functionName.name};
	m_currentNode->functionCall = _functionCall.functionName.name;

		ScopedSaveAndRestore breakNode(m_break, nullptr);
		ScopedSaveAndRestore continueNode(m_continue, nullptr);

		if (m_functionCalls.count(_function))
		for (YulString callee: m_functionCalls.at(_function))

		#include <stack>
		#include <optional>

Control flow side effects of user defined functions #12082

Control flow side effects of user defined functions #12082

Uh oh!

Conversation

chriseth commented Oct 4, 2021

Uh oh!

chriseth commented Oct 4, 2021

Uh oh!

chriseth commented Oct 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stackenbotten commented Oct 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hrkrshnn commented Oct 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants