Function Call Graph v2 #10973
Function Call Graph v2 #10973
Conversation
cameel
force-pushed
the
function-call-graph-v2
branch
2 times, most recently
from
b526921 to
11ef3a7
Compare
| std::set<FunctionDefinition const*> m_creationFunctionList; | ||
| std::set<FunctionDefinition const*> m_deployedFunctionList; |
There was a problem hiding this comment.
Wouldn't it be better to keep generated functions in the IRGenerationContext and retrieve the set from there?
I wanted to change this but one problem is that generate() resets the context when it switches from generating construction code to generating deployment code. It would be possible to preserve it but having resetContext() not reset the context fully does not seem right to me.
But apart from that - yeah, I think that having it in the context would make this nicer. Currently IRGenerator fills the list and then destroys it if you call verifyGraphs(). But if you don't call it, the list just stays there and could be reused (I added assertions against that).
There was a problem hiding this comment.
Discussion also here: #10973 (comment)
| // assigned to state variables and as such may be reachable after deployment as well. | ||
| builder.m_currentNode = SpecialNode::InternalDispatch; | ||
| set<Node, CompareByID> emptySet; | ||
| for (Node const& dispatchTarget: valueOrDefault(_creationGraph.edges, SpecialNode::InternalDispatch, emptySet)) |
There was a problem hiding this comment.
Why not?
| for (Node const& dispatchTarget: valueOrDefault(_creationGraph.edges, SpecialNode::InternalDispatch, emptySet)) | |
| for (Node const& dispatchTarget: valueOrDefault(_creationGraph.edges, SpecialNode::InternalDispatch, set<Node, CompareByID>{})) |
This has the advantage that the variable cannot be referenced later on.
There was a problem hiding this comment.
It does seem to work for me (even though it returns a temporary) and was my preferred solution but @bshastry reported that asan complains about stack-use-after-scope.
There was a problem hiding this comment.
That is weird - isn't its lifetime extended to the end of the range-based for loop?
There was a problem hiding this comment.
Might be a false-positive due to the fact that I am passing dispatchTarget into a function which does actually stored it. It's safe only because the set is empty and asan might not be accounting for that.
Should I just use the temporary and ignore the potential warning? The old version wasn't failing in CI so it would work.
There are also two other uses of valueOrDefault() which @bshastry did not point out so I guess there were no warnings for them. I changed them along with this one but if we think it's safe, I could change back at least those two places.
There was a problem hiding this comment.
I have just pushed two fixups that restore the temporaries.
Oh, and it was actually one other place that uses valueOrDefault(), not two. Looks like removing the BFS eliminated the one in IRGenerator.cpp.
| string IRGenerator::generate( | ||
| ContractDefinition const& _contract, | ||
| map<ContractDefinition const*, string_view const> const& _otherYulSources | ||
| ) | ||
| { | ||
| // Remember to call verifyCallGraphs() (which clears the list of generated functions) if you | ||
| // want to reuse the generator. | ||
| solAssert(m_creationFunctionList.empty(), ""); |
There was a problem hiding this comment.
I'm pretty sure these can be local variables in this function - can't they?
There was a problem hiding this comment.
They only reason they're not is that they're used by verifyCallGraphs() which gets called separately by CompilerStack.
We could make them local if we made generate() call verifyCallGraphs() automatically. We would then also have to pass the graphs from CompilerStack either via generate() or IRGenerator constructor or the context.
There was a problem hiding this comment.
If they are in the annotations, then we can just call it here, can't we? It's not too elegant to call that function from the compilerstack anyway, I think.
There was a problem hiding this comment.
Sure. But as I said, I'll be making a separate PR to change them to annotations. In that case I'll also change this to use locals that that other PR.
cameel
force-pushed
the
function-call-graph-v2
branch
from
f024d07 to
0d46edb
Compare
… to use destructuring
- This should make it easier to realize that one of the analysis phases has not been executed.
|
Please squash so it can be merged. |
cameel
force-pushed
the
function-call-graph-v2
branch
from
4767bc6 to
e9b527d
Compare
cameel
force-pushed
the
function-call-graph-v2
branch
from
e9b527d to
051995a
Compare
|
Squashed. I have also added a small change in the builder docstring to account for the fact that after my changes it's not only the bottom-most constructor that has an edge from |
| // All functions present in internal dispatch at creation time could potentially be pointers | ||
| // assigned to state variables and as such may be reachable after deployment as well. | ||
| builder.m_currentNode = SpecialNode::InternalDispatch; | ||
| for (Node const& dispatchTarget: valueOrDefault(_creationGraph.edges, SpecialNode::InternalDispatch, {})) |
There was a problem hiding this comment.
iiuc life-time extension of dispatchTarget happens until the loop lives. But is the rvalue temporary {} live until it is bound to the range variable?
There was a problem hiding this comment.
Why wouldn't it be? I might be wrong but I think this is just a false-positive (#10973 (comment)).
BTW, do you get warnings about both uses of valueOrDefault() or just this one?
There was a problem hiding this comment.
I will take a closer look and post an update.
There was a problem hiding this comment.
Update posted here: https://gist.github.com/bshastry/16bf461806a56fd2bcb8499a4e7b0151
There was a problem hiding this comment.
Well, I'll need to read up a bit on the rules for lifetime extension for temporaries. For now I just found an SO question on a similar topic that indicates that this indeed might be a problem (Is it safe to use a C++11 range-based for-loop with an rvalue range-init?). The example cited there is safe but in our case I think that this condition for lifetime extension is not satisfied:
The temporary to which the reference is bound or the temporary that is the complete object of a subobject to which the reference is bound persists for the lifetime of the reference except:
- (...)
- A temporary bound to a reference parameter in a function call (...)
I remember that @ekpyron had to try hard to even make this function work and I'm not sure if he expected that the case where the default is not a simple value would be relevant :) I wonder if the function could be reworked to account for that...
There was a problem hiding this comment.
Yeah, that function needs to be debugged for complex objects like sets of structs or something to evaluate lifetime of temporary. Temporary fix would be to bind the default param to a value on the stack e.g., local variable of the same type.
This PR replaces #10332.
I fixed the problems I listed in #10332 (comment) and I think I have implemented all the changes that actually affect what's in the graph.
I'm still not done with some refactors suggested in the old PR like using a queue when visiting nodes adjusting some names or changing the way checks are done. I'm pushing what I have now as a draft but these changes are pretty small so I think I'll be done. The PR should at least already be correct and pass all tests.The PR is now ready.