Re: r338667 - [analyzer] Extend NoStoreFuncVisitor to follow fields.

[George Karpenkov via cfe-commits]

Should be fixed in trunk

> On Aug 3, 2018, at 1:30 PM, George Karpenkov via cfe-commits 
>  wrote:
> 
> Yeah, saw that as well, fix coming.
> 
>> On Aug 3, 2018, at 10:32 AM, Alexander Kornienko > > wrote:
>> 
>> 
>> 
>> On Thu, Aug 2, 2018 at 4:03 AM George Karpenkov via cfe-commits 
>> mailto:cfe-commits@lists.llvm.org>> wrote:
>> Author: george.karpenkov
>> Date: Wed Aug  1 19:02:40 2018
>> New Revision: 338667
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=338667=rev 
>> 
>> Log:
>> [analyzer] Extend NoStoreFuncVisitor to follow fields.
>> 
>> rdar://39701823 
>> 
>> Differential Revision: https://reviews.llvm.org/D49901 
>> 
>> 
>> Modified:
>> cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
>> cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.c
>> cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.cpp
>> cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.m
>> 
>> Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=338667=338666=338667=diff
>>  
>> 
>> ==
>> --- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
>> +++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Wed Aug  1 
>> 19:02:40 2018
>> @@ -269,10 +269,14 @@ namespace {
>>  /// pointer dereference outside.
>>  class NoStoreFuncVisitor final : public BugReporterVisitor {
>>const SubRegion *RegionOfInterest;
>> +  MemRegionManager 
>>const SourceManager 
>>const PrintingPolicy 
>> -  static constexpr const char *DiagnosticsMsg =
>> -  "Returning without writing to '";
>> +
>> +  /// Recursion limit for dereferencing fields when looking for the
>> +  /// region of interest.
>> +  /// The limit of two indicates that we will dereference fields only once.
>> +  static const unsigned DEREFERENCE_LIMIT = 2;
>> 
>>/// Frames writing into \c RegionOfInterest.
>>/// This visitor generates a note only if a function does not write into
>> @@ -285,15 +289,17 @@ class NoStoreFuncVisitor final : public
>>llvm::SmallPtrSet FramesModifyingRegion;
>>llvm::SmallPtrSet 
>> FramesModifyingCalculated;
>> 
>> +  using RegionVector = SmallVector;
>>  public:
>>NoStoreFuncVisitor(const SubRegion *R)
>> -  : RegionOfInterest(R),
>> -SM(R->getMemRegionManager()->getContext().getSourceManager()),
>> -PP(R->getMemRegionManager()->getContext().getPrintingPolicy()) {}
>> +  : RegionOfInterest(R), MmrMgr(*R->getMemRegionManager()),
>> +SM(MmrMgr.getContext().getSourceManager()),
>> +PP(MmrMgr.getContext().getPrintingPolicy()) {}
>> 
>>void Profile(llvm::FoldingSetNodeID ) const override {
>>  static int Tag = 0;
>>  ID.AddPointer();
>> +ID.AddPointer(RegionOfInterest);
>>}
>> 
>>std::shared_ptr VisitNode(const ExplodedNode *N,
>> @@ -307,48 +313,69 @@ public:
>>  auto CallExitLoc = N->getLocationAs();
>> 
>>  // No diagnostic if region was modified inside the frame.
>> -if (!CallExitLoc)
>> +if (!CallExitLoc || isRegionOfInterestModifiedInFrame(N))
>>return nullptr;
>> 
>>  CallEventRef<> Call =
>>  BRC.getStateManager().getCallEventManager().getCaller(SCtx, State);
>> 
>> +if (SM.isInSystemHeader(Call->getDecl()->getSourceRange().getBegin()))
>> +  return nullptr;
>> +
>>  // Region of interest corresponds to an IVar, exiting a method
>>  // which could have written into that IVar, but did not.
>> -if (const auto *MC = dyn_cast(Call))
>> -  if (const auto *IvarR = dyn_cast(RegionOfInterest))
>> -if 
>> (potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
>> -  IvarR->getDecl()) &&
>> -!isRegionOfInterestModifiedInFrame(N))
>> -  return notModifiedMemberDiagnostics(
>> -  Ctx, *CallExitLoc, Call, MC->getReceiverSVal().getAsRegion());
>> +if (const auto *MC = dyn_cast(Call)) {
>> +  if (const auto *IvarR = dyn_cast(RegionOfInterest)) {
>> +const MemRegion *SelfRegion = MC->getReceiverSVal().getAsRegion();
>> +if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
>> +
>> potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
>> +  IvarR->getDecl()))
>> +  return notModifiedDiagnostics(Ctx, *CallExitLoc, Call, {}, 
>> SelfRegion,
>> +"self", 
>> /*FirstIsReferenceType=*/false,
>> +1);
>> +  }
>> +}
>> 
>>  if (const auto *CCall = 

Re: r338667 - [analyzer] Extend NoStoreFuncVisitor to follow fields.

[George Karpenkov via cfe-commits]

Yeah, saw that as well, fix coming.

> On Aug 3, 2018, at 10:32 AM, Alexander Kornienko  wrote:
> 
> 
> 
> On Thu, Aug 2, 2018 at 4:03 AM George Karpenkov via cfe-commits 
> mailto:cfe-commits@lists.llvm.org>> wrote:
> Author: george.karpenkov
> Date: Wed Aug  1 19:02:40 2018
> New Revision: 338667
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=338667=rev 
> 
> Log:
> [analyzer] Extend NoStoreFuncVisitor to follow fields.
> 
> rdar://39701823
> 
> Differential Revision: https://reviews.llvm.org/D49901 
> 
> 
> Modified:
> cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
> cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.c
> cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.cpp
> cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.m
> 
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=338667=338666=338667=diff
>  
> 
> ==
> --- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Wed Aug  1 
> 19:02:40 2018
> @@ -269,10 +269,14 @@ namespace {
>  /// pointer dereference outside.
>  class NoStoreFuncVisitor final : public BugReporterVisitor {
>const SubRegion *RegionOfInterest;
> +  MemRegionManager 
>const SourceManager 
>const PrintingPolicy 
> -  static constexpr const char *DiagnosticsMsg =
> -  "Returning without writing to '";
> +
> +  /// Recursion limit for dereferencing fields when looking for the
> +  /// region of interest.
> +  /// The limit of two indicates that we will dereference fields only once.
> +  static const unsigned DEREFERENCE_LIMIT = 2;
> 
>/// Frames writing into \c RegionOfInterest.
>/// This visitor generates a note only if a function does not write into
> @@ -285,15 +289,17 @@ class NoStoreFuncVisitor final : public
>llvm::SmallPtrSet FramesModifyingRegion;
>llvm::SmallPtrSet FramesModifyingCalculated;
> 
> +  using RegionVector = SmallVector;
>  public:
>NoStoreFuncVisitor(const SubRegion *R)
> -  : RegionOfInterest(R),
> -SM(R->getMemRegionManager()->getContext().getSourceManager()),
> -PP(R->getMemRegionManager()->getContext().getPrintingPolicy()) {}
> +  : RegionOfInterest(R), MmrMgr(*R->getMemRegionManager()),
> +SM(MmrMgr.getContext().getSourceManager()),
> +PP(MmrMgr.getContext().getPrintingPolicy()) {}
> 
>void Profile(llvm::FoldingSetNodeID ) const override {
>  static int Tag = 0;
>  ID.AddPointer();
> +ID.AddPointer(RegionOfInterest);
>}
> 
>std::shared_ptr VisitNode(const ExplodedNode *N,
> @@ -307,48 +313,69 @@ public:
>  auto CallExitLoc = N->getLocationAs();
> 
>  // No diagnostic if region was modified inside the frame.
> -if (!CallExitLoc)
> +if (!CallExitLoc || isRegionOfInterestModifiedInFrame(N))
>return nullptr;
> 
>  CallEventRef<> Call =
>  BRC.getStateManager().getCallEventManager().getCaller(SCtx, State);
> 
> +if (SM.isInSystemHeader(Call->getDecl()->getSourceRange().getBegin()))
> +  return nullptr;
> +
>  // Region of interest corresponds to an IVar, exiting a method
>  // which could have written into that IVar, but did not.
> -if (const auto *MC = dyn_cast(Call))
> -  if (const auto *IvarR = dyn_cast(RegionOfInterest))
> -if (potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
> -  IvarR->getDecl()) &&
> -!isRegionOfInterestModifiedInFrame(N))
> -  return notModifiedMemberDiagnostics(
> -  Ctx, *CallExitLoc, Call, MC->getReceiverSVal().getAsRegion());
> +if (const auto *MC = dyn_cast(Call)) {
> +  if (const auto *IvarR = dyn_cast(RegionOfInterest)) {
> +const MemRegion *SelfRegion = MC->getReceiverSVal().getAsRegion();
> +if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
> +potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
> +  IvarR->getDecl()))
> +  return notModifiedDiagnostics(Ctx, *CallExitLoc, Call, {}, 
> SelfRegion,
> +"self", 
> /*FirstIsReferenceType=*/false,
> +1);
> +  }
> +}
> 
>  if (const auto *CCall = dyn_cast(Call)) {
>const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();
>if (RegionOfInterest->isSubRegionOf(ThisR)
> -  && !CCall->getDecl()->isImplicit()
> -  && !isRegionOfInterestModifiedInFrame(N))
> -return 

Re: r338667 - [analyzer] Extend NoStoreFuncVisitor to follow fields.

[Alexander Kornienko via cfe-commits]

On Thu, Aug 2, 2018 at 4:03 AM George Karpenkov via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: george.karpenkov
> Date: Wed Aug  1 19:02:40 2018
> New Revision: 338667
>
> URL: http://llvm.org/viewvc/llvm-project?rev=338667=rev
> Log:
> [analyzer] Extend NoStoreFuncVisitor to follow fields.
>
> rdar://39701823
>
> Differential Revision: https://reviews.llvm.org/D49901
>
> Modified:
> cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
> cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.c
> cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.cpp
> cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.m
>
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=338667=338666=338667=diff
>
> ==
> --- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Wed Aug  1
> 19:02:40 2018
> @@ -269,10 +269,14 @@ namespace {
>  /// pointer dereference outside.
>  class NoStoreFuncVisitor final : public BugReporterVisitor {
>const SubRegion *RegionOfInterest;
> +  MemRegionManager 
>const SourceManager 
>const PrintingPolicy 
> -  static constexpr const char *DiagnosticsMsg =
> -  "Returning without writing to '";
> +
> +  /// Recursion limit for dereferencing fields when looking for the
> +  /// region of interest.
> +  /// The limit of two indicates that we will dereference fields only
> once.
> +  static const unsigned DEREFERENCE_LIMIT = 2;
>
>/// Frames writing into \c RegionOfInterest.
>/// This visitor generates a note only if a function does not write into
> @@ -285,15 +289,17 @@ class NoStoreFuncVisitor final : public
>llvm::SmallPtrSet FramesModifyingRegion;
>llvm::SmallPtrSet
> FramesModifyingCalculated;
>
> +  using RegionVector = SmallVector;
>  public:
>NoStoreFuncVisitor(const SubRegion *R)
> -  : RegionOfInterest(R),
> -SM(R->getMemRegionManager()->getContext().getSourceManager()),
> -PP(R->getMemRegionManager()->getContext().getPrintingPolicy()) {}
> +  : RegionOfInterest(R), MmrMgr(*R->getMemRegionManager()),
> +SM(MmrMgr.getContext().getSourceManager()),
> +PP(MmrMgr.getContext().getPrintingPolicy()) {}
>
>void Profile(llvm::FoldingSetNodeID ) const override {
>  static int Tag = 0;
>  ID.AddPointer();
> +ID.AddPointer(RegionOfInterest);
>}
>
>std::shared_ptr VisitNode(const ExplodedNode *N,
> @@ -307,48 +313,69 @@ public:
>  auto CallExitLoc = N->getLocationAs();
>
>  // No diagnostic if region was modified inside the frame.
> -if (!CallExitLoc)
> +if (!CallExitLoc || isRegionOfInterestModifiedInFrame(N))
>return nullptr;
>
>  CallEventRef<> Call =
>  BRC.getStateManager().getCallEventManager().getCaller(SCtx,
> State);
>
> +if (SM.isInSystemHeader(Call->getDecl()->getSourceRange().getBegin()))
> +  return nullptr;
> +
>  // Region of interest corresponds to an IVar, exiting a method
>  // which could have written into that IVar, but did not.
> -if (const auto *MC = dyn_cast(Call))
> -  if (const auto *IvarR = dyn_cast(RegionOfInterest))
> -if
> (potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
> -  IvarR->getDecl()) &&
> -!isRegionOfInterestModifiedInFrame(N))
> -  return notModifiedMemberDiagnostics(
> -  Ctx, *CallExitLoc, Call,
> MC->getReceiverSVal().getAsRegion());
> +if (const auto *MC = dyn_cast(Call)) {
> +  if (const auto *IvarR = dyn_cast(RegionOfInterest))
> {
> +const MemRegion *SelfRegion = MC->getReceiverSVal().getAsRegion();
> +if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
> +
> potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
> +  IvarR->getDecl()))
> +  return notModifiedDiagnostics(Ctx, *CallExitLoc, Call, {},
> SelfRegion,
> +"self",
> /*FirstIsReferenceType=*/false,
> +1);
> +  }
> +}
>
>  if (const auto *CCall = dyn_cast(Call)) {
>const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();
>if (RegionOfInterest->isSubRegionOf(ThisR)
> -  && !CCall->getDecl()->isImplicit()
> -  && !isRegionOfInterestModifiedInFrame(N))
> -return notModifiedMemberDiagnostics(Ctx, *CallExitLoc, Call,
> ThisR);
> +  && !CCall->getDecl()->isImplicit())
> +return notModifiedDiagnostics(Ctx, *CallExitLoc, Call, {}, ThisR,
> +  "this",
> +  /*FirstIsReferenceType=*/false, 1);
> +
> +  // Do not generate 

r338667 - [analyzer] Extend NoStoreFuncVisitor to follow fields.

[George Karpenkov via cfe-commits]

Author: george.karpenkov
Date: Wed Aug  1 19:02:40 2018
New Revision: 338667

URL: http://llvm.org/viewvc/llvm-project?rev=338667=rev
Log:
[analyzer] Extend NoStoreFuncVisitor to follow fields.

rdar://39701823

Differential Revision: https://reviews.llvm.org/D49901

Modified:
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.c
cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.cpp
cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.m

Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=338667=338666=338667=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Wed Aug  1 
19:02:40 2018
@@ -269,10 +269,14 @@ namespace {
 /// pointer dereference outside.
 class NoStoreFuncVisitor final : public BugReporterVisitor {
   const SubRegion *RegionOfInterest;
+  MemRegionManager 
   const SourceManager 
   const PrintingPolicy 
-  static constexpr const char *DiagnosticsMsg =
-  "Returning without writing to '";
+
+  /// Recursion limit for dereferencing fields when looking for the
+  /// region of interest.
+  /// The limit of two indicates that we will dereference fields only once.
+  static const unsigned DEREFERENCE_LIMIT = 2;
 
   /// Frames writing into \c RegionOfInterest.
   /// This visitor generates a note only if a function does not write into
@@ -285,15 +289,17 @@ class NoStoreFuncVisitor final : public
   llvm::SmallPtrSet FramesModifyingRegion;
   llvm::SmallPtrSet FramesModifyingCalculated;
 
+  using RegionVector = SmallVector;
 public:
   NoStoreFuncVisitor(const SubRegion *R)
-  : RegionOfInterest(R),
-SM(R->getMemRegionManager()->getContext().getSourceManager()),
-PP(R->getMemRegionManager()->getContext().getPrintingPolicy()) {}
+  : RegionOfInterest(R), MmrMgr(*R->getMemRegionManager()),
+SM(MmrMgr.getContext().getSourceManager()),
+PP(MmrMgr.getContext().getPrintingPolicy()) {}
 
   void Profile(llvm::FoldingSetNodeID ) const override {
 static int Tag = 0;
 ID.AddPointer();
+ID.AddPointer(RegionOfInterest);
   }
 
   std::shared_ptr VisitNode(const ExplodedNode *N,
@@ -307,48 +313,69 @@ public:
 auto CallExitLoc = N->getLocationAs();
 
 // No diagnostic if region was modified inside the frame.
-if (!CallExitLoc)
+if (!CallExitLoc || isRegionOfInterestModifiedInFrame(N))
   return nullptr;
 
 CallEventRef<> Call =
 BRC.getStateManager().getCallEventManager().getCaller(SCtx, State);
 
+if (SM.isInSystemHeader(Call->getDecl()->getSourceRange().getBegin()))
+  return nullptr;
+
 // Region of interest corresponds to an IVar, exiting a method
 // which could have written into that IVar, but did not.
-if (const auto *MC = dyn_cast(Call))
-  if (const auto *IvarR = dyn_cast(RegionOfInterest))
-if (potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
-  IvarR->getDecl()) &&
-!isRegionOfInterestModifiedInFrame(N))
-  return notModifiedMemberDiagnostics(
-  Ctx, *CallExitLoc, Call, MC->getReceiverSVal().getAsRegion());
+if (const auto *MC = dyn_cast(Call)) {
+  if (const auto *IvarR = dyn_cast(RegionOfInterest)) {
+const MemRegion *SelfRegion = MC->getReceiverSVal().getAsRegion();
+if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
+potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
+  IvarR->getDecl()))
+  return notModifiedDiagnostics(Ctx, *CallExitLoc, Call, {}, 
SelfRegion,
+"self", /*FirstIsReferenceType=*/false,
+1);
+  }
+}
 
 if (const auto *CCall = dyn_cast(Call)) {
   const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();
   if (RegionOfInterest->isSubRegionOf(ThisR)
-  && !CCall->getDecl()->isImplicit()
-  && !isRegionOfInterestModifiedInFrame(N))
-return notModifiedMemberDiagnostics(Ctx, *CallExitLoc, Call, ThisR);
+  && !CCall->getDecl()->isImplicit())
+return notModifiedDiagnostics(Ctx, *CallExitLoc, Call, {}, ThisR,
+  "this",
+  /*FirstIsReferenceType=*/false, 1);
+
+  // Do not generate diagnostics for not modified parameters in
+  // constructors.
+  return nullptr;
 }
 
 ArrayRef parameters = getCallParameters(Call);
 for (unsigned I = 0; I < Call->getNumArgs() && I < parameters.size(); ++I) 
{
-  const ParmVarDecl *PVD = parameters[I];
+  Optional AdjustedIdx =