[PATCH] D64272: [analyzer] Note last writes to a condition only in a nested stackframe

2019-08-14 Thread Kristóf Umann via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL368817: [analyzer] Note last writes to a condition only in a 
nested stackframe (authored by Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64272?vs=211758=215062#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64272

Files:
  cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  cfe/trunk/test/Analysis/track-control-dependency-conditions.cpp

Index: cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -132,6 +132,7 @@
 
   using TrackingKind = bugreporter::TrackingKind;
   TrackingKind TKind;
+  const StackFrameContext *OriginSFC;
 
 public:
   /// Creates a visitor for every VarDecl inside a Stmt and registers it with
@@ -145,11 +146,18 @@
   /// \param EnableNullFPSuppression Whether we should employ false positive
   /// suppression (inlined defensive checks, returned null).
   /// \param TKind May limit the amount of notes added to the bug report.
+  /// \param OriginSFC Only adds notes when the last store happened in a
+  ///different stackframe to this one. Disregarded if the tracking kind
+  ///is thorough.
+  ///This is useful, because for non-tracked regions, notes about
+  ///changes to its value in a nested stackframe could be pruned, and
+  ///this visitor can prevent that without polluting the bugpath too
+  ///much.
   FindLastStoreBRVisitor(KnownSVal V, const MemRegion *R,
- bool InEnableNullFPSuppression,
- TrackingKind TKind)
+ bool InEnableNullFPSuppression, TrackingKind TKind,
+ const StackFrameContext *OriginSFC = nullptr)
   : R(R), V(V), EnableNullFPSuppression(InEnableNullFPSuppression),
-TKind(TKind) {
+TKind(TKind), OriginSFC(OriginSFC) {
 assert(R);
   }
 
Index: cfe/trunk/test/Analysis/track-control-dependency-conditions.cpp
===
--- cfe/trunk/test/Analysis/track-control-dependency-conditions.cpp
+++ cfe/trunk/test/Analysis/track-control-dependency-conditions.cpp
@@ -127,10 +127,9 @@
 void test() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
-  if (int flag = foo()) // tracking-note{{'flag' initialized here}}
-// debug-note@-1{{Tracking condition 'flag'}}
-// expected-note@-2{{Assuming 'flag' is not equal to 0}}
-// expected-note@-3{{Taking true branch}}
+  if (int flag = foo()) // debug-note{{Tracking condition 'flag'}}
+// expected-note@-1{{Assuming 'flag' is not equal to 0}}
+// expected-note@-2{{Taking true branch}}
 
 *x = 5; // expected-warning{{Dereference of null pointer}}
 // expected-note@-1{{Dereference of null pointer}}
@@ -157,6 +156,28 @@
 
 } // end of namespace variable_declaration_in_condition
 
