[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker
baloghadamsoftware abandoned this revision. baloghadamsoftware added a comment. Whole checker superseeded by https://reviews.llvm.org/D31975. https://reviews.llvm.org/D28771 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker
NoQ added a comment. Hello, Because i couldn't reproduce the loc-nonloc problem on my standard library, could you share a preprocessed file with the issue? I'm really curious at what's going on here, and it's the only issue remaining, so maybe we could squash it together. https://reviews.llvm.org/D28771 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530
+ auto value = RVal;
+ if (auto loc = value.getAs()) {
+value = State->getRawSVal(*loc);
NoQ wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > baloghadamsoftware wrote:
> > > > NoQ wrote:
> > > > > baloghadamsoftware wrote:
> > > > > > NoQ wrote:
> > > > > > > Is there a test case for this hack?
> > > > > > >
> > > > > > > I'd also consider inspecting the AST (probably before passing the
> > > > > > > values to `handleRandomIncrOrDecr()`) and making the decision
> > > > > > > based on that. Because even though this pattern ("if a value is a
> > > > > > > loc and we expect a nonloc, do an extra dereference") is present
> > > > > > > in many places in the analyzer, in most of these places it
> > > > > > > doesn't work correctly (what if we try to discriminate between
> > > > > > > `int*` and `int*&`?).
> > > > > > I just want to get the sign of the integer value (if it is
> > > > > > available). It turned out that I cannot do comparison between loc
> > > > > > and nonloc. (Strange, because I can do anything else). After I
> > > > > > created this hack, the Analyzer did not crash anymore on the
> > > > > > llvm/clang code.
> > > > > >
> > > > > > I do not fully understand what I should fix here and how? In this
> > > > > > particular place we expect some integer, thus no int* or int*&.
> > > > > Loc value, essentially, *is* a pointer or reference value. If you're
> > > > > getting a Loc, then your expectations of an integer are not met in
> > > > > the actual code. In this case you *want* to know why they are not
> > > > > met, otherwise you may avoid the crash, but do incorrect things and
> > > > > run into false positives. So i'd rather have this investigated
> > > > > carefully.
> > > > >
> > > > > You say that you are crashing otherwise - and then it should be
> > > > > trivial for you to attach a debugger and `dump()` the expression for
> > > > > which you expect to take the integer value, and see why it suddenly
> > > > > has a pointer type in a particular case. From that you'd easily see
> > > > > what to do.
> > > > >
> > > > > Also, crashes are often easy to auto-reduce using tools like
> > > > > `creduce`. Unlike false positives, which may turn into true positives
> > > > > during reduction.
> > > > >
> > > > > If you still don't see the reason why your workaround is necessary
> > > > > and what exactly it does, could you attach a preprocessed file and an
> > > > > analyzer runline for the crash, so that we could have a look together?
> > > > Just to be clear: I know why it crashes without the hack: I simply
> > > > cannot compare loc and nonloc. Since concrete 0 is nonloc I need
> > > > another nonloc. I suppose this happens if an integer reference is
> > > > passed to the operator +, +=, - or -=. So I thought that dereferencing
> > > > it by getting the raw SVal is the correct thing to do.
> > > Yep, in this case the correct thing to do would be to check AST types
> > > rather than SVal types. Eg.,
> > > ```
> > > if (Arg->getType()->isReferenceType())
> > > value = State->getRawSVal(*loc);
> > > ```
> > >
> > > (you might need to do it in the caller function, which still has access
> > > to the expressions)
> > >
> > > It is better this way because expectations are explicitly stated, and the
> > > assertion would still catch the situation when expectations are not met.
> > >
> > > Also, please still add a test case to cover this branch :)
> > I tried it and failed in std::vector::back(). It seems that the problem is
> > not the reference, but loc::ConcreteInt. I added a test case, but in our
> > mocked vector the integer 1 in *(end()-1) is nonloc::ConcreteInt, but in
> > the real one it is loc::ConcreteInt. I do not see why is there a
> > difference, neither do I know how could something be a location and a
> > concrete integer at once. What is loc::ConcreteInt and what to do with it?
> > What is loc::ConcreteInt and what to do with it?
>
> It is a concrete memory address. The null pointer, for example, or maybe a
> fixed magic pointer in some embedded driver code.
>
> Could you post an AST dump for the real `(end()-1)`on which you are failing?
> It might be that we end up looking at the other `operator-()` as in `(end() -
> begin())`, while iterators are implemented as pointers; no idea how that
> could be, but i'm suspecting something like that.
Also, dereferencing a `loc::ConcreteInt` loc (through `getSVal`/`getRawSVal`)
would always yield `UndefinedVal` value.
https://reviews.llvm.org/D28771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530
+ auto value = RVal;
+ if (auto loc = value.getAs()) {
+value = State->getRawSVal(*loc);
baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > baloghadamsoftware wrote:
> > > > > NoQ wrote:
> > > > > > Is there a test case for this hack?
> > > > > >
> > > > > > I'd also consider inspecting the AST (probably before passing the
> > > > > > values to `handleRandomIncrOrDecr()`) and making the decision based
> > > > > > on that. Because even though this pattern ("if a value is a loc and
> > > > > > we expect a nonloc, do an extra dereference") is present in many
> > > > > > places in the analyzer, in most of these places it doesn't work
> > > > > > correctly (what if we try to discriminate between `int*` and
> > > > > > `int*&`?).
> > > > > I just want to get the sign of the integer value (if it is
> > > > > available). It turned out that I cannot do comparison between loc and
> > > > > nonloc. (Strange, because I can do anything else). After I created
> > > > > this hack, the Analyzer did not crash anymore on the llvm/clang code.
> > > > >
> > > > > I do not fully understand what I should fix here and how? In this
> > > > > particular place we expect some integer, thus no int* or int*&.
> > > > Loc value, essentially, *is* a pointer or reference value. If you're
> > > > getting a Loc, then your expectations of an integer are not met in the
> > > > actual code. In this case you *want* to know why they are not met,
> > > > otherwise you may avoid the crash, but do incorrect things and run into
> > > > false positives. So i'd rather have this investigated carefully.
> > > >
> > > > You say that you are crashing otherwise - and then it should be trivial
> > > > for you to attach a debugger and `dump()` the expression for which you
> > > > expect to take the integer value, and see why it suddenly has a pointer
> > > > type in a particular case. From that you'd easily see what to do.
> > > >
> > > > Also, crashes are often easy to auto-reduce using tools like `creduce`.
> > > > Unlike false positives, which may turn into true positives during
> > > > reduction.
> > > >
> > > > If you still don't see the reason why your workaround is necessary and
> > > > what exactly it does, could you attach a preprocessed file and an
> > > > analyzer runline for the crash, so that we could have a look together?
> > > Just to be clear: I know why it crashes without the hack: I simply cannot
> > > compare loc and nonloc. Since concrete 0 is nonloc I need another nonloc.
> > > I suppose this happens if an integer reference is passed to the operator
> > > +, +=, - or -=. So I thought that dereferencing it by getting the raw
> > > SVal is the correct thing to do.
> > Yep, in this case the correct thing to do would be to check AST types
> > rather than SVal types. Eg.,
> > ```
> > if (Arg->getType()->isReferenceType())
> > value = State->getRawSVal(*loc);
> > ```
> >
> > (you might need to do it in the caller function, which still has access to
> > the expressions)
> >
> > It is better this way because expectations are explicitly stated, and the
> > assertion would still catch the situation when expectations are not met.
> >
> > Also, please still add a test case to cover this branch :)
> I tried it and failed in std::vector::back(). It seems that the problem is
> not the reference, but loc::ConcreteInt. I added a test case, but in our
> mocked vector the integer 1 in *(end()-1) is nonloc::ConcreteInt, but in the
> real one it is loc::ConcreteInt. I do not see why is there a difference,
> neither do I know how could something be a location and a concrete integer at
> once. What is loc::ConcreteInt and what to do with it?
> What is loc::ConcreteInt and what to do with it?
It is a concrete memory address. The null pointer, for example, or maybe a
fixed magic pointer in some embedded driver code.
Could you post an AST dump for the real `(end()-1)`on which you are failing? It
might be that we end up looking at the other `operator-()` as in `(end() -
begin())`, while iterators are implemented as pointers; no idea how that could
be, but i'm suspecting something like that.
https://reviews.llvm.org/D28771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker
baloghadamsoftware added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530
+ auto value = RVal;
+ if (auto loc = value.getAs()) {
+value = State->getRawSVal(*loc);
NoQ wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > baloghadamsoftware wrote:
> > > > NoQ wrote:
> > > > > Is there a test case for this hack?
> > > > >
> > > > > I'd also consider inspecting the AST (probably before passing the
> > > > > values to `handleRandomIncrOrDecr()`) and making the decision based
> > > > > on that. Because even though this pattern ("if a value is a loc and
> > > > > we expect a nonloc, do an extra dereference") is present in many
> > > > > places in the analyzer, in most of these places it doesn't work
> > > > > correctly (what if we try to discriminate between `int*` and
> > > > > `int*&`?).
> > > > I just want to get the sign of the integer value (if it is available).
> > > > It turned out that I cannot do comparison between loc and nonloc.
> > > > (Strange, because I can do anything else). After I created this hack,
> > > > the Analyzer did not crash anymore on the llvm/clang code.
> > > >
> > > > I do not fully understand what I should fix here and how? In this
> > > > particular place we expect some integer, thus no int* or int*&.
> > > Loc value, essentially, *is* a pointer or reference value. If you're
> > > getting a Loc, then your expectations of an integer are not met in the
> > > actual code. In this case you *want* to know why they are not met,
> > > otherwise you may avoid the crash, but do incorrect things and run into
> > > false positives. So i'd rather have this investigated carefully.
> > >
> > > You say that you are crashing otherwise - and then it should be trivial
> > > for you to attach a debugger and `dump()` the expression for which you
> > > expect to take the integer value, and see why it suddenly has a pointer
> > > type in a particular case. From that you'd easily see what to do.
> > >
> > > Also, crashes are often easy to auto-reduce using tools like `creduce`.
> > > Unlike false positives, which may turn into true positives during
> > > reduction.
> > >
> > > If you still don't see the reason why your workaround is necessary and
> > > what exactly it does, could you attach a preprocessed file and an
> > > analyzer runline for the crash, so that we could have a look together?
> > Just to be clear: I know why it crashes without the hack: I simply cannot
> > compare loc and nonloc. Since concrete 0 is nonloc I need another nonloc. I
> > suppose this happens if an integer reference is passed to the operator +,
> > +=, - or -=. So I thought that dereferencing it by getting the raw SVal is
> > the correct thing to do.
> Yep, in this case the correct thing to do would be to check AST types rather
> than SVal types. Eg.,
> ```
> if (Arg->getType()->isReferenceType())
> value = State->getRawSVal(*loc);
> ```
>
> (you might need to do it in the caller function, which still has access to
> the expressions)
>
> It is better this way because expectations are explicitly stated, and the
> assertion would still catch the situation when expectations are not met.
>
> Also, please still add a test case to cover this branch :)
I tried it and failed in std::vector::back(). It seems that the problem is not
the reference, but loc::ConcreteInt. I added a test case, but in our mocked
vector the integer 1 in *(end()-1) is nonloc::ConcreteInt, but in the real one
it is loc::ConcreteInt. I do not see why is there a difference, neither do I
know how could something be a location and a concrete integer at once. What is
loc::ConcreteInt and what to do with it?
https://reviews.llvm.org/D28771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker
baloghadamsoftware updated this revision to Diff 89211.
baloghadamsoftware added a comment.
checkBind replaces checking of DeclStmt, CXXConstructExpr and assignment
operators. Incremention by 0 is not a bug anymore regardless the position of
the iterator.
https://reviews.llvm.org/D28771
Files:
lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp
test/Analysis/Inputs/system-header-simulator-cxx.h
test/Analysis/diagnostics/explicit-suppression.cpp
test/Analysis/iterator-past-end.cpp
Index: test/Analysis/iterator-past-end.cpp
===
--- test/Analysis/iterator-past-end.cpp
+++ test/Analysis/iterator-past-end.cpp
@@ -203,3 +203,50 @@
start = ++item; // no-warning
}
}
+
+void good_overwrite(std::vector ) {
+ auto i = vec.end();
+ i = vec.begin();
+ *i; // no-warning
+}
+
+void good_overwrite_find(std::vector , int e) {
+ auto i = std::find(vec.begin(), vec.end(), e);
+ if(i == vec.end()) {
+i = vec.begin();
+ }
+ *i; // no-warning
+}
+
+void bad_overwrite(std::vector ) {
+ auto i = vec.begin();
+ i = vec.end();
+ *i; // expected-warning{{Iterator accessed past its end}}
+}
+
+void bad_overwrite_find(std::vector , int e) {
+ auto i = std::find(vec.begin(), vec.end(), e);
+ if(i != vec.end()) {
+i = vec.begin();
+ }
+ *i; // expected-warning{{Iterator accessed past its end}}
+}
+
+void good_advance(std::vector ) {
+ auto i = vec.end();
+ std::advance(i, -1);
+ *i; // no-warning
+}
+
+void good_prev(std::vector ) {
+ auto i = std::prev(vec.end());
+ *i; // no-warning
+}
+
+void front(const std::vector ) {
+ vec.front();
+}
+
+void back(const std::vector ) {
+ vec.back();
+}
Index: test/Analysis/diagnostics/explicit-suppression.cpp
===
--- test/Analysis/diagnostics/explicit-suppression.cpp
+++ test/Analysis/diagnostics/explicit-suppression.cpp
@@ -18,6 +18,6 @@
void testCopyNull(C *I, C *E) {
std::copy(I, E, (C *)0);
#ifndef SUPPRESSED
- // expected-warning@../Inputs/system-header-simulator-cxx.h:191 {{Called C++ object pointer is null}}
+ // expected-warning@../Inputs/system-header-simulator-cxx.h:303 {{Called C++ object pointer is null}}
#endif
}
Index: test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- test/Analysis/Inputs/system-header-simulator-cxx.h
+++ test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -8,18 +8,60 @@
typedef unsigned char uint8_t;
typedef __typeof__(sizeof(int)) size_t;
+typedef __typeof__((char*)0-(char*)0) ptrdiff_t;
void *memmove(void *s1, const void *s2, size_t n);
-template struct __iterator {
- typedef __iterator iterator;
- typedef __iterator const_iterator;
+namespace std {
+ struct input_iterator_tag { };
+ struct output_iterator_tag { };
+ struct forward_iterator_tag : public input_iterator_tag { };
+ struct bidirectional_iterator_tag : public forward_iterator_tag { };
+ struct random_access_iterator_tag : public bidirectional_iterator_tag { };
- __iterator(const Ptr p) : ptr(p) {}
+ template struct iterator_traits {
+typedef typename Iterator::difference_type difference_type;
+typedef typename Iterator::value_type value_type;
+typedef typename Iterator::pointer pointer;
+typedef typename Iterator::reference reference;
+typedef typename Iterator::iterator_category iterator_category;
+ };
+}
+
+template struct __vector_iterator {
+ typedef __vector_iterator iterator;
+ typedef __vector_iterator const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::random_access_iterator_tag iterator_category;
+
+ __vector_iterator(const Ptr p) : ptr(p) {}
+ __vector_iterator operator++() { ++ ptr; return *this; }
+ __vector_iterator operator++(int) {
+auto tmp = *this;
+++ ptr;
+return tmp;
+ }
+ __vector_iterator operator--() { -- ptr; return *this; }
+ __vector_iterator operator--(int) {
+auto tmp = *this; -- ptr;
+return tmp;
+ }
+ __vector_iterator operator+(difference_type n) {
+return ptr + n;
+ }
+ __vector_iterator operator-(difference_type n) {
+return ptr - n;
+ }
+ __vector_iterator operator+=(difference_type n) {
+return ptr += n;
+ }
+ __vector_iterator operator-=(difference_type n) {
+return ptr -= n;
+ }
- __iterator operator++() { return *this; }
- __iterator operator++(int) { return *this; }
- __iterator operator--() { return *this; }
- __iterator operator--(int) { return *this; }
Ref operator*() const { return *ptr; }
Ptr operator->() const { return *ptr; }
@@ -33,7 +75,45 @@
Ptr ptr;
};
[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530
+ auto value = RVal;
+ if (auto loc = value.getAs()) {
+value = State->getRawSVal(*loc);
baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > Is there a test case for this hack?
> > > >
> > > > I'd also consider inspecting the AST (probably before passing the
> > > > values to `handleRandomIncrOrDecr()`) and making the decision based on
> > > > that. Because even though this pattern ("if a value is a loc and we
> > > > expect a nonloc, do an extra dereference") is present in many places in
> > > > the analyzer, in most of these places it doesn't work correctly (what
> > > > if we try to discriminate between `int*` and `int*&`?).
> > > I just want to get the sign of the integer value (if it is available). It
> > > turned out that I cannot do comparison between loc and nonloc. (Strange,
> > > because I can do anything else). After I created this hack, the Analyzer
> > > did not crash anymore on the llvm/clang code.
> > >
> > > I do not fully understand what I should fix here and how? In this
> > > particular place we expect some integer, thus no int* or int*&.
> > Loc value, essentially, *is* a pointer or reference value. If you're
> > getting a Loc, then your expectations of an integer are not met in the
> > actual code. In this case you *want* to know why they are not met,
> > otherwise you may avoid the crash, but do incorrect things and run into
> > false positives. So i'd rather have this investigated carefully.
> >
> > You say that you are crashing otherwise - and then it should be trivial for
> > you to attach a debugger and `dump()` the expression for which you expect
> > to take the integer value, and see why it suddenly has a pointer type in a
> > particular case. From that you'd easily see what to do.
> >
> > Also, crashes are often easy to auto-reduce using tools like `creduce`.
> > Unlike false positives, which may turn into true positives during reduction.
> >
> > If you still don't see the reason why your workaround is necessary and what
> > exactly it does, could you attach a preprocessed file and an analyzer
> > runline for the crash, so that we could have a look together?
> Just to be clear: I know why it crashes without the hack: I simply cannot
> compare loc and nonloc. Since concrete 0 is nonloc I need another nonloc. I
> suppose this happens if an integer reference is passed to the operator +, +=,
> - or -=. So I thought that dereferencing it by getting the raw SVal is the
> correct thing to do.
Yep, in this case the correct thing to do would be to check AST types rather
than SVal types. Eg.,
```
if (Arg->getType()->isReferenceType())
value = State->getRawSVal(*loc);
```
(you might need to do it in the caller function, which still has access to the
expressions)
It is better this way because expectations are explicitly stated, and the
assertion would still catch the situation when expectations are not met.
Also, please still add a test case to cover this branch :)
https://reviews.llvm.org/D28771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker
baloghadamsoftware added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530
+ auto value = RVal;
+ if (auto loc = value.getAs()) {
+value = State->getRawSVal(*loc);
NoQ wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > Is there a test case for this hack?
> > >
> > > I'd also consider inspecting the AST (probably before passing the values
> > > to `handleRandomIncrOrDecr()`) and making the decision based on that.
> > > Because even though this pattern ("if a value is a loc and we expect a
> > > nonloc, do an extra dereference") is present in many places in the
> > > analyzer, in most of these places it doesn't work correctly (what if we
> > > try to discriminate between `int*` and `int*&`?).
> > I just want to get the sign of the integer value (if it is available). It
> > turned out that I cannot do comparison between loc and nonloc. (Strange,
> > because I can do anything else). After I created this hack, the Analyzer
> > did not crash anymore on the llvm/clang code.
> >
> > I do not fully understand what I should fix here and how? In this
> > particular place we expect some integer, thus no int* or int*&.
> Loc value, essentially, *is* a pointer or reference value. If you're getting
> a Loc, then your expectations of an integer are not met in the actual code.
> In this case you *want* to know why they are not met, otherwise you may avoid
> the crash, but do incorrect things and run into false positives. So i'd
> rather have this investigated carefully.
>
> You say that you are crashing otherwise - and then it should be trivial for
> you to attach a debugger and `dump()` the expression for which you expect to
> take the integer value, and see why it suddenly has a pointer type in a
> particular case. From that you'd easily see what to do.
>
> Also, crashes are often easy to auto-reduce using tools like `creduce`.
> Unlike false positives, which may turn into true positives during reduction.
>
> If you still don't see the reason why your workaround is necessary and what
> exactly it does, could you attach a preprocessed file and an analyzer runline
> for the crash, so that we could have a look together?
Just to be clear: I know why it crashes without the hack: I simply cannot
compare loc and nonloc. Since concrete 0 is nonloc I need another nonloc. I
suppose this happens if an integer reference is passed to the operator +, +=, -
or -=. So I thought that dereferencing it by getting the raw SVal is the
correct thing to do.
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:553
+
+ // When increasing by positive or decreasing by negative an iterator past its
+ // end, then it is a bug. We check for bugs before the operator call.
NoQ wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > I think incrementing `end()` by `0` is not a bug (?)
> > I think it is not a bug, but how to solve it properly? If I chose just
> > greaterThanZero, then we have the same problem for decrementing end() by 0.
> > Is it worth to create three states here?
> Yep, i believe that indeed, we need three states here. There are three
> possible cases.
OK, I will do it.
https://reviews.llvm.org/D28771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530
+ auto value = RVal;
+ if (auto loc = value.getAs()) {
+value = State->getRawSVal(*loc);
baloghadamsoftware wrote:
> NoQ wrote:
> > Is there a test case for this hack?
> >
> > I'd also consider inspecting the AST (probably before passing the values to
> > `handleRandomIncrOrDecr()`) and making the decision based on that. Because
> > even though this pattern ("if a value is a loc and we expect a nonloc, do
> > an extra dereference") is present in many places in the analyzer, in most
> > of these places it doesn't work correctly (what if we try to discriminate
> > between `int*` and `int*&`?).
> I just want to get the sign of the integer value (if it is available). It
> turned out that I cannot do comparison between loc and nonloc. (Strange,
> because I can do anything else). After I created this hack, the Analyzer did
> not crash anymore on the llvm/clang code.
>
> I do not fully understand what I should fix here and how? In this particular
> place we expect some integer, thus no int* or int*&.
Loc value, essentially, *is* a pointer or reference value. If you're getting a
Loc, then your expectations of an integer are not met in the actual code. In
this case you *want* to know why they are not met, otherwise you may avoid the
crash, but do incorrect things and run into false positives. So i'd rather have
this investigated carefully.
You say that you are crashing otherwise - and then it should be trivial for you
to attach a debugger and `dump()` the expression for which you expect to take
the integer value, and see why it suddenly has a pointer type in a particular
case. From that you'd easily see what to do.
Also, crashes are often easy to auto-reduce using tools like `creduce`. Unlike
false positives, which may turn into true positives during reduction.
If you still don't see the reason why your workaround is necessary and what
exactly it does, could you attach a preprocessed file and an analyzer runline
for the crash, so that we could have a look together?
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:553
+
+ // When increasing by positive or decreasing by negative an iterator past its
+ // end, then it is a bug. We check for bugs before the operator call.
baloghadamsoftware wrote:
> NoQ wrote:
> > I think incrementing `end()` by `0` is not a bug (?)
> I think it is not a bug, but how to solve it properly? If I chose just
> greaterThanZero, then we have the same problem for decrementing end() by 0.
> Is it worth to create three states here?
Yep, i believe that indeed, we need three states here. There are three possible
cases.
https://reviews.llvm.org/D28771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker
baloghadamsoftware added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530
+ auto value = RVal;
+ if (auto loc = value.getAs()) {
+value = State->getRawSVal(*loc);
NoQ wrote:
> Is there a test case for this hack?
>
> I'd also consider inspecting the AST (probably before passing the values to
> `handleRandomIncrOrDecr()`) and making the decision based on that. Because
> even though this pattern ("if a value is a loc and we expect a nonloc, do an
> extra dereference") is present in many places in the analyzer, in most of
> these places it doesn't work correctly (what if we try to discriminate
> between `int*` and `int*&`?).
I just want to get the sign of the integer value (if it is available). It
turned out that I cannot do comparison between loc and nonloc. (Strange,
because I can do anything else). After I created this hack, the Analyzer did
not crash anymore on the llvm/clang code.
I do not fully understand what I should fix here and how? In this particular
place we expect some integer, thus no int* or int*&.
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:553
+
+ // When increasing by positive or decreasing by negative an iterator past its
+ // end, then it is a bug. We check for bugs before the operator call.
NoQ wrote:
> I think incrementing `end()` by `0` is not a bug (?)
I think it is not a bug, but how to solve it properly? If I chose just
greaterThanZero, then we have the same problem for decrementing end() by 0. Is
it worth to create three states here?
https://reviews.llvm.org/D28771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker
baloghadamsoftware updated this revision to Diff 86428.
baloghadamsoftware added a comment.
Updates based on the comments.
https://reviews.llvm.org/D28771
Files:
lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp
test/Analysis/Inputs/system-header-simulator-cxx.h
test/Analysis/diagnostics/explicit-suppression.cpp
test/Analysis/iterator-past-end.cpp
Index: test/Analysis/iterator-past-end.cpp
===
--- test/Analysis/iterator-past-end.cpp
+++ test/Analysis/iterator-past-end.cpp
@@ -203,3 +203,42 @@
start = ++item; // no-warning
}
}
+
+void good_overwrite(std::vector ) {
+ auto i = vec.end();
+ i = vec.begin();
+ *i; // no-warning
+}
+
+void good_overwrite_find(std::vector , int e) {
+ auto i = std::find(vec.begin(), vec.end(), e);
+ if(i == vec.end()) {
+i = vec.begin();
+ }
+ *i; // no-warning
+}
+
+void bad_overwrite(std::vector ) {
+ auto i = vec.begin();
+ i = vec.end();
+ *i; // expected-warning{{Iterator accessed past its end}}
+}
+
+void bad_overwrite_find(std::vector , int e) {
+ auto i = std::find(vec.begin(), vec.end(), e);
+ if(i != vec.end()) {
+i = vec.begin();
+ }
+ *i; // expected-warning{{Iterator accessed past its end}}
+}
+
+void good_advance(std::vector ) {
+ auto i = vec.end();
+ std::advance(i, -1);
+ *i; // no-warning
+}
+
+void good_prev(std::vector ) {
+ auto i = std::prev(vec.end());
+ *i; // no-warning
+}
Index: test/Analysis/diagnostics/explicit-suppression.cpp
===
--- test/Analysis/diagnostics/explicit-suppression.cpp
+++ test/Analysis/diagnostics/explicit-suppression.cpp
@@ -18,6 +18,6 @@
void testCopyNull(C *I, C *E) {
std::copy(I, E, (C *)0);
#ifndef SUPPRESSED
- // expected-warning@../Inputs/system-header-simulator-cxx.h:191 {{Called C++ object pointer is null}}
+ // expected-warning@../Inputs/system-header-simulator-cxx.h:293 {{Called C++ object pointer is null}}
#endif
}
Index: test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- test/Analysis/Inputs/system-header-simulator-cxx.h
+++ test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -8,18 +8,60 @@
typedef unsigned char uint8_t;
typedef __typeof__(sizeof(int)) size_t;
+typedef __typeof__((char*)0-(char*)0) ptrdiff_t;
void *memmove(void *s1, const void *s2, size_t n);
-template struct __iterator {
- typedef __iterator iterator;
- typedef __iterator const_iterator;
+namespace std {
+ struct input_iterator_tag { };
+ struct output_iterator_tag { };
+ struct forward_iterator_tag : public input_iterator_tag { };
+ struct bidirectional_iterator_tag : public forward_iterator_tag { };
+ struct random_access_iterator_tag : public bidirectional_iterator_tag { };
+
+ template struct iterator_traits {
+typedef typename Iterator::difference_type difference_type;
+typedef typename Iterator::value_type value_type;
+typedef typename Iterator::pointer pointer;
+typedef typename Iterator::reference reference;
+typedef typename Iterator::iterator_category iterator_category;
+ };
+}
- __iterator(const Ptr p) : ptr(p) {}
+template struct __vector_iterator {
+ typedef __vector_iterator iterator;
+ typedef __vector_iterator const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::random_access_iterator_tag iterator_category;
+
+ __vector_iterator(const Ptr p) : ptr(p) {}
+ __vector_iterator operator++() { ++ ptr; return *this; }
+ __vector_iterator operator++(int) {
+auto tmp = *this;
+++ ptr;
+return tmp;
+ }
+ __vector_iterator operator--() { -- ptr; return *this; }
+ __vector_iterator operator--(int) {
+auto tmp = *this; -- ptr;
+return tmp;
+ }
+ __vector_iterator operator+(difference_type n) {
+return ptr + n;
+ }
+ __vector_iterator operator-(difference_type n) {
+return ptr - n;
+ }
+ __vector_iterator operator+=(difference_type n) {
+return ptr += n;
+ }
+ __vector_iterator operator-=(difference_type n) {
+return ptr -= n;
+ }
- __iterator operator++() { return *this; }
- __iterator operator++(int) { return *this; }
- __iterator operator--() { return *this; }
- __iterator operator--(int) { return *this; }
Ref operator*() const { return *ptr; }
Ptr operator->() const { return *ptr; }
@@ -33,7 +75,45 @@
Ptr ptr;
};
+template struct __list_iterator {
+ typedef __vector_iterator iterator;
+ typedef __vector_iterator const_iterator;
+
+ typedef
[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker
NoQ added a comment.
Thanks for the update, most welcome!
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:234
+if (Call.getNumArgs() >= 1) {
+ const auto = static_cast(Call);
+ handleRandomIncrOrDecr(C, Func->getOverloadedOperator(),
We usually write these as
```
const auto *InstCall = cast();
```
Mostly because our custom `cast<>` has an assertion inside.
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:514
+CheckerContext , OverloadedOperatorKind Op, const SVal ,
+const SVal , const SVal , QualType RValType,
+bool PreCheck) const {
I'd suggest renaming the parameters to "LHS" and "RHS" or something like that,
so that not to confuse those with "lvalue" and "rvalue".
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530
+ auto value = RVal;
+ if (auto loc = value.getAs()) {
+value = State->getRawSVal(*loc);
Is there a test case for this hack?
I'd also consider inspecting the AST (probably before passing the values to
`handleRandomIncrOrDecr()`) and making the decision based on that. Because even
though this pattern ("if a value is a loc and we expect a nonloc, do an extra
dereference") is present in many places in the analyzer, in most of these
places it doesn't work correctly (what if we try to discriminate between `int*`
and `int*&`?).
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:553
+
+ // When increasing by positive or decreasing by negative an iterator past its
+ // end, then it is a bug. We check for bugs before the operator call.
I think incrementing `end()` by `0` is not a bug (?)
https://reviews.llvm.org/D28771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker
baloghadamsoftware created this revision.
baloghadamsoftware added reviewers: zaks.anna, NoQ.
baloghadamsoftware added subscribers: xazax.hun, o.gyorgy, cfe-commits.
This patch fixes some issues for the IteratorPastEnd checkers. There are
basically two main issues this patch targets: one is the handling of
assignments between iterators, the other one is correct handling of the +, +=,
- and -= operators of random-access iterators. The handling of these operators
also checks the sign of the argument, e.g. a negative number for + or += is
handled as decrementation.
https://reviews.llvm.org/D28771
Files:
lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp
test/Analysis/Inputs/system-header-simulator-cxx.h
test/Analysis/diagnostics/explicit-suppression.cpp
test/Analysis/iterator-past-end.cpp
Index: test/Analysis/iterator-past-end.cpp
===
--- test/Analysis/iterator-past-end.cpp
+++ test/Analysis/iterator-past-end.cpp
@@ -203,3 +203,49 @@
start = ++item; // no-warning
}
}
+
+void good_overwrite(std::vector ) {
+ auto i = vec.end();
+ i = vec.begin();
+ *i; // no-warning
+}
+
+void good_overwrite_find(std::vector , int e) {
+ auto i = std::find(vec.begin(), vec.end(), e);
+ if(i == vec.end()) {
+i = vec.begin();
+ }
+ *i; // no-warning
+}
+
+void bad_overwrite(std::vector ) {
+ auto i = vec.begin();
+ i = vec.end();
+ *i; // expected-warning{{Iterator accessed past its end}}
+}
+
+void bad_overwrite_find(std::vector , int e) {
+ auto i = std::find(vec.begin(), vec.end(), e);
+ if(i != vec.end()) {
+i = vec.begin();
+ }
+ *i; // expected-warning{{Iterator accessed past its end}}
+}
+
+void good_advance(std::vector ) {
+ auto i = vec.end();
+ std::advance(i, -1);
+ *i; // no-warning
+}
+
+void good_prev(std::vector ) {
+ auto i = std::prev(vec.end());
+ *i; // no-warning
+}
+
+struct init_value_prev {
+ init_value_prev(std::list l) : Back(std::prev(l.end())), Item(*Back) {} // no-warning
+private:
+ std::list::iterator Back;
+ int Item;
+};
Index: test/Analysis/diagnostics/explicit-suppression.cpp
===
--- test/Analysis/diagnostics/explicit-suppression.cpp
+++ test/Analysis/diagnostics/explicit-suppression.cpp
@@ -18,6 +18,6 @@
void testCopyNull(C *I, C *E) {
std::copy(I, E, (C *)0);
#ifndef SUPPRESSED
- // expected-warning@../Inputs/system-header-simulator-cxx.h:191 {{Called C++ object pointer is null}}
+ // expected-warning@../Inputs/system-header-simulator-cxx.h:293 {{Called C++ object pointer is null}}
#endif
}
Index: test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- test/Analysis/Inputs/system-header-simulator-cxx.h
+++ test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -8,18 +8,60 @@
typedef unsigned char uint8_t;
typedef __typeof__(sizeof(int)) size_t;
+typedef __typeof__((char*)0-(char*)0) ptrdiff_t;
void *memmove(void *s1, const void *s2, size_t n);
-template struct __iterator {
- typedef __iterator iterator;
- typedef __iterator const_iterator;
+namespace std {
+ struct input_iterator_tag { };
+ struct output_iterator_tag { };
+ struct forward_iterator_tag : public input_iterator_tag { };
+ struct bidirectional_iterator_tag : public forward_iterator_tag { };
+ struct random_access_iterator_tag : public bidirectional_iterator_tag { };
+
+ template struct iterator_traits {
+typedef typename Iterator::difference_type difference_type;
+typedef typename Iterator::value_type value_type;
+typedef typename Iterator::pointer pointer;
+typedef typename Iterator::reference reference;
+typedef typename Iterator::iterator_category iterator_category;
+ };
+}
- __iterator(const Ptr p) : ptr(p) {}
+template struct __vector_iterator {
+ typedef __vector_iterator iterator;
+ typedef __vector_iterator const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::random_access_iterator_tag iterator_category;
+
+ __vector_iterator(const Ptr p) : ptr(p) {}
+ __vector_iterator operator++() { ++ ptr; return *this; }
+ __vector_iterator operator++(int) {
+auto tmp = *this;
+++ ptr;
+return tmp;
+ }
+ __vector_iterator operator--() { -- ptr; return *this; }
+ __vector_iterator operator--(int) {
+auto tmp = *this; -- ptr;
+return tmp;
+ }
+ __vector_iterator operator+(difference_type n) {
+return ptr + n;
+ }
+ __vector_iterator operator-(difference_type n) {
+return ptr - n;
+ }
+ __vector_iterator operator+=(difference_type n) {
+return ptr += n;
+ }
+ __vector_iterator operator-=(difference_type n) {
+return
