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

[Csaba Dabis via Phabricator via cfe-commits]

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 with this week.


@NoQ, please take your time, that new bug-report logic is the end of my work on 
reporting business (as I mentioned I wanted to create the same patch), so that 
is more important than this.


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

https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

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 = dyn_cast_or_null(DRE->getDecl())) {
+  SVal DeclSVal = State->getSVal(State->getLValue(VD, LCtx));
+  if (auto DeclCI = DeclSVal.getAs())
+return >getValue();
+}

I'll take a closer look at the rest of the patch, but this code snippet looks 
suspicious to me at a glance. Does it ever add anything on top of the 
"hard-coded" case? When it does, is it correct?

I mean, the "hard-coded" case does not actually correspond to an integer 
literal expression; it corresponds to any circumstances in which the Static 
Analyzer could compute that the value is going to be exactly that concrete 
integer on this execution path. If the Static Analyzer could not compute that, 
i doubt that this simple procedure will do better.

It might be that this is needed because you're looking at a wrong 
`ExplodedNode` on which the condition expression is either not computed yet or 
already dropped. In this case i'd prefer to try harder to find the right node, 
because `getSVal(CondVarExpr, LCtx)` is just so much more powerful.


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

https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

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.




Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:86-96
+  const auto *DRE = dyn_cast_or_null(CondVarExpr);
+  const auto *BExpr = dyn_cast_or_null(CondVarExpr);
+  
+  if (BExpr && !DRE)
+DRE = 
dyn_cast_or_null(BExpr->getLHS()->IgnoreParenImpCasts());
+  
+  if (BExpr && !DRE)

The usual style in LLVM for this sort of stuff is

```
if (const auto *DRE = dyn_cast_or_null(CondVarExpr)) {
  // Do stuff with DRE.
}
```

`CondVarExpr` definitely cannot successfully cast to both `DeclRefExpr` and 
`BinaryOperator` at the same time, so this sort of structure is much easier to 
understand. Also checks after `&&` are definitely redundant.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:116
+
+  const ConstraintRangeTy Ranges = State->get();
+  for (auto I = Ranges.begin(); I != Ranges.end(); ++I) {

Hmm, why are you even able to access it here? The `get<>` key should only be 
visible in a single translation unit in which it is defined, otherwise it's 
buggy because it relies on addresses of static globals to work.

Actually, could you separate the work of displaying the exact range boundary 
(this whole `RangeSet` business) into a separate diff? I think it's a separate 
change that requires separate discussion.

Also instead of accessing the constraint manager's private trait directly, you 
should use the `getRange()` method. It adds all the necessary implicit 
assumptions that aren't represented explicitly as items in the map.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:127
+
+static Optional getConcreteIntegerValue(const Expr *CondVarExpr,
+ const ExplodedNode *N) {

Why `int`?

That's not what `getExtValue()` returns (so you can accidentally print a 
truncated value), and generally the size of the `int` type on the host machine 
shouldn't affect analysis results.

I think you should print the `APSInt` directly instead of converting it into a 
raw integer.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:131-133
+  // If there is no value return.
+  if (!State->getStore())
+return {};

Even if the Store is currently null, every live variable in the program 
inevitably has a value, so i don't think we should bail out here.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:140-142
+  // If symbolic range of the SVal is found that means the value is unknown.
+  if (getRangeSet(CondVarExpr, N))
+return {};

There are cornercases where the range consists of exactly one point, so the 
value is technically concrete but wasn't yet collapsed into a concrete value by 
the engine.

In any case, as far as i understand, this code is there only for optimization, 
and i don't think it's actually making things faster because the lookup is 
already expensive.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:145
+  // The declaration of the value may rely on a pointer so take its l-value.
+  const auto DeclSVal = State->getSVal(State->getLValue(CondVarVD, LCtx));
+

`auto` shouldn't be used here and in a few other places according to [[ 
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
 | the coding guidelines ]].

Essentially, it should only be used for cast results, where the type is already 
written out.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:147
+
+  // The value may inline.
+  const auto InlineSVal = State->getSVal(CondVarExpr, LCtx);

What do you mean by "inline"? There are no function calls going on here.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1885-1886
   ProgramPoint progPoint = N->getLocation();