+namespace note_from_different_but_not_nested_stackframe {
+
+void nullptrDeref(int *ptr, bool True) {
+  if (True) // expected-note{{'True' is true}}
+// expected-note@-1{{Taking true branch}}
+// debug-note@-2{{Tracking condition 'True}}
+*ptr = 5;
+  // expected-note@-1{{Dereference of null pointer (loaded from variable 'ptr')}}
+  // expected-warning@-2{{Dereference of null pointer (loaded from variable 'ptr')}}
+}
+
+void f() {
+  int *ptr = nullptr;
+  // expected-note@-1{{'ptr' initialized to a null pointer value}}
+  bool True = true;
+  nullptrDeref(ptr, True);
+  // expected-note@-1{{Passing null pointer value via 1st parameter 'ptr'}}
+  // expected-note@-2{{Calling 'nullptrDeref'}}
+}
+
+} // end of namespace note_from_different_but_not_nested_stackframe
+
 namespace important_returning_pointer_loaded_from {
 bool coin();
 
@@ -194,8 +215,8 @@
 int *getIntPtr();
 
 int *conjurePointer() {
-  int *i = getIntPtr(); // tracking-note{{'i' initialized here}}
-  return i; // tracking-note{{Returning pointer (loaded from 'i')}}
+  int *i = getIntPtr();
+  return i;
 }
 
 void f(int *ptr) {
@@ -203,11 +224,9 @@
// expected-note@-1{{Taking false branch}}
 ;
   if (!conjurePointer())
-// tracking-note@-1{{Calling 'conjurePointer'}}
-// tracking-note@-2{{Returning from 'conjurePointer'}}
-// debug-note@-3{{Tracking condition '!conjurePointer()'}}
-// expected-note@-4{{Assuming the condition is

[PATCH] D64272: [analyzer] Note last writes to a condition only in a nested stackframe

2019-07-25 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus updated this revision to Diff 211758.
Szelethus added a comment.

- Only in a **nested** stackframe. Not different stackframe.


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

https://reviews.llvm.org/D64272

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- clang/test/Analysis/track-control-dependency-conditions.cpp
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -127,10 +127,9 @@
 void test() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
-  if (int flag = foo()) // tracking-note{{'flag' initialized here}}
-// debug-note@-1{{Tracking condition 'flag'}}
-// expected-note@-2{{Assuming 'flag' is not equal to 0}}
-// expected-note@-3{{Taking true branch}}
+  if (int flag = foo()) // debug-note{{Tracking condition 'flag'}}
+// expected-note@-1{{Assuming 'flag' is not equal to 0}}
+// expected-note@-2{{Taking true branch}}
 
 *x = 5; // expected-warning{{Dereference of null pointer}}
 // expected-note@-1{{Dereference of null pointer}}
@@ -157,6 +156,28 @@
 
 } // end of namespace variable_declaration_in_condition
 
+namespace note_from_different_but_not_nested_stackframe {
+
+void nullptrDeref(int *ptr, bool True) {
+  if (True) // expected-note{{'True' is true}}
+// expected-note@-1{{Taking true branch}}
+// debug-note@-2{{Tracking condition 'True}}
+*ptr = 5;
+// expected-note@-1{{Dereference of null pointer (loaded from variable 'ptr')}}
+// expected-warning@-2{{Dereference of null pointer (loaded from variable 'ptr')}}
+}
+
+void f() {
+  int *ptr = nullptr;
+  // expected-note@-1{{'ptr' initialized to a null pointer value}}
+  bool True = true;
+  nullptrDeref(ptr, True);
+  // expected-note@-1{{Passing null pointer value via 1st parameter 'ptr'}}
+  // expected-note@-2{{Calling 'nullptrDeref'}}
+}
+
+} // end of namespace note_from_different_but_not_nested_stackframe
+
 namespace important_returning_pointer_loaded_from {
 bool coin();
 
@@ -194,8 +215,8 @@
 int *getIntPtr();
 
 int *conjurePointer() {
-  int *i = getIntPtr(); // tracking-note{{'i' initialized here}}
-  return i; // tracking-note{{Returning pointer (loaded from 'i')}}
+  int *i = getIntPtr();
+  return i;
 }
 
 void f(int *ptr) {
@@ -203,11 +224,9 @@
// expected-note@-1{{Taking false branch}}
 ;
   if (!conjurePointer())
-// tracking-note@-1{{Calling 'conjurePointer'}}
-// tracking-note@-2{{Returning from 'conjurePointer'}}
-// debug-note@-3{{Tracking condition '!conjurePointer()'}}
-// expected-note@-4{{Assuming the condition is true}}
-// expected-note@-5{{Taking true branch}}
+// debug-note@-1{{Tracking condition '!conjurePointer()'}}
+// expected-note@-2{{Assuming the condition is true}}
+// expected-note@-3{{Taking true branch}}
 *ptr = 5; // expected-warning{{Dereference of null pointer}}
   // expected-note@-1{{Dereference of null pointer}}
 }
@@ -225,10 +244,9 @@
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
   if (cast(conjure()))
-// tracking-note@-1{{Passing value via 1st parameter 'P'}}
-// debug-note@-2{{Tracking condition 'cast(conjure())'}}
-// expected-note@-3{{Assuming the condition is false}}
-// expected-note@-4{{Taking false branch}}
+// debug-note@-1{{Tracking condition 'cast(conjure())'}}
+// expected-note@-2{{Assuming the condition is false}}
+// expected-note@-3{{Taking false branch}}
 return;
   *x = 5; // expected-warning{{Dereference of null pointer}}
   // expected-note@-1{{Dereference of null pointer}}
@@ -284,7 +302,7 @@
 int getInt();
 
 void f() {
-  int flag = getInt(); // tracking-note{{'flag' initialized here}}
+  int flag = getInt();
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   if (flag) // expected-note{{Assuming 'flag' is not equal to 0}}
 // expected-note@-1{{Taking true branch}}
@@ -299,8 +317,8 @@
 int getInt();
 
 void f(int y) {
-  y = 1; // tracking-note{{The value 1 is assigned to 'y'}}
-  flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
+  y = 1;
+  flag = y;
 
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   if (flag) // expected-note{{'flag' is 1}}
@@ -317,9 +335,8 @@
 
 void foo() {
   int y;
-  y = 1; // tracking-note{{The value 1 is assigned to 'y'}}
+  y = 1;
   flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
-
 }
 
 void f(int y) {
@@ -350,7 +367,7 @@
 
   foo(); // tracking-note{{Calling 'foo'}}
  //

[PATCH] D64272: [analyzer] Note last writes to a condition only in a nested stackframe

2019-07-18 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus marked 2 inline comments as done.
Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1447-1454
   // If we have an expression that provided the value, try to track where it
   // came from.
   if (InitE) {
 if (!IsParam)
   InitE = InitE->IgnoreParenCasts();
 
 trackExpressionValue(StoreSite, InitE, BR, EnableNullFPSuppression, TKind);

NoQ wrote:
> Yeah, interesting, i guess we should also not track the value further. 
> Otherwise it'd be weird that one piece in the middle is missing but 
> everything else is still there.
That is what D64271 was for, and I think it would be a bad idea. What we should 
rather do is have a better note message:
"The value of 'flag' will be used in a condition expression later" or something 
like that.



Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:263
 void foo() {
   flag = getInt(); // tracking-note{{Value assigned to 'flag'}}
 }

Like here. We'd lose this note if we didn't track further. I'm already 
gathering data tho!


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

https://reviews.llvm.org/D64272



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

[PATCH] D64272: [analyzer] Note last writes to a condition only in a nested stackframe

2019-07-17 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1447-1454
   // If we have an expression that provided the value, try to track where it
   // came from.
   if (InitE) {
 if (!IsParam)
   InitE = InitE->IgnoreParenCasts();
 
 trackExpressionValue(StoreSite, InitE, BR, EnableNullFPSuppression, TKind);

Yeah, interesting, i guess we should also not track the value further. 
Otherwise it'd be weird that one piece in the middle is missing but everything 
else is still there.


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

https://reviews.llvm.org/D64272



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

[PATCH] D64272: [analyzer] Note last writes to a condition only in a nested stackframe

2019-07-17 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

I have one small question otherwise looks good.




Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1457
+  if (TKind == TrackingKind::ConditionTracking &&
+  StoreSite->getStackFrame() == OriginSFC)
+return nullptr;

I wonder if "nested" is a good term in the title of this revision. We could 
also show notes in the caller right? So it might also be an "enclosing" frame. 
And if so, is it desired? Since the "enclosing" frames should already be 
visible for the user without the additional notes. But correct me if I'm wrong 
:)


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

https://reviews.llvm.org/D64272



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

[PATCH] D64272: [analyzer] Note last writes to a condition only in a nested stackframe

2019-07-17 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus updated this revision to Diff 210340.

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

https://reviews.llvm.org/D64272

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- clang/test/Analysis/track-control-dependency-conditions.cpp
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -127,10 +127,9 @@
 void test() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
-  if (int flag = foo()) // tracking-note{{'flag' initialized here}}
-// debug-note@-1{{Tracking condition 'flag'}}
-// expected-note@-2{{Assuming 'flag' is not equal to 0}}
-// expected-note@-3{{Taking true branch}}
+  if (int flag = foo()) // debug-note{{Tracking condition 'flag'}}
+// expected-note@-1{{Assuming 'flag' is not equal to 0}}
+// expected-note@-2{{Taking true branch}}
 
 *x = 5; // expected-warning{{Dereference of null pointer}}
 // expected-note@-1{{Dereference of null pointer}}
@@ -205,7 +204,7 @@
 int getInt();
 
 void f() {
-  int flag = getInt(); // tracking-note{{'flag' initialized here}}
+  int flag = getInt();
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   if (flag) // expected-note{{Assuming 'flag' is not equal to 0}}
 // expected-note@-1{{Taking true branch}}
@@ -220,8 +219,8 @@
 int getInt();
 
 void f(int y) {
-  y = 1; // tracking-note{{The value 1 is assigned to 'y'}}
-  flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
+  y = 1;
+  flag = y;
 
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   if (flag) // expected-note{{'flag' is 1}}
@@ -238,9 +237,8 @@
 
 void foo() {
   int y;
-  y = 1; // tracking-note{{The value 1 is assigned to 'y'}}
+  y = 1;
   flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
-
 }
 
 void f(int y) {
@@ -271,7 +269,7 @@
 
   foo(); // tracking-note{{Calling 'foo'}}
  // tracking-note@-1{{Returning from 'foo'}}
-  y = flag; // tracking-note{{Value assigned to 'y'}}
+  y = flag;
 
   if (y) // expected-note{{Assuming 'y' is not equal to 0}}
  // expected-note@-1{{Taking true branch}}
@@ -320,7 +318,7 @@
 void f(int flag) {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
-  flag = getInt(); // tracking-note{{Value assigned to 'flag'}}
+  flag = getInt();
   assert(flag); // tracking-note{{Calling 'assert'}}
 // tracking-note@-1{{Returning from 'assert'}}
 
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1453,6 +1453,10 @@
 trackExpressionValue(StoreSite, InitE, BR, EnableNullFPSuppression, TKind);
   }
 
+  if (TKind == TrackingKind::ConditionTracking &&
+  StoreSite->getStackFrame() == OriginSFC)
+return nullptr;
+
   // Okay, we've found the binding. Emit an appropriate message.
   SmallString<256> sbuf;
   llvm::raw_svector_ostream os(sbuf);
@@ -1478,7 +1482,7 @@
   if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
 if (auto KV = State->getSVal(OriginalR).getAs())
   BR.addVisitor(llvm::make_unique(
-  *KV, OriginalR, EnableNullFPSuppression, TKind));
+  *KV, OriginalR, EnableNullFPSuppression, TKind, OriginSFC));
   }
 }
   }
@@ -1910,6 +1914,7 @@
 return false;
 
   ProgramStateRef LVState = LVNode->getState();
+	const StackFrameContext *SFC = LVNode->getStackFrame();
 
   // We only track expressions if we believe that they are important. Chances
   // are good that control dependencies to the tracking point are also improtant
@@ -1945,7 +1950,7 @@
 if (RR && !LVIsNull)
   if (auto KV = LVal.getAs())
 report.addVisitor(llvm::make_unique(
-  *KV, RR, EnableNullFPSuppression, TKind));
+  *KV, RR, EnableNullFPSuppression, TKind, SFC));
 
 // In case of C++ references, we want to differentiate between a null
 // reference and reference to null pointer.
@@ -1982,7 +1987,7 @@
 
   if (auto KV = V.getAs())
 report.addVisitor(llvm::make_unique(
-  *KV, R, EnableNullFPSuppression, TKind));
+  *KV, R, EnableNullFPSuppression, TKind, SFC));
   return true;
 }
   }
