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

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

Szelethus added a comment.

In D66042#1629019 , @alexfh wrote:

> In D66042#1627760 , @NoQ wrote:
>
> > In D66042#1627193 , @alexfh wrote:
> >
> > > But without this patch clang seems to have the same two ANALYZE log lines 
> > > regardless of whether I enable `core` checkers or not
> >
> >
> > Yup, it seems as if clang-tidy enables `core` as long as at least one 
> > Static Analyzer checker is enabled (even if it's path-insensitive).
>
>
> It would be nice, if Static Analyzer would hide this from the users 
> completely. The user would specify the list of checkers they need and CSA 
> would enable whatever additional checkers it needs (if any) and then filter 
> out their results. Is it feasible?


Yes, it is! I've been working on a checker dependency system for a while, and 
just proposed some improvements on it on the mailing list that would achieve 
exactly this :)


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D66042#1627760 , @NoQ wrote:

> In D66042#1627193 , @alexfh wrote:
>
> > But without this patch clang seems to have the same two ANALYZE log lines 
> > regardless of whether I enable `core` checkers or not
>
>
> Yup, it seems as if clang-tidy enables `core` as long as at least one Static 
> Analyzer checker is enabled (even if it's path-insensitive).


It would be nice, if Static Analyzer would hide this from the users completely. 
The user would specify the list of checkers they need and CSA would enable 
whatever additional checkers it needs (if any) and then filter out their 
results. Is it feasible?


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I'm convinced, let's keep everything as is but turn this into an 
`-analyzer-config` flag. We generally wanted to reduce the number of frontend 
flags because they are more taxing on backwards compatibility, so it makes 
perfect sense.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

Szelethus added a comment.

In D66042#1627842 , @Charusso wrote:

> Any analyzer config flag is equally accessible to anyone as the driver flags 
> as they are both flags. The only difference is the config flags are more code 
> to implement, and a lot more difficult to use. @NoQ, why the hell would we 
> pick another type of flag which makes zero improvement? The goal is to 
> introduce the best possible solution, which is already here.


Well, `-analyzer-checker` and `-analyzer-silence-checker` would be on the same 
level, while there isn't a consensus on that silencing checkers is desired. 
Config flags are exactly one layer lower, that much more harder is it to 
discover. They are *just* a tad bit more uncomfortable to use, but that 
shouldn't matter inside `scan-build`s implementation, should it?

By the way, are they longer to implement? We gain 5 LOC by deleting the 
existing flags. Depending on how long you're like to make your description, the 
config flag, with a newline, would add 3-4 more.

  ANALYZER_OPTION(
  StringRef, CheckersAndPackagesToSilence, "analyzer-silence-checkers",
  "A comma-separated list of checkers and packages to silence.", "")

The changes you made in `clang/lib/Frontend/CompilerInvocation.cpp` should be 
moved after the call to `parseAnalyzerConfigs`, and instead of this:

  Opts.CheckerSilenceVector.clear();
  for (const Arg *A : Args.filtered(OPT_analyzer_silence_checker)) {
A->claim();
// We can have a list of comma separated checker names, e.g:
// '-analyzer-checker=cocoa,unix'
StringRef checkerList = A->getValue();
SmallVector checkers;
checkerList.split(checkers, ",");
for (auto checker : checkers)
  Opts.CheckerSilenceVector.emplace_back(checker);
  }

We should do this:

  assert(Opts.CheckerSilenceVector.empty());
  llvm::SmallVector CheckerList;
  Opts.CheckersAndPackagesToSilence.split(CheckerList, ',');
  for (StringRef Checker : CheckerList)
  Opts.CheckerSilenceVector.emplace_back(checker);

Thats another 5 LOC minus, we actually lost 7! I suspect not even the 
additional scan-build would make it worse.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D66042#1627842 , @Charusso wrote:

> @Szelethus, here is a really cool example: 
> https://clang.llvm.org/docs/ClangCommandLineReference.html.


These are driver flags. They are indeed well-documented and user-facing. 
Frontend flags aren't.

In D66042#1627842 , @Charusso wrote:

> The goal is to introduce the best possible solution


This sounds like a fairly impractical goal to set. Also perfection is highly 
subjective. There are much bigger problems in the Static Analyzer than this 
whole debate of "what kind of flag do we want it to be?". We've already 
received some useful input, but i feel it's just not worth it to spend much 
more time debating here.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Any analyzer config flag is equally accessible to anyone as the driver flags as 
they are both flags. The only difference is the config flags are more code to 
implement, and a lot more difficult to use. @NoQ, why the hell would we pick 
another type of flag which makes zero improvement? The goal is to introduce the 
best possible solution, which is already here.

@Szelethus, here is a really cool example: 
https://clang.llvm.org/docs/ClangCommandLineReference.html. Man, I would love 
to use all of the best possible flags by hand and know all the flags in my 
head. At least from the tooling side my flags are easy to use, "out of the 
box", with zero overhead. Also the idea of this type of flag is invented by 
@NoQ and I could not say no to the best possible way.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D66042#162 , @Szelethus wrote:

> Yup. We will be able to present the drastic improvement made on LLVM 
> analyses, while giving us some breathing room to polish a long-term solution. 
> I suspect `-analyzer-config silence-checkers=core -analyzer-config 
> silence-checkers=something.Else` wont work (I believe the last value will be 
> used in the invocation), but if only scan-build is using it, we can do 
> `-analyzer-config silence-checkers=core,something.Else`.


Note that scan-build exposes `-analyzer-config` as, well, `-analyzer-config`. 
So if we turn this into a config, changes in scan-build become relatively 
unnecessary.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

Szelethus added a comment.

In D66042#1627765 , @NoQ wrote:

> In D66042#1626631 , @Szelethus wrote:
>
> > In D66042#1626513 , @Charusso 
> > wrote:
> >
> > > I really appreacite your ideas. It is unbelievable you guys bring up 20 
> > > different ideas for 5 LOC. I cannot really argue about any idea, as every 
> > > of them could be a possible solution. I have to pick the right solution, 
> > > and drop the other 19. I believe with that in mind what is an 
> > > experimental feature and how we support to use the Analyzer, none of the 
> > > 19 ideas would born. I did not want to refuse that many ideas, but I have 
> > > to, because we could pick at most 1 to implement per patch. That is why I 
> > > really try to emphasize it is under that experimental feature umbrella 
> > > and we have to think no more about that patch from that point: since the 
> > > beginning.
> >
> >
> > Given our discussion, we've thrown out all but 1 of the 4, by the way 
> > (fixing the actual problem, making this a config, creating checker/package 
> > options, solving this in scan-build only), ideas. Make this a config. 
> > You're correct, thats about 5 LOC change in this patch, at which point I'd 
> > be happy to accept :)
>
>
> You mean something like `-analyzer-config silence-checkers=core.DivideZero`? 
> I guess we can do that, right?


