[analyzer] VirtualCallChecker overhaul.

[NoQ]

Summary

This is slightly controversial, please hold me back / bikeshed if you don't like it.

The optin.cplusplus.VirtualCall checker in its current shape was implemented by @wangxindsb during his GSoC'2017 project - see D34275. It is a path-sensitive checker that finds calls to virtual functions during construction and destruction. This is a problem because virtual dispatch won't occur during construction or destruction. In particular, if the function is pure virtual, it is outright undefined behavior; in other cases it is simply an unexpected behavior. The checker has an option PureOnly to control whether the user is only interested in calls to pure virtual functions. It defaults to false, i.e. warn on non-pure-virtual calls as well.

I propose the following changes:

• Remove the option.
• Instead, split the checker in two:
  • cplusplus.PureVirtualCall will only check pure virtual calls and it will be on by default.
  • optin.cplusplus.VirtualCall will check all calls and will remain opt-in under the old name. It would automatically load cplusplus.PureVirtualCall.
• Additionally, remove the bug visitor. The reasons for that have been described in D34275?id=111862#inline-321253: essentially, the visitor doesn't add any new interesting information to the report. It's also currently broken: it places its note in a really weird spot.

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 :)

See also http://lists.llvm.org/pipermail/cfe-dev/2018-December/060558.html (the thread was bumped recently)

[Szelethus]

I happen to have very recent analyses on a couple projects, I'll throw this in: LLVM+Clang+Clang-tools-extra. 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
       \                    /
         ------------------>

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

572

Dependencies<[PureVirtualCallChecker]>,

clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

205–213

auto *Checker = mgr.getChecker<VirtualCallChecker>();

[NoQ]

Hmm, wait, i don't really break backwards compatibility. Fridays...

Previously we have impure-checking when we enable the optin checker and pureonly-checking when we disable the option.

I can easily bring back the option, only for the purposes of backwards compatibility, so that it was turning off impure-checking.

In this case we'll still have impure-checking when we enable the optin checker and pureonly-checking when we disable the option. The only difference is that pureonly-checking is now going to be on by default.

[NoQ]

Mmm, no, not really; it seems that if i introduce a checker dependency, i also have to put the option onto the base checker, otherwise the checker name wouldn't match when i do getCheckerBooleanOption(getChecker<VirtualCallChecker>(), "PureOnly"). Which means that the option name will inevitably change. @Szelethus, do i understand this correctly?

[NoQ]

Fix the crash on enabling both checkers, add a test.

[Szelethus]

In D64274#1574118, @NoQ wrote:

Mmm, no, not really; it seems that if i introduce a checker dependency, i also have to put the option onto the base checker, otherwise the checker name wouldn't match when i do getCheckerBooleanOption(getChecker<VirtualCallChecker>(), "PureOnly"). Which means that the option name will inevitably change. @Szelethus, do i understand this correctly?

I don't think it would change, the only "problem" would be that the *checker object's* name would be cplusplus.PureVirtualCall. You can still however invoke getChecker*Option by passing the optin.cplusplus.VirtualCall name as the first argument, which you can retrieve through CheckerManager::getCurrentCheckName (?).

In D64274#1574086, @NoQ wrote:

Hmm, wait, i don't really break backwards compatibility. Fridays...

Previously we have impure-checking when we enable the optin checker and pureonly-checking when we disable the option.

I can easily bring back the option, only for the purposes of backwards compatibility, so that it was turning off impure-checking.

In this case we'll still have impure-checking when we enable the optin checker and pureonly-checking when we disable the option. The only difference is that pureonly-checking is now going to be on by default.

I think we could just remove the option altogether. I'll take a second look on CodeCheckers side, but im reasonably sure we dont use it.

[NoQ]

Bring back the option in order to preserve backwards compatibility.

Sneak in an implicit conversion from CheckName to StringRef so that not to repeat myself.

[Szelethus]

Please know that I'm currently out of town, so it'll be a while before I can formally accept. Its on top of my list when I get home though! :^)

[Charusso]

The impure-warning sounds like some alpha, not-well-defined warning, other than that I like the movement to rethink existing checkers.

[baloghadamsoftware]

Hmm, I still fail to understand the problem with the current VirtualCall checker. Is it unstable? Does it report many false positives?

[Szelethus]

In D64274#1574086, @NoQ wrote:

Hmm, wait, i don't really break backwards compatibility. Fridays...

Ackchyually, it doesn't per se break anything, but will result in CodeChecker no longer enabling optin.cplusplus.VirtualCall :^) Sorry, oversight on my end. Observe the following monster of a clang invocation by

CodeChecker check -b "g++ -c clang/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp" --verbose debug_analyzer

(I put arrows at the important lines, this is with your patch applied)

