-
Notifications
You must be signed in to change notification settings - Fork 275
Goto analyzer 5 part3 #966
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure about all concerns, so I'm not making this quite as firm as "request changes".
src/analyses/ai.h
Outdated
virtual bool is_bottom() const=0; | ||
|
||
// the analysis doesn't use this, | ||
// and domains may refuse to implement it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment just tells us why it should not be here. Maybe also explain why it is being added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... I'm going to remove the comment because we're not using is_top() right now but ... it's the kind of thing we may well start using. As there is a make_top() method the domain must have a representation of it so it shouldn't be too great a burden on the programmer.
src/analyses/constant_propagator.cpp
Outdated
// values | ||
const constant_propagator_ait *cp= | ||
dynamic_cast<constant_propagator_ait *>(&ai); | ||
bool have_dirty=cp!=NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use nullptr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
else | ||
two_way_propagate_rec(g, ns); | ||
two_way_propagate_rec(g, ns); | ||
assert(!values.is_bottom); // for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it says "for now": what will happen later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question... @danpoe? @lucasccordeiro? I don't think I wrote this.
I can give a very meta-answer; this will be deprecated in favour of the variable precision domain configured to handle constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the two way propagation (which handles the variable propagation through gotos, e.g. [a == 3] goto ...) is disabled in this commit (the body of two_way_propagate_rec()) is commented out). Therefore propagating through the goto can never yield bottom which the assertion checks for. When two way propagation is enabled again in the future the assertion will not hold anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added a comment saying this.
src/analyses/constant_propagator.h
Outdated
@@ -106,28 +150,32 @@ class constant_propagator_domaint:public ai_domain_baset | |||
class constant_propagator_ait:public ait<constant_propagator_domaint> | |||
{ | |||
public: | |||
explicit constant_propagator_ait(const goto_functionst &goto_functions) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: no space in front of ":" I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/analyses/constant_propagator.h
Outdated
constant_propagator_ait( | ||
goto_functionst &goto_functions, | ||
const namespacet &ns) | ||
const namespacet &ns) : dirty(goto_functions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above: no space around ":"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/util/replace_symbol.cpp
Outdated
if(!replace(ie.array(), replace_with_const)) | ||
result=false; | ||
|
||
if(!replace(ie.index())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it not ok to replace the index by a constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure what you mean but it seems (from the diff and context -- please confirm @danpoe) that the replace_with_const flag is to ensure that an l-value is still a valid l-value after substitution. For example if your expression is "a[i]" then substituting i for a constant results in an l-value but substituting a for a constant (array) wouldn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martin-cs: Exactly.
@tautschnig: The index is always replaced here. The if
is there to check the return value of replace()
which indicates whether the expression was simplified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have documented what the flag does and why it is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I just got confused by the fact the default value is true
.
src/util/replace_symbol.cpp
Outdated
dest=it->second; | ||
const exprt &e=it->second; | ||
|
||
if(!replace_with_const && e.is_constant()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test could be placed even before the to_symbol_expr
to spare some unnecessary lookups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it works because you need to to_symbol_expr to get the symbol to look up in the substitution table, which gives you e (what you are substituting) and you need that for the condition (because it is only an issue if you are replacing with a constant).
else | ||
{ | ||
Forall_operands(it, dest) | ||
if(!replace(*it)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not forward the second parameter here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed as it's only the "outermost" address-of, index, member, etc. that you need to catch. The logic is similar to:
https://github.com/diffblue/cbmc/blob/master/src/analyses/ai.cpp#L83
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I now understand why I have once been told that default values should not be used. (Yes, I do also use them, but reading code with limited context is a lot harder when default values are used.)
I've had a look through the diff and it all looks good to me. |
45c564f
to
7c0f038
Compare
I have addressed all comments and pushed an updated version. |
7c0f038
to
aad0591
Compare
Rebased to deal with documentation changes. |
aad0591
to
5606ce5
Compare
|
||
if(r_it != replace_const.expr_map.end()) | ||
Function: constant_propagator_domaint::valuest::set_dirty_to_top |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapt to doxygen comments.
@@ -38,7 +38,21 @@ class replace_symbolt | |||
type_map.insert(std::pair<irep_idt, typet>(identifier, type)); | |||
} | |||
|
|||
virtual bool replace(exprt &dest) const; | |||
/* If you are replacing symbols with constants in an l-value, you can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapt to doxygen comments.
out << " bottom\n"; | ||
assert(replace_const.expr_map.empty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use INVARIANT etc (whole PR)
const namespacet &ns) const override; | ||
const namespacet &ns) const final override; | ||
|
||
virtual void make_bottom() final override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtual
not needed (and other functions below)
{ | ||
operator()(goto_function, ns); | ||
replace(goto_function, ns); | ||
} | ||
|
||
dirtyt dirty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should this be public?
Obsoleted by #1475 |
The next part of the goto-analyzer grand merge. This fixes a variety of issues with the constant domain and adds a few utility functions needed to improve ait.
Problems:
*. There are test cases that show why these are needed but they are intertwined with a number of other changes (what will become part4!) and the association between test case and fix has been lost.
Mitigations:
*. This doesn't break any of the existing test cases.
*. The constant domain isn't in heavy use.
*. When we start merging in the variable sensitivity domain ( #708 dependent on this / #472 ) we will have a much better constant propagator, one that is actually correct with respect to aliasing and indirection, for example. I hope this will become the default constant domain so these changes might become largely a legacy issue.
If it would make more sense, I can squash together the constant domain fixes.