Charusso requested changes to this revision. Charusso added a comment. This revision now requires changes to proceed.
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.) 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. 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>. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62883/new/ https://reviews.llvm.org/D62883 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