clang --analyze -Qunused-arguments --analyzer-no-default-checks
  -Xclang analyzer-opt-analyze-headers
  -Xclang analyzer-output=plist-multi-file -o /tmp/tmpxmqqTL/cxx-uninitialized-object-ptr-ref.cpp_e4f3d9e72b29ea0cba420c8888e947ee.plist
  -Xclang analyzer-config -Xclang expand-macros=true
  -Xclang analyzer-checker=core.CallAndMessage
  -Xclang analyzer-checker=core.DivideZero
  -Xclang analyzer-checker=core.DynamicTypePropagation
  -Xclang analyzer-checker=core.NonNullParamChecker
  -Xclang analyzer-checker=core.NullDereference
  -Xclang analyzer-checker=core.StackAddressEscape
  -Xclang analyzer-checker=core.UndefinedBinaryOperatorResult
  -Xclang analyzer-checker=core.VLASize
  -Xclang analyzer-checker=core.uninitialized.ArraySubscript
  -Xclang analyzer-checker=core.uninitialized.Assign
  -Xclang analyzer-checker=core.uninitialized.Branch
  -Xclang analyzer-checker=core.uninitialized.CapturedBlockVariable
  -Xclang analyzer-checker=core.uninitialized.UndefReturn
  -Xclang analyzer-checker=cplusplus.InnerPointer
  -Xclang analyzer-checker=cplusplus.Move
  -Xclang analyzer-checker=cplusplus.NewDelete
  -Xclang analyzer-checker=cplusplus.NewDeleteLeaks
  -Xclang analyzer-disable-checker=cplusplus.PureVirtualCall <------------------------------------------ problem right here
  -Xclang analyzer-checker=deadcode.DeadStores
  -Xclang analyzer-checker=nullability.NullPassedToNonnull
  -Xclang analyzer-checker=nullability.NullReturnedFromNonnull
  -Xclang analyzer-disable-checker=nullability.NullableDereferenced
  -Xclang analyzer-disable-checker=nullability.NullablePassedToNonnull
  -Xclang analyzer-disable-checker=nullability.NullableReturnedFromNonnull
  -Xclang analyzer-disable-checker=optin.cplusplus.UninitializedObject
  -Xclang analyzer-checker=optin.cplusplus.VirtualCall <------------------------------------------ problem right here
  -Xclang analyzer-disable-checker=optin.mpi.MPI-Checker
  -Xclang analyzer-disable-checker=optin.osx.OSObjectCStyleCast
  -Xclang analyzer-disable-checker=optin.osx.cocoa.localizability.EmptyLocalizationContextChecker
  -Xclang analyzer-disable-checker=optin.osx.cocoa.localizability.NonLocalizedStringChecker
  -Xclang analyzer-disable-checker=optin.performance.GCDAntipattern
  -Xclang analyzer-disable-checker=optin.performance.Padding
  -Xclang analyzer-checker=optin.portability.UnixAPI
  -Xclang analyzer-disable-checker=osx.API
  -Xclang analyzer-disable-checker=osx.MIG
  -Xclang analyzer-disable-checker=osx.NumberObjectConversion
  -Xclang analyzer-disable-checker=osx.OSObjectRetainCount
  -Xclang analyzer-disable-checker=osx.ObjCProperty
  -Xclang analyzer-disable-checker=osx.SecKeychainAPI
  -Xclang analyzer-disable-checker=osx.cocoa.AtSync
  -Xclang analyzer-disable-checker=osx.cocoa.AutoreleaseWrite
  -Xclang analyzer-disable-checker=osx.cocoa.ClassRelease
  -Xclang analyzer-disable-checker=osx.cocoa.Dealloc
  -Xclang analyzer-disable-checker=osx.cocoa.IncompatibleMethodTypes
  -Xclang analyzer-disable-checker=osx.cocoa.Loops
  -Xclang analyzer-disable-checker=osx.cocoa.MissingSuperCall
  -Xclang analyzer-disable-checker=osx.cocoa.NSAutoreleasePool
  -Xclang analyzer-disable-checker=osx.cocoa.NSError
  -Xclang analyzer-disable-checker=osx.cocoa.NilArg
  -Xclang analyzer-disable-checker=osx.cocoa.NonNilReturnValue
  -Xclang analyzer-disable-checker=osx.cocoa.ObjCGenerics
  -Xclang analyzer-disable-checker=osx.cocoa.RetainCount
  -Xclang analyzer-disable-checker=osx.cocoa.RunLoopAutoreleaseLeak
  -Xclang analyzer-disable-checker=osx.cocoa.SelfInit
  -Xclang analyzer-disable-checker=osx.cocoa.SuperDealloc
  -Xclang analyzer-disable-checker=osx.cocoa.UnusedIvars
  -Xclang analyzer-disable-checker=osx.cocoa.VariadicMethodTypes
  -Xclang analyzer-disable-checker=osx.coreFoundation.CFError
  -Xclang analyzer-disable-checker=osx.coreFoundation.CFNumber
  -Xclang analyzer-disable-checker=osx.coreFoundation.CFRetainRelease
  -Xclang analyzer-disable-checker=osx.coreFoundation.containers.OutOfBounds
  -Xclang analyzer-disable-checker=osx.coreFoundation.containers.PointerSizedValues
  -Xclang analyzer-checker=security.FloatLoopCounter
  -Xclang analyzer-disable-checker=security.insecureAPI.DeprecatedOrUnsafeBufferHandling
  -Xclang analyzer-checker=security.insecureAPI.UncheckedReturn
  -Xclang analyzer-disable-checker=security.insecureAPI.bcmp
  -Xclang analyzer-disable-checker=security.insecureAPI.bcopy
  -Xclang analyzer-disable-checker=security.insecureAPI.bzero
  -Xclang analyzer-checker=security.insecureAPI.getpw
  -Xclang analyzer-checker=security.insecureAPI.gets
  -Xclang analyzer-checker=security.insecureAPI.mkstemp
  -Xclang analyzer-checker=security.insecureAPI.mktemp
  -Xclang analyzer-checker=security.insecureAPI.rand
  -Xclang analyzer-disable-checker=security.insecureAPI.strcpy
  -Xclang analyzer-checker=security.insecureAPI.vfork
  -Xclang analyzer-checker=unix.API
  -Xclang analyzer-checker=unix.Malloc
  -Xclang analyzer-checker=unix.MallocSizeof
  -Xclang analyzer-checker=unix.MismatchedDeallocator
  -Xclang analyzer-checker=unix.Vfork
  -Xclang analyzer-checker=unix.cstring.BadSizeArg
  -Xclang analyzer-checker=unix.cstring.NullArg
  -Xclang analyzer-checker=valist.CopyToSelf
  -Xclang analyzer-checker=valist.Uninitialized
  -Xclang analyzer-checker=valist.Unterminated
  -Xclang analyzer-config -Xclang aggressive-binary-operation-simplification=true
  -Xclang analyzer-config -Xclang crosscheck-with-z3=true
  -x c++ --target=x86_64-linux-gnu -std=gnu++14 -nobuiltininc
  -isystem /home/szelethus/Documents/llvm-project/build/lib/clang/9.0.0/include
  -isystem /usr/include/c++/7
  -isystem /usr/include/x86_64-linux-gnu/c++/7
  -isystem /usr/include/c++/7/backward
  -isystem /usr/local/include
  -isystem /usr/include/x86_64-linux-gnu
  -isystem /usr/include
  /home/szelethus/Documents/llvm-project/clang/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp

echo "-Xclang -analyzer-list-enabled-checkers" > saargs.txt
CodeChecker check -b "g++ -c clang/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp" --saargs saargs.txt --verbose debug_analyzer`

OVERVIEW: Clang Static Analyzer Enabled Checkers List

core.CallAndMessage
core.DivideZero
core.DynamicTypePropagation
core.NonNullParamChecker
core.NullDereference
core.StackAddrEscapeBase
core.StackAddressEscape
core.UndefinedBinaryOperatorResult
core.VLASize
core.uninitialized.ArraySubscript
core.uninitialized.Assign
core.uninitialized.Branch
core.uninitialized.CapturedBlockVariable
core.uninitialized.UndefReturn
unix.cstring.CStringModeling
unix.DynamicMemoryModeling
cplusplus.InnerPointer
cplusplus.Move
cplusplus.NewDelete
cplusplus.NewDeleteLeaks
deadcode.DeadStores
nullability.NullabilityBase
nullability.NullPassedToNonnull
nullability.NullReturnedFromNonnull
optin.portability.UnixAPI
security.insecureAPI.SecuritySyntaxChecker
security.FloatLoopCounter
security.insecureAPI.UncheckedReturn
security.insecureAPI.getpw
security.insecureAPI.gets
security.insecureAPI.mkstemp
security.insecureAPI.mktemp
security.insecureAPI.rand
security.insecureAPI.vfork
unix.API
unix.Malloc
unix.MallocSizeof
unix.MismatchedDeallocator
unix.Vfork
unix.cstring.BadSizeArg
unix.cstring.NullArg
valist.ValistBase
valist.CopyToSelf
valist.Uninitialized
valist.Unterminated

This stems from CodeChecker explicitly disabling everyrthing that isn't explicitly enabled. I vaguely remember @o.gyorgy mentioning that we do this for some legacy reasons, but plan to move away from it. Would it be trouble for you to reverse the dependency please?

In D64274#1584974, @baloghadamsoftware wrote:

Hmm, I still fail to understand the problem with the current VirtualCall checker. Is it unstable? Does it report many false positives?

Yup. While painfully artificial, take a look at this little code snippet:

struct Base {
  Base() {
    log();
  }

  virtual void log();
};

struct Derived : public Base {};

void f() { Derived d; }

In this example, no error is made, Derived doesn't override log(), there is no chance for a misuse, yet the checker warns. Now, of course, its only a matter of time until another derived class actually implements log() and we'd potentially run into a problem, but similarly to UninitializedObjectChecker, we detect code smell, not error. Calls to pure virtual functions is UB.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

569

Lets hide it as well.

CmdLineOption<Boolean,
              "PureOnly",
              "Disables the checker. Keeps cplusplus.PureVirtualCall "
              "enabled. This option is only provided for backwards "
              "compatibility.",
              "false",
              InAlpha,
              Hide>

572

Ackchyually, we should have this the other way around for backward compatibility reasons :^) Sorry, oversight on my end.

572

Forgot this inline here, woops.

[Szelethus]

Also, shouldn't we add this to the release notes? In general, it's be around time to sort it out (might do that myself before the new branch).

[NoQ]

In D64274#1584974, @baloghadamsoftware wrote:

Hmm, I still fail to understand the problem with the current VirtualCall checker. Is it unstable? Does it report many false positives?

Yeah, pretty much. It's basically defined to find non-bugs and so far i've seen no indication that a lot of them are actually bugs, but it's rather the opposite, and it's rather noisy. It defines a good practice to follow ("if you truly want to call a virtual function and you understand that no virtual dispatch will happen, add an explicit qualifier"), but i feel uncomfy to force this recommendation upon people by default. That's still a good check but that's not a kind of thing that people ask for when they're using the analyzer. Btw, this check could probably benefit from a fixit hint (which adds the missing qualifier).

When the function is pure virtual, it's an immediate UB, so it's something we can always warn about.

[NoQ]

In D64274#1586282, @Szelethus wrote:

Ackchyually, it doesn't per se break anything, but will result in CodeChecker no longer enabling optin.cplusplus.VirtualCall :^) Sorry, oversight on my end. Observe the following monster of a clang invocation...

Mmm. Aha. Ok, i can reproduce your problem, but i don't think reversing the dependencies is gonna work. If we make the pure checker depend on the optin checker, we would always have the opt-in checker registered first, and therefore there's no way to figure out if we really wanted to opt in into the optin checker or we are simply loading it as a dependency. Which, in particular, makes it impossible to discriminate between -analyzer-checker cplusplus.PureVirtualCall and -analyzer-checker optin.cplusplus.VirtualCall when the option is unset: in both cases we'll first register the opt-in checker and then the pure checker.

I'm confused though; the way i was previously understanding your work, when disabling a dependency and then enabling a dependent checker *later* in the run-line, it should re-enable the dependency automatically. Did you consciously decide not to implement it that way?

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

569

Yup!

[Szelethus]

If either checker emits an error, the current implementation would say it originates from cplusplus.PureVirtualCall. Could you please create a new ProgramPointTag with the correct checker name for optin.cplusplus.VirtualCall? You may retrieve that name through CheckerManager::getCurrentCheckName().

In D64274#1598226, @NoQ wrote:

In D64274#1586282, @Szelethus wrote:

Ackchyually, it doesn't per se break anything, but will result in CodeChecker no longer enabling optin.cplusplus.VirtualCall :^) Sorry, oversight on my end. Observe the following monster of a clang invocation...

Mmm. Aha. Ok, i can reproduce your problem, but i don't think reversing the dependencies is gonna work. If we make the pure checker depend on the optin checker, we would always have the opt-in checker registered first, and therefore there's no way to figure out if we really wanted to opt in into the optin checker or we are simply loading it as a dependency. Which, in particular, makes it impossible to discriminate between -analyzer-checker cplusplus.PureVirtualCall and -analyzer-checker optin.cplusplus.VirtualCall when the option is unset: in both cases we'll first register the opt-in checker and then the pure checker.

I'm sold. Let's drop this issue and we'll make sense of the CodeChecker runline by the time Clang 10.0.0 drops.

I'm confused though; the way i was previously understanding your work, when disabling a dependency and then enabling a dependent checker *later* in the run-line, it should re-enable the dependency automatically. Did you consciously decide not to implement it that way?

Yes, somewhat. I chose another direction, which is simply not exposing base checkers (which I detailed in D60925) to the users (-analyzer-checker-help-developer is a thing now), so they aren't ever enabled if none of their dependent checkers are. This worked wonders, because, interestingly, none of the base checkers were emitting diagnostics.

I find this clearer, if I disable something because it crashes on my code, I don't want to see it again. Though, 9.0.0-final isn't out, and I'm sorry that I didn't make this decision clear enough -- shall I fix it?

[NoQ]

In D64274#1600834, @Szelethus wrote:

If either checker emits an error, the current implementation would say it originates from cplusplus.PureVirtualCall. Could you please create a new ProgramPointTag with the correct checker name for optin.cplusplus.VirtualCall? You may retrieve that name through CheckerManager::getCurrentCheckName().

Whoops right! I addressed this in D65180 because it's annoying to rebase the fix through the broken IsSink flag in the reportBug() function.

In D64274#1600834, @Szelethus wrote:

In D64274#1598226, @NoQ wrote:

I'm confused though; the way i was previously understanding your work, when disabling a dependency and then enabling a dependent checker *later* in the run-line, it should re-enable the dependency automatically. Did you consciously decide not to implement it that way?

Yes, somewhat. I chose another direction, which is simply not exposing base checkers (which I detailed in D60925) to the users (-analyzer-checker-help-developer is a thing now), so they aren't ever enabled if none of their dependent checkers are. This worked wonders, because, interestingly, none of the base checkers were emitting diagnostics.

I find this clearer, if I disable something because it crashes on my code, I don't want to see it again. Though, 9.0.0-final isn't out, and I'm sorry that I didn't make this decision clear enough -- shall I fix it?

Yes, generally i think it's good to have later options override earlier options, because it allows people who run analysis through multiple layers of scripts (eg., buildbots) to override their previous decisions without digging through the whole pile of scripts. This isn't *that* important because the use case is pretty rare, but if you have any immediate ideas on how to fix this i'd be pretty happy :)

[Szelethus]

In D64274#1623644, @NoQ wrote:

In D64274#1600834, @Szelethus wrote:

In D64274#1598226, @NoQ wrote:

I'm confused though; the way i was previously understanding your work, when disabling a dependency and then enabling a dependent checker *later* in the run-line, it should re-enable the dependency automatically. Did you consciously decide not to implement it that way?

Yes, somewhat. I chose another direction, which is simply not exposing base checkers (which I detailed in D60925) to the users (-analyzer-checker-help-developer is a thing now), so they aren't ever enabled if none of their dependent checkers are. This worked wonders, because, interestingly, none of the base checkers were emitting diagnostics.

I find this clearer, if I disable something because it crashes on my code, I don't want to see it again. Though, 9.0.0-final isn't out, and I'm sorry that I didn't make this decision clear enough -- shall I fix it?

Yes, generally i think it's good to have later options override earlier options, because it allows people who run analysis through multiple layers of scripts (eg., buildbots) to override their previous decisions without digging through the whole pile of scripts. This isn't *that* important because the use case is pretty rare, but if you have any immediate ideas on how to fix this i'd be pretty happy :)

The immediate idea is to never allow checkers that emit diagnostics to be dependencies. That way, we'll let the analyzer handle enabling/disabling them under the hood, and the checker dependency structure would be far more accurate, since no checker depends on diagnostics. What do you think? This seems to a hot topic anyways now, and would solve the issue behind core checkers as mentioned in D66042. I think it wouldn't be too much work, especially that I'm very familiar with this part of the codebase. That said, the solution might be lengthier than what folks would be comfortable merging so late into the release cycle.

My entire checker dependency project was about getting rid of bugs to eventually list analyzer/checker options, but I think the idea of separating modeling/diagnostic parts should be a project-wide rule. I feel bad for dragging you through this, as I previously said

In D64274#1572407, @Szelethus wrote:

[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. ]

but now I believe having a common base would have been the ideal solution. Please feel free to commit as is, I'll pick this up as I enforce the rule.

Maybe its also time to escalate this issue to cfe-dev.

[NoQ]

I'll merge D65180 into this patch because it's otherwise too painful to update. Like, D65180 untangles bug types and controls enabledness by initializing bug types, but in order to untangle the checkers into a pair with a common dependency, i already need such control, and i don't want to implement a temporary solution only to discard it immediately.

This update only merges the two patches; for ease of review it doesn't address any comments.

[NoQ]

Make the checkers independent and extract modeling into a dependency.

This means that you can potentially enable the non-pure-virtual call checker without enabling the pure virtual call checker. This doesn't contradict backwards compatibility as long as the driver's default checker list is trusted, because the pure virtual call checker is enabled by default anyway and nobody was ever disabling it manually.

[Szelethus]

Thank you, and sorry for dragging you through this! At the very least, we got to learn a lot from it :)

clang/include/clang/StaticAnalyzer/Core/CheckerManager.h

93–96

Yea, so this comment isn't incorrect, but it isn't obvious the reason behind this: CheckerRegistry gets destructed almost immediately after loading the checkers to CheckerManager, the thing we should emphasize here is that those checker names are actually generated as string literals with the inclusion of Checkers.inc, which is why their lifetime is long enough (practically infinite).

97

Grrrr, this has been bugging me for far too long. We should totally rename this to CheckerName eventually.

clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp

14–20 ↗

(On Diff #215007)

static constexpr StringLiteral? Why is there a need to have a header and a cpp file for this? Not that it matters much (definitely not in the context of this patch), but compile-time stuff is cool.

[NoQ]

clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp

14–20 ↗

(On Diff #215007)

Ancient stuff, i guess. We only have C++11 for like 3 years i think.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

CmdLineOption<String,
                   "user has intentionally asked us to completely eliminate "
                   "use-after-move in his code. Values: \"KnownsOnly\", "
                   "\"KnownsAndLocals\", \"All\".",
                   "KnownsAndLocals",
                   Released>
   ]>,
   Documentation<HasDocumentation>;
 
+def PureVirtualCallChecker : Checker<"PureVirtualCall">,
+  HelpText<"Check pure virtual function calls during construction/destruction">,
+  Documentation<HasDocumentation>;
 } // end: "cplusplus"
 
 let ParentPackage = CplusplusOptIn in {
 
 def UninitializedObjectChecker: Checker<"UninitializedObject">,
   HelpText<"Reports uninitialized fields after object construction">,
   CheckerOptions<[
     CmdLineOption<Boolean,
CmdLineOption<Boolean,
                   "whether the found uninitialized object is used without a "
                   "preceding assert call. Defaults to false.",
                   "false",
                   InAlpha>
   ]>,
   Documentation<HasAlphaDocumentation>;
 
 def VirtualCallChecker : Checker<"VirtualCall">,
-  HelpText<"Check virtual function calls during construction or destruction">,
+  HelpText<"Check virtual function calls during construction/destruction">,
   CheckerOptions<[
     CmdLineOption<Boolean,
                   "PureOnly",
-                  "Whether to only report calls to pure virtual methods.",
+                  "Disables the checker. Keeps cplusplus.PureVirtualCall "
+                  "enabled. This option is only provided for backwards "
+                  "compatibility.",
                   "false",
-                  Released>
+                  InAlpha>
   ]>,
+  Dependencies<[PureVirtualCallChecker]>,
   Documentation<HasDocumentation>;
 
 } // end: "optin.cplusplus"
 
 let ParentPackage = CplusplusAlpha in {
 
 def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">,
   HelpText<"Reports destructions of polymorphic objects with a non-virtual "
            "destructor in their base class">,

clang/include/clang/StaticAnalyzer/Core/CheckerManager.h

enum PointerEscapeKind {
   /// argument to a function.
   PSK_IndirectEscapeOnCall,
 
   /// The reason for pointer escape is unknown. For example,
   /// a region containing this pointer is invalidated.
   PSK_EscapeOther
 };
 
 // This wrapper is used to ensure that only StringRefs originating from the
 // CheckerRegistry are used as check names. We want to make sure all check
 // name strings have a lifetime that keeps them alive at least until the path
 // diagnostics have been processed.
 class CheckName {
   friend class ::clang::ento::CheckerRegistry;
 
   StringRef Name;
 
   explicit CheckName(StringRef Name) : Name(Name) {}
 
 public:
   CheckName() = default;
 
   StringRef getName() const { return Name; }
+  operator StringRef() const { return Name; }
 };
 
 enum class ObjCMessageVisitKind {
   Pre,
   Post,
   MessageNil
 };
 

clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

 
 namespace {
 class VirtualCallChecker
     : public Checker<check::BeginFunction, check::EndFunction, check::PreCall> {
   mutable std::unique_ptr<BugType> BT;
 
 public:
   // The flag to determine if pure virtual functions should be issued only.
-  DefaultBool IsPureOnly;
+  bool IsPureOnly = true;
 
   void checkBeginFunction(CheckerContext &C) const;
   void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
 
 private:
   void registerCtorDtorCallInState(bool IsBeginFunction,
                                    CheckerContext &C) const;
   void reportBug(StringRef Msg, bool PureError, const MemRegion *Reg,
                  CheckerContext &C) const;
-
-  class VirtualBugVisitor : public BugReporterVisitor {
-  private:
-    const MemRegion *ObjectRegion;
-    bool Found;
-
-  public:
-    VirtualBugVisitor(const MemRegion *R) : ObjectRegion(R), Found(false) {}
-
-    void Profile(llvm::FoldingSetNodeID &ID) const override {
-      static int X = 0;
-      ID.AddPointer(&X);
-      ID.AddPointer(ObjectRegion);
-    }
-
-    std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
-                                                   BugReporterContext &BRC,
-                                                   BugReport &BR) override;
-  };
 };
 } // end namespace
 
 // GDM (generic data map) to the memregion of this for the ctor and dtor.
 REGISTER_MAP_WITH_PROGRAMSTATE(CtorDtorMap, const MemRegion *, ObjectState)
 
-std::shared_ptr<PathDiagnosticPiece>
-VirtualCallChecker::VirtualBugVisitor::VisitNode(const ExplodedNode *N,
-                                                 BugReporterContext &BRC,
-                                                 BugReport &) {
-  // We need the last ctor/dtor which call the virtual function.
-  // The visitor walks the ExplodedGraph backwards.
-  if (Found)
-    return nullptr;
-
-  ProgramStateRef State = N->getState();
-  const LocationContext *LCtx = N->getLocationContext();
-  const CXXConstructorDecl *CD =
-      dyn_cast_or_null<CXXConstructorDecl>(LCtx->getDecl());
-  const CXXDestructorDecl *DD =
-      dyn_cast_or_null<CXXDestructorDecl>(LCtx->getDecl());
-
-  if (!CD && !DD)
-    return nullptr;
-
-  ProgramStateManager &PSM = State->getStateManager();
-  auto &SVB = PSM.getSValBuilder();
-  const auto *MD = dyn_cast<CXXMethodDecl>(LCtx->getDecl());
-  if (!MD)
-    return nullptr;
-  auto ThiSVal =
-      State->getSVal(SVB.getCXXThis(MD, LCtx->getStackFrame()));
-  const MemRegion *Reg = ThiSVal.castAs<loc::MemRegionVal>().getRegion();
-  if (!Reg)
-    return nullptr;
-  if (Reg != ObjectRegion)
-    return nullptr;
-
-  const Stmt *S = PathDiagnosticLocation::getStmt(N);
-  if (!S)
-    return nullptr;
-  Found = true;
-
-  std::string InfoText;
-  if (CD)
-    InfoText = "This constructor of an object of type '" +
-               CD->getNameAsString() +
-               "' has not returned when the virtual method was called";
-  else
-    InfoText = "This destructor of an object of type '" +
-               DD->getNameAsString() +
-               "' has not returned when the virtual method was called";
-
-  // Generate the extra diagnostic.
-  PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
-                             N->getLocationContext());
-  return std::make_shared<PathDiagnosticEventPiece>(Pos, InfoText, true);
-}
-
 // The function to check if a callexpr is a virtual function.
 static bool isVirtualCall(const CallExpr *CE) {
   bool CallIsNonVirtual = false;
 
   if (const MemberExpr *CME = dyn_cast<MemberExpr>(CE->getCallee())) {
     // The member access is fully qualified (i.e., X::F).
     // Treat this as a non-virtual call and do not warn.
     if (CME->getQualifier())
void VirtualCallChecker::reportBug(StringRef Msg, bool IsSink,
   if (!N)
     return;
   if (!BT)
     BT.reset(new BugType(
         this, "Call to virtual function during construction or destruction",
         "C++ Object Lifecycle"));
 
   auto Reporter = llvm::make_unique<BugReport>(*BT, Msg, N);
-  Reporter->addVisitor(llvm::make_unique<VirtualBugVisitor>(Reg));
   C.emitReport(std::move(Reporter));
 }
 
-void ento::registerVirtualCallChecker(CheckerManager &mgr) {
-  VirtualCallChecker *checker = mgr.registerChecker<VirtualCallChecker>();
+void ento::registerPureVirtualCallChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<VirtualCallChecker>();
+}
+
+void ento::registerVirtualCallChecker(CheckerManager &Mgr) {
+  auto *Chk = Mgr.getChecker<VirtualCallChecker>();
+  Chk->IsPureOnly = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
+      Mgr.getCurrentCheckName(), "PureOnly");
+}
 
-  checker->IsPureOnly =
-      mgr.getAnalyzerOptions().getCheckerBooleanOption(checker, "PureOnly");
+bool ento::shouldRegisterPureVirtualCallChecker(const LangOptions &LO) {
+  return true;
 }
 
 bool ento::shouldRegisterVirtualCallChecker(const LangOptions &LO) {
   return true;
 }

clang/test/Analysis/virtualcall.h

 namespace header {
   class Z {
   public:
     Z() {
-      foo();
-#if !PUREONLY
-  // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
-  // expected-note-re@-3 {{{{^}}This constructor of an object of type 'Z' has not returned when the virtual method was called}}
-  // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}  
-#endif
+      foo(); // impure-warning {{Call to virtual function during construction}}
     }
     virtual int foo();
   };
 }

clang/test/Analysis/virtualcall.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-output=text -verify -std=c++11 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
+// RUN:                    -std=c++11 -verify=expected,impure %s
 
-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-config optin.cplusplus.VirtualCall:PureOnly=true -DPUREONLY=1 -analyzer-output=text -verify -std=c++11 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.PureVirtualCall \
+// RUN:                    -std=c++11 -verify=expected -std=c++11 %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
+// RUN:                    -analyzer-config \
+// RUN:                        optin.cplusplus.VirtualCall:PureOnly=true \
+// RUN:                    -std=c++11 -verify=expected %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.PureVirtualCall \
+// RUN:                    -analyzer-checker=optin.cplusplus.VirtualCall \
+// RUN:                    -std=c++11 -verify=expected,impure -std=c++11 %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.PureVirtualCall \
+// RUN:                    -analyzer-checker=optin.cplusplus.VirtualCall \
+// RUN:                    -analyzer-config \
+// RUN:                        optin.cplusplus.VirtualCall:PureOnly=true \
+// RUN:                    -std=c++11 -verify=expected %s
 
 #include "virtualcall.h"
 
 class A {
 public:
   A();
 
   ~A(){};
 
   virtual int foo() = 0;
   virtual void bar() = 0;
   void f() {
-    foo();
-  // expected-warning-re@-1 {{{{^}}Call to pure virtual function during construction}}
-  // expected-note-re@-2 {{{{^}}Call to pure virtual function during construction}}
+    foo(); // expected-warning{{Call to pure virtual function during construction}}
   }
 };
 
 class B : public A {
 public:
-  B() { // expected-note {{Calling default constructor for 'A'}}
-    foo(); 
-#if !PUREONLY
-    // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
-  // expected-note-re@-3 {{{{^}}This constructor of an object of type 'B' has not returned when the virtual method was called}}
-    // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}
-#endif
+  B() {
+    foo(); // impure-warning {{Call to virtual function during construction}}
   }
   ~B();
 
   virtual int foo();
   virtual void bar() {
-    foo(); 
-#if !PUREONLY
-    // expected-warning-re@-2 {{{{^}}Call to virtual function during destruction}}
-    // expected-note-re@-3 {{{{^}}Call to virtual function during destruction}}
-#endif
-  } 
+    foo(); // impure-warning{{Call to virtual function during destruction}}
+  }
 };
 
-A::A() { 
-  f(); 
-// expected-note-re@-1 {{{{^}}This constructor of an object of type 'A' has not returned when the virtual method was called}}
-// expected-note-re@-2 {{{{^}}Calling 'A::f'}}
+A::A() {
+  f();
 }
 
 B::~B() {
   this->B::foo(); // no-warning
   this->B::bar();
-#if !PUREONLY
-   // expected-note-re@-2 {{{{^}}This destructor of an object of type '~B' has not returned when the virtual method was called}}
-   // expected-note-re@-3 {{{{^}}Calling 'B::bar'}}
-#endif
-  this->foo(); 
-#if !PUREONLY
-   // expected-warning-re@-2 {{{{^}}Call to virtual function during destruction}}
-   // expected-note-re@-3 {{{{^}}This destructor of an object of type '~B' has not returned when the virtual method was called}}
-   // expected-note-re@-4 {{{{^}}Call to virtual function during destruction}}
-#endif
-  
+  this->foo(); // impure-warning {{Call to virtual function during destruction}}
 }
 
 class C : public B {
 public:
   C();
   ~C();
 
   virtual int foo();
   void f(int i);
 };
 
 C::C() {
-  f(foo()); 
-#if !PUREONLY
-    // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
-  // expected-note-re@-3 {{{{^}}This constructor of an object of type 'C' has not returned when the virtual method was called}}
-    // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}
-#endif
+  f(foo()); // impure-warning {{Call to virtual function during construction}}
 }
 
 class D : public B {
 public:
   D() {
     foo(); // no-warning
   }
   ~D() { bar(); }
   int foo() final;
   void bar() final { foo(); } // no-warning
 };
 
 class E final : public B {
 public:
   E() {
     foo(); // no-warning
   }
   ~E() { bar(); }
-#if !PUREONLY
-   // expected-note-re@-2 2{{{{^}}Calling '~B'}}
-#endif
   int foo() override;
 };
 
 class F {
 public:
   F() {
     void (F::*ptr)() = &F::foo;
     (this->*ptr)();
void init() {
     if (initState)
       f(); // no warning
   }
 
   H(int i) {
     G g;
     g.foo();
     g.bar(); // no warning
-    f();     
-#if !PUREONLY
-    // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
-  // expected-note-re@-3 {{{{^}}This constructor of an object of type 'H' has not returned when the virtual method was called}}
-    // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}
-#endif
+    f(); // impure-warning {{Call to virtual function during construction}}
     H &h = *this;
-    h.f(); 
-#if !PUREONLY
-    // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
-    // expected-note-re@-3 {{{{^}}This constructor of an object of type 'H' has not returned when the virtual method was called}}
-    // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}
-#endif
+    h.f(); // impure-warning {{Call to virtual function during construction}}
   }
 };
 
 class X {
 public:
   X() {
-    g(); 
-#if !PUREONLY
-    // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
-  // expected-note-re@-3 {{{{^}}This constructor of an object of type 'X' has not returned when the virtual method was called}}
-    // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}
-#endif
+    g(); // impure-warning {{Call to virtual function during construction}}
   }
   X(int i) {
     if (i > 0) {
-#if !PUREONLY
-  // expected-note-re@-2 {{{{^}}'i' is > 0}}
-  // expected-note-re@-3 {{{{^}}Taking true branch}}
-  // expected-note-re@-4 {{{{^}}'i' is <= 0}}
-  // expected-note-re@-5 {{{{^}}Taking false branch}}
-#endif
       X x(i - 1);
-#if !PUREONLY
-  // expected-note-re@-2 {{{{^}}Calling constructor for 'X'}}
-#endif
       x.g(); // no warning
     }
-    g(); 
-#if !PUREONLY
-    // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
-  // expected-note-re@-3 {{{{^}}This constructor of an object of type 'X' has not returned when the virtual method was called}}
-    // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}
-#endif
+    g(); // impure-warning {{Call to virtual function during construction}}
   }
   virtual void g();
 };
 
 class M;
 class N {
 public:
   virtual void virtualMethod();
   void callFooOfM(M *);
 };
 class M {
 public:
   M() {
     N n;
     n.virtualMethod(); // no warning
     n.callFooOfM(this);
-#if !PUREONLY
-    // expected-note-re@-2 {{{{^}}This constructor of an object of type 'M' has not returned when the virtual method was called}}
-  // expected-note-re@-3 {{{{^}}Calling 'N::callFooOfM'}}
-#endif
   }
   virtual void foo();
 };
 void N::callFooOfM(M *m) {
-  m->foo(); 
-#if !PUREONLY
-    // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
-    // expected-note-re@-3 {{{{^}}Call to virtual function during construction}}
-#endif
+  m->foo(); // impure-warning {{Call to virtual function during construction}}
 }
 
 class Y {
 public:
   virtual void foobar();
   void fooY() {
     F f1;
-    foobar(); 
-#if !PUREONLY
-    // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
-    // expected-note-re@-3 {{{{^}}Call to virtual function during construction}}
-#endif
+    foobar(); // impure-warning {{Call to virtual function during construction}}
   }
   Y() { fooY(); }
-#if !PUREONLY
-    // expected-note-re@-2 {{{{^}}This constructor of an object of type 'Y' has not returned when the virtual method was called}}
-    // expected-note-re@-3 {{{{^}}Calling 'Y::fooY'}}
-#endif
 };
 
 int main() {
   B b;
-#if PUREONLY
-  //expected-note-re@-2 {{{{^}}Calling default constructor for 'B'}}
-#else 
-  //expected-note-re@-4 2{{{{^}}Calling default constructor for 'B'}}
-#endif
   C c;
-#if !PUREONLY
-  //expected-note-re@-2 {{{{^}}Calling default constructor for 'C'}}
-#endif
   D d;
   E e;
   F f;
   G g;
   H h;
   H h1(1);
-#if !PUREONLY
-  //expected-note-re@-2 {{{{^}}Calling constructor for 'H'}}
-  //expected-note-re@-3 {{{{^}}Calling constructor for 'H'}}
-#endif
   X x; 
-#if !PUREONLY
-  //expected-note-re@-2 {{{{^}}Calling default constructor for 'X'}}
-#endif
   X x1(1);
-#if !PUREONLY
-  //expected-note-re@-2 {{{{^}}Calling constructor for 'X'}}
-#endif
   M m;
-#if !PUREONLY
-  //expected-note-re@-2 {{{{^}}Calling default constructor for 'M'}}
-#endif
   Y *y = new Y;
-#if !PUREONLY
-  //expected-note-re@-2 {{{{^}}Calling default constructor for 'Y'}}
-#endif
   delete y;
   header::Z z;
-#if !PUREONLY
-  // expected-note-re@-2 {{{{^}}Calling default constructor for 'Z'}}
-#endif
-}
-#if !PUREONLY
-  //expected-note-re@-2 2{{{{^}}Calling '~E'}}
-#endif
+}
 
 namespace PR34451 {
 struct a {
   void b() {
     a c[1];
     c->b();
   }
 };