[analyzer] Analysis: Silence checkers

[Charusso]

Summary

This patch introduces a new analyzer-config configuration:
-analyzer-config silence-checkers
which could be used to silence the given checkers.

It accepts a semicolon separated list, packed into quotation marks, e.g:
-analyzer-config silence-checkers="core.DivideZero;core.NullDereference"

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-config silence-checkers=core.DivideZero.

[Charusso]

• Remove one misleading 'no-warning' comment.

[NoQ]

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

clang/include/clang/Driver/CC1Options.td

127–128 ↗

(On Diff #214490)

How about -analyzer-silence-checker?

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

1924

startswith?

clang/tools/scan-build/bin/scan-build

1749

Also some sort of startswith.

[Charusso]

• Fix.

clang/include/clang/Driver/CC1Options.td

127–128 ↗

(On Diff #214490)

Cool!

clang/tools/scan-build/bin/scan-build

1749

It is a big trouble since 2017 https://rt.perl.org/Public/Bug/Display.html?id=132301.

[Charusso]

• Fix a comment.

[NoQ]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

192–196

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?

[NoQ]

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

[Charusso]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

192–196

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.

[Szelethus]

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! :)

[Szelethus]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

192–196

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?

[Charusso]

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.

[Szelethus]

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, &DiagnosticCheckerTag);
  // etc etc
}

void registerDiagnosticChecker(CheckerManager &Mgr) {
  Mgr.getChecker<ModelingChecker>()->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?

[Charusso]

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, &DiagnosticCheckerTag);
  // etc etc
}

void registerDiagnosticChecker(CheckerManager &Mgr) {
  Mgr.getChecker<ModelingChecker>()->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.

[Szelethus]

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?

[Charusso]

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.

[Szelethus]

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.

[Szelethus]

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

[Charusso]

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.

[Szelethus]

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 legitimate concerns. Please don't rush this in before addressing these matters.

[NoQ]

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

[Charusso]

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.

[NoQ]

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 :)

[Szelethus]

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 CheckerProgramPointTags), but again, what if we forget about it? If we fix this (add tags for each checker that suffers from this issue) we're literally 10 lines of code away from a far better solution after which silencing checkers wouldn't be needed.

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

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

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

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

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

[alexfh]

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?

[NoQ]

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 sloooooow

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.

[NoQ]

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?

[Charusso]

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.

[NoQ]

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

[Charusso]

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 &CheckerName : Opts.CheckerSilenceVector)
4    if (ErrorTag.startswith(CheckerName))
5      return nullptr;

[Charusso]

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.

[NoQ]

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.

[Charusso]

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.

[Szelethus]

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

Why is this the only way? Why are the previously mentioned solution any worse? Why is this being comfortable important, if its a development-only feature, and supposedly only used by scan-build?

You yourself stated that the construction of the trimmed graph, exploded graph and whatever else is of no concern to you, so why do you worry about some overhead? To be clear, what do you refer to when you mention "overhead"?

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.

There is no need to be this drastic! We implement in-development checkers in the alpha package, and in-development other features as off-by-default analyzer configs, this has been a tradition long before us. Since this patch is not only experimental but known to be faulty, there it should reside. Its not a drastic change.

In D66042#1626280, @Charusso wrote:

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

This feature is faulty, and its the responsibility of the reviewers not to let it through if it is so, at least not as-is. This is a bandaid to a problem we should fix, and shouldn't be a feature. I don't agree with this feature for the long term. While I have a firm stance on this, I might be wrong, this is why we need a discussion, and the problem of this code already being written could've been avoided if we had a round on the mailing list.

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.

Please find my mail in which I propose how to solve this problem. While I read all of your responses and I also greatly appreciate the discussion (we've mentioned plenty of other things not related to this patch that we should talk about even more), and did so with an open mind, my stance is largely unchanged.

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 :)

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. I find my concerns genuine, yet you chose to not to answer them at all. You still didn't say a word about my ultimate, 5 LOC change of making this a config. I have worked plenty on this part of the project, and while it doesn't make me a code owner, and doesn't even make my opinion necessarily correct, these things shouldn't be dismissed.

While I understand your frustration, especially since the deadline is nearing, having a friendly, constructive discussion where we respond to each other points would be more beneficial I think. My ideas have been proven wrong or impractical countless times, but there only is a chance for that if people respond to my points.