@@ -2022,7 +2027,7 @@
 if (CanDereference)
   if (auto KV = RVal.getAs())
 report.addVisitor(llvm::make_unique(
-*KV, L->getRegion(), EnableNullFPSuppression, TKind));
+

[PATCH] D64272: [analyzer] Note last writes to a condition only in a nested stackframe

2019-07-14 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Yup, looks good!




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:146-149
+  ///This is useful, because for non-tracked regions, notes about
+  ///changes to its value in a nested stackframe could be pruned, and
+  ///this visitor can prevent that without polluting the bugpath too
+  ///much.

I definitely don't don't don't don't dislike quadruple negations.


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

https://reviews.llvm.org/D64272



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

[PATCH] D64272: [analyzer] Note last writes to a condition only in a nested stackframe

2019-07-14 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus added a comment.

Gentle ping.


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

https://reviews.llvm.org/D64272



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

[PATCH] D64272: [analyzer] Note last writes to a condition only in a nested stackframe

2019-07-06 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus updated this revision to Diff 208275.
Szelethus added a comment.

Rebase after D64271  being abandoned.


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

https://reviews.llvm.org/D64272

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- clang/test/Analysis/track-control-dependency-conditions.cpp
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -129,10 +129,9 @@
 
   if (int flag = foo()) // tracking-note{{Calling 'foo'}}
 // tracking-note@-1{{Returning from 'foo'}}
