[PATCH] D64274: [analyzer] VirtualCallChecker overhaul.

Sat Jul 6 11:16:50 PDT 2019

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

I happen to have very recent analyses on a couple projects, I'll throw this in: LLVM+Clang+Clang-tools-extra <http://cc.elte.hu:15001/Default/#is-unique=on&run=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions%20WITH%20moderate%20tracking&review-status=Unreviewed&review-status=Confirmed&detection-status=New&detection-status=Reopened&detection-status=Unresolved&checker-name=optin.cplusplus.VirtualCall&tab=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions%20WITH%20moderate%20tracking>. No findings on Xerces or Bitcoin.

I caught up on the thread you linked, and the checker code since it wasn't that long. Overall, what you'd like to do with this checker sounds amazing! I agree that the visitor does more harm then good, and that pure only analysis should be on-by-default. Also, thanks for cleaning up the testfile!!

> I'd like to point out that it breaks backwards compatibility. I'm not going to be hurt by this and i haven't heard a lot about users of this opt-in check but if you know them, please let me know. If we agree to break backwards compatibility, we should make sure that the result is the best, so i'd like to hear @Szelethus's opinion, as he usually has strong opinions on checker options and dependencies :)

CodeChecker turns on `optin.cplusplus.VirtualCall` by default, so this wouldn't hurt its users much. Also, you can enable/disable checkers from an arbitrary version of clang, their names aren't hardcoded (only the on-by-default list).

That said, dependencywise, there is a directed circle here, despite `optin.cplusplus.VirtualCall` being a glorified checker option (this is what I like to label as a "subchecker"). [side note: since this checker doesn't really do any modeling, in fact it wouldn even store a state if it inspected the call stack, it wouldn't be logical to create a modeling checker that would be a dependency of both `PureVirtualCall` and `VirtualCall`. Since `PureVirtualCall` is a strict subset of `VirtualCall`, if it causes any crashes, you have to disable both anyways. ] Your current implementation would cause an assertion failure if you enabled both checkers due to attempting to register the checker object 2 times, to avoid that, *see inlines*

           <------------------
         /                    \
  PureVirtualCall        VirtualCall     and should be:     PureVirtualCall <------ VirtualCall
         \                    /
           ------------------>

================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:562
+  HelpText<"Check virtual function calls during construction/destruction">,
   Documentation<HasDocumentation>;

----------------
`Dependencies<[PureVirtualCallChecker]>,`

================
Comment at: clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:210
 void ento::registerVirtualCallChecker(CheckerManager &mgr) {
   VirtualCallChecker *checker = mgr.registerChecker<VirtualCallChecker>();
+  checker->IsPureOnly = false;
----------------
`auto *Checker = mgr.getChecker<VirtualCallChecker>();`

Repository:
  rC Clang

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

https://reviews.llvm.org/D64274