baloghadamsoftware added inline comments.
================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:423
+
+void IteratorPastEndChecker::handleComparison(CheckerContext &C,
+ const SVal &LVal,
----------------
NoQ wrote:
> a.sidorin wrote:
> > What will happen if we write something like this:
> > ```
> > bool Eq1 = it1 == it2;
> > bool Eq2 = it3 == it4;
> > if (Eq1) {...}?
> > ```
> >
> > As I understand, we'll assume the second condition instead of first.
> Had a look. So the problem is, we obtain the result of the comparison as a
> symbol, from which it is too hard to recover the operands in order to move
> iterator position data from one value to another.
>
> Normally we obtain a simple SymbolConjured for the return value of the
> `operator==()` call (or, similarly, `operator!=()`). For plain-value
> iterators (eg. `typedef T *iterator`) we might be obtaining an actual binary
> symbolic expression, but even then it's not quite clear how to obtain
> operands (the structure of the expression might have changed due to algebraic
> simplifications). Additionally, LHS and RHS aren't necessarily symbols (they
> might be semi-concrete), so composing symbolic expressions from them in
> general is problematic with our symbol hierarchy, which is rarely a problem
> for numbers but for structural symbols it'd be a mess.
>
> For now i suggest, instead of storing only the last LHS and RHS, to save a
> map from symbols (which are results of comparisons) to (LHS value, RHS value)
> pairs. This map should, apart from the obvious, be cleaned up whenever one of
> the iterators in the pair gets mutated (incremented or decremented). This
> should take care of the problem Alexey points out, and should work with
> semi-concrete stuff.
>
> For the future i suggest to let users construct their own symbols and
> symbolic expressions more easily. In fact, if only we had all iterators as
> regions, we should have probably used SymbolMetadata for this purpose: it's
> easy to both recover the parent region from it and use it in symbolic
> expressions. We could also deprecate the confusing structural symbols
> (provide default-bound lazy compound values for conjured structures instead),
> and then it'd be possible to transition to SymbolMetadata entirely.
Thank you for the suggestion. I am not sure if I fully understand you. If I
create a map where the key is the resulting symbol of the comparison, it will
not work because evalAssume is called for the innermost comparison. So if the
body of operator== (or operator!=) is inlined, then I get a binary symbolic
expression in evalAssume, not the SymbolConjured. This binary Symbolic
expression is a comparison of the internals of the iterators, e.g. the internal
pointer. So the key will not match any LHS and RHS value pair in the map. I
also thought on such solution earlier but I dismissed it because of this.
================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:580
+ C.addTransition(stateFound);
+ C.addTransition(stateNotFound);
+}
----------------
NoQ wrote:
> NoQ wrote:
> > Ouch, i have one more concern, which can be expressed with the following
> > false-positive test which currently fails:
> >
> > ```
> > void foo() {
> > std::vector<int> vec;
> > vec.push_back(2016);
> > auto i = vec.find(vec.begin(), vec.end(), 2016);
> > *i; // no-warning
> > }
> > ```
> >
> > Not instantly sure what to do with this. You can avoid state splits until
> > you are actually sure if both branches are possible, but that'd suppress a
> > lot of useful positives. Such positives could be suppressed with
> > assertions, of course, but i'd still hope there aren't too many of those.
> I mean, `std::find(...` ><
False positives can occur whenever we are sure that we will find the element so
we do not check for the result to be equal with end().
https://reviews.llvm.org/D25660
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
