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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This looks good at a glance to me! I only have minor nits left.

It looks like we still need the option to be in place even though we moved the warnings into a separate checker, so that not to accidentally behave worse than with conservative modeling, but it becomes much less important.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:792-794
+  OverloadedOperatorKind getOverloadedOperator() const {
+    return getOriginExpr()->getOperator();
+  }
----------------
Nice!


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


================
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));
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:81
                                 CheckerContext &C) const {
-  if (!isNullAfterMoveMethod(Call))
+  if (isNullAfterMoveMethod(Call)) {
+    ProgramStateRef State = C.getState();
----------------
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/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:50
+  void handleSwap(const CallEvent &Call, CheckerContext &C) const;
+  bool checkDeferenceOps(const CallEvent &Call, CheckerContext &C) const;
+
----------------
Looks like dead code.


================
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())
----------------
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).


================
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\
----------------
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.


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

https://reviews.llvm.org/D81315