Yup. We will be able to present the drastic improvement made on LLVM analyses, 
while giving us some breathing room to polish a long-term solution. I suspect 
`-analyzer-config silence-checkers=core -analyzer-config 
silence-checkers=something.Else` wont work (I believe the last value will be 
used in the invocation), but if only scan-build is using it, we can do 
`-analyzer-config silence-checkers=core,something.Else`.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D66042#1626631 , @Szelethus wrote:

> In D66042#1626513 , @Charusso wrote:
>
> > I really appreacite your ideas. It is unbelievable you guys bring up 20 
> > different ideas for 5 LOC. I cannot really argue about any idea, as every 
> > of them could be a possible solution. I have to pick the right solution, 
> > and drop the other 19. I believe with that in mind what is an experimental 
> > feature and how we support to use the Analyzer, none of the 19 ideas would 
> > born. I did not want to refuse that many ideas, but I have to, because we 
> > could pick at most 1 to implement per patch. That is why I really try to 
> > emphasize it is under that experimental feature umbrella and we have to 
> > think no more about that patch from that point: since the beginning.
>
>
> Given our discussion, we've thrown out all but 1 of the 4, by the way (fixing 
> the actual problem, making this a config, creating checker/package options, 
> solving this in scan-build only), ideas. Make this a config. You're correct, 
> thats about 5 LOC change in this patch, at which point I'd be happy to accept 
> :)


You mean something like `-analyzer-config silence-checkers=core.DivideZero`? I 
guess we can do that, right?

In D66042#1626631 , @Szelethus wrote:

> In D66042#1626513 , @Charusso wrote:
>
> > I am so sorry I have to be a dictator here, but someone - probably me or 
> > the code owner - has to decide to move forward.
>
>
> I feel very uncomfortable with this statement.


F9787467: photo_2019-08-13_13-23-13.jpg 


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D66042#1627193 , @alexfh wrote:

> Should this be different with clang and this patch?


Nope. Moreover, this patch in fact introduces the same problem in scan-build :)

In D66042#1627193 , @alexfh wrote:

> But without this patch clang seems to have the same two ANALYZE log lines 
> regardless of whether I enable `core` checkers or not


