[PATCH] D65724: [analyzer] Don't make ConditionBRVisitor events prunable when the condition is an interesting field

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rL369589: [analyzer] Dont make ConditionBRVisitor events 
prunable when the condition is… (authored by Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65724?vs=216283=216497#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65724

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/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -180,21 +180,44 @@
 RLCV->getStore() == RightNode->getState()->getStore();
 }
 
-static Optional
-getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) {
+static Optional getSValForVar(const Expr *CondVarExpr,
+const ExplodedNode *N) {
   ProgramStateRef State = N->getState();
   const LocationContext *LCtx = N->getLocationContext();
 
+  assert(CondVarExpr);
+  CondVarExpr = CondVarExpr->IgnoreImpCasts();
+
   // 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();
-}
-  }
+  // FIXME: As seen in VisitCommonDeclRefExpr, sometimes DeclRefExpr may
+  // evaluate to a FieldRegion when it refers to a declaration of a lambda
+  // capture variable. We most likely need to duplicate that logic here.
+  if (const auto *DRE = dyn_cast(CondVarExpr))
+if (const auto *VD = dyn_cast(DRE->getDecl()))
+  return State->getSVal(State->getLValue(VD, LCtx));
+
+  if (const auto *ME = dyn_cast(CondVarExpr))
+if (const auto *FD = dyn_cast(ME->getMemberDecl()))
+  if (auto FieldL = State->getSVal(ME, LCtx).getAs())
+return State->getRawSVal(*FieldL, FD->getType());
 
-  return {};
+  return None;
+}
+
+static Optional
+getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) {
+
+  if (Optional V = getSValForVar(CondVarExpr, N))
+if (auto CI = V->getAs())
+  return >getValue();
+  return None;
+}
+
+static bool isInterestingExpr(const Expr *E, const ExplodedNode *N,
+  const BugReport *B) {
+  if (Optional V = getSValForVar(E, N))
+return B->getInterestingnessKind(*V).hasValue();
+  return false;
 }
 
 /// \return name of the macro inside the location \p Loc.
@@ -2475,17 +2498,11 @@
 
   const LocationContext *LCtx = N->getLocationContext();
   PathDiagnosticLocation Loc(CondVarExpr, BRC.getSourceManager(), LCtx);
+
   auto event = std::make_shared(Loc, Out.str());
 
-  if (const auto *DR = dyn_cast(CondVarExpr)) {
-if (const auto *VD = dyn_cast(DR->getDecl())) {
-  const ProgramState *state = N->getState().get();
-  if (const MemRegion *R = state->getLValue(VD, LCtx).getAsRegion()) {
-if (report.isInteresting(R))
-  event->setPrunable(false);
-  }
-}
-  }
+  if (isInterestingExpr(CondVarExpr, N, ))
+event->setPrunable(false);
 
   return event;
 }
@@ -2515,16 +2532,10 @@
 
   PathDiagnosticLocation Loc(Cond, BRC.getSourceManager(), LCtx);
   auto event = std::make_shared(Loc, Out.str());
-  const ProgramState *state = N->getState().get();
-  if (const MemRegion *R = state->getLValue(VD, LCtx).getAsRegion()) {
-if (report.isInteresting(R))
-  event->setPrunable(false);
-else {
-  SVal V = state->getSVal(R);
-  if (report.isInteresting(V))
-event->setPrunable(false);
-}
-  }
+
+  if (isInterestingExpr(DRE, N, ))
+event->setPrunable(false);
+
   return std::move(event);
 }
 
@@ -2555,7 +2566,10 @@
   if (!IsAssuming)
 return std::make_shared(Loc, Out.str());
 
-  return std::make_shared(Loc, Out.str());
+  auto event = std::make_shared(Loc, Out.str());
+  if (isInterestingExpr(ME, N, ))
+event->setPrunable(false);
+  return event;
 }
 
 bool ConditionBRVisitor::printValue(const Expr *CondVarExpr, raw_ostream ,
@@ -2595,10 +2609,8 @@
   return true;
 }
 
