[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG06d100a69a08: [Analyzer] Support note tags for smart ptr 
checker (authored by vrnithinkumar).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtr.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/smart-ptr-text-output.cpp
  clang/test/Analysis/smart-ptr.cpp

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -12,7 +12,7 @@
   std::unique_ptr Q = std::move(P);
   if (Q)
 clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
-  *Q.get() = 1; // no-warning
+  *Q.get() = 1; // no-warning
   if (P)
 clang_analyzer_warnIfReached(); // no-warning
   // TODO: Report a null dereference (instead).
@@ -50,37 +50,38 @@
 
 void derefAfterDefaultCtr() {
   std::unique_ptr P;
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterCtrWithNull() {
   std::unique_ptr P(nullptr);
-  *P; // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  *P; // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
-void derefAfterCtrWithNullReturnMethod() {
-  std::unique_ptr P(return_null());
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+void derefAfterCtrWithNullVariable() {
+  A *InnerPtr = nullptr;
+  std::unique_ptr P(InnerPtr);
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterRelease() {
   std::unique_ptr P(new A());
   P.release();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterReset() {
   std::unique_ptr P(new A());
   P.reset();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNull() {
   std::unique_ptr P(new A());
   P.reset(nullptr);
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNonNull() {
@@ -102,6 +103,12 @@
   AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
 }
 
+void derefOnReleasedValidRawPtr() {
+  std::unique_ptr P(new A());
+  A *AP = P.release();
+  AP->foo(); // No warning.
+}
+
 void pass_smart_ptr_by_ref(std::unique_ptr &a);
 void pass_smart_ptr_by_const_ref(const std::unique_ptr &a);
 void pass_smart_ptr_by_rvalue_ref(std::unique_ptr &&a);
@@ -118,7 +125,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ref(P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -128,7 +135,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_rvalue_ref(std::move(P));
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -138,7 +145,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ptr(&P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
 
@@ -162,7 +169,7 @@
   {
 StructWithSmartPtr S;
 pass_struct_with_smart_ptr_by_const_ref(S);
-S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+S.P->foo(); // expected-warning {{Dereference of null smart pointer 'S.P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 StructWithSmartPtr S;
@@ -172,7 +179,7 @@
   {
 StructWithSmartPtr S;
 pass_struct_with

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Let's land this! Damn, we've learned //a lot// from this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:79
+
+// TODO: Enabale this test when "std::swap" is modeled seperately.
+void derefOnStdSwappedNullPtr() {

NoQ wrote:
> vrnithinkumar wrote:
> > vrnithinkumar wrote:
> > > NoQ wrote:
> > > > Instead of commenting out tests, i recommend testing the incorrect 
> > > > behavior (with a FIXME comment telling us why it's incorrect). This way 
> > > > we'll be notified when the test is fixed, accidentally or 
> > > > intentionally, and also generally that's more testing for everybody.
> > > I have commented out this since right now `std::swap` is using 
> > > `unique_ptr.swap`.
> > > So note tag for std::swap is added in header file  
> > > `system-header-simulator-cxx.h`
> > > eg:
> > > `system-header-simulator-cxx.h Line 978: Swapped null smart pointer 
> > > 'PNull' with smart pointer 'P'`
> > - Is it okay to add a expected-note on `system-header-simulator-cxx.h`?
> Aha, i see.
> 
> It's obviously not ok because the header is included by multiple tests but 
> only some tests will have the note show up.
> 
> The usual solution is to put the expected-note comment in the current test 
> file but make it refer to the header. Here's doc from 
> https://clang.llvm.org/doxygen/classclang_1_1VerifyDiagnosticConsumer.html:
> > If the diagnostic is generated in a separate file, for example in a shared 
> > header file, it may be beneficial to be able to declare the file in which 
> > the diagnostic will appear, rather than placing the expected-* directive in 
> > the actual file itself. This can be done using the following syntax:
> > ```// expected-error@path/include.h:15 {{error message}}```
> 
> It still is a bit of a problem that you have to hardcode the line number (so 
> everybody who modifies the header above your note will have to update your 
> test) but that's not a big deal.
Thanks!
Added the tags for the line from header file.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar updated this revision to Diff 284237.
vrnithinkumar marked an inline comment as done.
vrnithinkumar added a comment.

- Updating test with tags from header file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtr.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/smart-ptr-text-output.cpp
  clang/test/Analysis/smart-ptr.cpp

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -12,7 +12,7 @@
   std::unique_ptr Q = std::move(P);
   if (Q)
 clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
-  *Q.get() = 1; // no-warning
+  *Q.get() = 1; // no-warning
   if (P)
 clang_analyzer_warnIfReached(); // no-warning
   // TODO: Report a null dereference (instead).
@@ -50,37 +50,38 @@
 
 void derefAfterDefaultCtr() {
   std::unique_ptr P;
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterCtrWithNull() {
   std::unique_ptr P(nullptr);
-  *P; // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  *P; // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
-void derefAfterCtrWithNullReturnMethod() {
-  std::unique_ptr P(return_null());
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+void derefAfterCtrWithNullVariable() {
+  A *InnerPtr = nullptr;
+  std::unique_ptr P(InnerPtr);
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterRelease() {
   std::unique_ptr P(new A());
   P.release();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterReset() {
   std::unique_ptr P(new A());
   P.reset();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNull() {
   std::unique_ptr P(new A());
   P.reset(nullptr);
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNonNull() {
@@ -102,6 +103,12 @@
   AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
 }
 
+void derefOnReleasedValidRawPtr() {
+  std::unique_ptr P(new A());
+  A *AP = P.release();
+  AP->foo(); // No warning.
+}
+
 void pass_smart_ptr_by_ref(std::unique_ptr &a);
 void pass_smart_ptr_by_const_ref(const std::unique_ptr &a);
 void pass_smart_ptr_by_rvalue_ref(std::unique_ptr &&a);
@@ -118,7 +125,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ref(P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -128,7 +135,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_rvalue_ref(std::move(P));
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -138,7 +145,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ptr(&P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
 
@@ -162,7 +169,7 @@
   {
 StructWithSmartPtr S;
 pass_struct_with_smart_ptr_by_const_ref(S);
-S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+S.P->foo(); // expected-warning {{Dereference of null smart pointer 'S.P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 StructWithSmartPtr S;
@@ -172,7 +179,7 @@
   {
 StructWithSmartPtr S;
 pass_struct_with_smart_ptr_by_const_rvalue_ref(std::move(S));
-S.P->foo(); // ex

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:79
+
+// TODO: Enabale this test when "std::swap" is modeled seperately.
+void derefOnStdSwappedNullPtr() {

vrnithinkumar wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > Instead of commenting out tests, i recommend testing the incorrect 
> > > behavior (with a FIXME comment telling us why it's incorrect). This way 
> > > we'll be notified when the test is fixed, accidentally or intentionally, 
> > > and also generally that's more testing for everybody.
> > I have commented out this since right now `std::swap` is using 
> > `unique_ptr.swap`.
> > So note tag for std::swap is added in header file  
> > `system-header-simulator-cxx.h`
> > eg:
> > `system-header-simulator-cxx.h Line 978: Swapped null smart pointer 'PNull' 
> > with smart pointer 'P'`
> - Is it okay to add a expected-note on `system-header-simulator-cxx.h`?
Aha, i see.

It's obviously not ok because the header is included by multiple tests but only 
some tests will have the note show up.

The usual solution is to put the expected-note comment in the current test file 
but make it refer to the header. Here's doc from 
https://clang.llvm.org/doxygen/classclang_1_1VerifyDiagnosticConsumer.html:
> If the diagnostic is generated in a separate file, for example in a shared 
> header file, it may be beneficial to be able to declare the file in which the 
> diagnostic will appear, rather than placing the expected-* directive in the 
> actual file itself. This can be done using the following syntax:
> ```// expected-error@path/include.h:15 {{error message}}```

It still is a bit of a problem that you have to hardcode the line number (so 
everybody who modifies the header above your note will have to update your 
test) but that's not a big deal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:79
+
+// TODO: Enabale this test when "std::swap" is modeled seperately.
+void derefOnStdSwappedNullPtr() {

vrnithinkumar wrote:
> NoQ wrote:
> > Instead of commenting out tests, i recommend testing the incorrect behavior 
> > (with a FIXME comment telling us why it's incorrect). This way we'll be 
> > notified when the test is fixed, accidentally or intentionally, and also 
> > generally that's more testing for everybody.
> I have commented out this since right now `std::swap` is using 
> `unique_ptr.swap`.
> So note tag for std::swap is added in header file  
> `system-header-simulator-cxx.h`
> eg:
> `system-header-simulator-cxx.h Line 978: Swapped null smart pointer 'PNull' 
> with smart pointer 'P'`
- Is it okay to add a expected-note on `system-header-simulator-cxx.h`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added a comment.

In D84600#2203604 , @Szelethus wrote:

> Layering violations are a running theme in the analyzer -- CheckerRegistry 
> and the entire MallocChecker fiasco 
>  are two glaring examples. 
> Luckily, this isn't a severe case so I wouldn't worry about it much.
>
> I've been following your patches, but them mentors answer way ahead of me and 
> are very thorough :^) Best of luck with the finale!

Thanks for sharing checker dependency talk.
Also fo suggesting to separate checker and modeling from beginning.  :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:33
 
+// Static global pointer to NullDereferenceBugType.
+static const BugType *NullDereferenceBugTypePtr;

xazax.hun wrote:
> I find this comment redundant as well. It just repeats what is already 
> evident from the code.
removed



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:63
+void derefOnReleasedNullRawPtr() {
+  std::unique_ptr P;
+  A *AP = P.release(); // expected-note {{'AP' initialized to a null pointer 
value}}

NoQ wrote:
> Unlike the next line, this line deserves a note that `P` holds a null value.
Added a FIXEM to add note "Default constructed smart pointer 'P' is null"



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:65
+  A *AP = P.release(); // expected-note {{'AP' initialized to a null pointer 
value}}
+  //TODO add note "Smart pointer 'P' is released and set to null"
+  AP->foo(); // expected-warning {{Called C++ object pointer is null 
[core.CallAndMessage]}}

NoQ wrote:
> Such note is unnecessary. We don't care what happens to `P` after it's 
> released; we only care about its old value.
Removed this.



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:79
+
+// TODO: Enabale this test when "std::swap" is modeled seperately.
+void derefOnStdSwappedNullPtr() {

NoQ wrote:
> Instead of commenting out tests, i recommend testing the incorrect behavior 
> (with a FIXME comment telling us why it's incorrect). This way we'll be 
> notified when the test is fixed, accidentally or intentionally, and also 
> generally that's more testing for everybody.
I have commented out this since right now `std::swap` is using 
`unique_ptr.swap`.
So note tag for std::swap is added in header file  
`system-header-simulator-cxx.h`
eg:
`system-header-simulator-cxx.h Line 978: Swapped null smart pointer 'PNull' 
with smart pointer 'P'`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar updated this revision to Diff 284137.
vrnithinkumar marked 4 inline comments as done.
vrnithinkumar added a comment.

- Review comment changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtr.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/smart-ptr-text-output.cpp
  clang/test/Analysis/smart-ptr.cpp

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -12,7 +12,7 @@
   std::unique_ptr Q = std::move(P);
   if (Q)
 clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
-  *Q.get() = 1; // no-warning
+  *Q.get() = 1; // no-warning
   if (P)
 clang_analyzer_warnIfReached(); // no-warning
   // TODO: Report a null dereference (instead).
@@ -50,37 +50,38 @@
 
 void derefAfterDefaultCtr() {
   std::unique_ptr P;
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterCtrWithNull() {
   std::unique_ptr P(nullptr);
-  *P; // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  *P; // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
-void derefAfterCtrWithNullReturnMethod() {
-  std::unique_ptr P(return_null());
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+void derefAfterCtrWithNullVariable() {
+  A *InnerPtr = nullptr;
+  std::unique_ptr P(InnerPtr);
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterRelease() {
   std::unique_ptr P(new A());
   P.release();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterReset() {
   std::unique_ptr P(new A());
   P.reset();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNull() {
   std::unique_ptr P(new A());
   P.reset(nullptr);
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNonNull() {
@@ -102,6 +103,12 @@
   AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
 }
 
+void derefOnReleasedValidRawPtr() {
+  std::unique_ptr P(new A());
+  A *AP = P.release();
+  AP->foo(); // No warning.
+}
+
 void pass_smart_ptr_by_ref(std::unique_ptr &a);
 void pass_smart_ptr_by_const_ref(const std::unique_ptr &a);
 void pass_smart_ptr_by_rvalue_ref(std::unique_ptr &&a);
@@ -118,7 +125,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ref(P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -128,7 +135,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_rvalue_ref(std::move(P));
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -138,7 +145,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ptr(&P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
 
@@ -162,7 +169,7 @@
   {
 StructWithSmartPtr S;
 pass_struct_with_smart_ptr_by_const_ref(S);
-S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+S.P->foo(); // expected-warning {{Dereference of null smart pointer 'S.P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 StructWithSmartPtr S;
@@ -172,7 +179,7 @@
   {
 StructWithSmartPtr S;
 pass_struct_with_smart_ptr_by_const_rvalue_ref(std::move(S));
-S.P->foo(); // expected-warning {{D

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

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

Szelethus added a comment.

Layering violations are a running theme in the analyzer -- CheckerRegistry and 
the entire MallocChecker fiasco  
are two glaring examples. Luckily, this isn't a severe case so I wouldn't worry 
about it much.

I've been following your patches, but them mentors answer way ahead of me and 
are very thorough :^) Best of luck with the finale!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:63
+void derefOnReleasedNullRawPtr() {
+  std::unique_ptr P;
+  A *AP = P.release(); // expected-note {{'AP' initialized to a null pointer 
value}}

Unlike the next line, this line deserves a note that `P` holds a null value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:65
+  A *AP = P.release(); // expected-note {{'AP' initialized to a null pointer 
value}}
+  //TODO add note "Smart pointer 'P' is released and set to null"
+  AP->foo(); // expected-warning {{Called C++ object pointer is null 
[core.CallAndMessage]}}

Such note is unnecessary. We don't care what happens to `P` after it's 
released; we only care about its old value.



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:79
+
+// TODO: Enabale this test when "std::swap" is modeled seperately.
+void derefOnStdSwappedNullPtr() {

Instead of commenting out tests, i recommend testing the incorrect behavior 
(with a FIXME comment telling us why it's incorrect). This way we'll be 
notified when the test is fixed, accidentally or intentionally, and also 
generally that's more testing for everybody.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

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

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

LGTM, thanks!

I think all the problems that left should be solved in a separate patch or even 
out of the scope for the GSoC.




Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:33
 
+// Static global pointer to NullDereferenceBugType.
+static const BugType *NullDereferenceBugTypePtr;

I find this comment redundant as well. It just repeats what is already evident 
from the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:312
+  const NoteTag *getNoteTag(
+  std::function 
Cb,
+  bool IsPrunable = false) {

xazax.hun wrote:
> The callback is taken is an rvalue reference in other `getNoteTag` APIs. I 
> think these overloads should be consistent.
> Also, I wonder if the caller should be able to manipulate the buffer size of 
> the small string (as a template parameter), but I do not have strong feelings 
> about this. 
> 
> As a side note, since Clang is using C++14, maybe the lambda captures in the 
> `getNoteTag` overloads above should utilize it and capture the callback by 
> move. This is more of a note to ourselves independent of this patch. 
> 
> Side note 2: maybe a modernize tidy check would be useful to discover where 
> rvalue references are captured by value in lambdas?
Changed to rvalue reference.



Comment at: clang/lib/StaticAnalyzer/Checkers/NullDereference.h:21
+namespace ento {
+namespace nullDereference {
+

NoQ wrote:
> Namespaces are traditionally snake_case rather than camelCase.
removed the new namespace



Comment at: clang/lib/StaticAnalyzer/Checkers/NullDereference.h:23
+
+/// Returns NullDereferenceBugType.
+const BugType *getNullDereferenceBugType();

xazax.hun wrote:
> I think this comment does not really add much value.
Yeah.
removed it.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:110
+  SmartPtrChecker *Checker = Mgr.registerChecker();
+  NullDereferenceBugTypePtr = &Checker->NullDereferenceBugType;
 }

NoQ wrote:
> vrnithinkumar wrote:
> > xazax.hun wrote:
> > > NoQ wrote:
> > > > vrnithinkumar wrote:
> > > > > NoQ wrote:
> > > > > > Wait, i don't understand again. You're taking the bug type from 
> > > > > > that checker and using it in that checker. Why did you need to make 
> > > > > > it global? I thought your problem was about capturing the bug type 
> > > > > > from the //raw// null dereference checker?
> > > > > > Wait, i don't understand again. You're taking the bug type from 
> > > > > > that checker and using it in that checker. Why did you need to make 
> > > > > > it global? I thought your problem was about capturing the bug type 
> > > > > > from the //raw// null dereference checker?
> > > > > 
> > > > > The check for bug type is in `SmartPtrModeling` but the bug type is 
> > > > > defined in `SmartPtrChecker`
> > > > Aha, ok, i misunderstood again. So i guess we didn't need a new header 
> > > > then. That said, it's an interesting layering violation that we 
> > > > encounter in this design: it used to be up to the checker to attach a 
> > > > visitor and so should be activating a note tag, but note tags are part 
> > > > of modeling, not checking.
> > > > 
> > > > I guess that's just how it is and it's the responsibility of the 
> > > > checkers to inform the modeling about bug types. I guess the ultimate 
> > > > API could look like `BugReport->activateNoteTag(NoteTagTag TT)` (where 
> > > > `TT` is a crying smiley that cries about "tag tags" T_T). But that's a 
> > > > story for another time.
> > > I hope we will be able to figure out a nicer solution at some point. But 
> > > I do not have a better alternative now, so I am ok with committing as is.
> > > 
> > > The API Artem is suggesting looks good to me (although it is out of scope 
> > > for the GSoC). But I think that might not solve the problem of how 
> > > multiple checkers should share the information about those tags. (Or at 
> > > least I do not see how.)
> > > 
> > > Note that if we had both checks in the same file we might be able to work 
> > > this problem around using `CheckerManager::getChecker`. I am not 
> > > suggesting merging them, only wanted to point out.
> > >  So i guess we didn't need a new header then.
> > So we should remove the `NullDereference.h` and add the inter checker api 
> > to get the BugType to `SmartPtr.h`?
> > 
> > >  I guess the ultimate API could look like 
> > > `BugReport->activateNoteTag(NoteTagTag TT)`
> > Do we have to make these api changes on this review? Or some follow up 
> > changes?
> > So we should remove the `NullDereference.h` and add the inter checker api 
> > to get the BugType to `SmartPtr.h`?
> Yes.
> 
> > Do we have to make these api changes on this review? Or some follow up 
> > changes?
> You don't have to do this at all; i think you have enough stuff on your 
> plate. But if you like you can put up another patch later.
Removed the new header file and added to `SmartPtr.h`



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:131
+  }
+  return nullptr;
+}

NoQ wrote:
> You never ever check for this case. Therefore this function entirely boils 
> down to `Call.getArgExpr(0)` which is shorter than `getFirstArgExpr(Call)` 
> anyway.
remov

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar updated this revision to Diff 283602.
vrnithinkumar marked 13 inline comments as done.
vrnithinkumar added a comment.



- Changes from review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtr.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/smart-ptr-text-output.cpp
  clang/test/Analysis/smart-ptr.cpp

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -12,7 +12,7 @@
   std::unique_ptr Q = std::move(P);
   if (Q)
 clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
-  *Q.get() = 1; // no-warning
+  *Q.get() = 1; // no-warning
   if (P)
 clang_analyzer_warnIfReached(); // no-warning
   // TODO: Report a null dereference (instead).
@@ -50,37 +50,38 @@
 
 void derefAfterDefaultCtr() {
   std::unique_ptr P;
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterCtrWithNull() {
   std::unique_ptr P(nullptr);
-  *P; // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  *P; // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
-void derefAfterCtrWithNullReturnMethod() {
-  std::unique_ptr P(return_null());
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+void derefAfterCtrWithNullVariable() {
+  A *InnerPtr = nullptr;
+  std::unique_ptr P(InnerPtr);
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterRelease() {
   std::unique_ptr P(new A());
   P.release();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterReset() {
   std::unique_ptr P(new A());
   P.reset();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNull() {
   std::unique_ptr P(new A());
   P.reset(nullptr);
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNonNull() {
@@ -102,6 +103,12 @@
   AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
 }
 
+void derefOnReleasedValidRawPtr() {
+  std::unique_ptr P(new A());
+  A *AP = P.release();
+  AP->foo(); // No warning.
+}
+
 void pass_smart_ptr_by_ref(std::unique_ptr &a);
 void pass_smart_ptr_by_const_ref(const std::unique_ptr &a);
 void pass_smart_ptr_by_rvalue_ref(std::unique_ptr &&a);
@@ -118,7 +125,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ref(P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -128,7 +135,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_rvalue_ref(std::move(P));
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -138,7 +145,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ptr(&P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
 
@@ -162,7 +169,7 @@
   {
 StructWithSmartPtr S;
 pass_struct_with_smart_ptr_by_const_ref(S);
-S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+S.P->foo(); // expected-warning {{Dereference of null smart pointer 'S.P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 StructWithSmartPtr S;
@@ -172,7 +179,7 @@
   {
 StructWithSmartPtr S;
 pass_struct_with_smart_ptr_by_const_rvalue_ref(std::move(S));
-S.P->foo(); // expected-wa

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:110
+  SmartPtrChecker *Checker = Mgr.registerChecker();
+  NullDereferenceBugTypePtr = &Checker->NullDereferenceBugType;
 }

vrnithinkumar wrote:
> xazax.hun wrote:
> > NoQ wrote:
> > > vrnithinkumar wrote:
> > > > NoQ wrote:
> > > > > Wait, i don't understand again. You're taking the bug type from that 
> > > > > checker and using it in that checker. Why did you need to make it 
> > > > > global? I thought your problem was about capturing the bug type from 
> > > > > the //raw// null dereference checker?
> > > > > Wait, i don't understand again. You're taking the bug type from that 
> > > > > checker and using it in that checker. Why did you need to make it 
> > > > > global? I thought your problem was about capturing the bug type from 
> > > > > the //raw// null dereference checker?
> > > > 
> > > > The check for bug type is in `SmartPtrModeling` but the bug type is 
> > > > defined in `SmartPtrChecker`
> > > Aha, ok, i misunderstood again. So i guess we didn't need a new header 
> > > then. That said, it's an interesting layering violation that we encounter 
> > > in this design: it used to be up to the checker to attach a visitor and 
> > > so should be activating a note tag, but note tags are part of modeling, 
> > > not checking.
> > > 
> > > I guess that's just how it is and it's the responsibility of the checkers 
> > > to inform the modeling about bug types. I guess the ultimate API could 
> > > look like `BugReport->activateNoteTag(NoteTagTag TT)` (where `TT` is a 
> > > crying smiley that cries about "tag tags" T_T). But that's a story for 
> > > another time.
> > I hope we will be able to figure out a nicer solution at some point. But I 
> > do not have a better alternative now, so I am ok with committing as is.
> > 
> > The API Artem is suggesting looks good to me (although it is out of scope 
> > for the GSoC). But I think that might not solve the problem of how multiple 
> > checkers should share the information about those tags. (Or at least I do 
> > not see how.)
> > 
> > Note that if we had both checks in the same file we might be able to work 
> > this problem around using `CheckerManager::getChecker`. I am not suggesting 
> > merging them, only wanted to point out.
> >  So i guess we didn't need a new header then.
> So we should remove the `NullDereference.h` and add the inter checker api to 
> get the BugType to `SmartPtr.h`?
> 
> >  I guess the ultimate API could look like 
> > `BugReport->activateNoteTag(NoteTagTag TT)`
> Do we have to make these api changes on this review? Or some follow up 
> changes?
> So we should remove the `NullDereference.h` and add the inter checker api to 
> get the BugType to `SmartPtr.h`?
Yes.

> Do we have to make these api changes on this review? Or some follow up 
> changes?
You don't have to do this at all; i think you have enough stuff on your plate. 
But if you like you can put up another patch later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:110
+  SmartPtrChecker *Checker = Mgr.registerChecker();
+  NullDereferenceBugTypePtr = &Checker->NullDereferenceBugType;
 }

xazax.hun wrote:
> NoQ wrote:
> > vrnithinkumar wrote:
> > > NoQ wrote:
> > > > Wait, i don't understand again. You're taking the bug type from that 
> > > > checker and using it in that checker. Why did you need to make it 
> > > > global? I thought your problem was about capturing the bug type from 
> > > > the //raw// null dereference checker?
> > > > Wait, i don't understand again. You're taking the bug type from that 
> > > > checker and using it in that checker. Why did you need to make it 
> > > > global? I thought your problem was about capturing the bug type from 
> > > > the //raw// null dereference checker?
> > > 
> > > The check for bug type is in `SmartPtrModeling` but the bug type is 
> > > defined in `SmartPtrChecker`
> > Aha, ok, i misunderstood again. So i guess we didn't need a new header 
> > then. That said, it's an interesting layering violation that we encounter 
> > in this design: it used to be up to the checker to attach a visitor and so 
> > should be activating a note tag, but note tags are part of modeling, not 
> > checking.
> > 
> > I guess that's just how it is and it's the responsibility of the checkers 
> > to inform the modeling about bug types. I guess the ultimate API could look 
> > like `BugReport->activateNoteTag(NoteTagTag TT)` (where `TT` is a crying 
> > smiley that cries about "tag tags" T_T). But that's a story for another 
> > time.
> I hope we will be able to figure out a nicer solution at some point. But I do 
> not have a better alternative now, so I am ok with committing as is.
> 
> The API Artem is suggesting looks good to me (although it is out of scope for 
> the GSoC). But I think that might not solve the problem of how multiple 
> checkers should share the information about those tags. (Or at least I do not 
> see how.)
> 
> Note that if we had both checks in the same file we might be able to work 
> this problem around using `CheckerManager::getChecker`. I am not suggesting 
> merging them, only wanted to point out.
>  So i guess we didn't need a new header then.
So we should remove the `NullDereference.h` and add the inter checker api to 
get the BugType to `SmartPtr.h`?

>  I guess the ultimate API could look like 
> `BugReport->activateNoteTag(NoteTagTag TT)`
Do we have to make these api changes on this review? Or some follow up changes?



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:352
+// Gets the SVal to track for a smart pointer memory region
+SVal SmartPtrModeling::getSValForRegion(const CallEvent &Call,
+CheckerContext &C) const {

xazax.hun wrote:
> I am a bit unhappy with this function.
> I feel like it does not really have a purpose. It returns the null constant 
> for default constructor calls, for every other case it just returns the first 
> argument. The branch we have in this function would completely go away if we 
> would inline this to all the call sites and the result would be a single line 
> each place. I'd rather just remove this function.
Also we have to assert that we are adding a pointer value to TrackedRegionMap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:199
+} else {
+  const auto *TrackingExpr = getFirstArgExpr(Call);
+  C.addTransition(State, C.getNoteTag([ThisRegion, TrackingExpr](

xazax.hun wrote:
> I am a bit confused. As far as I understand we ALWAYS add the note tag below, 
> regardless of the first argument being null.
> I think I do understand that we only trigger this note tag when there is a 
> null dereference but consider the code below:
> 
> ```
> void f(int *p) {
>   std::unique_ptr u(p); // p is not always null here.
>   if (!p) {
> *u; // p is always null here
>   }
> }
> ```
> 
> Do we ant to output the message that the smart pointer is constructed using a 
> null value? While this is technically true, there is a later event in the 
> path that uncovers the fact that `p` is null.
> 
> @NoQ what do you think? I do not have any strong feelings about this, I only 
> wonder how users would react to a bug report like that.
Indeed, nice!! We can only proclaim the pointer to be null if it's known to be 
null in the current state. I.e., if previous steps of the report indicated that 
it's null. If it's discovered to be null later, we cannot call it null yet. I 
think we should check the current state and say "is constructed using a null 
value" if it's already null or simply "is constructed" if it's not yet known to 
be null.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

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

xazax.hun added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:312
+  const NoteTag *getNoteTag(
+  std::function 
Cb,
+  bool IsPrunable = false) {

The callback is taken is an rvalue reference in other `getNoteTag` APIs. I 
think these overloads should be consistent.
Also, I wonder if the caller should be able to manipulate the buffer size of 
the small string (as a template parameter), but I do not have strong feelings 
about this. 

As a side note, since Clang is using C++14, maybe the lambda captures in the 
`getNoteTag` overloads above should utilize it and capture the callback by 
move. This is more of a note to ourselves independent of this patch. 

Side note 2: maybe a modernize tidy check would be useful to discover where 
rvalue references are captured by value in lambdas?



Comment at: clang/lib/StaticAnalyzer/Checkers/NullDereference.h:23
+
+/// Returns NullDereferenceBugType.
+const BugType *getNullDereferenceBugType();

I think this comment does not really add much value.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:110
+  SmartPtrChecker *Checker = Mgr.registerChecker();
+  NullDereferenceBugTypePtr = &Checker->NullDereferenceBugType;
 }

NoQ wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > Wait, i don't understand again. You're taking the bug type from that 
> > > checker and using it in that checker. Why did you need to make it global? 
> > > I thought your problem was about capturing the bug type from the //raw// 
> > > null dereference checker?
> > > Wait, i don't understand again. You're taking the bug type from that 
> > > checker and using it in that checker. Why did you need to make it global? 
> > > I thought your problem was about capturing the bug type from the //raw// 
> > > null dereference checker?
> > 
> > The check for bug type is in `SmartPtrModeling` but the bug type is defined 
> > in `SmartPtrChecker`
> Aha, ok, i misunderstood again. So i guess we didn't need a new header then. 
> That said, it's an interesting layering violation that we encounter in this 
> design: it used to be up to the checker to attach a visitor and so should be 
> activating a note tag, but note tags are part of modeling, not checking.
> 
> I guess that's just how it is and it's the responsibility of the checkers to 
> inform the modeling about bug types. I guess the ultimate API could look like 
> `BugReport->activateNoteTag(NoteTagTag TT)` (where `TT` is a crying smiley 
> that cries about "tag tags" T_T). But that's a story for another time.
I hope we will be able to figure out a nicer solution at some point. But I do 
not have a better alternative now, so I am ok with committing as is.

The API Artem is suggesting looks good to me (although it is out of scope for 
the GSoC). But I think that might not solve the problem of how multiple 
checkers should share the information about those tags. (Or at least I do not 
see how.)

Note that if we had both checks in the same file we might be able to work this 
problem around using `CheckerManager::getChecker`. I am not suggesting merging 
them, only wanted to point out.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:134
+
+static std::string getSmartPtrName(const MemRegion *Region) {
+  std::string SmartPtrName = "";

I wonder whether `MemRegion::printPretty` is what we want here.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:199
+} else {
+  const auto *TrackingExpr = getFirstArgExpr(Call);
+  C.addTransition(State, C.getNoteTag([ThisRegion, TrackingExpr](

I am a bit confused. As far as I understand we ALWAYS add the note tag below, 
regardless of the first argument being null.
I think I do understand that we only trigger this note tag when there is a null 
dereference but consider the code below:

```
void f(int *p) {
  std::unique_ptr u(p); // p is not always null here.
  if (!p) {
*u; // p is always null here
  }
}
```

Do we ant to output the message that the smart pointer is constructed using a 
null value? While this is technically true, there is a later event in the path 
that uncovers the fact that `p` is null.

@NoQ what do you think? I do not have any strong feelings about this, I only 
wonder how users would react to a bug report like that.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:352
+// Gets the SVal to track for a smart pointer memory region
+SVal SmartPtrModeling::getSValForRegion(const CallEvent &Call,
+CheckerContext &C) const {

I am a bit unhappy with this function.
I feel like it does not really have a purpose. It returns the null constant for 
default constructor calls, for every other case it just returns the first 
arg

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Generally i think this is shaping up nicely, with the couple of minor nits 
addressed i'm happy to land this. Like, there are a few cornercases that we 
could address but we could do that in follow-up patches. Folks, do you see 
anything else here?




Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:131
+  }
+  return nullptr;
+}

You never ever check for this case. Therefore this function entirely boils down 
to `Call.getArgExpr(0)` which is shorter than `getFirstArgExpr(Call)` anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:110
+  SmartPtrChecker *Checker = Mgr.registerChecker();
+  NullDereferenceBugTypePtr = &Checker->NullDereferenceBugType;
 }

vrnithinkumar wrote:
> NoQ wrote:
> > Wait, i don't understand again. You're taking the bug type from that 
> > checker and using it in that checker. Why did you need to make it global? I 
> > thought your problem was about capturing the bug type from the //raw// null 
> > dereference checker?
> > Wait, i don't understand again. You're taking the bug type from that 
> > checker and using it in that checker. Why did you need to make it global? I 
> > thought your problem was about capturing the bug type from the //raw// null 
> > dereference checker?
> 
> The check for bug type is in `SmartPtrModeling` but the bug type is defined 
> in `SmartPtrChecker`
Aha, ok, i misunderstood again. So i guess we didn't need a new header then. 
That said, it's an interesting layering violation that we encounter in this 
design: it used to be up to the checker to attach a visitor and so should be 
activating a note tag, but note tags are part of modeling, not checking.

I guess that's just how it is and it's the responsibility of the checkers to 
inform the modeling about bug types. I guess the ultimate API could look like 
`BugReport->activateNoteTag(NoteTagTag TT)` (where `TT` is a crying smiley that 
cries about "tag tags" T_T). But that's a story for another time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:110
+  SmartPtrChecker *Checker = Mgr.registerChecker();
+  NullDereferenceBugTypePtr = &Checker->NullDereferenceBugType;
 }

NoQ wrote:
> Wait, i don't understand again. You're taking the bug type from that checker 
> and using it in that checker. Why did you need to make it global? I thought 
> your problem was about capturing the bug type from the //raw// null 
> dereference checker?
> Wait, i don't understand again. You're taking the bug type from that checker 
> and using it in that checker. Why did you need to make it global? I thought 
> your problem was about capturing the bug type from the //raw// null 
> dereference checker?

The check for bug type is in `SmartPtrModeling` but the bug type is defined in 
`SmartPtrChecker`



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:95-96
+
+  const auto *RecordDecl = MethodDecl->getParent();
+  if (RecordDecl && RecordDecl->getDeclName().isIdentifier()) {
+OS << " of type '" << RecordDecl->getQualifiedNameAsString() << "'";

NoQ wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > Aha, this time you checked for anonymous declarations! Good.
> > > 
> > > That said, i'm not sure type is actually useful for this warning, because 
> > > they're roughly all the same.
> > Okay. I was trying to be consistent with `MoveChecker`.
> > Just removed smart pointer type.
> These were important in `MoveChecker` because use-after-move rules for smart 
> pointers are different from use-after-move rules of any other class in the 
> standard library and the checker has to behave differently 
> (https://wiki.sei.cmu.edu/confluence/x/O3s-BQ).
Oh
That makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/NullDereference.h:21
+namespace ento {
+namespace nullDereference {
+

Namespaces are traditionally snake_case rather than camelCase.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:110
+  SmartPtrChecker *Checker = Mgr.registerChecker();
+  NullDereferenceBugTypePtr = &Checker->NullDereferenceBugType;
 }

Wait, i don't understand again. You're taking the bug type from that checker 
and using it in that checker. Why did you need to make it global? I thought 
your problem was about capturing the bug type from the //raw// null dereference 
checker?



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:95-96
+
+  const auto *RecordDecl = MethodDecl->getParent();
+  if (RecordDecl && RecordDecl->getDeclName().isIdentifier()) {
+OS << " of type '" << RecordDecl->getQualifiedNameAsString() << "'";

vrnithinkumar wrote:
> NoQ wrote:
> > Aha, this time you checked for anonymous declarations! Good.
> > 
> > That said, i'm not sure type is actually useful for this warning, because 
> > they're roughly all the same.
> Okay. I was trying to be consistent with `MoveChecker`.
> Just removed smart pointer type.
These were important in `MoveChecker` because use-after-move rules for smart 
pointers are different from use-after-move rules of any other class in the 
standard library and the checker has to behave differently 
(https://wiki.sei.cmu.edu/confluence/x/O3s-BQ).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:86
+  if (const auto *DR = dyn_cast(DerefRegion)) {
+auto SmartPtrName = DR->getDecl()->getName();
+OS << " '" << SmartPtrName << "'";

NoQ wrote:
> Please `getNameAsString()` because `getName()` crashes on anonymous 
> declarations (eg., lambda captures, anonymous nested structs/unions, etc.).
Changed to use `getNameAsString()`



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:95-96
+
+  const auto *RecordDecl = MethodDecl->getParent();
+  if (RecordDecl && RecordDecl->getDeclName().isIdentifier()) {
+OS << " of type '" << RecordDecl->getQualifiedNameAsString() << "'";

NoQ wrote:
> Aha, this time you checked for anonymous declarations! Good.
> 
> That said, i'm not sure type is actually useful for this warning, because 
> they're roughly all the same.
Okay. I was trying to be consistent with `MoveChecker`.
Just removed smart pointer type.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:179
+
+  switch (SmartPtrMethod) {
+  case Constructor: {

NoQ wrote:
> NoQ wrote:
> > I feel this is a bit over-engineered. There's no need to create an enum and 
> > later convert it into a string when you can capture a string directly. 
> > Like, this entire "details" structure of your, it should be just captures 
> > instead, and your strings would make it immediately obvious what kind of 
> > note is emitted:
> > ```lang=c++
> > C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
> >   if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
> > return "";
> >  
> >   return Twine("Default constructed smart pointer '") + getSmartPtrName(R) 
> > + "' is null";
> > }));
> > ```
> > 
> > Hmm, maybe we should also provide an overload with lambdas of 
> > signature`void(PathSensitiveBugReport &BR, llvm::raw_ostream OS)` so that 
> > to make the same one-liners possible with streams? Something like this:
> > 
> > ```lang=c++
> > class CheckerContext {
> > // ...
> >   NoteTag *getNoteTag(std::function > llvm::raw_ostream OS)> f) {
> > return getNoteTag([](PathSensitiveBugReport &BR) -> std::string {
> >   llvm::SmallString<128> Str;
> >   llvm::raw_svector_ostream OS(Str);
> >   f(BR, OS);
> >   return OS.str();
> > });
> >   }
> > // ...
> > };
> > ```
> > 
> > Then you could do:
> > ```lang=c++
> > C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
> >   if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
> > return;
> > 
> >   OS << "Default constructed smart pointer '" << getSmartPtrName(R) << "' 
> > is null";
> > }));
> > ```
> (forgot `, llvm::raw_ostream OS` in the last snippet; probably forgot a bunch 
> of other stuff because i didn't try to actually compile these snippets)
- Added overload for `getNoteTag` with lambda accepting a `llvm::raw_ostream`
- Used the overload instead of `SmartPtrTagDetails` structure to add tags.
- Removed the `SmartPtrTagDetails` and changed related code.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+SmallString<128> Msg;
+llvm::raw_svector_ostream Out(Msg);
+TagDetails.trackValidExpr(BR);
+TagDetails.explainSmartPtrAction(Out);
+return std::string(Out.str());

NoQ wrote:
> NoQ wrote:
> > vrnithinkumar wrote:
> > > NoQ wrote:
> > > > NoQ wrote:
> > > > > vrnithinkumar wrote:
> > > > > > NoQ wrote:
> > > > > > > NoQ wrote:
> > > > > > > > Ok, note that note tags are attached to nodes independently of 
> > > > > > > > bug reports; when the report is thrown, only then we know what 
> > > > > > > > are the smart pointers that should be explained.
> > > > > > > > 
> > > > > > > > So there are two checks that you should do here:
> > > > > > > > 
> > > > > > > > 1. Check that the bug report is emitted by your checker (eg., 
> > > > > > > > by comparing bug types). If not, don't add notes.
> > > > > > > > 
> > > > > > > > 2. Check that the region about which the note speaks is related 
> > > > > > > > to your report (i.e., it's not a completely unrelated smart 
> > > > > > > > pointer). You can do that by marking the smart pointer as 
> > > > > > > > "interesting" (i.e., 
> > > > > > > > `PathSensitiveBugReport::markIntersting()`) when you emit the 
> > > > > > > > report, and then in the lambda you check whether the smart 
> > > > > > > > pointer is interesting before you emit a note. Additionally, 
> > > > > > > > you can carry over interestingness when smart pointers are 
> > > > > > > > copied.
> > > > > > > > 
> > > > > > > > This is what i was trying to accomplish with this code snippet 
> > > > > > > > that i included in the examples in the other comment:
> > > > > > > > ```lang=c++
> > > > > > > >   if (&BR.getBugType() != &NullDerefe

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar updated this revision to Diff 282771.
vrnithinkumar marked 10 inline comments as done.
vrnithinkumar edited the summary of this revision.
vrnithinkumar added a comment.

- Addressing review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Checkers/NullDereference.h
  clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/smart-ptr-text-output.cpp
  clang/test/Analysis/smart-ptr.cpp

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -12,7 +12,7 @@
   std::unique_ptr Q = std::move(P);
   if (Q)
 clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
-  *Q.get() = 1; // no-warning
+  *Q.get() = 1; // no-warning
   if (P)
 clang_analyzer_warnIfReached(); // no-warning
   // TODO: Report a null dereference (instead).
@@ -50,37 +50,38 @@
 
 void derefAfterDefaultCtr() {
   std::unique_ptr P;
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterCtrWithNull() {
   std::unique_ptr P(nullptr);
-  *P; // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  *P; // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
-void derefAfterCtrWithNullReturnMethod() {
-  std::unique_ptr P(return_null());
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+void derefAfterCtrWithNullVariable() {
+  A *InnerPtr = nullptr;
+  std::unique_ptr P(InnerPtr);
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterRelease() {
   std::unique_ptr P(new A());
   P.release();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterReset() {
   std::unique_ptr P(new A());
   P.reset();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNull() {
   std::unique_ptr P(new A());
   P.reset(nullptr);
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNonNull() {
@@ -102,6 +103,12 @@
   AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
 }
 
+void derefOnReleasedValidRawPtr() {
+  std::unique_ptr P(new A());
+  A *AP = P.release();
+  AP->foo(); // No warning.
+}
+
 void pass_smart_ptr_by_ref(std::unique_ptr &a);
 void pass_smart_ptr_by_const_ref(const std::unique_ptr &a);
 void pass_smart_ptr_by_rvalue_ref(std::unique_ptr &&a);
@@ -118,7 +125,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ref(P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -128,7 +135,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_rvalue_ref(std::move(P));
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -138,7 +145,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ptr(&P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
 
@@ -162,7 +169,7 @@
   {
 StructWithSmartPtr S;
 pass_struct_with_smart_ptr_by_const_ref(S);
-S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+S.P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
   {
 StructWithSmartPtr S;
@@ -172,7 +179,7 @@
   {
 StructWithSmartPtr S;
 pass_struct_with_smart_ptr_by_const_rvalu

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+SmallString<128> Msg;
+llvm::raw_svector_ostream Out(Msg);
+TagDetails.trackValidExpr(BR);
+TagDetails.explainSmartPtrAction(Out);
+return std::string(Out.str());

NoQ wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > NoQ wrote:
> > > > vrnithinkumar wrote:
> > > > > NoQ wrote:
> > > > > > NoQ wrote:
> > > > > > > Ok, note that note tags are attached to nodes independently of 
> > > > > > > bug reports; when the report is thrown, only then we know what 
> > > > > > > are the smart pointers that should be explained.
> > > > > > > 
> > > > > > > So there are two checks that you should do here:
> > > > > > > 
> > > > > > > 1. Check that the bug report is emitted by your checker (eg., by 
> > > > > > > comparing bug types). If not, don't add notes.
> > > > > > > 
> > > > > > > 2. Check that the region about which the note speaks is related 
> > > > > > > to your report (i.e., it's not a completely unrelated smart 
> > > > > > > pointer). You can do that by marking the smart pointer as 
> > > > > > > "interesting" (i.e., `PathSensitiveBugReport::markIntersting()`) 
> > > > > > > when you emit the report, and then in the lambda you check 
> > > > > > > whether the smart pointer is interesting before you emit a note. 
> > > > > > > Additionally, you can carry over interestingness when smart 
> > > > > > > pointers are copied.
> > > > > > > 
> > > > > > > This is what i was trying to accomplish with this code snippet 
> > > > > > > that i included in the examples in the other comment:
> > > > > > > ```lang=c++
> > > > > > >   if (&BR.getBugType() != &NullDereferenceBugType || 
> > > > > > > !R->isInteresting())
> > > > > > > return "";
> > > > > > > ```
> > > > > > (i strongly recommend having test cases for both of these issues)
> > > > > I was stuck on how to check the 2 cases from `SmartPtrModeling`.
> > > > > 
> > > > > 1. I was not able to figure out how to access 
> > > > > `NullDereferenceBugType` defined in the `SmartPtrChecker` in 
> > > > > `SmartPtrModeling` to check `&BR.getBugType() != 
> > > > > &NullDereferenceBugType`. Since `NullDereferenceBugType` is part of 
> > > > > the `SmartPtrChecker` I could not access it from 
> > > > > `PathSensitiveBugReport`.  One way I figured out is to use 
> > > > > `getCheckerName()` on BugType and compare the string. I feel this one 
> > > > > as little hacky.
> > > > > 
> > > > > 
> > > > > 2. I got stuck on how will we implement the `!R->isInteresting()` in 
> > > > > case of the bug report is added by some other checker on some other 
> > > > > region. For below test case, bug report is added on a raw pointer by 
> > > > > `CallAndMessageChecker` and the `!R->isInteresting()` will not 
> > > > > satisfy and we will not be adding note tags where `unique_ptr` is 
> > > > > released. I tried getting the LHS region for `A *AP = P.release();` 
> > > > > assignment operation and check if the region is interesting but not 
> > > > > sure whether its gonna work for some complex assignment cases.
> > > > > 
> > > > > ```
> > > > > void derefOnReleasedNullRawPtr() {
> > > > >   std::unique_ptr P;
> > > > >   A *AP = P.release(); // expected-note {{'AP' initialized to a null 
> > > > > pointer value}}
> > > > >   // expected-note@-1 {{Smart pointer 'P' is released and set to 
> > > > > null}}
> > > > >   AP->foo(); // expected-warning {{Called C++ object pointer is null 
> > > > > [core.CallAndMessage]}}
> > > > >   // expected-note@-1{{Called C++ object pointer is null}}
> > > > > }
> > > > > ```
> > > > > Since `NullDereferenceBugType` is part of the `SmartPtrChecker` I 
> > > > > could not access it from `PathSensitiveBugReport`.
> > > > 
> > > > You shouldn't be accessing it from the bug report, you should be 
> > > > accessing it from the lambda. See the example code snippets in 
> > > > D84600#inline-779418
> > > > 
> > > > > For below test case, bug report is added on a raw pointer by 
> > > > > `CallAndMessageChecker` and the `!R->isInteresting()` will not 
> > > > > satisfy and we will not be adding note tags where `unique_ptr` is 
> > > > > released.
> > > > 
> > > > That's an interesting question (no pun intended). The way i imagine 
> > > > this working is: the note tag for `.release()` should try to figure out 
> > > > whether the raw pointer is tracked and mark the smart pointer as 
> > > > interesting based on that. If the raw pointer was a symbol that would 
> > > > have been easy (either null dereference checker or 
> > > > `trackExpressionValue()` could mark it as interesting). But for 
> > > > concrete null pointer this won't work.
> > > > 
> > > > Maybe we should consider introducing interesting expressions. I.e., 
> > > > when `trackExpressionValue()` reaches the call-expression 
> > > > `P.release()`, it has to stop there. But if it also marked the 
> > > > call-expressi

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+SmallString<128> Msg;
+llvm::raw_svector_ostream Out(Msg);
+TagDetails.trackValidExpr(BR);
+TagDetails.explainSmartPtrAction(Out);
+return std::string(Out.str());

vrnithinkumar wrote:
> NoQ wrote:
> > NoQ wrote:
> > > vrnithinkumar wrote:
> > > > NoQ wrote:
> > > > > NoQ wrote:
> > > > > > Ok, note that note tags are attached to nodes independently of bug 
> > > > > > reports; when the report is thrown, only then we know what are the 
> > > > > > smart pointers that should be explained.
> > > > > > 
> > > > > > So there are two checks that you should do here:
> > > > > > 
> > > > > > 1. Check that the bug report is emitted by your checker (eg., by 
> > > > > > comparing bug types). If not, don't add notes.
> > > > > > 
> > > > > > 2. Check that the region about which the note speaks is related to 
> > > > > > your report (i.e., it's not a completely unrelated smart pointer). 
> > > > > > You can do that by marking the smart pointer as "interesting" 
> > > > > > (i.e., `PathSensitiveBugReport::markIntersting()`) when you emit 
> > > > > > the report, and then in the lambda you check whether the smart 
> > > > > > pointer is interesting before you emit a note. Additionally, you 
> > > > > > can carry over interestingness when smart pointers are copied.
> > > > > > 
> > > > > > This is what i was trying to accomplish with this code snippet that 
> > > > > > i included in the examples in the other comment:
> > > > > > ```lang=c++
> > > > > >   if (&BR.getBugType() != &NullDereferenceBugType || 
> > > > > > !R->isInteresting())
> > > > > > return "";
> > > > > > ```
> > > > > (i strongly recommend having test cases for both of these issues)
> > > > I was stuck on how to check the 2 cases from `SmartPtrModeling`.
> > > > 
> > > > 1. I was not able to figure out how to access `NullDereferenceBugType` 
> > > > defined in the `SmartPtrChecker` in `SmartPtrModeling` to check 
> > > > `&BR.getBugType() != &NullDereferenceBugType`. Since 
> > > > `NullDereferenceBugType` is part of the `SmartPtrChecker` I could not 
> > > > access it from `PathSensitiveBugReport`.  One way I figured out is to 
> > > > use `getCheckerName()` on BugType and compare the string. I feel this 
> > > > one as little hacky.
> > > > 
> > > > 
> > > > 2. I got stuck on how will we implement the `!R->isInteresting()` in 
> > > > case of the bug report is added by some other checker on some other 
> > > > region. For below test case, bug report is added on a raw pointer by 
> > > > `CallAndMessageChecker` and the `!R->isInteresting()` will not satisfy 
> > > > and we will not be adding note tags where `unique_ptr` is released. I 
> > > > tried getting the LHS region for `A *AP = P.release();` assignment 
> > > > operation and check if the region is interesting but not sure whether 
> > > > its gonna work for some complex assignment cases.
> > > > 
> > > > ```
> > > > void derefOnReleasedNullRawPtr() {
> > > >   std::unique_ptr P;
> > > >   A *AP = P.release(); // expected-note {{'AP' initialized to a null 
> > > > pointer value}}
> > > >   // expected-note@-1 {{Smart pointer 'P' is released and set to null}}
> > > >   AP->foo(); // expected-warning {{Called C++ object pointer is null 
> > > > [core.CallAndMessage]}}
> > > >   // expected-note@-1{{Called C++ object pointer is null}}
> > > > }
> > > > ```
> > > > Since `NullDereferenceBugType` is part of the `SmartPtrChecker` I could 
> > > > not access it from `PathSensitiveBugReport`.
> > > 
> > > You shouldn't be accessing it from the bug report, you should be 
> > > accessing it from the lambda. See the example code snippets in 
> > > D84600#inline-779418
> > > 
> > > > For below test case, bug report is added on a raw pointer by 
> > > > `CallAndMessageChecker` and the `!R->isInteresting()` will not satisfy 
> > > > and we will not be adding note tags where `unique_ptr` is released.
> > > 
> > > That's an interesting question (no pun intended). The way i imagine this 
> > > working is: the note tag for `.release()` should try to figure out 
> > > whether the raw pointer is tracked and mark the smart pointer as 
> > > interesting based on that. If the raw pointer was a symbol that would 
> > > have been easy (either null dereference checker or 
> > > `trackExpressionValue()` could mark it as interesting). But for concrete 
> > > null pointer this won't work.
> > > 
> > > Maybe we should consider introducing interesting expressions. I.e., when 
> > > `trackExpressionValue()` reaches the call-expression `P.release()`, it 
> > > has to stop there. But if it also marked the call-expression as 
> > > interesting, the note tag provided by the checker could read that 
> > > interestingness information and act upon it by marking the smart pointer 
> > > region as interesting.
> > >  That's an interesting question
> > 
> > 

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+SmallString<128> Msg;
+llvm::raw_svector_ostream Out(Msg);
+TagDetails.trackValidExpr(BR);
+TagDetails.explainSmartPtrAction(Out);
+return std::string(Out.str());

NoQ wrote:
> NoQ wrote:
> > vrnithinkumar wrote:
> > > NoQ wrote:
> > > > NoQ wrote:
> > > > > Ok, note that note tags are attached to nodes independently of bug 
> > > > > reports; when the report is thrown, only then we know what are the 
> > > > > smart pointers that should be explained.
> > > > > 
> > > > > So there are two checks that you should do here:
> > > > > 
> > > > > 1. Check that the bug report is emitted by your checker (eg., by 
> > > > > comparing bug types). If not, don't add notes.
> > > > > 
> > > > > 2. Check that the region about which the note speaks is related to 
> > > > > your report (i.e., it's not a completely unrelated smart pointer). 
> > > > > You can do that by marking the smart pointer as "interesting" (i.e., 
> > > > > `PathSensitiveBugReport::markIntersting()`) when you emit the report, 
> > > > > and then in the lambda you check whether the smart pointer is 
> > > > > interesting before you emit a note. Additionally, you can carry over 
> > > > > interestingness when smart pointers are copied.
> > > > > 
> > > > > This is what i was trying to accomplish with this code snippet that i 
> > > > > included in the examples in the other comment:
> > > > > ```lang=c++
> > > > >   if (&BR.getBugType() != &NullDereferenceBugType || 
> > > > > !R->isInteresting())
> > > > > return "";
> > > > > ```
> > > > (i strongly recommend having test cases for both of these issues)
> > > I was stuck on how to check the 2 cases from `SmartPtrModeling`.
> > > 
> > > 1. I was not able to figure out how to access `NullDereferenceBugType` 
> > > defined in the `SmartPtrChecker` in `SmartPtrModeling` to check 
> > > `&BR.getBugType() != &NullDereferenceBugType`. Since 
> > > `NullDereferenceBugType` is part of the `SmartPtrChecker` I could not 
> > > access it from `PathSensitiveBugReport`.  One way I figured out is to use 
> > > `getCheckerName()` on BugType and compare the string. I feel this one as 
> > > little hacky.
> > > 
> > > 
> > > 2. I got stuck on how will we implement the `!R->isInteresting()` in case 
> > > of the bug report is added by some other checker on some other region. 
> > > For below test case, bug report is added on a raw pointer by 
> > > `CallAndMessageChecker` and the `!R->isInteresting()` will not satisfy 
> > > and we will not be adding note tags where `unique_ptr` is released. I 
> > > tried getting the LHS region for `A *AP = P.release();` assignment 
> > > operation and check if the region is interesting but not sure whether its 
> > > gonna work for some complex assignment cases.
> > > 
> > > ```
> > > void derefOnReleasedNullRawPtr() {
> > >   std::unique_ptr P;
> > >   A *AP = P.release(); // expected-note {{'AP' initialized to a null 
> > > pointer value}}
> > >   // expected-note@-1 {{Smart pointer 'P' is released and set to null}}
> > >   AP->foo(); // expected-warning {{Called C++ object pointer is null 
> > > [core.CallAndMessage]}}
> > >   // expected-note@-1{{Called C++ object pointer is null}}
> > > }
> > > ```
> > > Since `NullDereferenceBugType` is part of the `SmartPtrChecker` I could 
> > > not access it from `PathSensitiveBugReport`.
> > 
> > You shouldn't be accessing it from the bug report, you should be accessing 
> > it from the lambda. See the example code snippets in D84600#inline-779418
> > 
> > > For below test case, bug report is added on a raw pointer by 
> > > `CallAndMessageChecker` and the `!R->isInteresting()` will not satisfy 
> > > and we will not be adding note tags where `unique_ptr` is released.
> > 
> > That's an interesting question (no pun intended). The way i imagine this 
> > working is: the note tag for `.release()` should try to figure out whether 
> > the raw pointer is tracked and mark the smart pointer as interesting based 
> > on that. If the raw pointer was a symbol that would have been easy (either 
> > null dereference checker or `trackExpressionValue()` could mark it as 
> > interesting). But for concrete null pointer this won't work.
> > 
> > Maybe we should consider introducing interesting expressions. I.e., when 
> > `trackExpressionValue()` reaches the call-expression `P.release()`, it has 
> > to stop there. But if it also marked the call-expression as interesting, 
> > the note tag provided by the checker could read that interestingness 
> > information and act upon it by marking the smart pointer region as 
> > interesting.
> >  That's an interesting question
> 
> I'd rather make a separate commit for this endeavor because it sounds pretty 
> nasty.
> You shouldn't be accessing it from the bug report, you should be accessing it 
> from the lambda. See the exam

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+SmallString<128> Msg;
+llvm::raw_svector_ostream Out(Msg);
+TagDetails.trackValidExpr(BR);
+TagDetails.explainSmartPtrAction(Out);
+return std::string(Out.str());

NoQ wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > NoQ wrote:
> > > > Ok, note that note tags are attached to nodes independently of bug 
> > > > reports; when the report is thrown, only then we know what are the 
> > > > smart pointers that should be explained.
> > > > 
> > > > So there are two checks that you should do here:
> > > > 
> > > > 1. Check that the bug report is emitted by your checker (eg., by 
> > > > comparing bug types). If not, don't add notes.
> > > > 
> > > > 2. Check that the region about which the note speaks is related to your 
> > > > report (i.e., it's not a completely unrelated smart pointer). You can 
> > > > do that by marking the smart pointer as "interesting" (i.e., 
> > > > `PathSensitiveBugReport::markIntersting()`) when you emit the report, 
> > > > and then in the lambda you check whether the smart pointer is 
> > > > interesting before you emit a note. Additionally, you can carry over 
> > > > interestingness when smart pointers are copied.
> > > > 
> > > > This is what i was trying to accomplish with this code snippet that i 
> > > > included in the examples in the other comment:
> > > > ```lang=c++
> > > >   if (&BR.getBugType() != &NullDereferenceBugType || 
> > > > !R->isInteresting())
> > > > return "";
> > > > ```
> > > (i strongly recommend having test cases for both of these issues)
> > I was stuck on how to check the 2 cases from `SmartPtrModeling`.
> > 
> > 1. I was not able to figure out how to access `NullDereferenceBugType` 
> > defined in the `SmartPtrChecker` in `SmartPtrModeling` to check 
> > `&BR.getBugType() != &NullDereferenceBugType`. Since 
> > `NullDereferenceBugType` is part of the `SmartPtrChecker` I could not 
> > access it from `PathSensitiveBugReport`.  One way I figured out is to use 
> > `getCheckerName()` on BugType and compare the string. I feel this one as 
> > little hacky.
> > 
> > 
> > 2. I got stuck on how will we implement the `!R->isInteresting()` in case 
> > of the bug report is added by some other checker on some other region. For 
> > below test case, bug report is added on a raw pointer by 
> > `CallAndMessageChecker` and the `!R->isInteresting()` will not satisfy and 
> > we will not be adding note tags where `unique_ptr` is released. I tried 
> > getting the LHS region for `A *AP = P.release();` assignment operation and 
> > check if the region is interesting but not sure whether its gonna work for 
> > some complex assignment cases.
> > 
> > ```
> > void derefOnReleasedNullRawPtr() {
> >   std::unique_ptr P;
> >   A *AP = P.release(); // expected-note {{'AP' initialized to a null 
> > pointer value}}
> >   // expected-note@-1 {{Smart pointer 'P' is released and set to null}}
> >   AP->foo(); // expected-warning {{Called C++ object pointer is null 
> > [core.CallAndMessage]}}
> >   // expected-note@-1{{Called C++ object pointer is null}}
> > }
> > ```
> > Since `NullDereferenceBugType` is part of the `SmartPtrChecker` I could not 
> > access it from `PathSensitiveBugReport`.
> 
> You shouldn't be accessing it from the bug report, you should be accessing it 
> from the lambda. See the example code snippets in D84600#inline-779418
> 
> > For below test case, bug report is added on a raw pointer by 
> > `CallAndMessageChecker` and the `!R->isInteresting()` will not satisfy and 
> > we will not be adding note tags where `unique_ptr` is released.
> 
> That's an interesting question (no pun intended). The way i imagine this 
> working is: the note tag for `.release()` should try to figure out whether 
> the raw pointer is tracked and mark the smart pointer as interesting based on 
> that. If the raw pointer was a symbol that would have been easy (either null 
> dereference checker or `trackExpressionValue()` could mark it as 
> interesting). But for concrete null pointer this won't work.
> 
> Maybe we should consider introducing interesting expressions. I.e., when 
> `trackExpressionValue()` reaches the call-expression `P.release()`, it has to 
> stop there. But if it also marked the call-expression as interesting, the 
> note tag provided by the checker could read that interestingness information 
> and act upon it by marking the smart pointer region as interesting.
>  That's an interesting question

I'd rather make a separate commit for this endeavor because it sounds pretty 
nasty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+SmallString<128> Msg;
+llvm::raw_svector_ostream Out(Msg);
+TagDetails.trackValidExpr(BR);
+TagDetails.explainSmartPtrAction(Out);
+return std::string(Out.str());

vrnithinkumar wrote:
> NoQ wrote:
> > NoQ wrote:
> > > Ok, note that note tags are attached to nodes independently of bug 
> > > reports; when the report is thrown, only then we know what are the smart 
> > > pointers that should be explained.
> > > 
> > > So there are two checks that you should do here:
> > > 
> > > 1. Check that the bug report is emitted by your checker (eg., by 
> > > comparing bug types). If not, don't add notes.
> > > 
> > > 2. Check that the region about which the note speaks is related to your 
> > > report (i.e., it's not a completely unrelated smart pointer). You can do 
> > > that by marking the smart pointer as "interesting" (i.e., 
> > > `PathSensitiveBugReport::markIntersting()`) when you emit the report, and 
> > > then in the lambda you check whether the smart pointer is interesting 
> > > before you emit a note. Additionally, you can carry over interestingness 
> > > when smart pointers are copied.
> > > 
> > > This is what i was trying to accomplish with this code snippet that i 
> > > included in the examples in the other comment:
> > > ```lang=c++
> > >   if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
> > > return "";
> > > ```
> > (i strongly recommend having test cases for both of these issues)
> I was stuck on how to check the 2 cases from `SmartPtrModeling`.
> 
> 1. I was not able to figure out how to access `NullDereferenceBugType` 
> defined in the `SmartPtrChecker` in `SmartPtrModeling` to check 
> `&BR.getBugType() != &NullDereferenceBugType`. Since `NullDereferenceBugType` 
> is part of the `SmartPtrChecker` I could not access it from 
> `PathSensitiveBugReport`.  One way I figured out is to use `getCheckerName()` 
> on BugType and compare the string. I feel this one as little hacky.
> 
> 
> 2. I got stuck on how will we implement the `!R->isInteresting()` in case of 
> the bug report is added by some other checker on some other region. For below 
> test case, bug report is added on a raw pointer by `CallAndMessageChecker` 
> and the `!R->isInteresting()` will not satisfy and we will not be adding note 
> tags where `unique_ptr` is released. I tried getting the LHS region for `A 
> *AP = P.release();` assignment operation and check if the region is 
> interesting but not sure whether its gonna work for some complex assignment 
> cases.
> 
> ```
> void derefOnReleasedNullRawPtr() {
>   std::unique_ptr P;
>   A *AP = P.release(); // expected-note {{'AP' initialized to a null pointer 
> value}}
>   // expected-note@-1 {{Smart pointer 'P' is released and set to null}}
>   AP->foo(); // expected-warning {{Called C++ object pointer is null 
> [core.CallAndMessage]}}
>   // expected-note@-1{{Called C++ object pointer is null}}
> }
> ```
> Since `NullDereferenceBugType` is part of the `SmartPtrChecker` I could not 
> access it from `PathSensitiveBugReport`.

You shouldn't be accessing it from the bug report, you should be accessing it 
from the lambda. See the example code snippets in D84600#inline-779418

> For below test case, bug report is added on a raw pointer by 
> `CallAndMessageChecker` and the `!R->isInteresting()` will not satisfy and we 
> will not be adding note tags where `unique_ptr` is released.

That's an interesting question (no pun intended). The way i imagine this 
working is: the note tag for `.release()` should try to figure out whether the 
raw pointer is tracked and mark the smart pointer as interesting based on that. 
If the raw pointer was a symbol that would have been easy (either null 
dereference checker or `trackExpressionValue()` could mark it as interesting). 
But for concrete null pointer this won't work.

Maybe we should consider introducing interesting expressions. I.e., when 
`trackExpressionValue()` reaches the call-expression `P.release()`, it has to 
stop there. But if it also marked the call-expression as interesting, the note 
tag provided by the checker could read that interestingness information and act 
upon it by marking the smart pointer region as interesting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+SmallString<128> Msg;
+llvm::raw_svector_ostream Out(Msg);
+TagDetails.trackValidExpr(BR);
+TagDetails.explainSmartPtrAction(Out);
+return std::string(Out.str());

NoQ wrote:
> NoQ wrote:
> > Ok, note that note tags are attached to nodes independently of bug reports; 
> > when the report is thrown, only then we know what are the smart pointers 
> > that should be explained.
> > 
> > So there are two checks that you should do here:
> > 
> > 1. Check that the bug report is emitted by your checker (eg., by comparing 
> > bug types). If not, don't add notes.
> > 
> > 2. Check that the region about which the note speaks is related to your 
> > report (i.e., it's not a completely unrelated smart pointer). You can do 
> > that by marking the smart pointer as "interesting" (i.e., 
> > `PathSensitiveBugReport::markIntersting()`) when you emit the report, and 
> > then in the lambda you check whether the smart pointer is interesting 
> > before you emit a note. Additionally, you can carry over interestingness 
> > when smart pointers are copied.
> > 
> > This is what i was trying to accomplish with this code snippet that i 
> > included in the examples in the other comment:
> > ```lang=c++
> >   if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
> > return "";
> > ```
> (i strongly recommend having test cases for both of these issues)
I was stuck on how to check the 2 cases from `SmartPtrModeling`.

1. I was not able to figure out how to access `NullDereferenceBugType` defined 
in the `SmartPtrChecker` in `SmartPtrModeling` to check `&BR.getBugType() != 
&NullDereferenceBugType`. Since `NullDereferenceBugType` is part of the 
`SmartPtrChecker` I could not access it from `PathSensitiveBugReport`.  One way 
I figured out is to use `getCheckerName()` on BugType and compare the string. I 
feel this one as little hacky.


2. I got stuck on how will we implement the `!R->isInteresting()` in case of 
the bug report is added by some other checker on some other region. For below 
test case, bug report is added on a raw pointer by `CallAndMessageChecker` and 
the `!R->isInteresting()` will not satisfy and we will not be adding note tags 
where `unique_ptr` is released. I tried getting the LHS region for `A *AP = 
P.release();` assignment operation and check if the region is interesting but 
not sure whether its gonna work for some complex assignment cases.

```
void derefOnReleasedNullRawPtr() {
  std::unique_ptr P;
  A *AP = P.release(); // expected-note {{'AP' initialized to a null pointer 
value}}
  // expected-note@-1 {{Smart pointer 'P' is released and set to null}}
  AP->foo(); // expected-warning {{Called C++ object pointer is null 
[core.CallAndMessage]}}
  // expected-note@-1{{Called C++ object pointer is null}}
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600

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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:179
+
+  switch (SmartPtrMethod) {
+  case Constructor: {

NoQ wrote:
> I feel this is a bit over-engineered. There's no need to create an enum and 
> later convert it into a string when you can capture a string directly. Like, 
> this entire "details" structure of your, it should be just captures instead, 
> and your strings would make it immediately obvious what kind of note is 
> emitted:
> ```lang=c++
> C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
>   if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
> return "";
>  
>   return Twine("Default constructed smart pointer '") + getSmartPtrName(R) + 
> "' is null";
> }));
> ```
> 
> Hmm, maybe we should also provide an overload with lambdas of 
> signature`void(PathSensitiveBugReport &BR, llvm::raw_ostream OS)` so that to 
> make the same one-liners possible with streams? Something like this:
> 
> ```lang=c++
> class CheckerContext {
> // ...
>   NoteTag *getNoteTag(std::function llvm::raw_ostream OS)> f) {
> return getNoteTag([](PathSensitiveBugReport &BR) -> std::string {
>   llvm::SmallString<128> Str;
>   llvm::raw_svector_ostream OS(Str);
>   f(BR, OS);
>   return OS.str();
> });
>   }
> // ...
> };
> ```
> 
> Then you could do:
> ```lang=c++
> C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
>   if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
> return;
> 
>   OS << "Default constructed smart pointer '" << getSmartPtrName(R) << "' is 
> null";
> }));
> ```
(forgot `, llvm::raw_ostream OS` in the last snippet; probably forgot a bunch 
of other stuff because i didn't try to actually compile these snippets)



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+SmallString<128> Msg;
+llvm::raw_svector_ostream Out(Msg);
+TagDetails.trackValidExpr(BR);
+TagDetails.explainSmartPtrAction(Out);
+return std::string(Out.str());

NoQ wrote:
> Ok, note that note tags are attached to nodes independently of bug reports; 
> when the report is thrown, only then we know what are the smart pointers that 
> should be explained.
> 
> So there are two checks that you should do here:
> 
> 1. Check that the bug report is emitted by your checker (eg., by comparing 
> bug types). If not, don't add notes.
> 
> 2. Check that the region about which the note speaks is related to your 
> report (i.e., it's not a completely unrelated smart pointer). You can do that 
> by marking the smart pointer as "interesting" (i.e., 
> `PathSensitiveBugReport::markIntersting()`) when you emit the report, and 
> then in the lambda you check whether the smart pointer is interesting before 
> you emit a note. Additionally, you can carry over interestingness when smart 
> pointers are copied.
> 
> This is what i was trying to accomplish with this code snippet that i 
> included in the examples in the other comment:
> ```lang=c++
>   if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
> return "";
> ```
(i strongly recommend having test cases for both of these issues)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600



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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:86
+  if (const auto *DR = dyn_cast(DerefRegion)) {
+auto SmartPtrName = DR->getDecl()->getName();
+OS << " '" << SmartPtrName << "'";

Please `getNameAsString()` because `getName()` crashes on anonymous 
declarations (eg., lambda captures, anonymous nested structs/unions, etc.).



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:95-96
+
+  const auto *RecordDecl = MethodDecl->getParent();
+  if (RecordDecl && RecordDecl->getDeclName().isIdentifier()) {
+OS << " of type '" << RecordDecl->getQualifiedNameAsString() << "'";

Aha, this time you checked for anonymous declarations! Good.

That said, i'm not sure type is actually useful for this warning, because 
they're roughly all the same.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:179
+
+  switch (SmartPtrMethod) {
+  case Constructor: {

I feel this is a bit over-engineered. There's no need to create an enum and 
later convert it into a string when you can capture a string directly. Like, 
this entire "details" structure of your, it should be just captures instead, 
and your strings would make it immediately obvious what kind of note is emitted:
```lang=c++
C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
  if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
return "";
 
  return Twine("Default constructed smart pointer '") + getSmartPtrName(R) + "' 
is null";
}));
```

Hmm, maybe we should also provide an overload with lambdas of 
signature`void(PathSensitiveBugReport &BR, llvm::raw_ostream OS)` so that to 
make the same one-liners possible with streams? Something like this:

```lang=c++
class CheckerContext {
// ...
  NoteTag *getNoteTag(std::function f) {
return getNoteTag([](PathSensitiveBugReport &BR) -> std::string {
  llvm::SmallString<128> Str;
  llvm::raw_svector_ostream OS(Str);
  f(BR, OS);
  return OS.str();
});
  }
// ...
};
```

Then you could do:
```lang=c++
C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
  if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
return;

  OS << "Default constructed smart pointer '" << getSmartPtrName(R) << "' is 
null";
}));
```



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+SmallString<128> Msg;
+llvm::raw_svector_ostream Out(Msg);
+TagDetails.trackValidExpr(BR);
+TagDetails.explainSmartPtrAction(Out);
+return std::string(Out.str());

Ok, note that note tags are attached to nodes independently of bug reports; 
when the report is thrown, only then we know what are the smart pointers that 
should be explained.

So there are two checks that you should do here:

1. Check that the bug report is emitted by your checker (eg., by comparing bug 
types). If not, don't add notes.

2. Check that the region about which the note speaks is related to your report 
(i.e., it's not a completely unrelated smart pointer). You can do that by 
marking the smart pointer as "interesting" (i.e., 
`PathSensitiveBugReport::markIntersting()`) when you emit the report, and then 
in the lambda you check whether the smart pointer is interesting before you 
emit a note. Additionally, you can carry over interestingness when smart 
pointers are copied.

This is what i was trying to accomplish with this code snippet that i included 
in the examples in the other comment:
```lang=c++
  if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
return "";
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600



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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar created this revision.
Herald added subscribers: cfe-commits, ASDenysPetrov, martong, Charusso, 
dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, 
baloghadamsoftware, xazax.hun.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84600

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/smart-ptr-text-output.cpp
  clang/test/Analysis/smart-ptr.cpp

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -12,7 +12,7 @@
   std::unique_ptr Q = std::move(P);
   if (Q)
 clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
-  *Q.get() = 1; // no-warning
+  *Q.get() = 1; // no-warning
   if (P)
 clang_analyzer_warnIfReached(); // no-warning
   // TODO: Report a null dereference (instead).
@@ -50,37 +50,38 @@
 
 void derefAfterDefaultCtr() {
   std::unique_ptr P;
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterCtrWithNull() {
   std::unique_ptr P(nullptr);
-  *P; // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  *P; // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
 }
 
-void derefAfterCtrWithNullReturnMethod() {
-  std::unique_ptr P(return_null());
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+void derefAfterCtrWithNullVariable() {
+  A *InnerPtr = nullptr;
+  std::unique_ptr P(InnerPtr);
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterRelease() {
   std::unique_ptr P(new A());
   P.release();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterReset() {
   std::unique_ptr P(new A());
   P.reset();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNull() {
   std::unique_ptr P(new A());
   P.reset(nullptr);
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNonNull() {
@@ -102,6 +103,12 @@
   AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
 }
 
+void derefOnReleasedValidRawPtr() {
+  std::unique_ptr P(new A());
+  A *AP = P.release();
+  AP->foo(); // No warning.
+}
+
 void pass_smart_ptr_by_ref(std::unique_ptr &a);
 void pass_smart_ptr_by_const_ref(const std::unique_ptr &a);
 void pass_smart_ptr_by_rvalue_ref(std::unique_ptr &&a);
@@ -118,7 +125,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ref(P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -128,7 +135,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_rvalue_ref(std::move(P));
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -138,7 +145,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ptr(&P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
   }
 }
 
@@ -162,7 +169,7 @@
   {
 StructWithSmartPtr S;
 pass_struct_with_smart_ptr_by_const_ref(S);
-S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+S.P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
   }
   {
 StructWithSmartPtr S;
@@ -