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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added a comment.

In D62883#1545324 <https://reviews.llvm.org/D62883#1545324>, @Charusso wrote:

> Hey! It is a cool patch as the smallest the scope the better to understand what is going on. I like the direction, but we saw with @NoQ, we have to think about "In which range do we track?".
>
> In D62883#1545282 <https://reviews.llvm.org/D62883#1545282>, @Szelethus wrote:
>
> > - Hide the current implementation behind the off-by-default analyzer config `"track-conditions"` Do you think that this patch looks good enough now that it's off by default? Maybe it'd be easier to develop/review followup changes separately.
>
>
> It would be helpful to see what is exactly new, what trackExpressionValue() cannot provide. Could you adjust the test case with a second run, where you switch off that new feature, please? (I highly recommend that for future developments also.)


You can see how this would look like by checking the history of this revision: https://reviews.llvm.org/D62883?vs=204554&id=204973&whitespace=ignore-most#toc. Adding a second `RUN:` line might be a good idea though.

> In D62883#1544609 <https://reviews.llvm.org/D62883#1544609>, @Szelethus wrote:
> 
>> In D62883#1544302 <https://reviews.llvm.org/D62883#1544302>, @NoQ wrote:
>>
>> > 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?
>> >
>> > How about we track the condition value past its collapse point only if it involves an overwrite of a variable (or other region) from which the value is loaded? Like, if there are no overwrites, stop at the collapse point. If there are overwrites, stop at the oldest overwrite point (i.e., the value was loaded from 'x' which was previously overwritten by the value of value 'y' which was previously overwritten by the initial value of variable 'z' => stop at the overwrite point of 'y').
>> >
>> > (then, again, not sure what happens if more nested stack frames are around)
>>
>>
>> For now, I'd like to restrict my efforts to simply "we should track this"  or "we shouldn't track this". I think simply not tracking cases where the condition is a single variable assignment is a good initial approach. Let how the tracking is done be a worry for another day. Also, I'm a little confused, isn't this comment about how D62978 <https://reviews.llvm.org/D62978> could be solved?
> 
> 
> As @NoQ pointed out we have some problem with that function. We are tracking *values* without using the `redecl()`-chain (see in https://clang.llvm.org/doxygen/DeclBase_8h_source.html#l00948), or without tracking conditions. You have solved(?) the latter, but the former is equally important to solve.

I'm a little confused, which is the "formet" and "latter" you're referring to?

> As I see that is the current situation:
> 
> - ReturnVisitor: give us too much unrelated information as we go out of scope.
> - FindLastStoreBRVisitor: do not use `redecls()` so gets confused with all the hackish SVal conversions.
> - TrackControlDependencyCondBRVisitor: swaps notes which is questionable if I want to see what the above two kindly provides for me, or I would remove them instead.
> 
>   I believe this patch should be on by default, but it requires to fix the "In which range do we track?" problem with D62978 <https://reviews.llvm.org/D62978>.

I disagree with this. The reason why I'd like to make this an off-by-default option is to implement my followup improvements incrementally (not only that, but a whole family of conditions is going to be added), and allow us to observe the changes those make in relation to this patch -- besides, I don't really see this patch changing even if I manage to fix those issues. Would you like to see a change being made to this specific patch?


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

https://reviews.llvm.org/D62883