-  ProgramStateRef CurrentState = N->getState();
-  ProgramStateRef PrevState = N->getFirstPred()->getState();
-
-  // Compare the GDMs of the state, because that is where constraints
-  // are managed.  Note that ensure that we only look at nodes that
-  // were generated by the analyzer engine proper, not checkers.
-  if (CurrentState->getGDM().getRoot() ==
-  PrevState->getGDM().getRoot())
+  const ExplodedNode *PredNode = N->getFirstPred();
+  const ExplodedNode *PredPredNode = PredNode->getFirstPred();
+

I'm worried that checker transitions can screw up the order of nodes here.

Every time a checker does `addTransition()` or `generateNonFatalErrorNode()`, 
it injects an intermediate node, and there can be indefinitely many of those 
between the 

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

[Csaba Dabis via Phabricator via cfe-commits]

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 branch` seems considerably more 
> readable.


I wanted to create something identical to the current reports. If we are about 
to change readability, I would change that two first:

- identical operator printing with `<, <=, >...`, so `equal to` is `=` and `not 
equal to` is `!=`.
- emphasize the value information with quotes, so `Assuming 'i' >= '2'`.




Comment at: test/Analysis/uninit-vals.m:324
   testObj->origin = makeIntPoint2D(1, 2);
-  if (testObj->size > 0) { ; } // expected-note{{Taking false branch}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming 'testObj->size' is 
<= 0}}
// expected-note@-1{{Taking false branch}}

george.karpenkov wrote:
> That does not seem right: from `calloc` the analyzer should know that the 
> `testObj->size` is actually zero.
That `Assuming...` piece is rely on the change between the last two diffs: I 
just print out more `VarDecls` in `patternMatch()`. I thought everything works 
fine, so I will check that problem if @NoQ will not be faster as he already 
found some problematic code piece in that test file.



Comment at: test/Analysis/virtualcall.cpp:170
+   // expected-note-re@-4 ^}}Assuming 'i' is <= 0}}
+   // expected-note-re@-5 ^}}Taking false branch}}
 #endif

george.karpenkov wrote:
> Could you describe what happens here?
> Why the `assuming` notes weren't there before?
We do not have range information in the *newly seen `Assuming...` pieces. 
Because the analyzer has not learnt new information - as there is no 
information - we have not entered to `VisitTrueTest()` to report these. A good 
reference for that new behaviour is in 
`test/Analysis/Inputs/expected-plists/cxx-for-range.cpp.plist`.

*I have added these with https://reviews.llvm.org/D53076?id=175177


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

https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Karpenkov via Phabricator via cfe-commits]

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) { ; } // expected-note{{Taking false branch}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming 'testObj->size' is 
<= 0}}
// expected-note@-1{{Taking false branch}}

Same here: we should know that `testObj->size == 0`



Comment at: test/Analysis/uninit-vals.m:324
   testObj->origin = makeIntPoint2D(1, 2);
-  if (testObj->size > 0) { ; } // expected-note{{Taking false branch}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming 'testObj->size' is 
<= 0}}
// expected-note@-1{{Taking false branch}}

That does not seem right: from `calloc` the analyzer should know that the 
`testObj->size` is actually zero.



Comment at: test/Analysis/virtualcall.cpp:170
+   // expected-note-re@-4 ^}}Assuming 'i' is <= 0}}
+   // expected-note-re@-5 ^}}Taking false branch}}
 #endif

Could you describe what happens here?
Why the `assuming` notes weren't there before?


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

https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Karpenkov via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

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 
> constraint solver we're using, and adding more non-general code `BugReporter` 
> is most probably a bad approach.


It was an unimplemented feature to write out known stuff. There is no 
constraint solving, it is rather constant value dumping.

> How about we tinker with `ContraintManager` a little more, maybe add an 
> `explainValueOfVarDecl`, that would return the value of a variable in a given 
> `ProgramState`? We could implement it for `RangeConstraintSolver` 
> (essentially move the code you already wrote there), and leave a `TODO` with 
> a super generic implementation for Z3.

I agree with @george.karpenkov, we want to have the smallest scope.




Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1892-1920
+  const auto CurrentCR = N->getState()->get();
+  const auto PredCR = PredState->get();
+  const auto PredPredCR = PredPredState->get();
+
+  // We only want to check BlockEdges once so we have to determine if the 
change
+  // of the range information is not happened because of dead symbols.
+  //

Szelethus wrote:
> `ConstraintRange`, as far as I know, is the data  `RangedConstraintManager` 
> stores in the GDM. What if we're using a different constraint solver? I think 
> it'd be better to take the more abstract approach, extend 
> `ConstraintManager`'s interface with an `isEqual` or `operator==` pure 
> virtual method, and implement it within it's descendants.
Because we only operate in the `ConditionBRVisitor` it would be useless for now.


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

https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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 we tinker with `ContraintManager` a little 
more, maybe add an `explainValueOfVarDecl`, that would return the value of a 
variable in a given `ProgramState`? We could implement it for 
`RangeConstraintSolver` (essentially move the code you already wrote there), 
and leave a `TODO` with a super generic implementation for Z3.

Although, I can't quite write an essay on top of my head about golden rules of 
constraint solving, so take my advice with a grain of salt.




Comment at: 
include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:164
 class ConditionBRVisitor final : public BugReporterVisitor {
+  bool IsAssuming;
+

We only get to know what this field is for while reading the actual 
implementation, please add comments.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1892-1920
+  const auto CurrentCR = N->getState()->get();
+  const auto PredCR = PredState->get();
+  const auto PredPredCR = PredPredState->get();
+
+  // We only want to check BlockEdges once so we have to determine if the 
change
+  // of the range information is not happened because of dead symbols.
+  //

`ConstraintRange`, as far as I know, is the data  `RangedConstraintManager` 
stores in the GDM. What if we're using a different constraint solver? I think 
it'd be better to take the more abstract approach, extend `ConstraintManager`'s 
interface with an `isEqual` or `operator==` pure virtual method, and implement 
it within it's descendants.


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

https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

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

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

[Csaba Dabis via Phabricator via cfe-commits]

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 
> tries to find out whether the symbols inside that condition has changed in 
> between the states, but this is overkill solution, as comparing the two 
> `ConstraintManager` objects would achieve the same thing.


I tried to investigate what is happening after we entered into a condition, 
which is working well with 'Knowing...' pieces, but with too much overhead. 
Because this patch is all about write out more information, rather than 
removing the GDM-checking I will move forward with 'Knowing...' pieces. The 
current questions are real for that project, just let me simplify the code and 
create that feature.


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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 previous state, 
not only when the `ConstraintManager` differs in between the changes [1]. The 
patch attempts to solve this by inspecting the actual condition, and tries to 
find out whether the symbols inside that condition has changed in between the 
states, but this is overkill solution, as comparing the two `ConstraintManager` 
objects would achieve the same thing. This should probably be executed by 
extending the interface of `ConstraintManager` with a comparison method.

[1] 
https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp#L1808


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

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}}
+  // expected-note@-1 {{Taking true branch}}

NoQ wrote:
> This one's good. Static Analyzer doesn't understand floats, so this branch is 
> indeed non-trivial. There should indeed be an assuming... piece here.
A little bit misunderstandable, because we do not know anything about doubles. 
May the generic message fit better?



Comment at: test/Analysis/diagnostics/undef-value-param.m:56
 SCDynamicStoreRef ref = anotherCreateRef(, x);
-if (err) { 
-   //expected-note@-1{{Assuming 'err' is not equal to 0}}
-   //expected-note@-2{{Taking true branch}}
-CFRelease(ref);
-ref = 0; // expected-note{{nil object reference stored to 'ref'}}
+if (err) { //expected-note{{Taking true branch}}
+  CFRelease(ref);

I am not sure why but the range here [1, (2^64)-1]. There is would not be any 
number like 0?



Comment at: test/Analysis/new-ctor-malloc.cpp:11
   void *x = malloc(size); // expected-note {{Memory is allocated}}
-  if (!x) // expected-note{{Assuming 'x' is non-null}}
-  // expected-note@-1 {{Taking false branch}}
+  if (!x) // expected-note {{Taking false branch}}
 return nullptr;

The range information is [1, (2^64)-1] but as I saw `malloc` could return null. 
Who is lying here?



Comment at: test/Analysis/uninit-vals.m:167
   testObj->origin = makePoint(0.0, 0.0);
-  if (testObj->size > 0) { ; } // expected-note{{Taking false branch}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming the condition is 
false}}
+   // expected-note@-1{{Taking false branch}}

NoQ wrote:
> These are pretty weird. As far as i understand the test, these should be 
> there. But i'm suddenly unable to debug why were they not shown before, 
> because there's either something wrong with exploded graph dumps or with the 
> exploded graph itself; it appears to be missing an edge right after `size > 
> 0` is assumed. I'll look more into those.
We gather information with `clang-analyzer-eval` functions so the conditions 
would be `Knowing...` pieces but the `BugReporter.cpp` does not know about 
these extra assumptations. Where to teach to do not `Assuming...`, but 
`Knowing...` these?



Comment at: test/Analysis/virtualcall.cpp:172
 #endif
   X x(i - 1);
 #if !PUREONLY

Because this `X x(i - 1);` we assume these conditions? This one is tricky to be 
`Knowing...`.


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

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 @george.karpenkov 
on the mailing list. The problem is the following: we go backwards on the 
bug-path, that is why we see range information changes backwards, but the user 
information goes forwards: from the top to the bottom. I have implemented the 
'check upwards' feature, so now everything working fine, but I think it is a 
very time-consuming approach. (The idea is copied from 
`BugReporter.cpp/generateVisitorsDiagnostics()` where I tried to flip the path, 
unsuccessfully: 
http://clang-developers.42468.n3.nabble.com/Visit-nodes-in-the-order-of-the-user-output-to-create-better-reports-td4062898.html
 )


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

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 know about constraints is that they are applied to 
"symbols", not to variables. We explain it to the user in terms of variables 
(when possible) because symbols are immaterial in the code and as such cannot 
be easily explained to the user, but in reality constraints are entirely about 
symbols.

Let me explain this with an example. If you say "let x denote the number of 
sheep in a herd" and then a new sheep is born and joins the herd, the number of 
sheep becomes (x + 1) and no longer remains equal to x, while x can still be 
used to represent the *original* number of sheep. In this example, the number 
of sheep is the variable, and x is the symbol.

The good thing about symbols is that they denote the same unknown value 
throughout the whole analysis. Values of variables may change (hence the 
nomenclature), but values denoted by symbols never change. They are kinda like 
Static Analyzer's substitute for transforming the program into SSA notation. 
When constraints are assumed, we make assumptions about the unknown value 
denoted by the symbol and it becomes "less unknown", but it is still the same 
value, it's just we know more about it. Which means that:

- //"The Golden Rule of Constraint Solving":// (i came up with this title 3 
seconds ago) In any two points A and B during analysis (i.e., nodes in the 
exploded graph), if A precedes B along any execution path (i.e., there's a 
directed path from A to B in the exploded graph), constraints for any symbol x 
in point B are a subset of constraints for x in point A.

For example, if Static Analyzer assumes that a symbol x is greater than 10, it 
cannot later assume that x is less than 5, because [-∞, 4] is not a subset of 
[11, +∞]. Therefore Static Analyzer will not explore the path on which x is 
less than 5, and it will not update range constraints for the symbol. But it 
will explore the path on which x is also less than 15 by setting range 
constraints for x to [11, 14]  because [11, 14] is indeed a subset of [11, +∞].

The formal definition of "Assuming" pieces is that they indicate that the 
constraints change for the symbol (eg., for the symbol that represents the 
*current* value of the variable mentioned in the message of the event piece). 
This is the precise meaning of "Static Analyzer is assuming". And when 
explained in terms of variables, this precise meaning is comprehend-able by the 
user and should be conveyed properly. The distinction between presence and 
absence of assumptions is crucial to convey. Therefore, if the branch condition 
is denoted by a symbol and constraints over that symbol do not change, there 
should be no "Assuming..." piece for the branch condition symbol, and the 
respective mechanism should not be removed. But there may be a new sort of 
event piece, eg. "Knowing...", if you still want to highlight the motivation 
behind "Taking..." a specific branch, which seems to be the purpose of your 
patch.

"Assuming..." pieces over the same variable may contradict each other (i.e., 
look as if they violate the Golden Rule) when contents of the variable change. 
So, yeah, in this case another "Assuming..." piece will be necessary in some 
cases. But it doesn't mean that the Golden Rule is actually violated; it just 
means that the variable now has different value and different assumptions can 
be made about it. This doesn't happen in the examples that i pointed out, so 
there should not be an "Assuming..." piece.

In an inline comment i show what this means on the test case that seems to be 
the easiest.




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 {{Taking false branch}}
+  // expected-note@-1 {{Assuming 'i' is not equal to 1}} expected-note@-1 
{{Taking false branch}}

Charusso wrote:
> NoQ wrote:
> > These assumptions were already made on the previous branches. There should 
> > be no extra assumptions here.
> Agree but only if there is no extra constraint EventPiece between them.
I think i answered this concern in the out-of-line comment. Because constraints 
never change in a contradictory manner but only grow shorter, a much stronger 
statement can be made here: there are also no extra assumptions when the 
variable is not reassigned since the last "Assuming..." piece. Of course, even 
if it is reassigned, we may still not want to have the "Assuming..." piece - it 
depends entirely on the current constraints over the symbol that represents the 
branch condition.

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

[Csaba Dabis via Phabricator via cfe-commits]

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.



Comment at: test/Analysis/Inputs/expected-plists/edges-new.mm.plist:9629
+ Variable z is equal to 0
+
+

@NoQ this is the only place (as I see) where SA works with flow-sensitive 
constraint-handling what you mentioned. Is it a good change?


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

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 {{Taking false branch}}
+  // expected-note@-1 {{Assuming 'i' is not equal to 1}} expected-note@-1 
{{Taking false branch}}

NoQ wrote:
> These assumptions were already made on the previous branches. There should be 
> no extra assumptions here.
Agree but only if there is no extra constraint EventPiece between them.



Comment at: test/Analysis/MisusedMovedObject.cpp:221
 }
-if (i > 5) { // expected-note {{Taking true branch}}
+if (i > 5) { // expected-note {{Assuming 'i' is > 5}} expected-note 
{{Taking true branch}}
   a.foo();   // expected-warning {{Method call on a 'moved-from' object 
'a'}} expected-note {{Method call on a 'moved-from' object 'a'}}

NoQ wrote:
> We have assumed that `i` is `>= 10` on the previous branch. It imples that 
> `i` is greater than `5`, so no additional assumption is being made here.
Agree but only if there is no extra constraint EventPiece between them.



Comment at: test/Analysis/NewDelete-path-notes.cpp:10
   if (p)
-// expected-note@-1 {{Taking true branch}}
+// expected-note@-1 {{Assuming 'p' is non-null}}
+// expected-note@-2 {{Taking true branch}}

NoQ wrote:
> Static Analyzer knows that the standard operator new never returns null. 
> Therefore no assumption is being made here.
As I see SA knows nothing. Where to teach it?



Comment at: test/Analysis/inline-plist.c:46
   if (p == 0) {
-// expected-note@-1 {{Taking true branch}}
+// expected-note@-1 {{Assuming 'p' is equal to null}}
+// expected-note@-2 {{Taking true branch}}

NoQ wrote:
> The condition `!!p` above being assumed to false ensures that `p` is equal to 
> `null` here. We are not assuming it again here.
Agree but only if there is no extra constraint EventPiece between them.


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

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 a bug 
> > somewhere.
>
>
> I think it is an unimplemented feature which appear like 1:500 time, but we 
> will see.


The original behavior is perfectly consistent with my understanding of Static 
Analyzer's reports that i've been reading continuously for years. There might 
have been a few places where assumption is made but isn't highlighted, but the 
opposite has never happened, and also the opposite is more confusing to the 
user because it demonstrates an explicit text that the user has to trust. So i 
think that one way or another, the new behavior is a regression from the 
original behavior on an on-by-default functionality on some test cases, and we 
should not commit this patch until this regression is debugged and fixed.

I highlight a few more test cases that i believe have regressed in 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 {{Taking false branch}}
+  // expected-note@-1 {{Assuming 'i' is not equal to 1}} expected-note@-1 
{{Taking false branch}}

These assumptions were already made on the previous branches. There should be 
no extra assumptions here.



Comment at: test/Analysis/MisusedMovedObject.cpp:221
 }
-if (i > 5) { // expected-note {{Taking true branch}}
+if (i > 5) { // expected-note {{Assuming 'i' is > 5}} expected-note 
{{Taking true branch}}
   a.foo();   // expected-warning {{Method call on a 'moved-from' object 
'a'}} expected-note {{Method call on a 'moved-from' object 'a'}}

We have assumed that `i` is `>= 10` on the previous branch. It imples that `i` 
is greater than `5`, so no additional assumption is being made here.



Comment at: test/Analysis/NewDelete-path-notes.cpp:10
   if (p)
-// expected-note@-1 {{Taking true branch}}
+// expected-note@-1 {{Assuming 'p' is non-null}}
+// expected-note@-2 {{Taking true branch}}

Static Analyzer knows that the standard operator new never returns null. 
Therefore no assumption is being made here.



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}}
+  // expected-note@-1 {{Taking true branch}}

This one's good. Static Analyzer doesn't understand floats, so this branch is 
indeed non-trivial. There should indeed be an assuming... piece here.



Comment at: test/Analysis/diagnostics/no-store-func-path-notes.cpp:23
   int initialize(int *p, int param) {
-if (param) { //expected-note{{Taking false branch}}
+if (param) { // expected-note{{Assuming 'param' is 0}}
+ // expected-note@-1{{Taking false branch}}

This method is called from `use()` with `param` equal to concrete 0. It is not 
analyzed as a top-level function. There is no assumption made here, like in 
most other places in this file.



Comment at: test/Analysis/diagnostics/no-store-func-path-notes.m:13
 - (int)initVar:(int *)var param:(int)param {
-  if (param) { // expected-note{{Taking false branch}}
+  if (param) { // expected-note{{Assuming 'param' is 0}}
+   // expected-note@-1{{Taking false branch}}

This method is called from `foo()` with `param` equal to concrete `0`. It is 
not analyzed as a top-level function. There is no assumption made here.



Comment at: test/Analysis/diagnostics/no-store-func-path-notes.m:26
 //expected-note@-1{{Returning from 
'initVar:param:'}}
-  if (out)  // expected-note{{Taking true branch}}
+  if (out)  // expected-note{{Assuming 'out' is not 
equal to 0}}
+// expected-note@-1{{Taking true branch}}

`-initVar` returns concrete `0` when called with these parameters. There is no 
assumption being made here.



Comment at: test/Analysis/diagnostics/no-store-func-path-notes.m:34
 int initializer1(int *p, int x) {
-  if (x) { // expected-note{{Taking false branch}}
+  if (x) { // expected-note{{Assuming 'x' is 0}}
+   // expected-note@-1{{Taking false branch}}

This function is called from `inifFromBlock()` with `x` equal to concrete `0`. 
It is not analyzed as a top-level function. 

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

[Csaba Dabis via Phabricator via cfe-commits]

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 which appear like 1:500 time, but we 
will see.


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

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 to add a 
value-dump method on integers/booleans and stuff I already mentioned in the 
beginning.


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

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

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

[George Karpenkov via Phabricator via cfe-commits]

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" and the things analyzer "knows".


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

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 it's always an `Expr`?
`VisitTrueTest` operates with `Expr` only, so I just left the type.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1970
 
+bool ConditionBRVisitor::insertOrUpdateConstraintMessage(const Stmt *Cond,
+ StringRef Message) {

george.karpenkov wrote:
> 1. What about the refactoring I have previously suggested twice?
> 2. I know you have started with that -- and sorry for spurious changes -- but 
> I also think your original name of `constraintsChanged` is better.
> We have multiple messages at the same place so we have to update the message. 
> The problem with your code is insert operates with disjunct keys, not values.

I have commented this after the last refactor, sorry for the inconvenience.


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Karpenkov via Phabricator via cfe-commits]

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: 
include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:168
 
-  virtual void EndPath(ProgramStateRef state) {}
-

From a brief inspection this indeed seems dead code. However, this removal 
should be moved into a separate revision (sorry!)



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:680
-ConstraintMgr->EndPath(St);
-  }
 };

Same: should be moved into a separate revision, same as the other removal.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1853
+
+const auto *Cond = cast(PS->getStmt());
+auto Piece = VisitTrueTest(Cond, tag == tags.first, BRC, BR, N);

How do we know that it's always an `Expr`?



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1854
+const auto *Cond = cast(PS->getStmt());
+auto Piece = VisitTrueTest(Cond, tag == tags.first, BRC, BR, N);
+if (!Piece)

`/*tookTrue=*/tag == tag.first`



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1970
 
+bool ConditionBRVisitor::insertOrUpdateConstraintMessage(const Stmt *Cond,
+ StringRef Message) {

1. What about the refactoring I have previously suggested twice?
2. I know you have started with that -- and sorry for spurious changes -- but I 
also think your original name of `constraintsChanged` is better.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2261
   PrettyStackTraceLocationContext CrashInfo(Pred->getLocationContext());
-  StateMgr.EndPath(Pred->getState());
 

Should be moved together with other two removals.



Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:29
 
+/// Increments the number of times this state is referenced.
 void ProgramStateRetain(const ProgramState *state) {

While this minor formatting is correct, it's better to remove it to simplify 
future archaeology.


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

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, not part of the 
> constraint manager. Could they be simply stored in the visitor?
My idea was to have a generic constraint map as @NoQ mentioned, then we could 
attach this to other places to reduce noisy reports. But probably this is the 
best place for now.



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:244
+if (I == Constraints.end() || !Message.equals(I->second)) {
+  Constraints[Cond] = Message;
+  return true;

george.karpenkov wrote:
> Isn't that equivalent to `Constraints.insert(make_pair(Cond, 
> Message)).second` ?
> I think I have written that before.
We have multiple messages at the same place so we have to update the message. 
The problem with your code is `insert` operates with disjunct keys, not values.


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Karpenkov via Phabricator via cfe-commits]

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;
+  typedef llvm::DenseMap  ConstraintMap;
 

Generally, `StringRef`'s shouldn't be stored in containers, as containers might 
outlive them.
It might be fine in this case, but I would prefer to be on the safe side and 
just use `std::string`



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

whisperity wrote:
> `insertOrUpdateContraintMessage`
> 
> and mention in the documentation that it returns whether or not the actual 
> insertion or update change took place
If constraints is now a property of the visitor, shouldn't this method with the 
typedef above be moved to the visitor as well?



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:244
+if (I == Constraints.end() || !Message.equals(I->second)) {
+  Constraints[Cond] = Message;
+  return true;

Isn't that equivalent to `Constraints.insert(make_pair(Cond, Message)).second` ?
I think I have written that before.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2232
+ const ExplodedNode *N, BugReport ) 
{
+  Constraints.clear();
+}

It does not seem necessary, because a new copy of the visitor is created for 
each new bug report.


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Whisperity via Phabricator via cfe-commits]

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 Message) const {

`insertOrUpdateContraintMessage`

and mention in the documentation that it returns whether or not the actual 
insertion or update change took place


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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);

I would not expect a function called `isChanged` to change the state of an 
object. Can you find a name that doesn't feel like it's a simple getter 
function?


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Karpenkov via Phabricator via cfe-commits]

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() {
+Constraints.clear();
+  }

These constraints are conceptually part of the visitor, not part of the 
constraint manager. Could they be simply stored in the visitor?



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:235
+  /// Check if the state has changed based on the constraint has changed.
+  bool isChanged(const Stmt *Cond, StringRef Message) const;
+

Probably should be replaced by the expression above and inlined.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:187
 
-void
-BugReporterVisitor::finalizeVisitor(BugReporterContext &,
-const ExplodedNode *, BugReport &) {}
+void BugReporterVisitor::finalizeVisitor(BugReporterContext &,
+ const ExplodedNode *, BugReport &) {}

spurious change



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1831
+  const Stmt *Cond = srcBlk->getTerminatorCondition();
+  auto piece = VisitTerminator(term, N, srcBlk, BE->getDst(), BR, BRC);
+  if (piece && State->isChanged(Cond, piece->getString()))

capital letter


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Karpenkov via Phabricator via cfe-commits]

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 `llvm::DenseMap`



Comment at: 
include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:183
+  bool isChanged(const Stmt *Cond, StringRef Message) {
+ConstraintMap::iterator I = Constraints.find(Cond);
+

`return Constraints.insert(make_pair(Cond, Message)).second` ?


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

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 what I have to cover later on. I have a patch 
where we print out known integers. The basic style is the following: `Assuming 
'x' is not equal to 1`. I would like to emphasize the value and if it a known 
value, make it looks like this: `Variable 'x' is equal to '1'`, or `Variable 
'*ptr' is equal to '1'`. (If this is the situation: `Constant 'x' is equal to 
'1'` would be cool as well.)

I made that patch in a separated file called `BugReporterHelpers.cpp` next to 
the `BugReporterVisitors`. I also would like to move all the helper functions 
from `BugReporterVisitors.cpp` to that source file. My first idea with that to 
create a live documentation, how would a new clang-hacker obtain a value from a 
certain position (me with testing those things out). Also what you mentioned 
with this flow-sensitive chaining, this is could not be a short patch, so I 
think this is the time when we want to have something like this.

What do you think? If this patch goes well, should I attach the mentioned new 
patch to this, or create a new one?


https://reviews.llvm.org/D53076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits