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

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:577-583
+  CheckerOptions<[
+    CmdLineOption<Boolean,
+                  "CheckSmartPtrDereference",
+                  "Enable check for SmartPtr null dereferences",
+                  "false",
+                  InAlpha>,
+  ]>,
----------------
Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > This goes against D81750 -- Sorry for not bringing this up earlier, but you can't emit diagnostics from a checker that is hidden :/
> > > 
> > > The proper solution would be to create a non-hidden checker, and emit a diagnostic under its name. You can take inspiration from `MallocChecker`, `NullabilityChecker`, and a variety of other sound in the stack of this patch: D77845
> > Aha, ok, that's what i meant at the end of D81315#inline-760088. @Szelethus insists, so let's turn ourselves into a separate checker right away!
> > 
> > In fact, let's *not* take the inspiration from poor-man's solutions in `MallocChecker` etc., but instead let's try a full-on modular approach:
> > - Make a separate .cpp file and a separate Checker class for emitting diagnostics.
> >   - Remove the checker option and instead add a separate checker entry in Checkers.td.
> > - Subscribe the new checker on PreCall only and move all the bug reporting logic there.
> > - Keep all the modeling logic in the old checker (it's called SmartPtr//Modeling// for a reason!).
> > - Put common functionality into a header file shared between the two checkers.
> >   - In particular, the GDM trait should be only accessed through such getter.
> >     - Take some inspiration from `Taint.h`.
> Sounds great! And of course, say the word if any help is needed! :)
Thanks...!

Created a new checker `alpha.cplusplus.SmartPtr` to emit the SmartPtr dereference diagnostic.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:139
+
+  const auto *MethodDecl = dyn_cast_or_null<CXXMethodDecl>(OC->getDecl());
+
----------------
xazax.hun wrote:
> You should never get null here due to `isStdSmartPointerClass` guarding above. I think the null check could be transformed into an assert.
Changed to dyn_cast.
Added assert 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:141
+
+  if (!MethodDecl || !MethodDecl->isOverloadedOperator())
+    return;
----------------
xazax.hun wrote:
> You have a `CXXMemberOperatorCall` which already represents an overloaded operator. This check is either redundant or could be an assert instead.
Removed the redundant check


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:144
+
+  OverloadedOperatorKind OOK = MethodDecl->getOverloadedOperator();
+  if (OOK == OO_Star || OOK == OO_Arrow) {
----------------
xazax.hun wrote:
> In case `CXXMemberOperatorCall ` class does not have a way to query the overloaded operator kind maybe it would be worth to add a helper method to that class. So you can do most of the checking using one interface only.
Added `getOverloadedOperator()` in `CXXMemberOperatorCall` class


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:148
+    if (InnerPointVal && InnerPointVal->isZeroConstant()) {
+      reportBug(C, Call);
+    }
----------------
NoQ wrote:
> This doesn't seem to be guarded by `ShouldCheckSmartPtrDereference`.
> 
> Consider adding a LIT test for the flag to make sure we're not leaking our project to the public until it's ready ^.^ Say, you could make two run-lines in your test like this:
> ```
> // RUN: %clang_analyze_cc1 -verify=expected -analyzer-config cplusplus.SmartPtr:CheckSmartPtrDereference=true \
> // RUN: ...
> // RUN: %clang_analyze_cc1 -verify=unexpected \
> // RUN: ...
> 
> // unexpected-no-diagnostics
> ```
> (see https://clang.llvm.org/doxygen/classclang_1_1VerifyDiagnosticConsumer.html for more fancy tricks with `-verify`)
Added one more run to verify it.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:198
+                            *InnerPointVal);
+    C.addTransition(State);
+  }
----------------
xazax.hun wrote:
> It is possible that both `updateTrackedRegion` and this adds a transition. Both transition will have the same predecessor, i.e. you introduce a split in the exploded graph. Is this intended?
This was not intended.
- Fixed to avoid split in the exploded graph.
- Updated test to check with `clang_analyzer_numTimesReached`


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

https://reviews.llvm.org/D81315