[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added a comment. In D53076#1403436 , @NoQ wrote: > I'll take a closer look in a few days, sorry for the delays! The progress > you're making is fantastic and i really appreciate your work, but i have an > urgent piece of work of my own to deal

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

NoQ added a comment. I'll take a closer look in a few days, sorry for the delays! The progress you're making is fantastic and i really appreciate your work, but i have an urgent piece of work of my own to deal with this week. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53076/new/

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:166-173 + // The declaration of the value may rely on a pointer so take its l-value. + if (const auto *DRE = dyn_cast_or_null(CondVarExpr)) { +if (const auto *VD =

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added a comment. Thanks you @NoQ! Luckily it is rely on `ProgramPoints` now. The only problem as I see the mentioned Z3-test, where I have no idea what happened. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53076/new/ https://reviews.llvm.org/D53076

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

NoQ added a comment. There still seem to be a couple of regressions with `Assuming...` pieces, but other than that, i believe that an awesome piece of progress has been made here. I really like how it looks now. I agree with George that dropping "Knowing" from the message looks fancier.

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso marked 2 inline comments as done. Charusso added a comment. @george.karpenkov thanks you for the comments! In D53076#1308641 , @george.karpenkov wrote: > What about just dropping the `Knowing` prefix? > Just having `arr is null, taking true

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

george.karpenkov requested changes to this revision. george.karpenkov added inline comments. This revision now requires changes to proceed. Comment at: test/Analysis/uninit-vals.m:222 testObj->origin = makeIntPoint(1, 2); - if (testObj->size > 0) { ; } //

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

george.karpenkov added a comment. This looks more reasonable, thanks! What about just dropping the `Knowing` prefix? Just having `arr is null, taking true branch` seems considerably more readable. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53076/new/ https://reviews.llvm.org/D53076

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added a comment. @Szelethus thanks you for the comments! In D53076#1307336 , @Szelethus wrote: > I think the reason why the printed message was either along the lines of > "this is 0" and "this is non-0", is that we don't necessarily know what

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Szelethus added a comment. I think the reason why the printed message was either along the lines of "this is 0" and "this is non-0", is that we don't necessarily know what constraint solver we're using, and adding more non-general code `BugReporter` is most probably a bad approach. How about

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added a comment. Sorry for the huge noise! It took me a while to see what is the problem. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53076/new/ https://reviews.llvm.org/D53076 ___ cfe-commits mailing list

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added a comment. In https://reviews.llvm.org/D53076#1305209, @Szelethus wrote: > In the meanwhile we managed to figure out where the problem lays, so if > you're interested, I'm going to document it here. > > The patch attempts to solve this by inspecting the actual condition, and >

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Szelethus added a comment. In the meanwhile we managed to figure out where the problem lays, so if you're interested, I'm going to document it here. The problem this patch attempts to solve is that `ConditionBRVisitor` adds event pieces to the bugpath when the state has changed from the

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added inline comments. Comment at: test/Analysis/diagnostics/macros.cpp:33 void testDoubleMacro(double d) { - if (d == DBL_MAX) { // expected-note {{Taking true branch}} + if (d == DBL_MAX) { // expected-note {{Assuming 'd' is equal to DBL_MAX}} +

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso marked 8 inline comments as done. Charusso added a comment. @NoQ thanks you for the great explanation! I really wanted to write out known constant integers and booleans but I think 'Knowing...' pieces would be more cool. I tried to minimize the changes after a little e-mailing with

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

NoQ added a comment. @Charusso, i believe you have some misconception about what constraints mean and how they work. Not sure what this misconception is, so i'll make a blind guess and annoy you a little bit with a narcissistic rant and you'll have to bear me, sry! The most important thing to

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added inline comments. Comment at: test/Analysis/Inputs/expected-plists/cxx-for-range.cpp.plist:837 + message + Variable fail is true + Double negating is not in standard English, so this behaviour is documented here.

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added inline comments. Comment at: test/Analysis/MisusedMovedObject.cpp:187 A a; -if (i == 1) { // expected-note {{Taking false branch}} expected-note {{Taking false branch}} +if (i == 1) { // expected-note {{Assuming 'i' is not equal to 1}} expected-note

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

NoQ added a comment. In https://reviews.llvm.org/D53076#1276315, @Charusso wrote: > In https://reviews.llvm.org/D53076#1276277, @NoQ wrote: > > > I mean, the idea of checking constraints instead of matching program points > > is generally good, but the example i gave above suggests that there's

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added a comment. In https://reviews.llvm.org/D53076#1276277, @NoQ wrote: > I mean, the idea of checking constraints instead of matching program points > is generally good, but the example i gave above suggests that there's a bug > somewhere. I think it is an unimplemented feature

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added a comment. In https://reviews.llvm.org/D53076#1276270, @george.karpenkov wrote: > @NoQ has a good point: we need to preserve the distinction between the things > analyzer "assumes" and the things analyzer "knows". Yes, but I think it should be a new patch because I would like

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

NoQ added a comment. I mean, the idea of checking constraints instead of matching program points is generally good, but the example i gave above suggests that there's a bug somewhere. https://reviews.llvm.org/D53076 ___ cfe-commits mailing list

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

george.karpenkov requested changes to this revision. george.karpenkov added a comment. This revision now requires changes to proceed. Actually, apologies again: I have rushed through this too much. @NoQ has a good point: we need to preserve the distinction between the things analyzer "assumes"

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added a comment. Thanks everyone for the review! @george.karpenkov could you commit it please? I am interested in your ideas in the little discussion started with @NoQ at the beginning of the project. https://reviews.llvm.org/D53076 ___

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added inline comments. Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1853 + +const auto *Cond = cast(PS->getStmt()); +auto Piece = VisitTrueTest(Cond, tag == tags.first, BRC, BR, N); george.karpenkov wrote: > How do we know that

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

george.karpenkov requested changes to this revision. george.karpenkov added a comment. This revision now requires changes to proceed. Thanks a lot, this is almost ready! I think it should be good to go after this round of nitpicks. Comment at:

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:202 + void finalizeConstraints() { +Constraints.clear(); + } george.karpenkov wrote: > These constraints are conceptually part of the visitor,

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

george.karpenkov requested changes to this revision. george.karpenkov added inline comments. This revision now requires changes to proceed. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:80 typedef llvm::ImmutableMap GenericDataMap;

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

whisperity added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:238 + /// message on that constraint being changed. + bool isChangedOrInsertConstraint(ConstraintMap , const Stmt *Cond, + StringRef

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Szelethus added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:182 + /// constraint being changed. + bool isChanged(const Stmt *Cond, StringRef Message) { +ConstraintMap::iterator I = Constraints.find(Cond);

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

george.karpenkov requested changes to this revision. george.karpenkov added inline comments. This revision now requires changes to proceed. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:202 + void finalizeConstraints() { +

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

george.karpenkov added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:80 class ConstraintManager { + using ConstraintMap = std::map; + ConstraintMap Constraints; Traditionally LLVM projects use

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

Charusso added a comment. In https://reviews.llvm.org/D53076#1261134, @NoQ wrote: > For example, in the `inline-plist.c`'s `bar()` on line 45, Static Analyzer > indeed doesn't assume that `p` is equal to null; instead, Static Analyzer > *knows* it for sure. Thanks you! This a great example