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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Yay!! I'm happy with this change and redirected all my wrath on its 
dependencies.


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] ConditionBRVisitor: Enhance to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks you @NoQ for the great idea!

Without the duplicated `BugReport.cpp` reports it could be an on-by-default 
patch, see:

Before:
F8685549: before.html 

After:
F8685550: after.html 

It is not perfect, but I like that and I have already got ideas how to use 
these new pieces.




Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:187-190
+  // The value may hard-coded.
+  SVal HardCodedSVal = State->getSVal(CondVarExpr, LCtx);
+  if (auto HardCodedCI = HardCodedSVal.getAs())
+return &HardCodedCI->getValue();

NoQ wrote:
> I don't see this triggered on tests. It looks to me that this function is for 
> now only applied to `DeclRefExpr`s that are always glvalues and therefore 
> should never be evaluated to a `nonloc` value.
Good catch!


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] ConditionBRVisitor: enhance to write out more information

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Instead of having those as events similar to "Assuming", we could turn the new 
"Knowing" pieces into floating pop-ups - imagine you hover the mouse over a 
condition `foo()` and it says "`foo()` is false". That is, instead of 
`PathDiagnosticsEventPiece`, a new kind of piece can be introduced that is 
shown in scan-build as a pop-up similar to macro pop-ups. Like this:

F8556875: mock.png 

We already have infrastructure for hover pop-up hints in HTML reports - we use 
it to display macro expansions, so it could be something similar, just of a 
different color.


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] ConditionBRVisitor: enhance to write out more information

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Btw, did you try running this patch against big projects? 'Cause it's a 
high-impact change and we'd have to be careful with it.

I ran it on LLVM real quick and noticed that 97.3% (!) of reports contained at 
least one of the new notes, with number of HTML path pieces increased by 23.7% 
on average (note that the impact on plist reports would be much higher because 
they don't have any text for gray pieces).

I'll try to get back with some actual hand-picked examples to demonstrate my 
impressions. My overall feel was that it increases quality of life quite a bit 
by reducing the amount of time spent scrolling and jumping around the report - 
the necessary information becomes available exactly where you need it and it's 
wonderful. At the same time, i noticed a few corner-cases where the new pieces 
were duplicating the existing information or otherwise not helpful:

F8520276: operator_question_mark.png  
F8520282: operator_question_mark.html 

F8520283: more_repeats.png  F8520285: 
more_repeats.html 

This doesn't look like a fundamental problem to me; it should be possible to 
improve upon these corner-cases by varying note messages; probably it also 
makes sense to skip some of them when they're clearly saying the same info 10 
times in a row, but i'm not sure it's easy to decide where to apply that.

Because it might be a large amount of work, i'm thinking about committing this 
new feature off-by-default first (eg., `-analyzer-config 
knowing-notes=[false|true]`) and then doing follow-up commits (you already have 
quite a bit!) to make sure it's polished when we turn it on for everyone, WDYT?




Comment at: 
include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:169-173
+  // If the constraint information is changed between the current and the
+  // previous program state we assuming the newly seen constraint information.
+  // If we cannot evaluate the condition (and the constraints are the same)
+  // the analyzer has no information about the value and just assuming it.
+  bool IsAssuming;

Does this absolutely need to be a member variable? Maybe pass it down the 
stack, like `tookTrue`? I understand that there are already a lot of flags 
passed down, but global mutable state is still more dangerous and can easily 
get out of hand.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:187-190
+  // The value may hard-coded.
+  SVal HardCodedSVal = State->getSVal(CondVarExpr, LCtx);
+  if (auto HardCodedCI = HardCodedSVal.getAs())
+return &HardCodedCI->getValue();

I don't see this triggered on tests. It looks to me that this function is for 
now only applied to `DeclRefExpr`s that are always glvalues and therefore 
should never be evaluated to a `nonloc` value.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1991-1992
   return std::make_shared(
-  Loc, tookTrue ? GenericTrueMessage : GenericFalseMessage);
+  Loc, IsAssuming ? tookTrue ? AssumingTrueMessage : AssumingFalseMessage
+  : tookTrue ? TrueMessage : FalseMessage);
 }

I advocate for more parentheses :)


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