[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Sat Aug 10 16:09:11 PDT 2019

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

In D66042#1624478 <https://reviews.llvm.org/D66042#1624478>, @Charusso wrote:

> In D66042#1624475 <https://reviews.llvm.org/D66042#1624475>, @Szelethus wrote:
>
> > In D66042#1624469 <https://reviews.llvm.org/D66042#1624469>, @Charusso wrote:
> >
> > > In a long-term rewriting the Analyzer from scratch would be ideal. There is no problem with this patch, it will not cause any issues like that. May I would like to disable the apiModeling as well, with my patch it is one command. With your approach it would require to rewrite all of the existing apiModeling checkers after the core ones for no reason as we have better way: this patch.
> >
> >
> > Why would we need to rewrite apiModeling? Its hidden from users, so the issue of enabling/disabling them is not a big topic.
>
>
> May the analyzer thinks that my `error()` function is inside some Apple product, which we model in some Apple way, but let us say I am at Microsoft, and I do not want to fix any Apple based product, but at least I want to hide those diagnostics. See that problem in https://reviews.llvm.org/D63915?id=207135#inline-570462.

If we cause an issue on any supported platforms, we can't go through with the change.

>> Here is a problem with your patch: How would you go about silencing diagnostics for `osx.cocoa.RetainCount`? I suppose `-analyzer-silence-checker=osx.cocoa.RetainCount`. The problem however, that the checker tag associated with it refers to `osx.cocoa.RetainCountBase` under the hood, so you'll need to silence that instead. From that point on, any other checker that suffers from the same issue is also silenced, that was not the intent.
>> 
>> I genuinely mean to cause no unnecessary headaches for you, but adding this is a significant API change that we'll have to support going forward if it gets into a release. If this is a quick fix to reduce the false positives on LLVM, I'm all for it, but would prefer to see a solution without such a commitment.
> 
> If I am at Microsoft, I would say `-disable-checker osx` to prevent issues like above. Of course it would be better if we do not have to suppress 7.000 reports running the Analyzer on LLVM using just the ReturnVisitor's suppressing. So on a long-term rewrite from scratch would be ideal. For now it is a cool patch without any overhead.
> 
> You have specified those checker dependencies very well, so if someone want to silence something then there is a cool place to starts with. Other than that the user's experimental features will remain experimental as it is still better than disabling the core and rush for Bugzilla how bad is the Analyzer. The goal here to have a way to hide meaningless reports which could produced by the core checkers as well. If we see some errors we rewrite it later, that means being experimental, that is how the usual developing process goes, that is why there is already a green mark: we are good to go.

I've just highlighted an issue where your patch doesn't work correctly. I'm still not convinced that fixing these would be a smaller headache then making a coreModeling package (keep in mind that for each new subchecker, this will always be a potential issue that is likely going slip through sometimes!). The example with `RetainCount` is not an isolated case, it was merely an example. You still didn't answer my question about potentially making this a config, which could still be promoted to a regular frontend flag eventually (that would also fit incremental development better). For an API change this significant, we might also want to invite other folks from the community to chip in. Your patch is called "[analyzer] Analysis: 'Disable' core checkers", yet this is a brand new functionality that affects all, not only core checkers. I agree with your concept, I understand the problem you're solving but these are legitimize concerns. Please don't rush this in before addressing these matters.

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

https://reviews.llvm.org/D66042