Yup, it seems as if clang-tidy enables `core` as long as at least one Static 
Analyzer checker is enabled (even if it's path-insensitive).


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D66042#1625898 , @NoQ wrote:

> While we're here: i poked your silencing mechanism a little bit and even 
> though i'm still pretty sure you couldn't have done it perfectly without our 
> help, it sounds as if the only problem you have with it is that the 
> path-sensitive checkers keep running even if only path-insensitive checkers 
> are enabled:
>
>   $ clang-tidy test.c -checks=-*,clang-analyzer-unix.cstring.BadSizeArg -- 
> -Xclang -analyzer-display-progress
>  
>   ANALYZE (Syntax): /Users/adergachev/test/test.c foo 
> // <== only this part will actually influence
>   
> // the results of analysis in this invocation
>   ANALYZE (Path,  Inline_Regular): /Users/adergachev/test/test.c foo  
> // <== however this part is sloow
>
>
> This may be a performance issue for users who want fast analysis but are 
> interested in some path-insensitive Static Analyzer checks (and they don't 
> seem to have a way around that when they limit themselves to clang-tidy's own 
> CLI), but apart from that you indeed seem to be fine.


Should this be different with clang and this patch? Unfortunately, it doesn't 
apply cleanly, and I can't verify. But without this patch clang seems to have 
the same two ANALYZE log lines regardless of whether I enable `core` checkers 
or not:

  $ clang -cc1 -analyze -analyzer-checker core,unix.cstring.BadSizeArg 
-analyzer-display-progress /tmp/q.cc
  /tmp/q.cc:2:12: warning: division by zero is undefined
(void)(1 / 0);
 ^ ~
  ANALYZE (Syntax): /tmp/q.cc test_disable_core_div_by_zero()
  ANALYZE (Path,  Inline_Regular): /tmp/q.cc test_disable_core_div_by_zero()
  /tmp/q.cc:2:12: warning: Division by zero
(void)(1 / 0);
   ~~^~~
  2 warnings generated.
  $ clang -cc1 -analyze -analyzer-checker unix.cstring.BadSizeArg 
-analyzer-display-progress /tmp/q.cc
  /tmp/q.cc:2:12: warning: division by zero is undefined
(void)(1 / 0);
 ^ ~
  ANALYZE (Syntax): /tmp/q.cc test_disable_core_div_by_zero()
  ANALYZE (Path,  Inline_Regular): /tmp/q.cc test_disable_core_div_by_zero()
  1 warning generated.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

Szelethus added a comment.

Too bad we're in different time zones, I can only respond in a giant comment, 
but I wouldn't want to make anyone feel left out :)

The conclusion to my responses is this:

- I recently sent out a mail to cfe-dev (I hope to have added your correct 
email adresses!) that proposes what I believe to be a far better, long term 
solution to the issue brought up in this thread. 
http://lists.llvm.org/pipermail/cfe-dev/2019-August/063070.html
- While I see that there are some disagreements on whether we should have 
experimental features as frontend flags, I still feel that this should be an 
analyzer config (or checker/package options). The solution in my mail makes 
this patch a temporary bandaid, not a feature.
- I understand that my stance on this is strong, but I'm still happy to go on 
with the discussion and be proven wrong.

As soon as we convert this into a config, I'll be happy to comment on the nits 
and accept the patch.

In D66042#1625971 , @NoQ wrote:

> In D66042#1625000 , @Szelethus wrote:
>
> > if we add this flag, people responsible for developing interafaces for the 
> > analyzer might end up using it.
>
>
> And this is fine, that's supported. There's a very limited list of such 
> people and we could talk to all of them easily if we wanted to. On the other 
> hand, an end-user running `clang --analyze -Xclang -flagflagflag` manually on 
> his desktop is not supported.


But even if this is what we wanted to pursue, incrementally fixing up the code 
and adding this flag as a crown on top wod be the way to go. Frontend flags 
dont have alpha packages. We've always developed configs for in-development 
features.

> In D66042#1625000 , @Szelethus wrote:
> 
>> Whenever we forget about adding a checker tag to a subchecker, this issue 
>> will arise again.
> 
> 
> Mmm, if we forget about adding a checker tag to a subchecker, then we already 
> have a problem, regardless of this patch, right? The checker name in the bug 
> report is going to be incorrect in this case, which will, at the very least, 
> hurt clang-tidy users.
> 
> In D66042#1625000 , @Szelethus wrote:
> 
>> 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.
> 
> 
> My impression is kinda the opposite: the more we put our hacks into the 
> imperative checker code, the harder it gets. `Checkers.td` "just works", but 
> whenever we try to mess with it, we always get something wrong. Which is 
> exactly why i greatly appreciated your work to formalize everything in 
> tablegen: you allowed everybody to re-use the code that is easy to get wrong.

Very true, which is why I proposed a solution on the mailing list that would 
enforce using the correct checker tag. Until then, lets not cascade one issue 
into many more I guess? Disabling a checker should only be about getting rid of 
the diagnostics, so I think we should pursue that direction instead of 
silencing, and I think that's the expectation users have as well, don't they?

> In D66042#1625000 , @Szelethus wrote:
> 
>> 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...
> 
> 
> I have no data to conclude that frontend flags are more discoverable than 
> `-analyzer-config` options. As a matter of our policy of what we actively 
> support vs. what is passively available, those aren't supported.

I dont have data per se, but its only since the last release they became 
listable. Though I agree that a more aggressive warning message should be 
accompanied with them, something like "These configurations are meant for 
development purposes only!".

On another note, my patches that added frontend flags were stalled for very 
long stemming from this debate. I see the expression "our policy", but it seems 
like everyone views "our policy" in their own way -- Devin seems to be very 
concerned to the level where even the source code should be as secretive as 
possible, I like to think that that's maybe a bit too strict, but the frontend 
flags are our user interface, while You and Csaba seem to have a more relaxed 
stance. Maybe its time to formalize this.

I also dislike that we don't have an interface through the driver. I don't see 
why we shouldn't, and it would naturally create layers where frontend flags 
would justifiably be called development-only flags.

In D66042#1626086 , @Charusso wrote:

> [...] I think not just scan-build would use that feature I have just 
> 

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1626460 , @NoQ wrote:

> I'd like to hear @Szelethus's opinion one more time on this patch. I'm 
> perfectly fine with abandoning the idea and going for in-checker 
> suppressions, simply because there's at least one strong opinion against it 
> and i don't want to push this further, but i just honestly think this patch 
> is a good idea. This discussion has so far been very useful regardless, at 
> least to me personally.


I really appreacite your ideas. It is unbelievable you guys bring up 20 
different ideas for 5 LOC. I cannot really argue about any idea, as every of 
them could be a possible solution. I have to pick the right solution, and drop 
the other 19. I believe with that in mind what is an experimental feature and 
how we support to use the Analyzer, none of the 19 ideas would born. I did not 
want to refuse that many ideas, but I have to, because we could pick at most 1 
to implement per patch. That is why I really try to emphasize it is under that 
experimental feature umbrella and we have to think no more about that patch 
from that point: since the beginning. I am so sorry I have to be a dictator 
here, but someone - probably me or the code owner - has to decide to move 
forward.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I'd like to hear @Szelethus's opinion one more time on this patch. I'm 
perfectly fine with abandoning the idea and going for in-checker suppressions, 
simply because there's at least one strong opinion against it and i don't want 
to push this further, but i just honestly think this patch is a good idea. This 
discussion has so far been very useful regardless, at least to me personally.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1625971 , @NoQ wrote:

> In D66042#1625000 , @Szelethus wrote:
>
> > if we add this flag, people responsible for developing interafaces for the 
> > analyzer might end up using it.
>
>
> And this is fine, that's supported. There's a very limited list of such 
> people and we could talk to all of them easily if we wanted to. On the other 
> hand, an end-user running `clang --analyze -Xclang -flagflagflag` manually on 
> his desktop is not supported.


So, as I am the owner of the patch, I have to take responsibilities for my 
experimental stuff. Every experimental flag we provide we provide for peoples 
who creates interfaces. Like my `PathDiagnosticPopUpPiece` is an experimental 
feature, and has not been arrived to CodeChecker yet. It is upstreamed and if 
someone wants to invoke it, it is possible since it is upstreamed under the 
estimation it is experimental and non-required to implement. It was broken for 
two months and no-one cared because it is an experimental feature. Experimental 
features could arrive without a perfect shape and only made for people who 
could deal with them so that if someone does not know how to use them, we 
assume that that user would not use that.

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


That experimental-feature-chain invented by @NoQ in his previous quoted comment 
explains very well how bad is the situation since the beginning for an advanced 
user. They are advanced users so they could handle any experimental feature 
pretty easily. Also please note that, **it is an experimental feature**.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1626122 , @NoQ wrote:

> > use it locally
>
> What do you mean here? If you want to use the patch for evaluating your 
> results, you might as well untick the checker in the scan-build's index.html 
> interface. The point of having this patch landed is to allow users who are 
> worried by false positives of specific core checkers to use the Static 
> Analyzer on their code anyway, without being overwhelmed with false 
> positives. It was never about us, it was always about them^^


Well, it is not that difficult to write up a documentation: apply that patch so 
LLVM reports will be more awesome and we have not got enough time to make this 
large-scale tough change upstreamed:

  1  // See whether we need to silence the checker.
  2  StringRef ErrorTag = 
ErrorNode->getLocation().getTag()->getTagDescription();
  3  for (const std::string  : Opts.CheckerSilenceVector)
  4if (ErrorTag.startswith(CheckerName))
  5  return nullptr;


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> use it locally

What do you mean here? If you want to use the patch for evaluating your 
results, you might as well untick the checker in the scan-build's index.html 
interface. The point of having this patch landed is to allow users who are 
worried by false positives of specific core checkers to use the Static Analyzer 
on their code anyway, without being overwhelmed with false positives. It was 
never about us, it was always about them^^


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1625971 , @NoQ wrote:

> In D66042#1625000 , @Szelethus wrote:
>
> > 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.
>
>
> My impression is kinda the opposite: the more we put our hacks into the 
> imperative checker code, the harder it gets. `Checkers.td` "just works", but 
> whenever we try to mess with it, we always get something wrong. Which is 
> exactly why i greatly appreciated your work to formalize everything in 
> tablegen: you allowed everybody to re-use the code that is easy to get wrong.
>
> Generally, though, i think we're currently only talking about two checkers: 
> the null dereference checker and the "calling a method on a null pointer" 
> checker (which is, suddenly, a separate checker). @Charusso, do i understand 
> correctly that we don't really want to disable uninitialized variable 
> checkers (there are, like, 5 of them) because you've more or less fixed their 
> false positives? Also we might want to disable MallocChecker but it doesn't 
> really do any modeling and doesn't really belong to `core` either. If it's 
> just two checkers, i definitely don't mind simply splitting them up for the 
> purposes of GSoC, while letting us come to a consensus on this patch.


We cannot be sure at the end of GSoC which checkers are needed to be silenced. 
For example I am about to model the casting enough well, so that every path we 
take should be meaningful according to the dynamic casting in LLVM.

> In D66042#1625000 , @Szelethus wrote:
> 
>> 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.
> 
> 
> Hmm. This would require scan-build to parse HTML output, which is slightly 
> annoying but not impossible, given that it already knows how to deduplicate 
> reports. Tools that consume plist files should be fine because they anyway 
> have to parse plist files. And clang-tidy already has their own silencing 
> mechanism. So i kinda like this idea, even though it's a bit more work. 
> @Charusso, could you take a look if it's actually too much work?

Because clang-tidy has its own feature for disabling the core (as just 
revealed) it would be possible to only affect scan-build with that patch. 
However touching that Perl-madness to read information and remove reports would 
require too much overhead compared to a comfy way to apply that existing patch 
locally. `sub ScanFile` in scan-build is already does that logic, so it would 
not hurt too much, just I do not like the idea touch that tool. I think not 
just scan-build would use that feature I have just implemented, as this is the 
only comfortable way to disable the core. That is why it is inside the 
Analyzer, and I believe every other place to implement would be a bad idea, as 
everyone really needs that feature. I think if we have strong problems with a 
patch, it meaningless to continue/rethink from 10 different angles and apply it 
locally for the given task is far more better solution. Also I cannot really 
force out to implement that new feature in every scan-build like tooling, and 
since then this patch would bring up overhead without actual implementations.

In my mind every new API/feature like the NoteTag or the CallDescriptionMap 
should arrive in the code base with removing the old API. So we will have 
better code and no-one would use the old API and instead would improve the new 
much better API without continuing to create more and more deprecated code 
(that is happening with NoteTags ATM). Because we are working with tons of 
experimental features, like that two new improvement, we do not have such 
policy to require to deprecate the old API. As we have no policies according to 
experimental features and that patch is an experimental feature, I see two 
directions: publish that as it is, or abandon it, and use it locally.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D66042#1625000 , @Szelethus wrote:

> if we add this flag, people responsible for developing interafaces for the 
> analyzer might end up using it.


And this is fine, that's supported. There's a very limited list of such people 
and we could talk to all of them easily if we wanted to. On the other hand, an 
end-user running `clang --analyze -Xclang -flagflagflag` manually on his 
desktop is not supported.

In D66042#1625000 , @Szelethus wrote:

> Whenever we forget about adding a checker tag to a subchecker, this issue 
> will arise again.


Mmm, if we forget about adding a checker tag to a subchecker, then we already 
have a problem, regardless of this patch, right? The checker name in the bug 
report is going to be incorrect in this case, which will, at the very least, 
hurt clang-tidy users.

In D66042#1625000 , @Szelethus wrote:

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


My impression is kinda the opposite: the more we put our hacks into the 
imperative checker code, the harder it gets. `Checkers.td` "just works", but 
whenever we try to mess with it, we always get something wrong. Which is 
exactly why i greatly appreciated your work to formalize everything in 
tablegen: you allowed everybody to re-use the code that is easy to get wrong.

Generally, though, i think we're currently only talking about two checkers: the 
null dereference checker and the "calling a method on a null pointer" checker 
(which is, suddenly, a separate checker). @Charusso, do i understand correctly 
that we don't really want to disable uninitialized variable checkers (there 
are, like, 5 of them) because you've more or less fixed their false positives? 
Also we might want to disable MallocChecker but it doesn't really do any 
modeling and doesn't really belong to `core` either. If it's just two checkers, 
i definitely don't mind simply splitting them up for the purposes of GSoC, 
while letting us come to a consensus on this patch.

