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

Sat Aug 10 15:09:22 PDT 2019

Szelethus added a comment.

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

> In D66042#1624468 <https://reviews.llvm.org/D66042#1624468>, @Szelethus wrote:
>
> > Speaking about performance impact, note where your patch does the actual silencing: by the time control reaches that point, we created bug report equivalence classes, constructed a trimmed version of the exploded graph, constructed a bug path from that trimmed graph and ran all visitors on and have gathered all diagnostic pieces for it (plenty of shared object creations there). I have a strong suspicion that not even creating the bug report is far faster.
> >
> > I think that adding yet another way of controlling checkers is confusing, and solves a problem that shouldn't exist in the first place. Now, that being said, the problem //does// exist, and I see this as an elegant solution for the time being.
>
>
> Let us say an average user using scan-build add that flag: `-disable-checker core.DivideZero`. I am 100% sure the user meant that to disable that checker's reports. Whatever it takes, the main modeling and main graph-building is what burns the companies money on electricity bills. We have tons of ways to suppress reports, because it makes zero overhead in cost.

Then I guess overhead isn't a big concern to us after all :)

>>>>> Also this patch aims to hide 600 `cast<>` related reports, and try to fix that ambiguity to "disable a core checker" as we really mean that to do not emit uninteresting reports. Personally I am really against the idea to make the core modeling disable-able, this scan-build addition fix that, you cannot disable it. If crash occurs with the core package then we should give it a 9000 priority level on Bugzilla and `fix-it`.
>>>> 
>>>> My wording may have been poor, apologies for the misunderstanding -- of course I do not intend to get rid of the modeling, just achieving the same goal from a different angle. `coreModeling` would be a dependency of all path sensitive checkers (we did talk about this in one of my patches), and the user would no longer be exposed to the option of disabling them, only their diagnostics. This is similar to the way how the checker dependency system was reimplemented as well, and I like to think that the analyzer's interface really benefited from it.
>>>> 
>>>> What do you think?
>>> 
>>> At the moment we have a way to disable core modeling because they could break the user's analysis. My patch only touch the scan-build's invocation as an **experimental** feature to make it impossible to disable the core modeling **as a side effect** and **only trough scan-build**. Mainly the idea was to use the fewest possible commands using scan-build therefore now one `-disable-checker` command does two things. I hope that it is useful for other users as well to "disable" the core checkers.
>>> 
>>> However, if you have time to continue your dependencies patch to make the core modeling non-disable-able with making the core checkers bulletproof, that would be neat! But that could be some other patch and out of the scope of that patch.
>> 
>> Most definitely! Would you agree that in the long term such a division would be a more ideal approach? Because if we're commiting ourselves to silencing checkers, there may be some annoying things to fix too, like, if a checker emits a warning with the incorrect name (this is unfortunately not too uncommon, for example when the checker tag is associated with the modeling checker), the user would be confused why the silence didn't go through, or silenced far too many things. If this is just a band aid solution only for scan-build, only for core checkers, wouldn't an analyzer-config be better?
> 
> 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.

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.

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

https://reviews.llvm.org/D66042