-// tracking-note@-2{{'flag' initialized here}}
-// debug-note@-3{{Tracking condition 'flag'}}
-// expected-note@-4{{Assuming 'flag' is not equal to 0}}
-// expected-note@-5{{Taking true branch}}
+// debug-note@-2{{Tracking condition 'flag'}}
+// expected-note@-3{{Assuming 'flag' is not equal to 0}}
+// expected-note@-4{{Taking true branch}}
 
 *x = 5; // expected-warning{{Dereference of null pointer}}
 // expected-note@-1{{Dereference of null pointer}}
@@ -211,7 +210,7 @@
 int getInt();
 
 void f() {
-  int flag = getInt(); // tracking-note{{'flag' initialized here}}
+  int flag = getInt();
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   if (flag) // expected-note{{Assuming 'flag' is not equal to 0}}
 // expected-note@-1{{Taking true branch}}
@@ -226,9 +225,8 @@
 int getInt();
 
 void f(int y) {
-  y = 1; // tracking-note{{The value 1 is assigned to 'y'}}
-  flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
-
+  y = 1;
+  flag = y;
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   if (flag) // expected-note{{'flag' is 1}}
 // expected-note@-1{{Taking true branch}}
@@ -244,7 +242,7 @@
 
 void foo() {
   int y;
-  y = 1; // tracking-note{{The value 1 is assigned to 'y'}}
+  y = 1;
   flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
 
 }
@@ -277,7 +275,7 @@
 
   foo(); // tracking-note{{Calling 'foo'}}
  // tracking-note@-1{{Returning from 'foo'}}
-  y = flag; // tracking-note{{Value assigned to 'y'}}
+  y = flag;
 
   if (y) // expected-note{{Assuming 'y' is not equal to 0}}
  // expected-note@-1{{Taking true branch}}
@@ -326,7 +324,7 @@
 void f(int flag) {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
-  flag = getInt(); // tracking-note{{Value assigned to 'flag'}}
+  flag = getInt();
   assert(flag); // tracking-note{{Calling 'assert'}}
 // tracking-note@-1{{Returning from 'assert'}}
 
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1421,6 +1421,7 @@
 
   if (!StoreSite)
 return nullptr;
+
   Satisfied = true;
 
   // If we have an expression that provided the value, try to track where it
@@ -1429,10 +1430,14 @@
 if (!IsParam)
   InitE = InitE->IgnoreParenCasts();
 
-bugreporter::trackExpressionValue(StoreSite, InitE, BR,
-  EnableNullFPSuppression);
+::trackExpressionValue(StoreSite, InitE, BR,
+   EnableNullFPSuppression, TKind);
   }
 
+  if (TKind == TrackingKind::ConditionTracking &&
+  StoreSite->getStackFrame() == OriginSFC)
+return nullptr;
+
   // Okay, we've found the binding. Emit an appropriate message.
   SmallString<256> sbuf;
   llvm::raw_svector_ostream os(sbuf);
@@ -1458,7 +1463,7 @@
   if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
 if (auto KV = State->getSVal(OriginalR).getAs())
   BR.addVisitor(llvm::make_unique(
-  *KV, OriginalR, EnableNullFPSuppression, TKind));
+  *KV, OriginalR, EnableNullFPSuppression, TKind, OriginSFC));
   }
 }
   }