In D66042#1625000 , @Szelethus wrote:

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


I have no data to conclude that frontend flags are more discoverable than 
`-analyzer-config` options. As a matter of our policy of what we actively 
support vs. what is passively available, those aren't supported.

In D66042#1625000 , @Szelethus wrote:

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


Hmm. This would require scan-build to parse HTML output, which is slightly 
annoying but not impossible, given that it already knows how to deduplicate 
reports. Tools that consume plist files should be fine because they anyway have 
to parse plist files. And clang-tidy already has their own silencing mechanism. 
So i kinda like this idea, even though it's a bit more work. @Charusso, could 
you take a look if it's actually too much work?


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D66042#1625235 , @alexfh wrote:

> In D66042#1624081 , @NoQ wrote:
>
> > +@alexfh because clang-tidy now finally has a way of safely disabling core 
> > checkers without causing crashes all over the place! Would you like to take 
> > the same approach as we picked in scan-build, i.e. when the user asks to 
> > disable a core checker, silence it instead?
>
>
> clang-tidy's native way to enable/disable diagnostics is applied to the 
> static analyzer twice: first time when the list of enabled checkers is 
> created (and then core checkers are always added to that list), and the 
> second time - to each diagnostic generated by the static analyzer (this time 
> the original check name filter is applied, without core checkers). This 
> already works consistently from a user's perspective: 
> https://gcc.godbolt.org/z/MEvSsP
>
> Are there any benefits in using the new CheckerSilenceVector mechanism in 
> clang-tidy?


Wait, you already did that on your own? Nice!! I missed that part. I guess we 
told you that core checkers need to always be enabled as long as the Static 
Analyzer is used at all, and you did exactly that. If you already have such 
silencing mechanism, then i think you won't really benefit from our new 
mechanism.



While we're here: i poked your silencing mechanism a little bit and even though 
i'm still pretty sure you couldn't have done it perfectly without our help, it 
sounds as if the only problem you have with it is that the path-sensitive 
checkers keep running even if only path-insensitive checkers are enabled:

  $ clang-tidy test.c -checks=-*,clang-analyzer-unix.cstring.BadSizeArg -- 
-Xclang -analyzer-display-progress
  
  ANALYZE (Syntax): /Users/adergachev/test/test.c foo 
// <== only this part will actually influence
  
// the results of analysis in this invocation
  ANALYZE (Path,  Inline_Regular): /Users/adergachev/test/test.c foo  
// <== however this part is sloow

This may be a performance issue for users who want fast analysis but are 
interested in some path-insensitive Static Analyzer checks (and they don't seem 
to have a way around that when they limit themselves to clang-tidy's own CLI), 
but apart from that you indeed seem to be fine.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D66042#1624081 , @NoQ wrote:

> +@alexfh because clang-tidy now finally has a way of safely disabling core 
> checkers without causing crashes all over the place! Would you like to take 
> the same approach as we picked in scan-build, i.e. when the user asks to 
> disable a core checker, silence it instead?


clang-tidy's native way to enable/disable diagnostics is applied to the static 
analyzer twice: first time when the list of enabled checkers is created (and 
then core checkers are always added to that list), and the second time - to 
each diagnostic generated by the static analyzer (this time the original check 
name filter is applied, without core checkers). This already works consistently 
from a user's perspective: https://gcc.godbolt.org/z/MEvSsP

Are there any benefits in using the new CheckerSilenceVector mechanism in 
clang-tidy?


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

Szelethus added a comment.

In D66042#1624697 , @Charusso wrote:

> In 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 , @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 , and I mentioned 
again in 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 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D66042#1624697 , @Charusso wrote:

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


Nice, but i suspect that `osx.OSObjectRetainCount` is still screwed :)


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

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


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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(?)


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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

In D66042#1624478 , @Charusso wrote:

> In D66042#1624475 , @Szelethus wrote:
>
> > In 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1624475 , @Szelethus wrote:

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

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


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

Szelethus added a comment.

In D66042#1624469 , @Charusso wrote:

> In 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 

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

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

Szelethus added a comment.

In 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 mean, simply changing where the silencing happens would solve most of these, 
of course, though there still would be things we'd have to pay for one way or 
another. I see your point with the boilerplate code however, we could 
definitely improve on that part of the interface. I wouldn't call the current 
situation so bad thought that I'd restrain myself from adding a couple more 
checkers.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1624468 , @Szelethus wrote:

