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

Mon Aug 12 04:46:50 PDT 2019

Szelethus added a comment.

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

> In D66042#1624684 <https://reviews.llvm.org/D66042#1624684>, @NoQ wrote:
>
> > My idea here was that this new feature isn't going to be user-facing. We aren't promising to support all combinations of enabled-disabled-silenced-dependent-registercheckerhacks, but instead use the new feature when we know it'll do exactly what we want. It is going to be up to the user-facing UI to decide how to use this feature, but not up to the end-users who simply want to silence diagnostics.
> >
> > > 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.
> >
> > Hmm, sounds like we need to hack up a fix for the checker tag on the bug node, so that it was appropriate to the presumed (as opposed to actual) checker name(?)
>
>
> `StringRef("osx.cocoa.RetainCountBase").startswith("osx.cocoa.RetainCount")` is true, so there is no real issue until we manage the prefixes well. I assume that the user who knows how to disable/silence a checker, knows where to read how to disable/silence it. At least with scan-build there will not be pitfalls with disabling the core modeling.

> scan-build
>  core modeling

This patch affects far more then either of those items. To me, it seems like we're trying to solve (a subset) of the checker dependency problem again with more hacking, and it didn't turn out too well last time.

In D66042#1624684 <https://reviews.llvm.org/D66042#1624684>, @NoQ wrote:

> My idea here was that this new feature isn't going to be user-facing. We aren't promising to support all combinations of enabled-disabled-silenced-dependent-registercheckerhacks, but instead use the new feature when we know it'll do exactly what we want. It is going to be up to the user-facing UI to decide how to use this feature, but not up to the end-users who simply want to silence diagnostics.

The frontend is our user interface, and until we develop a nicely thought out way to interact with the analyzer through the driver, it'll stay that way, unfortunately. The only way to use the analyzer is through the frontend (including `-Xclang`), and if we add this flag, people responsible for developing interafaces for the analyzer might end up using it. We don't promise anything, because we don't really submit release notes, so I don't see this as a good enough defense.

Please note how we restrained ourselves from adding checker plugins into the `examples` folder, we were so concerned with people discovering it. As you said in D57858#1498640 <https://reviews.llvm.org/D57858#1498640>, and I mentioned again in D62885#1530206 <https://reviews.llvm.org/D62885#1530206>, it is an unrealistic expectation that this will be hidden from view.

>> 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.
> 
> Hmm, sounds like we need to hack up a fix for the checker tag on the bug node, so that it was appropriate to the presumed (as opposed to actual) checker name(?)

I wouldn't call it hacking (we would need just a couple `CheckerProgramPointTag`s), but again, what if we forget about it? If we fix this (add tags for each checker that suffers from this issue) we're literally 10 lines of code away from a far better solution after which silencing checkers wouldn't be needed.

So, here are the things I'm worried about:

- Whenever we forget about adding a checker tag to a subchecker, this issue will arise again. We could test this by generating a testcase for each checker in which said checker is silenced, modify `-analyzer-list-enabled-checkers` so that it also displays whether the checker is silenced (and since some users might depend on this output, we might need an `-analyzer-list-silenced-checkers` flag that will add boilerplate code), and we check with whether everything works as intended.
- No checker depends on another checkers diagnostics. If this is how its represented in the implementation, thats a checker dependency issue that has to be solved with the existing interface, which will automatically grant us with all the benefits that comes with how checker dependecies are handled. This solution sounds far simpler, solves a problem instead of introducing a hack, and might not even take much more time compared to the above point.
- I think whether a checker should be silenced or disabled is confusing, not only for users but for beginner contributors as well.

At the end of the day, all of my concerns stem from checker dependencies, while the name of the patch still doesn't elude to that, and I don't think you're interested in fixing these, nor would I like to stall your progress on the last week of coding, so I'd like to offer some possible approaches that gets the job done without having to worry about the bigger problems.

- Your patch title, and the things things that you said make me believe that there are only a handful of core checkers that cause headaches, how about you add checker options to those instead? If the entirety of the core is problematic, you might as well add a package option to it.
- If you still want to make a new functionality that could silence any checker, create an analyzer config. Those really are meant for development purposes only, and I'm a hair away from separating them into categories as I did with checker options, but GSoC arrived sooner. And if the feature is imperfect, so be it, its meant for us, not for the public. If we for whatever reason would like to pursue this approach, we can fix up the code as I mentioned above and promote it to a frontend flag eventually.
- If this really is meant for scan-build only, you might as well get rid of the diagnostics at that level, without changing the analyzer. You said that you aren't really worried about the performance loss caused by bug report construction that will be suppressed later on, so it sounds ideal.

And I can't stress this enough, I'm not exercising my "reviewer powers" for the sake of it -- I legitimately see this as a bad design direction, and while I know that its already implemented, this is the only place where I was given the chance to voice my concerns. I'll do my best not to be in your way any more then necessary.

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

https://reviews.llvm.org/D66042