@@ -1890,6 +1895,7 @@
 return false;
 
   ProgramStateRef LVState = LVNode->getState();
+  const StackFrameContext *SFC = LVNode->getStackFrame();
 
   // We only track expressions if we believe that they are important. Chances
   // are good that control dependencies to the tracking point are also improtant
@@ -1925,7 +1931,7 @@
 if (RR && !LVIsNull)
   if (auto KV = LVal.getAs())
 report.addVisitor(llvm::make_unique(
-  *KV, RR, EnableNullFPSuppression, TKind));
+  *KV, RR, EnableNullFPSuppression, TKind, SFC));
 
 //

[PATCH] D64272: [analyzer] Note last writes to a condition only in a nested stackframe

2019-07-05 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, rnkovacs, dcoughlin, Charusso, 
baloghadamsoftware.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, szepet, whisperity.
Szelethus added a parent revision: D64271: [analyzer] Don't track the right 
hand side of the last store for conditions.

Exactly what it says on the tin! The comments in the code detail this a little 
more too.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64272

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- clang/test/Analysis/track-control-dependency-conditions.cpp
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -127,10 +127,9 @@
 void test() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
-  if (int flag = foo()) // tracking-note{{'flag' initialized here}}
-// debug-note@-1{{Tracking condition 'flag'}}
-// expected-note@-2{{Assuming 'flag' is not equal to 0}}
-// expected-note@-3{{Taking true branch}}
+  if (int flag = foo()) // debug-note{{Tracking condition 'flag'}}
+// expected-note@-1{{Assuming 'flag' is not equal to 0}}
+// expected-note@-2{{Taking true branch}}
 
 *x = 5; // expected-warning{{Dereference of null pointer}}
 // expected-note@-1{{Dereference of null pointer}}