[alexfh]

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 sloooooow

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.

[NoQ]

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

[NoQ]

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.

[Szelethus]

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.

[Szelethus]

We might also want to change the revision title, but the commit message for sure, to make it clear that this affects all checkers, something along the lines of "[analyzer] Add an analyzer config to silence diagnostics from user specified checkers/packages"

[NoQ]

In D66042#1627777, @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.

[Charusso]

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.

[NoQ]

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.

[Szelethus]

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-builds 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<StringRef, 16> checkers;
  checkerList.split(checkers, ",");
  for (auto checker : checkers)
    Opts.CheckerSilenceVector.emplace_back(checker);
}

We should do this:

assert(Opts.CheckerSilenceVector.empty());
llvm::SmallVector<StringRef, 16> 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 changes tp scan-build would make it worse.

[NoQ]

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.

[alexfh]

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?

[Szelethus]

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 :)

[Charusso]

• Pink Fluffy Unicorns Dancing On Rainbows

[Charusso]

I realized that it is meaningless what separator we use to list the checkers, so I have picked ;. The , is limited to the analyzer-config list.

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

192–196

SilencedCheckersAndPackagesAsCommaSeparatedListSplittedToStringRefVector simplified to SilencedCheckers and CheckersControlList to Checkers.

[Szelethus]

Thanks!!! Please note that BugReporter.cpp (especially the parts you touched) just got refactored extensively, so you'll need to rebase on master.

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

192

Hmm, this doesn't really describe what this field does, does it? This isn't even the entire list, only those explicitly specified by -analyzer-checker and -analyzer-disable-checker. Besides, package names are also in this container. How about any of these:

• EnabledCheckersAndPackages
• EnabledCheckers
• CheckerAndPackageUserInput (this may just be the name that describes whats happening here the best)
• CheckerUserInput

195

Same here. I realize the name of the field could get ridiculously long, how about we leave this as-is but mention it in the comments that packages are also included?

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

1924

Could you please add a FIXME about that this will not work if the report was emitted with the incorrect tag?

[Szelethus]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

392

Hmm, maybe we could elaborate on what we mean under silencing.

Silenced checkers will remain enabled and work as usual during analysis, but bug reports originating from them are suppressed.

[Szelethus]

clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

468–502

Also, you can validate whether the user-provided checker/package names actually exist by modifying this function a bit.

Only if you feel like it, I don't insist :)

[Charusso]

• Rainbow Butterfly Unicorn Kitty

[Szelethus]

Now that I had ever more time to think about this patch, I see a great potential in it for development use, for example, we could silence a checker before splitting it up to see whether we could disable it altogether or really having to go through the process of splitting it into a modeling and reporting portion.

clang/include/clang/Basic/DiagnosticCommonKinds.td

303 ↗

(On Diff #215159)

Cheers!

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

167

This has been bugging me for a while. Registered checkers are checkers that were, well, registered into the analyzer, that may also include plugins. This should rather be called getBuiltinCheckers, because it only retrieves the list of checkers declared in Checkers.td.

This is just thinking aloud, not related to your revision.

177–178

I wonder why the decision was made to hide debug checkers for clang-tidy. It so happened once that I wanted to enable one through clang-tidy's interface, but couldn't.

But this isn't the time to change it, just again, thinking aloud.

clang/lib/Frontend/CompilerInvocation.cpp

483–504

The reason why I suggested validating this in CheckerRegistry is that CheckerRegistry is the only class knowing the actual list of checkers and packages, and is able to emit diagnostics before the analysis starts. This solution wouldn't work with plugin checkers/packages.

[Szelethus]

I swear this is my last objection :) As soon as this is settled, I'll accept.

clang/lib/Frontend/CompilerInvocation.cpp

483–504

I don't see this being addressed actually?

I think it would be totally fine to just omit the validation part as I said earlier, the patch will be leaner, and cases in which we're using the silencing of checkers are very exotic anyways.

[Szelethus]

clang/lib/Frontend/CompilerInvocation.cpp

483–504

Also, we should probably compliment such validation by actually writing tests for plugins.

I've been through that process once. It isn't fun. Really-really isn't :^) Let's just collectively agree to "forget" this :)

