[PATCH] D64274: [analyzer] VirtualCallChecker overhaul.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

In D64274#1623644 <https://reviews.llvm.org/D64274#1623644>, @NoQ wrote:

> In D64274#1600834 <https://reviews.llvm.org/D64274#1600834>, @Szelethus wrote:
>
> > In D64274#1598226 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.


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

https://reviews.llvm.org/D64274