@@ -209,7 +208,7 @@
 int getInt();
 
 void f() {
-  int flag = getInt(); // tracking-note{{'flag' initialized here}}
+  int flag = getInt();
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   if (flag) // expected-note{{Assuming 'flag' is not equal to 0}}
 // expected-note@-1{{Taking true branch}}
@@ -225,7 +224,7 @@
 
 void f(int y) {
   y = 1;
-  flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
+  flag = y;
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   if (flag) // expected-note{{'flag' is 1}}
 // expected-note@-1{{Taking true branch}}
@@ -298,7 +297,7 @@
 void f(int flag) {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
-  flag = getInt(); // tracking-note{{Value assigned to 'flag'}}
+  flag = getInt();
   assert(flag); // tracking-note{{Calling 'assert'}}
 // tracking-note@-1{{Returning from 'assert'}}
 
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1421,6 +1421,11 @@
 
   if (!StoreSite)
 return nullptr;
+
+  if (TKind != TrackingKind::ThoroughTracking &&
+  StoreSite->getStackFrame() == OriginSFC)
+return nullptr;
+
   Satisfied = true;
 
   // If we have an expression that provided the value, try to track where it
@@ -1466,7 +1471,7 @@
   if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
 if (auto KV = State->getSVal(OriginalR).getAs())
   BR.addVisitor(llvm::make_unique(
-  *KV, OriginalR, EnableNullFPSuppression, TKind));
+  *KV, OriginalR, EnableNullFPSuppression, TKind, OriginSFC));
   }
 }
   }
@@ -1898,6 +1903,7 @@
 return false;
 
   ProgramStateRef LVState = LVNode->getState();
+  const StackFrameContext *SFC = LVNode->getStackFrame();
 
   // We only track expressions if we believe that they are important. Chances
   // are good that control dependencies to the tracking point are also improtant
@@ -1933,7 +1939,7 @@
 if (RR && !LVIsNull)
   if (auto KV = LVal.getAs())
 report.addVisitor(llvm::make_unique(
-  *KV, RR, EnableNullFPSuppression, TKind));
+  *KV, RR, EnableNullFPSuppression, TKind, SFC));
 
 // In case of C++ references, we want to differentiate between a null
 // reference and reference to null pointer.
@@ -1970,7 +1976,7 @@
 
   if (auto KV = V.getAs())
 report.addVisitor(llvm::make_unique(
-  *KV, R, EnableNullFPSuppression, TKind));
+  *KV, R, EnableNullFPSuppression, TKind, SFC));
   return true;
 }
   }
@@ -2008,7 +2014,7 @@
 if (CanDereference)
   if (auto KV = RVal.getAs())
 report.addVisitor(llvm::make_unique(
-*KV, L->getRegion(), EnableNullFPSuppression, TKind));
+*KV, L->getRegion(), EnableNullFPSuppression, TKind, SFC));
 
 const MemRegion *RegionRVal = RVal.getAsRegion();
 if (RegionRVal && isa(RegionRVal)) {
Index: