Page MenuHomePhabricator

[analyzer] Fix some checker's output plist not containing the checker name
AbandonedPublic

Authored by xazax.hun on Sep 4 2017, 7:15 AM.

Details

Reviewers
NoQ
dcoughlin
Summary

Unfortunately, currently, the analyzer core sets the checker name after the constructor was already run. So if we set the BugType in the constructor, the output plist will not contain a checker name. Right now the idiomatic solution is to create the BugType lazily. This patch updates the some checkers to follow this idiom. I also found some bugs, when some checkers were triggered even if they were not enabled or wrong checker name is emitted to the plist.

In the future probably it would be better to alter the signature of the checkers' constructor to set the name in the constructor so it is possible to create the BugType eagerly.

Diff Detail

Event Timeline

xazax.hun created this revision.Sep 4 2017, 7:15 AM
xazax.hun edited the summary of this revision. (Show Details)Sep 4 2017, 7:16 AM
NoQ edited edge metadata.Sep 4 2017, 7:42 AM

Cool. Thanks!

In the future probably it would be better to alter the signature of the checkers' constructor to set the name in the constructor so it is possible to create the BugType eagerly.

Still, should we add an assertion so that we could be sure that every bug type contains a checker name?

xazax.hun updated this revision to Diff 113837.Sep 5 2017, 5:12 AM
xazax.hun retitled this revision from [analyzer] Fix SimpleStreamChecker's output plist not containing the checker name to [analyzer] Fix some checker's output plist not containing the checker name.
xazax.hun edited the summary of this revision. (Show Details)
  • Fix more checkers
  • Minor style fixes along the way
  • Added an assert to prevent such errors in the future
In D37437#860311, @NoQ wrote:

Cool. Thanks!

In the future probably it would be better to alter the signature of the checkers' constructor to set the name in the constructor so it is possible to create the BugType eagerly.

Still, should we add an assertion so that we could be sure that every bug type contains a checker name?

Sure! I added the assert and discovered lots of other checks that had the same problem. I also discovered some other bugs, e.g.: when a checker was emitting diagnostic even without being turned on or the wrong name is emitted to the plist.

test/Analysis/malloc.c
1723

This test is deleted because the corresponding checker is not even turned on for this file and this warning should not be emitted at all. Emitting this warning with the settings above was a bug.

Looks like the need to have the checker name in BugType along with the checker names not being initialized early enough, leads to worse checker-writer experience. Is there a way to ensure that the checker names are set at construction?

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
131–137

nit: I prefer to use '{' here since the if body spans multiple lines.

lib/StaticAnalyzer/Checkers/CStringChecker.cpp
308

This seems to be related to the change in the test, correct? In the future, please, split changes like this one into their own separate commits.

xazax.hun updated this revision to Diff 119141.Oct 16 2017, 5:40 AM
xazax.hun marked 2 inline comments as done.
  • Address review comments.
xazax.hun added inline comments.Oct 16 2017, 5:40 AM
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
308

Yes. I will do so in the future, thanks.

Just to be clear, since this leads to regression to the checker API, I am asking to look into other ways of solving this problem. For example, is there a way to ensure that the checker names are set at construction?

Just to be clear, since this leads to regression to the checker API, I am asking to look into other ways of solving this problem. For example, is there a way to ensure that the checker names are set at construction?

Checkers are instantiated here: https://github.com/llvm-mirror/clang/blob/master/include/clang/StaticAnalyzer/Core/CheckerManager.h#L142
Checkers are created using the default constructor. After the constructor was run we set the name. If we want the checkers to be able to create the BugType with the correct name in the constructor (and not using mutable fields for the BugTypes), we need to add a new constructor parameter to CheckerBase and all the Checkers will need to forward a CheckerName to their base classes. So we need to break the API for every checkers basically. But if you prefer that solution I can rewrite this patch.

@dcoughlin do you have some input on this?

In case you do not like this solution, I uploaded an alternative approach: https://reviews.llvm.org/D41538

xazax.hun abandoned this revision.Jan 6 2018, 2:52 AM

https://reviews.llvm.org/D41538 is superior and committed.