Page MenuHomePhabricator

[analyzer] VirtualCallChecker overhaul.
ClosedPublic

Authored by NoQ on Jul 5 2019, 7:39 PM.

Details

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)

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Jul 5 2019, 7:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2019, 7:39 PM
Szelethus requested changes to this revision.Jul 6 2019, 11:15 AM

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
562 ↗(On Diff #208248)

Dependencies<[PureVirtualCallChecker]>,

clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
210 ↗(On Diff #208248)

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

This revision now requires changes to proceed.Jul 6 2019, 11:15 AM
NoQ added a comment.Jul 8 2019, 11:55 AM

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 added a comment.Jul 8 2019, 12:18 PM

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 updated this revision to Diff 208473.Jul 8 2019, 12:22 PM

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

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 updated this revision to Diff 208807.Jul 9 2019, 2:10 PM

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.

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 accepted this revision.Jul 11 2019, 8:43 PM

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

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

Szelethus added a comment.EditedJul 15 2019, 1:05 PM
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?

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
562 ↗(On Diff #208248)

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

562 ↗(On Diff #208248)

Forgot this inline here, woops.

569 ↗(On Diff #208807)

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>

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 added a comment.Jul 16 2019, 4:14 PM

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 marked an inline comment as done.Jul 23 2019, 5:07 PM

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 ↗(On Diff #208807)

Yup!

Szelethus requested changes to this revision.Jul 25 2019, 4:50 AM

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:

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?

This revision now requires changes to proceed.Jul 25 2019, 4:50 AM
NoQ added a comment.Aug 9 2019, 1:35 PM

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

NoQ requested review of this revision.Aug 9 2019, 1:35 PM
Szelethus accepted this revision.Aug 12 2019, 6:29 AM
In D64274#1623644, @NoQ 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

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

This revision is now accepted and ready to land.Aug 12 2019, 6:29 AM
NoQ updated this revision to Diff 215000.Aug 13 2019, 6:13 PM

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 updated this revision to Diff 215007.Aug 13 2019, 6:56 PM

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 accepted this revision.Aug 18 2019, 3:35 PM

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 ↗(On Diff #215007)

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 ↗(On Diff #215007)

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 marked an inline comment as done.Aug 20 2019, 1:30 PM
NoQ added inline comments.
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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2019, 2:40 PM