> In D66042#1624462 , @Charusso wrote:
>
> > My solution preserve the checkers as they are and yours definitely would 
> > rewrite them. Checker writing has tons of boilerplates, I think adding more 
> > should be avoided. Why would you touch the checkers as my solution is 
> > already implemented and working perfectly without any overhead?
>
>
> 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.

 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.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

Szelethus added a comment.

In D66042#1624462 , @Charusso wrote:

> My solution preserve the checkers as they are and yours definitely would 
> rewrite them. Checker writing has tons of boilerplates, I think adding more 
> should be avoided. Why would you touch the checkers as my solution is already 
> implemented and working perfectly without any overhead?


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.

>>> 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?


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1624432 , @Szelethus wrote:

> In D66042#1624365 , @Charusso wrote:
>
> > In D66042#1624320 , @Szelethus 
> > wrote:
> >
> > > I think it would make a lot more sense to create a separate (and hidden!) 
> > > coreModeling package that would do all the modeling, and regard core as a 
> > > highly recommended, but not mandatory set of checkers. Wouldn't this 
> > > create a cleaner interface?
> >
> >
> > Sadly separating a modeling package is impossible. The checkers 
> > subscription-based design is made for that purpose to make them both model 
> > the analysis, and both emit reports. None of the checkers has separated 
> > modeling and separated business logic, because they are so tied together. 
> > The reporting part fires on given modeling parts, like that is why they 
> > could report errors during modeling and stop the modeling. May if we 
> > redesign all the core checkers to separated business/modeling logic it 
> > would be helpful.
>
>
> I didn't mean to do too invasive changes, only something like this, which 
> would be tedious but not difficult:
>
>   struct ModelingChecker {
> bool AreDiagnosticsEnabled = false;
> void model() const;
>   };
>  
>   void ModelingChecker::model() const {
> // whatever
>  
> if (!AreDiagnosticsEnabled) {
>   C.generateSink(State, C.getPredecessor());
>   return;
> }
>  
> static CheckerProgramPointTag DiagnosticCheckerTag(this, 
> "DiagnosticChecker");
> const ExplodedNode *ErrorNode = C.generateErrorNode(State, 
> );
> // etc etc
>   }
>  
>   void registerDiagnosticChecker(CheckerManager ) {
> Mgr.getChecker()->AreDiagnosticsEnabled = true;
>   }
>
>
> A solution like this would preserve the current checker structures while 
> neatly hiding the implementation part.


My solution preserve the checkers as they are and yours definitely would 
rewrite them. Checker writing has tons of boilerplates, I think adding more 
should be avoided. Why would you touch the checkers as my solution is already 
implemented and working perfectly without any overhead?

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


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

Szelethus added a comment.

In D66042#1624365 , @Charusso wrote:

> In D66042#1624320 , @Szelethus wrote:
>
> > I think it would make a lot more sense to create a separate (and hidden!) 
> > coreModeling package that would do all the modeling, and regard core as a 
> > highly recommended, but not mandatory set of checkers. Wouldn't this create 
> > a cleaner interface?
>
>
> Sadly separating a modeling package is impossible. The checkers 
> subscription-based design is made for that purpose to make them both model 
> the analysis, and both emit reports. None of the checkers has separated 
> modeling and separated business logic, because they are so tied together. The 
> reporting part fires on given modeling parts, like that is why they could 
> report errors during modeling and stop the modeling. May if we redesign all 
> the core checkers to separated business/modeling logic it would be helpful.


I didn't mean to do too invasive changes, only something like this, which would 
be tedious but not difficult:

  struct ModelingChecker {
bool AreDiagnosticsEnabled = false;
void model() const;
  };
  
  void ModelingChecker::model() const {
// whatever
  
if (!AreDiagnosticsEnabled) {
  C.generateSink(State, C.getPredecessor());
  return;
}
  
static CheckerProgramPointTag DiagnosticCheckerTag(this, 
"DiagnosticChecker");
const ExplodedNode *ErrorNode = C.generateErrorNode(State, 
);
// etc etc
  }
  
  void registerDiagnosticChecker(CheckerManager ) {
Mgr.getChecker()->AreDiagnosticsEnabled = true;
  }

A solution like this would preserve the current checker structures while neatly 
hiding the implementation part.

> The problem with that it requires huge overhead for no reason. My idea here 
> is to create a single CoreChecker which does only the core modeling and 
> existing core checkers has the reporting logic so they message with the 
> CoreChecker. I believe it could not be scalable, as we already have 3k LOC 
> checkers and apiModeling would merge into that at some point.

I think these aren't anything to worry about if we use the above solution :^)

> 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?


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1624320 , @Szelethus wrote:

> I think it would make a lot more sense to create a separate (and hidden!) 
> coreModeling package that would do all the modeling, and regard core as a 
> highly recommended, but not mandatory set of checkers. Wouldn't this create a 
> cleaner interface?


Sadly separating a modeling package is impossible. The checkers 
subscription-based design is made for that purpose to make them both model the 
analysis, and both emit reports. None of the checkers has separated modeling 
and separated business logic, because they are so tied together. The reporting 
part fires on given modeling parts, like that is why they could report errors 
during modeling and stop the modeling. May if we redesign all the core checkers 
to separated business/modeling logic it would be helpful. The problem with that 
it requires huge overhead for no reason. My idea here is to create a single 
CoreChecker which does only the core modeling and existing core checkers has 
the reporting logic so they message with the CoreChecker. I believe it could 
not be scalable, as we already have 3k LOC checkers and apiModeling would merge 
into that at some point.

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


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

Szelethus added a comment.

There is a discussion to be had about the core package. So @NoQ suggested that 
we could hide the entire thing just as we do with apiModeling, but I argued 
that the users wouldn't be exposed to the checker descriptions. However, it 
makes little sense to caution our users not to disable it (because things might 
break), and at the same time giving them the option. If a core checker emits 
dumb messages, of course I'd like get rid of them (this entire patch is about 
that, isn't it?). I think it would make a lot more sense to create a separate 
(and hidden!) coreModeling package that would do all the modeling, and regard 
core as a highly recommended, but not mandatory set of checkers. Wouldn't this 
create a cleaner interface?

All in all, I sympathize with this problem, but admit that I don't particularly 
like the silencing approach, because we try hard to hide implementation details 
from users, and this forces them in a way to interact with it. I realize 
however that there isn't much time left of GSoC, and separating who knows how 
many checkers would take a long time, so if this is important, I don't insist. 
We could however place a couple FIXMEs indicating that this is a placeholder 
solution, and not advertise this flag too much.

Please know that this is a complaint on core checkers, not really your patch, 
and I appreciate your work in finally making sense of this! :)




Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:192-197
+  /// Pair of checker name and enable/disable to do analysis.
+  std::vector> CheckerAnalysisVector;
+
+  /// Vector of checker names to do not emit warnings.
+  std::vector CheckerSilenceVector;
 

