[PATCH] D81315: [analyzer] Warning for default constructed unique pointer dereferences

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:41
+
+  bool checkDeferenceOps(const CallEvent &Call, CheckerContext &C) const;
+};
----------------
NoQ wrote:
> Looks like dead code.
Thanks!
removed.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:72
+      NullDereferenceBugType, "Dereference of null smart pointer", ErrNode);
+  R->addRange(Call.getSourceRange());
+  C.emitReport(std::move(R));
----------------
NoQ wrote:
> This is probably unnecessary. The default `SourceRange` highlighted in path-sensitive report is the error node's statement and it should be exactly the same.
Removed.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:67-80
+bool isStdSmartPtrCall(const CallEvent &Call) {
+  const auto *MethodDecl = dyn_cast_or_null<CXXMethodDecl>(Call.getDecl());
+  if (!MethodDecl || !MethodDecl->getParent())
+    return false;
+
+  const auto *RecordDecl = MethodDecl->getParent();
+  if (!RecordDecl || !RecordDecl->getDeclContext()->isStdNamespace())
----------------
NoQ wrote:
> Ok, so the normal solution to this problem is to make this logic a part of your `CallDescriptionMap`:
> 
> ```lang=c++
>   CallDescriptionMap<SmartPtrMethodHandlerFn> SmartPtrMethodHandlers{
> 
>     {{"std", "unique_ptr", "reset"}, &SmartPtrModeling::handleReset},
>     {{"std", "unique_ptr", "release"}, &SmartPtrModeling::handleRelease},
>     {{"std", "unique_ptr", "swap", 1}, &SmartPtrModeling::handleSwap},
> 
>     {{"std", "shared_ptr", "reset"}, &SmartPtrModeling::handleReset},
>     {{"std", "shared_ptr", "release"}, &SmartPtrModeling::handleRelease},
>     {{"std", "shared_ptr", "swap", 1}, &SmartPtrModeling::handleSwap},
> 
>     {{"std", "weak_ptr", "reset"}, &SmartPtrModeling::handleReset},
>     {{"std", "weak_ptr", "release"}, &SmartPtrModeling::handleRelease},
>     {{"std", "weak_ptr", "swap", 1}, &SmartPtrModeling::handleSwap},
>   };
> ```
> 
> It looks like you're not doing this because `CallDescriptionMap` doesn't support operator calls yet. In fact, it looks like it doesn't support them because i'm not doing my homework by reviewing D81059. I just tried to catch up on it.
> 
> The other potential reason not to use `CallDescriptionMap` would be that you'll have to duplicate the list of methods for every smart pointer class you want to support. I don't think it's necessarily bad though, because different classes may in fact require different handling.
> 
> The downside of your solution, though, is that you're manually implementing the name matching logic that has been already implemented for you in `CallDescriptionMap`. And the reason we made `CallDescriptionMap` was because we really really wanted to re-use this logic because it's surprisingly difficult to implement correctly. One potential problem i see with your implementation is that you don't account for inline namespaces such as [[ https://github.com/llvm/llvm-project/blob/master/clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp#L127 | libcxx's `__1` ]]. `CallDescriptionMap` knows about these things but they're almost impossible to discover independently when you're matching names by hand.
> 
> Let's leave this code as-is for now but try to get rid of this function as soon as possible (i.e., when D81059 lands).
Thanks for the details.
If I recall correctly, I also found that  `CallDescriptionMap` was not supporting constructors also. 
I am not sure why.
Is it because constructor name is a special name?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:50
+  void handleSwap(const CallEvent &Call, CheckerContext &C) const;
+  bool checkDeferenceOps(const CallEvent &Call, CheckerContext &C) const;
+
----------------
NoQ wrote:
> Looks like dead code.
Thanks!
removed.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:81
                                 CheckerContext &C) const {
-  if (!isNullAfterMoveMethod(Call))
+  if (isNullAfterMoveMethod(Call)) {
+    ProgramStateRef State = C.getState();
----------------
NoQ wrote:
> vrnithinkumar wrote:
> > xazax.hun wrote:
> > > Don't we want this to be also protected by the `isStdSmartPointerClass` check?
> > added `isStdSmartPointerClass` check in the beginning.
> Uh-oh, i'm shocked this wasn't in place before. Maybe we should do some code review or something. What idiot wrote this code anyway? Wait, it was me.
:)


================
Comment at: clang/test/Analysis/smart-ptr.cpp:5
 // RUN:   -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core\
+// RUN:   -analyzer-checker alpha.cplusplus.SmartPtr\
----------------
NoQ wrote:
> This second run-line no longer tests the option. The checker is disabled, of course there won't be any diagnostics. I think we should remove the second run-line now and i don't have much better tests in mind that we won't eventually remove as we write more code.
Okay thanks
removed the second line.


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

https://reviews.llvm.org/D81315