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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