[Charusso]

clang/lib/Frontend/CompilerInvocation.cpp

483–504

Checker validation is in CheckerRegistry, configuration validation is in parseAnalyzerConfigs(). I have made a configuration, rather than a checker flag, so that I have not found more appropriate place and its does the job well. If it will be needed externally, I hope someone could do better.

[Szelethus]

clang/lib/Frontend/CompilerInvocation.cpp

483–504

Well isn't this checker validation?

[Szelethus]

You know what, I argued that we should use configs because this flag is incomplete. So I shouldn't be all up and down that it isn't.

LGTM!

[Szelethus]

clang/lib/Frontend/CompilerInvocation.cpp

483–504

In any case, could just throw in a FIXME before commiting please? :)

[NoQ]

Guys, thank you so much for working on establishing consensus on this change.
*bows*

[Charusso]

clang/lib/Frontend/CompilerInvocation.cpp

483–504

@NoQ, does the AnalyzerOptions::getRegisteredCheckers() contain the plugins?

[NoQ]

clang/lib/Frontend/CompilerInvocation.cpp

483–504

*/me doesn't know much about plugins*

Normally enable-disable flags do work on plugins, so plugin checkers must make it into the registry at some point.

[Charusso]

clang/lib/Frontend/CompilerInvocation.cpp

483–504

So do we need a FIXME or most likely it working with plugins?

[Szelethus]

clang/lib/Frontend/CompilerInvocation.cpp

483–504

Which doesn't happen here. CheckerRegistry is the only class supplied with this information.

A primitive way to demonstrate this is the following: AnalyzerOptions::getRegisteredCheckers() is a static function without parameters that doesn't use global variables, so it's a hair away from being constexpr as well. Plugins are an inherently runtime thing. A more concrete proof is that it only works with the inclusion of Checkers.inc, which is generated compile time by TableGen.

This was the primary reason behind us struggling really hard with checker dependencies and checker options, because we can't solve this problem with the use of TableGen only (relieving us of any runtime overhead). This is also the main reason behind my caution whenever this part of the project is touched -- we're in an okay spot now compared to when these systems got implemented, but we're quite a distance away from perfect.

You can read more about plugins and checker registration here, that I'll promise myself to finish after GSoC is wrapped up: D58065

[Szelethus]

clang/lib/Frontend/CompilerInvocation.cpp

483–504

I know for a fact that this wouldn't work with plugins, believe me :)

[NoQ]

clang/lib/Frontend/CompilerInvocation.cpp

483–504

Anyway, it's either "FIXME: This doesn't work with plugins" or "TODO: Does this work with plugins?".

[Charusso]

• Rebased.
• Added the remaining FIXME.

[Charusso]

Thanks for the reviews!

clang/lib/Frontend/CompilerInvocation.cpp

483–504

Okay, cool, thanks! I like the FIXME category:

// FIXME: Here we try to validate the silenced checkers or packages are valid.
// The current approach only validates the registered checkers which does not
// contain the runtime enabled checkers and optimally we would validate both.

[sylvestre.ledru]

@Charusso This probably should be added to the release notes:
https://clang.llvm.org/docs/ReleaseNotes.html#static-analyzer
and detailed in the doc.
Please let me know if you need help!

[Szelethus]

In D66042#1655119, @sylvestre.ledru wrote:

@Charusso This probably should be added to the release notes:
https://clang.llvm.org/docs/ReleaseNotes.html#static-analyzer
and detailed in the doc.
Please let me know if you need help!

For the time being, we are keeping this as a developer only feature, but we're working on potentially promoting it to a regular frontend flag!

clang-tools-extra/clang-tidy/ClangTidy.cpp

for (const auto &Opt : Opts.CheckOptions) {
     if (!OptName.startswith(AnalyzerPrefix))
       continue;
     AnalyzerOptions->Config[OptName.substr(AnalyzerPrefix.size())] = Opt.second;
   }
 }
 
 typedef std::vector<std::pair<std::string, bool>> CheckersList;
 