-const char *const ConditionBRVisitor::GenericTrueMessage =
-"Assuming the condition is true";
-const char *const ConditionBRVisitor::GenericFalseMessage =
-"Assuming the condition is false";
+constexpr llvm::StringLiteral ConditionBRVisitor::GenericTrueMessage;
+constexpr llvm::StringLiteral ConditionBRVisitor::GenericFalseMessage;
 
 bool ConditionBRVisitor::isPieceMessageGeneric(
 const PathDiagnosticPiece *Piece) {

[PATCH] D65724: [analyzer] Don't make ConditionBRVisitor events prunable when the condition is an interesting field

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

LGTM!


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

https://reviews.llvm.org/D65724



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

[PATCH] D65724: [analyzer] Don't make ConditionBRVisitor events prunable when the condition is an interesting field

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

Szelethus updated this revision to Diff 216283.
Szelethus added a comment.

- Add the `FIXME`
- Cascade test changes from D65575 


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

https://reviews.llvm.org/D65724

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
@@ -407,6 +407,91 @@
 }
 } // end of namespace condition_written_in_nested_stackframe_before_assignment
 
+namespace condition_lambda_capture_by_reference_last_write {
+int getInt();
+
+[[noreturn]] void halt();
+
+void f(int flag) {
+  int *x = 0; // expected-note-re^}}'x' initialized to a null pointer value{{$
+
+  auto lambda = []() {
+flag = getInt(); // tracking-note-re^}}Value assigned to 'flag', which participates in a condition later{{$
+  };
+
+  lambda(); // tracking-note-re^}}Calling 'operator()'{{$
+// tracking-note-re@-1^}}Returning from 'operator()'{{$
+
+  if (flag) // expected-note-re^}}Assuming 'flag' is not equal to 0{{$
+// expected-note-re@-1^}}Taking true branch{{$
+// debug-note-re@-2^}}Tracking condition 'flag'{{$
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace condition_lambda_capture_by_reference_last_write
+
+namespace condition_lambda_capture_by_value_assumption {
+int getInt();
+
+[[noreturn]] void halt();
+
+void bar(int ) {
+  flag = getInt(); // tracking-note-re^}}Value assigned to 'flag', which participates in a condition later{{$
+}
+
+void f(int flag) {
+  int *x = 0; // expected-note-re^}}'x' initialized to a null pointer value{{$
+
+  auto lambda = [flag]() {
+if (!flag) // tracking-note-re^}}Assuming 'flag' is not equal to 0{{$
+   // tracking-note-re@-1^}}Taking false branch{{$
+  halt();
+  };
+
+  bar(flag); // tracking-note-re^}}Calling 'bar'{{$
+ // tracking-note-re@-1^}}Returning from 'bar'{{$
+  lambda();  // tracking-note-re^}}Calling 'operator()'{{$
+ // tracking-note-re@-1^}}Returning from 'operator()'{{$
+
+  if (flag) // expected-note-re^}}Assuming 'flag' is not equal to 0{{$
+// expected-note-re@-1^}}Taking true branch{{$
+// debug-note-re@-2^}}Tracking condition 'flag'{{$
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace condition_lambda_capture_by_value_assumption
+
+namespace condition_lambda_capture_by_reference_assumption {
+int getInt();
+
+[[noreturn]] void halt();
+
+void bar(int ) {
+  flag = getInt(); // tracking-note-re^}}Value assigned to 'flag', which participates in a condition later{{$
+}
+
+void f(int flag) {
+  int *x = 0; // expected-note-re^}}'x' initialized to a null pointer value{{$
+
+  auto lambda = []() {
+if (!flag) // tracking-note-re^}}Assuming 'flag' is not equal to 0{{$
+   // tracking-note-re@-1^}}Taking false branch{{$
+  halt();
+  };
+
+  bar(flag); // tracking-note-re^}}Calling 'bar'{{$
+ // tracking-note-re@-1^}}Returning from 'bar'{{$
+  lambda();  // tracking-note-re^}}Calling 'operator()'{{$
+ // tracking-note-re@-1^}}Returning from 'operator()'{{$
+
+  if (flag) // expected-note-re^}}'flag' is not equal to 0{{$
+// expected-note-re@-1^}}Taking true branch{{$
+// debug-note-re@-2^}}Tracking condition 'flag'}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace condition_lambda_capture_by_reference_assumption
+
 namespace collapse_point_not_in_condition {
 
 [[noreturn]] void halt();
@@ -459,6 +544,35 @@
 
 } // end of namespace unimportant_write_before_collapse_point
 
+namespace collapse_point_not_in_condition_as_field {
+
+[[noreturn]] void halt();
+struct IntWrapper {
+  int b;
+  IntWrapper();
+
+  void check() {
+if (!b) // tracking-note-re^}}Assuming field 'b' is not equal to 0{{$
+// tracking-note-re@-1^}}Taking false branch{{$
+  halt();
+return;
+  }
+};
+
+void f(IntWrapper i) {
+  int *x = 0; // expected-note-re^}}'x' initialized to a null pointer value{{$
+
+  i.check(); // tracking-note-re^}}Calling 'IntWrapper::check'{{$
+

[PATCH] D65724: [analyzer] Don't make ConditionBRVisitor events prunable when the condition is an interesting field

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:192-194
+  if (const auto *DRE = dyn_cast(CondVarExpr))
+if (const auto *VD = dyn_cast(DRE->getDecl()))
+  return State->getSVal(State->getLValue(VD, LCtx));

> Also, could you please detail why we think this doesn't/shouldn't work?

FIXME: As seen in `VisitCommonDeclRefExpr`, sometimes `DeclRefExpr` may 
evaluate to a `FieldRegion` when it refers to a declaration of a lambda capture 
variable. We most likely need to duplicate that logic here.


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

https://reviews.llvm.org/D65724



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

[PATCH] D65724: [analyzer] Don't make ConditionBRVisitor events prunable when the condition is an interesting field

[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:192-194
+  if (const auto *DRE = dyn_cast(CondVarExpr))
+if (const auto *VD = dyn_cast(DRE->getDecl()))
+  return State->getSVal(State->getLValue(VD, LCtx));

Szelethus wrote:
> xazax.hun wrote:
> > NoQ wrote:
> > > Ugh, so this code intentionally avoids the complexity of 
> > > `ExprEngine::VisitCommonDeclRefExpr()` so that to work differently with 
> > > references. Okay.
> > > 
> > > @xazax.hun, do you think this works correctly with lambda captures? (when 
> > > the `DeclRefExpr` to a variable might evaluate to a field within the 
> > > lambda at run-time).
> > Good point! This definitely worth a test case somewhere somehow!
> > 
> > I would expect `DRE->getDecl()` to return the captured declaration rather 
> > than the field and thus `State->getSVal(State->getLValue(VD, LCtx))` to 
> > compute the wrong answer. I might be a bit rusty on this topic so of course 
> > we would need a carefully crafted test case to make sure :)
> So, we seem to be okay with this code in the context of this patch, but have 
> voiced concerns that there probably is a testcase on which this doesn't work. 
> What kind of `FIXME` message shall I put in before commiting?
Also, could you please detail why we think this doesn't/shouldn't work?


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

https://reviews.llvm.org/D65724



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

[PATCH] D65724: [analyzer] Don't make ConditionBRVisitor events prunable when the condition is an interesting field

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

Szelethus marked 4 inline comments as done.
Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:192-194
+  if (const auto *DRE = dyn_cast(CondVarExpr))
+if (const auto *VD = dyn_cast(DRE->getDecl()))
+  return State->getSVal(State->getLValue(VD, LCtx));

xazax.hun wrote:
> NoQ wrote:
> > Ugh, so this code intentionally avoids the complexity of 
> > `ExprEngine::VisitCommonDeclRefExpr()` so that to work differently with 
> > references. Okay.
> > 
> > @xazax.hun, do you think this works correctly with lambda captures? (when 
> > the `DeclRefExpr` to a variable might evaluate to a field within the lambda 
> > at run-time).
> Good point! This definitely worth a test case somewhere somehow!
> 
> I would expect `DRE->getDecl()` to return the captured declaration rather 
> than the field and thus `State->getSVal(State->getLValue(VD, LCtx))` to 
> compute the wrong answer. I might be a bit rusty on this topic so of course 
> we would need a carefully crafted test case to make sure :)
So, we seem to be okay with this code in the context of this patch, but have 
voiced concerns that there probably is a testcase on which this doesn't work. 
What kind of `FIXME` message shall I put in before commiting?


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

https://reviews.llvm.org/D65724



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

[PATCH] D65724: [analyzer] Don't make ConditionBRVisitor events prunable when the condition is an interesting field

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

Szelethus updated this revision to Diff 215195.
Szelethus marked an inline comment as done.
Szelethus added a comment.

- Make the generic messages in-class, `constexpr` fields.
- Provide plenty of test cases for captured lambda variables. In our meeting we 
had both had a sight of relief, and some still lingering concerns about this, 
so maybe it deserves fixing in the long term.


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

https://reviews.llvm.org/D65724

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
@@ -407,6 +407,91 @@
 }
 } // end of namespace condition_written_in_nested_stackframe_before_assignment
 
+namespace condition_lambda_capture_by_reference_last_write {
+int getInt();
+
+[[noreturn]] void halt();
+
+void f(int flag) {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  auto lambda = []() {
+flag = getInt(); // tracking-note{{Value assigned to 'flag', which will be (a part of a) condition}}
+  };
+
+  lambda(); // tracking-note{{Calling 'operator()'}}
+// tracking-note@-1{{Returning from 'operator()'}}
+
+  if (flag) // expected-note{{'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+// debug-note@-2{{Tracking condition 'flag'}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace condition_lambda_capture_by_reference_last_write
+
+namespace condition_lambda_capture_by_value_assumption {
+int getInt();
+
+[[noreturn]] void halt();
+
+void bar(int ) {
+  flag = getInt(); // tracking-note{{Value assigned to 'flag', which will be (a part of a) condition}}
+}
+
+void f(int flag) {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  auto lambda = [flag]() {
+if (!flag) // tracking-note{{Assuming 'flag' is not equal to 0}}
+   // tracking-note@-1{{Taking false branch}}
+  halt();
+  };
+
+  bar(flag); // tracking-note{{Calling 'bar'}}
+ // tracking-note@-1{{Returning from 'bar'}}
+  lambda();  // tracking-note{{Calling 'operator()'}}
+ // tracking-note@-1{{Returning from 'operator()'}}
+
+  if (flag) // expected-note{{'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+// debug-note@-2{{Tracking condition 'flag'}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace condition_lambda_capture_by_value_assumption
+
+namespace condition_lambda_capture_by_reference_assumption {
+int getInt();
+
+[[noreturn]] void halt();
+
+void bar(int ) {
+  flag = getInt(); // tracking-note{{Value assigned to 'flag', which will be (a part of a) condition}}
+}
+
+void f(int flag) {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  auto lambda = []() {
+if (!flag) // tracking-note{{Assuming 'flag' is not equal to 0}}
+   // tracking-note@-1{{Taking false branch}}
+  halt();
+  };
+
+  bar(flag); // tracking-note{{Calling 'bar'}}
+ // tracking-note@-1{{Returning from 'bar'}}
+  lambda();  // tracking-note{{Calling 'operator()'}}
+ // tracking-note@-1{{Returning from 'operator()'}}
+
+  if (flag) // expected-note{{'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+// debug-note@-2{{Tracking condition 'flag'}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace condition_lambda_capture_by_reference_assumption
+
 namespace collapse_point_not_in_condition {
 
 [[noreturn]] void halt();
@@ -459,6 +544,35 @@
 
 } // end of namespace unimportant_write_before_collapse_point
 
+namespace collapse_point_not_in_condition_as_field {
+
+[[noreturn]] void halt();
+struct IntWrapper {
+  int b;
+  IntWrapper();
+
+  void check() {
+if (!b) // tracking-note{{Assuming field 'b' is not equal to 0}}
+// tracking-note@-1{{Taking false branch}}
+  halt();
+return;
+  }
+};
+
+void f(IntWrapper i) {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  i.check(); // tracking-note{{Calling 'IntWrapper::check'}}
+ // tracking-note@-1{{Returning from 'IntWrapper::check'}}
+  if (i.b)   // expected-note{{Field 'b' is not equal to 0}}
+ // expected-note@-1{{Taking true branch}}
+ // debug-note@-2{{Tracking 

[PATCH] D65724: [analyzer] Don't make ConditionBRVisitor events prunable when the condition is an interesting field

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:192-194
+  if (const auto *DRE = dyn_cast(CondVarExpr))
+if (const auto *VD = dyn_cast(DRE->getDecl()))
+  return State->getSVal(State->getLValue(VD, LCtx));

NoQ wrote:
> Ugh, so this code intentionally avoids the complexity of 
> `ExprEngine::VisitCommonDeclRefExpr()` so that to work differently with 
> references. Okay.
> 
> @xazax.hun, do you think this works correctly with lambda captures? (when the 
> `DeclRefExpr` to a variable might evaluate to a field within the lambda at 
> run-time).
Good point! This definitely worth a test case somewhere somehow!

I would expect `DRE->getDecl()` to return the captured declaration rather than 
the field and thus `State->getSVal(State->getLValue(VD, LCtx))` to compute the 
wrong answer. I might be a bit rusty on this topic so of course we would need a 
carefully crafted test case to make sure :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65724



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

[PATCH] D65724: [analyzer] Don't make ConditionBRVisitor events prunable when the condition is an interesting field

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Nice!!




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:225-226
   // FIXME: constexpr initialization isn't supported by MSVC2013.
-  static const char *const GenericTrueMessage;
-  static const char *const GenericFalseMessage;
+  static llvm::StringLiteral GenericTrueMessage;
+  static llvm::StringLiteral GenericFalseMessage;
 

https://llvm.org/doxygen/classllvm_1_1StringLiteral.html:
> In order to avoid the invocation of a global constructor, `StringLiteral` 
> should only be used in a `constexpr` context



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:192-194
+  if (const auto *DRE = dyn_cast(CondVarExpr))
+if (const auto *VD = dyn_cast(DRE->getDecl()))
+  return State->getSVal(State->getLValue(VD, LCtx));

Ugh, so this code intentionally avoids the complexity of 
`ExprEngine::VisitCommonDeclRefExpr()` so that to work differently with 
references. Okay.

@xazax.hun, do you think this works correctly with lambda captures? (when the 
`DeclRefExpr` to a variable might evaluate to a field within the lambda at 
run-time).



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:208
+  if (Optional V = getSValForVar(CondVarExpr, N))
+if (auto DeclCI = V->getAs())
+  return >getValue();

`Decl`? I guess it made sense in the original code but there's no longer an 
obvious `Decl` floating around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65724



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

[PATCH] D65724: [analyzer] Don't make ConditionBRVisitor events prunable when the condition is an interesting field

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

Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, Charusso, baloghadamsoftware, 
rnkovacs, dcoughlin.
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: D65723: [analyzer][NFC] Add different 
interestingness kinds.

Exactly what it says on the tin! Note that we're talking about interestingness 
in general, hence this isn't a control-dependency-tracking specific patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65724

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
@@ -459,6 +459,35 @@
 
 } // end of namespace unimportant_write_before_collapse_point
 
+namespace collapse_point_not_in_condition_as_field {
+
+[[noreturn]] void halt();
+struct IntWrapper {
+  int b;
+  IntWrapper();
+
+  void check() {
+if (!b) // tracking-note{{Assuming field 'b' is not equal to 0}}
+// tracking-note@-1{{Taking false branch}}
+  halt();
+return;
+  }
+};
+
+void f(IntWrapper i) {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  i.check(); // tracking-note{{Calling 'IntWrapper::check'}}
+ // tracking-note@-1{{Returning from 'IntWrapper::check'}}
+  if (i.b)   // expected-note{{Field 'b' is not equal to 0}}
+ // expected-note@-1{{Taking true branch}}
+ // debug-note@-2{{Tracking condition 'i.b'}}
+*x = 5;  // expected-warning{{Dereference of null pointer}}
+ // expected-note@-1{{Dereference of null pointer}}
+}
+
+} // end of namespace collapse_point_not_in_condition_as_field
+
 namespace dont_track_assertlike_conditions {
 
 extern void __assert_fail (__const char *__assertion, __const char *__file,
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -180,21 +180,41 @@
 RLCV->getStore() == RightNode->getState()->getStore();
 }
 
-static Optional
-getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) {
+static Optional getSValForVar(const Expr *CondVarExpr,
+const ExplodedNode *N) {
   ProgramStateRef State = N->getState();
   const LocationContext *LCtx = N->getLocationContext();
 
+  assert(CondVarExpr);
+  CondVarExpr = CondVarExpr->IgnoreImpCasts();
+
   // 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();
-}
-  }
+  if (const auto *DRE = dyn_cast(CondVarExpr))
+if (const auto *VD = dyn_cast(DRE->getDecl()))
+  return State->getSVal(State->getLValue(VD, LCtx));
+
+  if (const auto *ME = dyn_cast(CondVarExpr))
+if (const auto *FD = dyn_cast(ME->getMemberDecl()))
+  if (auto FieldL = State->getSVal(ME, LCtx).getAs())
+return State->getRawSVal(*FieldL, FD->getType());
+
+  return None;
+}
+
+static Optional
+getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) {
+
+  if (Optional V = getSValForVar(CondVarExpr, N))
+if (auto DeclCI = V->getAs())
+  return >getValue();
+  return None;
+}
 
-  return {};
+static bool isInterestingExpr(const Expr *E, const ExplodedNode *N,
+  const BugReport *B) {
+  if (Optional V = getSValForVar(E, N))
+return B->getInterestingnessKind(*V).hasValue();
+  return false;
 }
 
 /// \return name of the macro inside the location \p Loc.
@@ -2494,17 +2514,11 @@
 
   const LocationContext *LCtx = N->getLocationContext();
   PathDiagnosticLocation Loc(CondVarExpr, BRC.getSourceManager(), LCtx);
+
   auto event = std::make_shared(Loc, Out.str());
 
-  if (const auto *DR = dyn_cast(CondVarExpr)) {
-if (const auto *VD = dyn_cast(DR->getDecl())) {
-  const ProgramState *state = N->getState().get();
-  if (const MemRegion *R = state->getLValue(VD, LCtx).getAsRegion()) {
-if (report.isInteresting(R))
-  event->setPrunable(false);
-  }
-}
-  }
+  if (isInterestingExpr(CondVarExpr, N, ))
+event->setPrunable(false);
 
   return event;
 }
@@ -2532,16 +2546,10 @@
 return std::make_shared(Loc, Out.str());
 
   auto event = std::make_shared(Loc, Out.str());
-  const