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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