-static CheckersList getCheckersControlList(ClangTidyContext &Context,
-                                           bool IncludeExperimental) {
+static CheckersList getAnalyzerCheckers(ClangTidyContext &Context,
   CheckersList List;
 
   const auto &RegisteredCheckers =
       AnalyzerOptions::getRegisteredCheckers(IncludeExperimental);
   bool AnalyzerChecksEnabled = false;
   for (StringRef CheckName : RegisteredCheckers) {
     std::string ClangTidyCheckName((AnalyzerCheckNamePrefix + CheckName).str());
     AnalyzerChecksEnabled |= Context.isCheckEnabled(ClangTidyCheckName);
ClangTidyASTConsumerFactory::CreateASTConsumer(
   }
 
   std::vector<std::unique_ptr<ASTConsumer>> Consumers;
   if (!Checks.empty())
     Consumers.push_back(Finder->newASTConsumer());
 
 #if CLANG_ENABLE_STATIC_ANALYZER
   AnalyzerOptionsRef AnalyzerOptions = Compiler.getAnalyzerOpts();
-  AnalyzerOptions->CheckersControlList =
-      getCheckersControlList(Context, Context.canEnableAnalyzerAlphaCheckers());
-  if (!AnalyzerOptions->CheckersControlList.empty()) {
+  AnalyzerOptions->Checkers =
+      getAnalyzerCheckers(Context, Context.canEnableAnalyzerAlphaCheckers());
+  if (!AnalyzerOptions->Checkers.empty()) {
     setStaticAnalyzerCheckerOpts(Context.getOptions(), AnalyzerOptions);
     AnalyzerOptions->AnalysisStoreOpt = RegionStoreModel;
     AnalyzerOptions->AnalysisDiagOpt = PD_NONE;
     AnalyzerOptions->AnalyzeNestedBlocks = true;
     AnalyzerOptions->eagerlyAssumeBinOpBifurcation = true;
     std::unique_ptr<ento::AnalysisASTConsumer> AnalysisConsumer =
         ento::CreateAnalysisConsumer(Compiler);
     AnalysisConsumer->AddDiagnosticConsumer(

 std::vector<std::string> ClangTidyASTConsumerFactory::getCheckNames() {
   std::vector<std::string> CheckNames;
   for (const auto &CheckFactory : *CheckFactories) {
     if (Context.isCheckEnabled(CheckFactory.first))
       CheckNames.push_back(CheckFactory.first);
   }
 
 #if CLANG_ENABLE_STATIC_ANALYZER
-  for (const auto &AnalyzerCheck : getCheckersControlList(
-           Context, Context.canEnableAnalyzerAlphaCheckers()))
+  for (const auto &AnalyzerCheck :
+       getAnalyzerCheckers(Context, Context.canEnableAnalyzerAlphaCheckers()))
     CheckNames.push_back(AnalyzerCheckNamePrefix + AnalyzerCheck.first);
 #endif // CLANG_ENABLE_STATIC_ANALYZER
 
   std::sort(CheckNames.begin(), CheckNames.end());
   return CheckNames;
 }
 
 ClangTidyOptions::OptionMap ClangTidyASTConsumerFactory::getCheckOptions() {

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

 /// and should be eventually converted into -analyzer-config flags. New analyzer
 /// options should not be implemented as frontend flags. Frontend flags still
 /// make sense for things that do not affect the actual analysis.
 class AnalyzerOptions : public RefCountedBase<AnalyzerOptions> {
 public:
   using ConfigTable = llvm::StringMap<std::string>;
 
   static std::vector<StringRef>
   getRegisteredCheckers(bool IncludeExperimental = false);
 
   /// Convenience function for printing options or checkers and their
   /// description in a formatted manner. If \p MinLineWidth is set to 0, no line
   /// breaks are introduced for the description.
   ///
   /// Format, depending whether the option name's length is less then
   /// \p OptionWidth:
   ///
   ///   <padding>EntryName<padding>Description
   ///   <---------padding--------->Description
   ///   <---------padding--------->Description
   ///
   ///   <padding>VeryVeryLongOptionName
   ///   <---------padding--------->Description
   ///   <---------padding--------->Description
   ///   ^~~~~~~~ InitialPad
   ///   ^~~~~~~~~~~~~~~~~~~~~~~~~~ EntryWidth
   ///   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~MinLineWidth
   static void printFormattedEntry(
       llvm::raw_ostream &Out,
       std::pair<StringRef, StringRef> EntryDescPair,
       size_t EntryWidth, size_t InitialPad, size_t MinLineWidth = 0);
 
-
   /// Pair of checker name and enable/disable.
-  std::vector<std::pair<std::string, bool>> CheckersControlList;
+  std::vector<std::pair<std::string, bool>> Checkers;
+
+  /// Vector of checker names which will not emit warnings.
+  std::vector<std::string> SilencedCheckers;
 
   /// A key-value table of use-specified configuration values.
   // TODO: This shouldn't be public.
   ConfigTable Config;
   AnalysisStores AnalysisStoreOpt = RegionStoreModel;
   AnalysisConstraints AnalysisConstraintsOpt = RangeConstraintsModel;
   AnalysisDiagClients AnalysisDiagOpt = PD_HTML;
   AnalysisPurgeMode AnalysisPurgeOpt = PurgeStmt;
 

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

ANALYZER_OPTION(
     "")
 
 ANALYZER_OPTION(
     StringRef, CXXMemberInliningMode, "c++-inlining",
     "Controls which C++ member functions will be considered for inlining. "
     "Value: \"constructors\", \"destructors\", \"methods\".",
     "destructors")
 
-ANALYZER_OPTION_DEPENDS_ON_USER_MODE(
-    StringRef, IPAMode, "ipa",
-    "Controls the mode of inter-procedural analysis. Value: \"none\", "
-    "\"basic-inlining\", \"inlining\", \"dynamic\", \"dynamic-bifurcate\".",
-    /* SHALLOW_VAL */ "inlining", /* DEEP_VAL */ "dynamic-bifurcate")
-
 ANALYZER_OPTION(
     StringRef, ExplorationStrategy, "exploration_strategy",
     "Value: \"dfs\", \"bfs\", \"unexplored_first\", "
     "\"unexplored_first_queue\", \"unexplored_first_location_queue\", "
     "\"bfs_block_dfs_contents\".",
     "unexplored_first_queue")
 
+ANALYZER_OPTION(
+    StringRef, RawSilencedCheckers, "silence-checkers",
+    "A semicolon separated list of checker and package names to silence.", "")
+
+ANALYZER_OPTION_DEPENDS_ON_USER_MODE(
+    StringRef, IPAMode, "ipa",
+    "Controls the mode of inter-procedural analysis. Value: \"none\", "
+    "\"basic-inlining\", \"inlining\", \"dynamic\", \"dynamic-bifurcate\".",
+    /* SHALLOW_VAL */ "inlining", /* DEEP_VAL */ "dynamic-bifurcate")
+
 #undef ANALYZER_OPTION_DEPENDS_ON_USER_MODE
 #undef ANALYZER_OPTION

clang/lib/Frontend/CompilerInvocation.cpp

#include "clang/StaticAnalyzer/Core/Analyses.def"
   Opts.TrimGraph = Args.hasArg(OPT_trim_egraph);
   Opts.maxBlockVisitOnPath =
       getLastArgIntValue(Args, OPT_analyzer_max_loop, 4, Diags);
   Opts.PrintStats = Args.hasArg(OPT_analyzer_stats);
   Opts.InlineMaxStackDepth =
       getLastArgIntValue(Args, OPT_analyzer_inline_max_stack_depth,
                          Opts.InlineMaxStackDepth, Diags);
 
-  Opts.CheckersControlList.clear();
+  Opts.Checkers.clear();
   for (const Arg *A :
        Args.filtered(OPT_analyzer_checker, OPT_analyzer_disable_checker)) {
     A->claim();
-    bool enable = (A->getOption().getID() == OPT_analyzer_checker);
+    bool IsEnabled = A->getOption().getID() == OPT_analyzer_checker;
     // We can have a list of comma separated checker names, e.g:
     // '-analyzer-checker=cocoa,unix'
-    StringRef checkerList = A->getValue();
-    SmallVector<StringRef, 4> checkers;
-    checkerList.split(checkers, ",");
-    for (auto checker : checkers)
-      Opts.CheckersControlList.emplace_back(checker, enable);
+    StringRef CheckerList = A->getValue();
+    SmallVector<StringRef, 16> Checkers;
+    CheckerList.split(Checkers, ",");
+    for (const StringRef Checker : Checkers)
+      Opts.Checkers.emplace_back(Checker, IsEnabled);
   }
 
   // Go through the analyzer configuration options.
   for (const auto *A : Args.filtered(OPT_analyzer_config)) {
 
     // We can have a list of comma separated config names, e.g:
     // '-analyzer-config key1=val1,key2=val2'
     StringRef configList = A->getValue();
#undef ANALYZER_OPTION_DEPENDS_ON_USER_MODE
   if (!AnOpts.CTUDir.empty() && !llvm::sys::fs::is_directory(AnOpts.CTUDir))
     Diags->Report(diag::err_analyzer_config_invalid_input) << "ctu-dir"
                                                            << "a filename";
 
   if (!AnOpts.ModelPath.empty() &&
       !llvm::sys::fs::is_directory(AnOpts.ModelPath))
     Diags->Report(diag::err_analyzer_config_invalid_input) << "model-path"
                                                            << "a filename";
+
+  if (!AnOpts.RawSilencedCheckers.empty()) {
+    SmallVector<StringRef, 16> Checkers;
+    AnOpts.RawSilencedCheckers.split(Checkers, ";");
+    for (const StringRef Checker : Checkers)
+      AnOpts.SilencedCheckers.emplace_back(Checker);
+  }
 }
 
 static bool ParseMigratorArgs(MigratorOptions &Opts, ArgList &Args) {
   Opts.NoNSAllocReallocError = Args.hasArg(OPT_migrator_no_nsalloc_error);
   Opts.NoFinalizeRemoval = Args.hasArg(OPT_migrator_no_finalize_removal);
   return true;
 }
 
 static void ParseCommentArgs(CommentOptions &Opts, ArgList &Args) {
   Opts.BlockCommandNames = Args.getAllArgValues(OPT_fcomment_block_commands);
   Opts.ParseAllComments = Args.hasArg(OPT_fparse_all_comments);
 }
 
 static StringRef getCodeModel(ArgList &Args, DiagnosticsEngine &Diags) {
   if (Arg *A = Args.getLastArg(OPT_mcode_model)) {
     StringRef Value = A->getValue();
     if (Value == "small" || Value == "kernel" || Value == "medium" ||
         Value == "large" || Value == "tiny")
       return Value;
     Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Value;
   }
   return "default";
 }
 

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

 /// edges.
 /// Otherwise, more detailed diagnostics is emitted for block edges, explaining
 /// the transitions in words.
 static std::unique_ptr<PathDiagnostic> generatePathDiagnosticForConsumer(
     PathDiagnosticConsumer::PathGenerationScheme ActiveScheme,
     PathDiagnosticBuilder &PDB,
     const ExplodedNode *ErrorNode,
     const VisitorsDiagnosticsTy &VisitorsDiagnostics) {
+  BugReport *R = PDB.getBugReport();
+  AnalyzerOptions &Opts = PDB.getBugReporter().getAnalyzerOptions();
+  StringRef ErrorTag = ErrorNode->getLocation().getTag()->getTagDescription();
+
+  // See whether we need to silence the checker.
+  for (const std::string &CheckerName : Opts.SilencedCheckers) {
+    if (ErrorTag.startswith(CheckerName))
+      return nullptr;
+  }
 
   bool GenerateDiagnostics = (ActiveScheme != PathDiagnosticConsumer::None);
   bool AddPathEdges = (ActiveScheme == PathDiagnosticConsumer::Extensive);
   SourceManager &SM = PDB.getSourceManager();
-  BugReport *R = PDB.getBugReport();
-  AnalyzerOptions &Opts = PDB.getBugReporter().getAnalyzerOptions();
   StackDiagVector CallStack;
   InterestingExprs IE;
   LocationContextMap LCM;
   std::unique_ptr<PathDiagnostic> PD = generateEmptyDiagnosticForReport(R, SM);
 
   if (GenerateDiagnostics) {
     auto EndNotes = VisitorsDiagnostics.find(ErrorNode);
     std::shared_ptr<PathDiagnosticPiece> LastPiece;
GRBugReporter::generatePathDiagnostics(
   auto ReportInfo = findValidReport(TrimG, ErrorGraph, bugReports,
                   getAnalyzerOptions(), *this);
   BugReport *R = ReportInfo.first;
 
   if (R && R->isValid()) {
     const ExplodedNode *ErrorNode = ErrorGraph.ErrorNode;
     for (PathDiagnosticConsumer *PC : consumers) {
       PathDiagnosticBuilder PDB(*this, R, ErrorGraph.BackMap, PC);
-      std::unique_ptr<PathDiagnostic> PD = generatePathDiagnosticForConsumer(
-          PC->getGenerationScheme(), PDB, ErrorNode, *ReportInfo.second);
+      if (std::unique_ptr<PathDiagnostic> PD =
+              generatePathDiagnosticForConsumer(PC->getGenerationScheme(), PDB,
+                                                ErrorNode,
+                                                *ReportInfo.second)) {
         (*Out)[PC] = std::move(PD);
       }
     }
+  }
 
   return Out;
 }
 
 void BugReporter::Register(const BugType *BT) {
   BugTypes = F.add(BugTypes, BT);
 }
 

clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

 #undef PACKAGE_OPTION
 #undef GET_PACKAGE_OPTIONS
 
   resolveDependencies();
   resolveCheckerAndPackageOptions();
 
   // Parse '-analyzer-checker' and '-analyzer-disable-checker' options from the
   // command line.
-  for (const std::pair<std::string, bool> &Opt : AnOpts.CheckersControlList) {
+  for (const std::pair<std::string, bool> &Opt : AnOpts.Checkers) {
     CheckerInfoListRange CheckerForCmdLineArg =
         getMutableCheckersForCmdLineArg(Opt.first);
 
     if (CheckerForCmdLineArg.begin() == CheckerForCmdLineArg.end()) {
       Diags.Report(diag::err_unknown_analyzer_checker) << Opt.first;
       Diags.Report(diag::note_suggest_disabling_all_checkers);
     }
 
isOptionContainedIn(const CheckerRegistry::CmdLineOptionList &OptionList,
 
   if (OptionIt == OptionList.end()) {
     Diags.Report(diag::err_analyzer_checker_option_unknown)
         << SuppliedChecker << SuppliedOption;
     return;
   }
 }
 
 void CheckerRegistry::validateCheckerOptions() const {
   for (const auto &Config : AnOpts.Config) {
 
     StringRef SuppliedChecker;
     StringRef SuppliedOption;
     std::tie(SuppliedChecker, SuppliedOption) = Config.getKey().split(':');
 
     if (SuppliedOption.empty())
       continue;
 
     // AnalyzerOptions' config table contains the user input, so an entry could
     // look like this:
     //
     //   cor:NoFalsePositives=true
     //
     // Since lower_bound would look for the first element *not less* than "cor",
     // it would return with an iterator to the first checker in the core, so we
     // we really have to use find here, which uses operator==.
     auto CheckerIt = llvm::find(Checkers, CheckerInfo(SuppliedChecker));
     if (CheckerIt != Checkers.end()) {
       isOptionContainedIn(CheckerIt->CmdLineOptions, SuppliedChecker,
                           SuppliedOption, AnOpts, Diags);
       continue;
     }
 
     auto PackageIt = llvm::find(Packages, PackageInfo(SuppliedChecker));
     if (PackageIt != Packages.end()) {
       isOptionContainedIn(PackageIt->CmdLineOptions, SuppliedChecker,
                           SuppliedOption, AnOpts, Diags);
       continue;
     }
 
     Diags.Report(diag::err_unknown_analyzer_checker) << SuppliedChecker;
   }
 }
 
 void CheckerRegistry::printCheckerWithDescList(raw_ostream &Out,
                                                size_t MaxNameChars) const {
   // FIXME: Print available packages.
 
   Out << "CHECKERS:\n";
 
   // Find the maximum option length.

clang/test/Analysis/analyzer-config.c

 // CHECK-NEXT: optin.performance.Padding:AllowedPad = 24
 // CHECK-NEXT: osx.NumberObjectConversion:Pedantic = false
 // CHECK-NEXT: osx.cocoa.RetainCount:CheckOSObject = true
 // CHECK-NEXT: osx.cocoa.RetainCount:TrackNSCFStartParam = false
 // CHECK-NEXT: prune-paths = true
 // CHECK-NEXT: region-store-small-struct-limit = 2
 // CHECK-NEXT: report-in-main-source-file = false
 // CHECK-NEXT: serialize-stats = false
+// CHECK-NEXT: silence-checkers = ""
 // CHECK-NEXT: stable-report-filename = false
 // CHECK-NEXT: suppress-c++-stdlib = true
 // CHECK-NEXT: suppress-inlined-defensive-checks = true
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: track-conditions = false
 // CHECK-NEXT: track-conditions-debug = false
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 89
+// CHECK-NEXT: num-entries = 90

clang/test/Analysis/silence-checker-core-all.cpp

+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-config silence-checkers=core \
+// RUN:  -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-config silence-checkers="core.DivideZero;core.NullDeref" \
+// 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'
+}
+
+void test_disable_null_deref(int *p) {
+  if (p)
+    return;
+
+  int x = p[0];
+  // no-warning: Array access (from variable 'p') results in a null pointer dereference
+}

clang/test/Analysis/silence-checker-core-div-by-zero.cpp

+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-config silence-checkers=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'
+}
+
+void test_disable_null_deref(int *p) {
+  if (p)
+    return;
+
+  int x = p[0];
+  // expected-warning@-1 {{Array access (from variable 'p') results in a null pointer dereference}}
+}

clang/tools/scan-build/bin/scan-build

my %Options = (
   IgnoreErrors => 0,         # Ignore build errors.
   KeepCC => 0,               # Do not override CC and CXX make variables
   ViewResults => 0,          # View results when the build terminates.
   ExitStatusFoundBugs => 0,  # Exit status reflects whether bugs were found
   ShowDescription => 0,      # Display the description of the defect in the list
   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.
   AnalyzerTarget => undef,
   StoreModel => undef,
   ConstraintsModel => undef,
   InternalStats => undef,
   OutputFormat => "html",
if ($arg eq "-enable-checker") {
       $Options{EnableCheckers}{$Checker} = $NumArgs;
       delete $Options{DisableCheckers}{$Checker};
       next;
     }
 
     if ($arg eq "-disable-checker") {
       shift @$Args;
       my $Checker = shift @$Args;
-      # Store $NumArgs to preserve the order the checkers were disabled.
+      # Store $NumArgs to preserve the order the checkers are disabled/silenced.
+      # 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;
     }
 
     if ($arg eq "--exclude") {
       shift @$Args;
       my $arg = shift @$Args;
       # Remove the trailing slash if any
       $arg =~ s|/$||;
foreach (sort { $Options{EnableCheckers}{$a} <=> $Options{EnableCheckers}{$b} }
   # Push checkers in order they were enabled.
   push @AnalysesToRun, "-analyzer-checker", $_;
 }
 foreach (sort { $Options{DisableCheckers}{$a} <=> $Options{DisableCheckers}{$b} }
          keys %{$Options{DisableCheckers}}) {
   # Push checkers in order they were disabled.
   push @AnalysesToRun, "-analyzer-disable-checker", $_;
 }
+foreach (sort { $Options{SilenceCheckers}{$a} <=> $Options{SilenceCheckers}{$b} }
+         keys %{$Options{SilenceCheckers}}) {
+  # Push checkers in order they were silenced.
+  push @AnalysesToRun, "-analyzer-config 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}"; }
 
 # Delay setting up other environment variables in case we can do true
 # interposition.
 my $CCC_ANALYZER_ANALYSIS = join ' ', @AnalysesToRun;
 my $CCC_ANALYZER_PLUGINS = join ' ', map { "-load ".$_ } @{$Options{PluginsToLoad}};

clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

 public:
   TestAction(llvm::raw_ostream &DiagsOutput) : DiagsOutput(DiagsOutput) {}
 
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
                                                  StringRef File) override {
     std::unique_ptr<AnalysisASTConsumer> AnalysisConsumer =
         CreateAnalysisConsumer(Compiler);
     AnalysisConsumer->AddDiagnosticConsumer(new DiagConsumer(DiagsOutput));
-    Compiler.getAnalyzerOpts()->CheckersControlList = {
-        {"custom.CustomChecker", true}};
+    Compiler.getAnalyzerOpts()->Checkers = {{"custom.CustomChecker", true}};
     AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
       Registry.addChecker<CheckerT>("custom.CustomChecker", "Description", "");
     });
     return std::move(AnalysisConsumer);
   }
 };
 
 template <typename CheckerT>