Charusso wrote:
> NoQ wrote:
> > `CheckerAnalysisVector` sounds like a vector of checkers that will be 
> > subject to analysis. But in reality they are the ones that are analyzing 
> > and nobody is analyzing them.
> > 
> > The old name isn't very expressive either, but at least it sounds cool >.<
> > 
> > Maybe `EnabledCheckers`, `SilencedCheckers`?
> The problem with `EnabledCheckers` it is a lie. It would be just `Checkers` 
> and then whether a given checker is enabled or disabled is determined later. 
> `CheckerEnableVector` and `CheckerSilenceVector` may would be okay.
Aye, I've come across this field multiple times and there isn't really a good 
name for it. However, is this correct? Are these *checkers*, or checkers *and* 
packages?


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:192-197
+  /// Pair of checker name and enable/disable to do analysis.
+  std::vector> CheckerAnalysisVector;
+
+  /// Vector of checker names to do not emit warnings.
+  std::vector CheckerSilenceVector;
 

NoQ wrote:
> `CheckerAnalysisVector` sounds like a vector of checkers that will be subject 
> to analysis. But in reality they are the ones that are analyzing and nobody 
> is analyzing them.
> 
> The old name isn't very expressive either, but at least it sounds cool >.<
> 
> Maybe `EnabledCheckers`, `SilencedCheckers`?
The problem with `EnabledCheckers` it is a lie. It would be just `Checkers` and 
then whether a given checker is enabled or disabled is determined later. 
`CheckerEnableVector` and `CheckerSilenceVector` may would be okay.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

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

But i definitely like it how smooth this patch turned out to be!

Also recent bugzilla requests for this feature:

https://bugs.llvm.org/show_bug.cgi?id=42816
https://bugs.llvm.org/show_bug.cgi?id=41812


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:192-197
+  /// Pair of checker name and enable/disable to do analysis.
+  std::vector> CheckerAnalysisVector;
+
+  /// Vector of checker names to do not emit warnings.
+  std::vector CheckerSilenceVector;
 

`CheckerAnalysisVector` sounds like a vector of checkers that will be subject 
to analysis. But in reality they are the ones that are analyzing and nobody is 
analyzing them.

The old name isn't very expressive either, but at least it sounds cool >.<

Maybe `EnabledCheckers`, `SilencedCheckers`?


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 214500.
Charusso added a comment.

- Fix a comment.


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

https://reviews.llvm.org/D66042

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  clang/test/Analysis/silence-checker-core-all.cpp
  clang/test/Analysis/silence-checker-core-div-by-zero.cpp
  clang/tools/scan-build/bin/scan-build
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -46,7 +46,7 @@
 std::unique_ptr AnalysisConsumer =
 CreateAnalysisConsumer(Compiler);
 AnalysisConsumer->AddDiagnosticConsumer(new DiagConsumer(DiagsOutput));
-Compiler.getAnalyzerOpts()->CheckersControlList = {
+Compiler.getAnalyzerOpts()->CheckerAnalysisVector = {
 {"custom.CustomChecker", true}};
 AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry ) {
   Registry.addChecker("custom.CustomChecker", "Description", "");
Index: clang/tools/scan-build/bin/scan-build
===
--- clang/tools/scan-build/bin/scan-build
+++ clang/tools/scan-build/bin/scan-build
@@ -57,6 +57,7 @@
   KeepEmpty => 0,# Don't remove output directory even with 0 results.
   EnableCheckers => {},
   DisableCheckers => {},
+  SilenceCheckers => {},
   Excludes => [],
   UseCC => undef,# C compiler to use for compilation.
   UseCXX => undef,   # C++ compiler to use for compilation.
@@ -1742,9 +1743,15 @@
 if ($arg eq "-disable-checker") {
   shift @$Args;
   my $Checker = shift @$Args;
-  # Store $NumArgs to preserve the order the checkers were disabled.
-  $Options{DisableCheckers}{$Checker} = $NumArgs;
-  delete $Options{EnableCheckers}{$Checker};
+  # Store $NumArgs to preserve the order the checkers/warnings are disabled.
+  # See whether it is a core checker to disable. That means we do not want
+  # to emit a report from that checker so we have to silence it.
+  if (index($Checker, "core") == 0) {
+$Options{SilenceCheckers}{$Checker} = $NumArgs;
+  } else {
+$Options{DisableCheckers}{$Checker} = $NumArgs;
+delete $Options{EnableCheckers}{$Checker};
+  }
   next;
 }
 
@@ -1882,6 +1889,11 @@
   # Push checkers in order they were disabled.
   push @AnalysesToRun, "-analyzer-disable-checker", $_;
 }
+foreach (sort { $Options{SilenceCheckers}{$a} <=> $Options{SilenceCheckers}{$b} }
+ keys %{$Options{SilenceCheckersl}}) {
+  # Push checkers in order they were silenced.
+  push @AnalysesToRun, "-analyzer-silence-checker", $_;
+}
 if ($Options{AnalyzeHeaders}) { push @AnalysesToRun, "-analyzer-opt-analyze-headers"; }
 if ($Options{AnalyzerStats}) { push @AnalysesToRun, '-analyzer-checker=debug.Stats'; }
 if ($Options{MaxLoop} > 0) { push @AnalysesToRun, "-analyzer-max-loop $Options{MaxLoop}"; }
Index: clang/test/Analysis/silence-checker-core-div-by-zero.cpp
===
--- /dev/null
+++ clang/test/Analysis/silence-checker-core-div-by-zero.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-silence-checker core.DivideZero \
+// RUN:  -verify %s
+
+void test_disable_core_div_by_zero() {
+  (void)(1 / 0);
+  // expected-warning@-1 {{division by zero is undefined}}
+  // no-warning: 'Division by zero'
+}
Index: clang/test/Analysis/silence-checker-core-all.cpp
===
--- /dev/null
+++ clang/test/Analysis/silence-checker-core-all.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-silence-checker=core \
+// RUN:  -verify %s
+
+// expected-no-diagnostics
+
+void test_disable_core_all(int *p) {
+  if (p)
+return;
+
+  int x = p[0];
+  // no-warning: Array access (from variable 'p') results in a null pointer dereference
+}
Index: clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -200,7 +200,7 @@
 
   // Parse '-analyzer-checker' and '-analyzer-disable-checker' options from the
   // command line.
