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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:124
+    return;
+  updateTrackedRegion(Call, C, ThisValRegion);
+}
----------------
NoQ wrote:
> Not all constructors behave this way. In particular, if it's a copy/move constructor this function would track the value of the original smart pointer to this smart pointer, but what we need is to track the value of the raw pointer instead.
Actually, let's add an assertion into `updateTrackedRegion` that the value that's put into the map is of pointer type. This would give us an automatic notification when we make such mistakes.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:137
+    if (IsRegDead) {
+      State = State->remove<TrackedRegionMap>(Region);
+    }
----------------
xazax.hun wrote:
> In LLVM we often omit braces for small single statement branches. Also, Artem mentioned that we could interact with the malloc checker. Maybe it is worth considering to notify the malloc checker to mark the appropriate region as deallocated? This would help find double release errors, avoid spurious leak errors and so on. 
> Maybe it is worth considering to notify the malloc checker to mark the appropriate region as deallocated?

This happens in destructor.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:213
+  if (NumArgs == 0) {
+    auto NullSVal = C.getSValBuilder().makeNull();
+    State = State->set<TrackedRegionMap>(ThisValRegion, NullSVal);
----------------
xazax.hun wrote:
> I wonder if we should use `makeNullWithType`. @NoQ what do you think? I am always confused when should we prefer one over the other.
I think `makeNull()` produces a null //pointer// (as opposed to numeric zero produced by `makeZeroVal(Ty)`) and there's not much choice when it comes to types of pointers.

Like, i mean, there were a few attempts to add support for architectures in which pointers come in different sizes but i don't think we care about that situation yet.


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

https://reviews.llvm.org/D81315