[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

Fri Jun 14 14:10:33 PDT 2019

NoQ added a comment.

In D62883#1542802 <https://reviews.llvm.org/D62883#1542802>, @Szelethus wrote:

> Some conclusions:
>
> - Cases where the condition is also a variable initialization tend to expand the bug path greatly. This isn't always bad, but should be noted. In general, unless we have a good heuristic to figure out whether there is meaningful information on the right hand side of the initialization, I think we just shouldn't track these. A good example for this is this one <http://cc.elte.hu:15001/Default/#is-unique=on&report-hash=&run=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&reportHash=20eb450bc76b250d0ffa40e0435d3823&report=2998&subtab=20eb450bc76b250d0ffa40e0435d3823>. The 37th event contains pretty much every information we need, it's obvious that the optional could either be None or non-None, since it's in a condition. dyn_cast is a prime example too: in this case <http://cc.elte.hu:15001/Default/#is-unique=on&report-hash=&run=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&reportHash=ae426512e10d1835e2b80b594d41226a&report=3126&subtab=ae426512e10d1835e2b80b594d41226a>, note 9 is all we really need.
> - We should probably believe that `operator bool()` is implemented correctly, and shouldn't track the value all the way there (at least, when we're only tracking the condition). Example <http://cc.elte.hu:15001/Default/#is-unique=on&report-hash=&run=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&reportHash=cb40d620796d9fb5257657817766ef6c&report=3775&subtab=cb40d620796d9fb5257657817766ef6c>
> - We shouldn't ever track assert-like conditions. Example <http://cc.elte.hu:15001/Default/#is-unique=on&report-hash=&run=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&reportHash=4a863a6cf99be5676bc7d1d9aedccfb1&report=3087&subtab=4a863a6cf99be5676bc7d1d9aedccfb1> (note 38-41)
> - When the report didn't suffer from any of the above issues, I found the extra notes to be helpful! :D

On the other hand, all of these problems seem to be examples of the problem of D62978 <https://reviews.llvm.org/D62978>. Might it be that it's the only thing that we're missing? Like, we need to formulate a rule that'll give us an understanding of when do we track the value past the point where it has been constrained to true or false - do we care about the origins of the value or do we care about its constraints only? In case of `flag` in the test examples, we clearly do. In case of these bools that come from boolean conversions and assertions and such, we clearly don't. What's the difference?

================
Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:66
+void foo() {
+  // TODO: It makes no sense at all for bar to have been assigned here.
+  flag = coin(); // expected-note {{Value assigned to 'flag'}}
----------------
It's invalidation of globals by `coin()` which potentially writes to `bar`.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62883/new/

https://reviews.llvm.org/D62883