-  for (const std::pair  : AnOpts.CheckersControlList) {
+  for (const std::pair  : AnOpts.CheckerAnalysisVector) {
 CheckerInfoListRange 

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 214498.
Charusso marked 5 inline comments as done.
Charusso added a comment.

- Fix.


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

https://reviews.llvm.org/D66042

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  clang/test/Analysis/silence-checker-core-all.cpp
  clang/test/Analysis/silence-checker-core-div-by-zero.cpp
  clang/tools/scan-build/bin/scan-build
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -46,7 +46,7 @@
 std::unique_ptr AnalysisConsumer =
 CreateAnalysisConsumer(Compiler);
 AnalysisConsumer->AddDiagnosticConsumer(new DiagConsumer(DiagsOutput));
-Compiler.getAnalyzerOpts()->CheckersControlList = {
+Compiler.getAnalyzerOpts()->CheckerAnalysisVector = {
 {"custom.CustomChecker", true}};
 AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry ) {
   Registry.addChecker("custom.CustomChecker", "Description", "");
Index: clang/tools/scan-build/bin/scan-build
===
--- clang/tools/scan-build/bin/scan-build
+++ clang/tools/scan-build/bin/scan-build
@@ -57,6 +57,7 @@
   KeepEmpty => 0,# Don't remove output directory even with 0 results.
   EnableCheckers => {},
   DisableCheckers => {},
+  SilenceCheckers => {},
   Excludes => [],
   UseCC => undef,# C compiler to use for compilation.
   UseCXX => undef,   # C++ compiler to use for compilation.
@@ -1742,9 +1743,15 @@
 if ($arg eq "-disable-checker") {
   shift @$Args;
   my $Checker = shift @$Args;
-  # Store $NumArgs to preserve the order the checkers were disabled.
-  $Options{DisableCheckers}{$Checker} = $NumArgs;
-  delete $Options{EnableCheckers}{$Checker};
+  # Store $NumArgs to preserve the order the checkers/warnings are disabled.
+  # See whether it is a core checker to disable. That means we do not want
+  # to emit a report from that checker so we have to silence it.
+  if (index($Checker, "core") == 0) {
+$Options{SilenceCheckers}{$Checker} = $NumArgs;
+  } else {
+$Options{DisableCheckers}{$Checker} = $NumArgs;
+delete $Options{EnableCheckers}{$Checker};
+  }
   next;
 }
 
@@ -1882,6 +1889,11 @@
   # Push checkers in order they were disabled.
   push @AnalysesToRun, "-analyzer-disable-checker", $_;
 }
+foreach (sort { $Options{SilenceCheckers}{$a} <=> $Options{SilenceCheckers}{$b} }
+ keys %{$Options{SilenceCheckersl}}) {
+  # Push checkers' warnings in order they were disabled.
+  push @AnalysesToRun, "-analyzer-silence-checker", $_;
+}
 if ($Options{AnalyzeHeaders}) { push @AnalysesToRun, "-analyzer-opt-analyze-headers"; }
 if ($Options{AnalyzerStats}) { push @AnalysesToRun, '-analyzer-checker=debug.Stats'; }
 if ($Options{MaxLoop} > 0) { push @AnalysesToRun, "-analyzer-max-loop $Options{MaxLoop}"; }
Index: clang/test/Analysis/silence-checker-core-div-by-zero.cpp
===
--- /dev/null
+++ clang/test/Analysis/silence-checker-core-div-by-zero.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-silence-checker core.DivideZero \
+// RUN:  -verify %s
+
+void test_disable_core_div_by_zero() {
+  (void)(1 / 0);
+  // expected-warning@-1 {{division by zero is undefined}}
+  // no-warning: 'Division by zero'
+}
Index: clang/test/Analysis/silence-checker-core-all.cpp
===
--- /dev/null
+++ clang/test/Analysis/silence-checker-core-all.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-silence-checker=core \
+// RUN:  -verify %s
+
+// expected-no-diagnostics
+
+void test_disable_core_all(int *p) {
+  if (p)
+return;
+
+  int x = p[0];
+  // no-warning: Array access (from variable 'p') results in a null pointer dereference
+}
Index: clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -200,7 +200,7 @@
 
   // Parse '-analyzer-checker' and '-analyzer-disable-checker' options from the
   // command line.
-  for (const std::pair  : AnOpts.CheckersControlList) {
+  for (const std::pair  : 

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/include/clang/Driver/CC1Options.td:127-128
 
+def analyzer_disable_warning : Separate<["-"], "analyzer-disable-warning">,
+  HelpText<"Choose analyzer checkers of the warnings to disable">;
+def analyzer_disable_warning_EQ : Joined<["-"], "analyzer-disable-warning=">,

NoQ wrote:
> How about `-analyzer-silence-checker`?
Cool!



Comment at: clang/tools/scan-build/bin/scan-build:1749
+  # to emit a report from the checker so we have to disable the warning.
+  if (index($Checker, "core") != -1) {
+$Options{DisableWarnings}{$Checker} = $NumArgs;

NoQ wrote:
> Also some sort of `startswith`.
It is a big trouble since 2017 
https://rt.perl.org/Public/Bug/Display.html?id=132301.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added reviewers: alexfh, Szelethus.
NoQ added a subscriber: alexfh.
NoQ added a comment.

+@alexfh because clang-tidy now finally has a way of safely disabling core 
checkers without causing crashes all over the place! Would you like to take the 
same approach as we picked in scan-build, i.e. when the user asks to disable a 
core checker, silence it instead?

+@Szelethus because enabling-disabling checkers is his realm.




Comment at: clang/include/clang/Driver/CC1Options.td:127-128
 
+def analyzer_disable_warning : Separate<["-"], "analyzer-disable-warning">,
+  HelpText<"Choose analyzer checkers of the warnings to disable">;
+def analyzer_disable_warning_EQ : Joined<["-"], "analyzer-disable-warning=">,

How about `-analyzer-silence-checker`?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:1924
+
+  // The 'CheckerName' could be 'core' so that 'contains()' is appropriate.
+  for (const std::string  : Opts.CheckerWarningVector) {

`startswith`?



Comment at: clang/tools/scan-build/bin/scan-build:1749
+  # to emit a report from the checker so we have to disable the warning.
+  if (index($Checker, "core") != -1) {
+$Options{DisableWarnings}{$Checker} = $NumArgs;

Also some sort of `startswith`.


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

https://reviews.llvm.org/D66042



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 214490.
Charusso added a comment.

- Remove one misleading 'no-warning' comment.


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

https://reviews.llvm.org/D66042

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  clang/test/Analysis/turn-off-warnings-core-all.cpp
  clang/test/Analysis/turn-off-warnings-core-div-by-zero.cpp
  clang/tools/scan-build/bin/scan-build
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -46,7 +46,7 @@
 std::unique_ptr AnalysisConsumer =
 CreateAnalysisConsumer(Compiler);
 AnalysisConsumer->AddDiagnosticConsumer(new DiagConsumer(DiagsOutput));
-Compiler.getAnalyzerOpts()->CheckersControlList = {
+Compiler.getAnalyzerOpts()->CheckerAnalysisVector = {
 {"custom.CustomChecker", true}};
 AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry ) {
   Registry.addChecker("custom.CustomChecker", "Description", "");
Index: clang/tools/scan-build/bin/scan-build
===
--- clang/tools/scan-build/bin/scan-build
+++ clang/tools/scan-build/bin/scan-build
@@ -57,6 +57,7 @@
   KeepEmpty => 0,# Don't remove output directory even with 0 results.
   EnableCheckers => {},
   DisableCheckers => {},
+  DisableWarnings => {},
   Excludes => [],
   UseCC => undef,# C compiler to use for compilation.
   UseCXX => undef,   # C++ compiler to use for compilation.
@@ -1742,9 +1743,15 @@
 if ($arg eq "-disable-checker") {
   shift @$Args;
   my $Checker = shift @$Args;
-  # Store $NumArgs to preserve the order the checkers were disabled.
-  $Options{DisableCheckers}{$Checker} = $NumArgs;
-  delete $Options{EnableCheckers}{$Checker};
+  # Store $NumArgs to preserve the order the checkers/warnings are disabled.
+  # See whether it is a core checker to disable. That means we do not want
+  # to emit a report from the checker so we have to disable the warning.
+  if (index($Checker, "core") != -1) {
+$Options{DisableWarnings}{$Checker} = $NumArgs;
+  } else {
+$Options{DisableCheckers}{$Checker} = $NumArgs;
+delete $Options{EnableCheckers}{$Checker};
+  }
   next;
 }
 
@@ -1882,6 +1889,11 @@
   # Push checkers in order they were disabled.
   push @AnalysesToRun, "-analyzer-disable-checker", $_;
 }
+foreach (sort { $Options{DisableWarnings}{$a} <=> $Options{DisableWarnings}{$b} }
+ keys %{$Options{DisableWarnings}}) {
+  # Push checkers' warnings in order they were disabled.
+  push @AnalysesToRun, "-analyzer-disable-warning", $_;
+}
 if ($Options{AnalyzeHeaders}) { push @AnalysesToRun, "-analyzer-opt-analyze-headers"; }
 if ($Options{AnalyzerStats}) { push @AnalysesToRun, '-analyzer-checker=debug.Stats'; }
 if ($Options{MaxLoop} > 0) { push @AnalysesToRun, "-analyzer-max-loop $Options{MaxLoop}"; }
Index: clang/test/Analysis/turn-off-warnings-core-div-by-zero.cpp
===
--- /dev/null
+++ clang/test/Analysis/turn-off-warnings-core-div-by-zero.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-disable-warning core.DivideZero \
+// RUN:  -verify %s
+
+void test_disable_core_div_by_zero() {
+  (void)(1 / 0);
+  // expected-warning@-1 {{division by zero is undefined}}
+  // no-warning: 'Division by zero'
+}
Index: clang/test/Analysis/turn-off-warnings-core-all.cpp
===
--- /dev/null
+++ clang/test/Analysis/turn-off-warnings-core-all.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-disable-warning=core \
+// RUN:  -verify %s
+
+// expected-no-diagnostics
+
+void test_disable_core_all(int *p) {
+  if (p)
+return;
+
+  int x = p[0];
+  // no-warning: Array access (from variable 'p') results in a null pointer dereference
+}
Index: clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -200,7 +200,7 @@
 
   // Parse '-analyzer-checker' and '-analyzer-disable-checker' options from the
   // command line.
-  for (const std::pair  : AnOpts.CheckersControlList) {
+  for (const std::pair  : 

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso created this revision.
Charusso added a reviewer: NoQ.
Charusso added a project: clang.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.

This patch introduces a new option:
`-analyzer-disable-warning`
which could be used to disable warnings of given checkers.

It could be used to "disable" core checkers, so they model the analysis as
before, just if some of them are too noisy it prevents to emit reports.

This patch also adds support for that new option to the scan-build.
Passing the option `-disable-checker core.DivideZero` to the scan-build
will be transferred to `-analyzer-disable-warning=core.DivideZero`.


Repository:
  rC Clang

https://reviews.llvm.org/D66042

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  clang/test/Analysis/turn-off-warnings-core-all.cpp
  clang/test/Analysis/turn-off-warnings-core-div-by-zero.cpp
  clang/tools/scan-build/bin/scan-build
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -46,7 +46,7 @@
 std::unique_ptr AnalysisConsumer =
 CreateAnalysisConsumer(Compiler);
 AnalysisConsumer->AddDiagnosticConsumer(new DiagConsumer(DiagsOutput));
-Compiler.getAnalyzerOpts()->CheckersControlList = {
+Compiler.getAnalyzerOpts()->CheckerAnalysisVector = {
 {"custom.CustomChecker", true}};
 AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry ) {
   Registry.addChecker("custom.CustomChecker", "Description", "");
Index: clang/tools/scan-build/bin/scan-build
===
--- clang/tools/scan-build/bin/scan-build
+++ clang/tools/scan-build/bin/scan-build
@@ -57,6 +57,7 @@
   KeepEmpty => 0,# Don't remove output directory even with 0 results.
   EnableCheckers => {},
   DisableCheckers => {},
+  DisableWarnings => {},
   Excludes => [],
   UseCC => undef,# C compiler to use for compilation.
   UseCXX => undef,   # C++ compiler to use for compilation.
@@ -1742,9 +1743,15 @@
 if ($arg eq "-disable-checker") {
   shift @$Args;
   my $Checker = shift @$Args;
-  # Store $NumArgs to preserve the order the checkers were disabled.
-  $Options{DisableCheckers}{$Checker} = $NumArgs;
-  delete $Options{EnableCheckers}{$Checker};
+  # Store $NumArgs to preserve the order the checkers/warnings are disabled.
+  # See whether it is a core checker to disable. That means we do not want
+  # to emit a report from the checker so we have to disable the warning.
+  if (index($Checker, "core") != -1) {
+$Options{DisableWarnings}{$Checker} = $NumArgs;
+  } else {
+$Options{DisableCheckers}{$Checker} = $NumArgs;
+delete $Options{EnableCheckers}{$Checker};
+  }
   next;
 }
 
@@ -1882,6 +1889,11 @@
   # Push checkers in order they were disabled.
   push @AnalysesToRun, "-analyzer-disable-checker", $_;
 }
+foreach (sort { $Options{DisableWarnings}{$a} <=> $Options{DisableWarnings}{$b} }
+ keys %{$Options{DisableWarnings}}) {
+  # Push checkers' warnings in order they were disabled.
+  push @AnalysesToRun, "-analyzer-disable-warning", $_;
+}
 if ($Options{AnalyzeHeaders}) { push @AnalysesToRun, "-analyzer-opt-analyze-headers"; }
 if ($Options{AnalyzerStats}) { push @AnalysesToRun, '-analyzer-checker=debug.Stats'; }
 if ($Options{MaxLoop} > 0) { push @AnalysesToRun, "-analyzer-max-loop $Options{MaxLoop}"; }
Index: clang/test/Analysis/turn-off-warnings-core-div-by-zero.cpp
===
--- /dev/null
+++ clang/test/Analysis/turn-off-warnings-core-div-by-zero.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-disable-warning core.DivideZero \
+// RUN:  -verify %s
+
+void test_disable_core_div_by_zero() {
+  (void)(1 / 0);
+  // expected-warning@-1 {{division by zero is undefined}}
+  // no-warning: 'The result of the '/' expression is undefined'
+  // no-warning: 'Division by zero'
+}
Index: clang/test/Analysis/turn-off-warnings-core-all.cpp
===
--- /dev/null
+++ clang/test/Analysis/turn-off-warnings-core-all.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-disable-warning=core \
+// RUN:  -verify %s
+
+// expected-no-diagnostics
+
+void