Page MenuHomePhabricator

[analyzer] add-new-checker.py: Introduction
Needs ReviewPublic

Authored by Charusso on Jan 27 2020, 6:48 PM.

Details

Reviewers
NoQ
Summary

This script could be used to generate a dummy Static Analyzer checker.
Example usage:
python add-new-checker.py alpha.package.Foo

It also helps to document checker-writing.

Diff Detail

Event Timeline

Charusso created this revision.Jan 27 2020, 6:48 PM
Charusso updated this revision to Diff 240759.Jan 27 2020, 7:37 PM
  • Make it runable. Whoops.
Charusso marked an inline comment as done.EditedJan 27 2020, 7:54 PM

This script dumps out a dummy checker which is the continuation of the Clang Tidy's https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/add_new_check.py

However, we could stick to the main-function-redeclaration-checker in a 2020 fashion. As I see peoples starting their careers with Tidy, that is why this checker born, but please feel free to swap or modify. I think that is the easiest way to document changes in requirements of checker-writing. I also wanted to document common mistakes, like how to use a Twine, what is a dyn_cast_or_null<> vs. cast<>, make sure the SVal type is written out and it is not const or const*.

NoQ added a comment.Mar 9 2020, 12:58 PM

I didn't use it yet but i like it. The code is easy to understand, so i'll be happy to maintain it as it lands.

The example checker is nice and i like how you made sure it demonstrates modern API usage. I'm a bit worried that it may quickly get out of date as we update our APIs, because this happens like ~multiple times a year, much more often than in clang-tidy. Is there a way to write a test that tests that the generated checker compiles? I don't have any specific ideas but it would have been awesome.

clang/utils/analyzer/add-new-checker.py
80–82

llvm-tblgen needs to be in the PATH, right? What if LLVM isn't installed on the host system or has the wrong version?

We might have to either tell the user to specify it explicitly in the invocation (and then update the LIT substitution to explicitly use the right binary), or, ideally, try to get the script installed into the build directory so that it could find the freshly built llvm-tblgen binary relative to itself.

574

I think we should say explicitly that these are examples, not required code, because a lot of this may look like magic to a beginner.

I.e., "Here's how you define a state trait", etc.

Charusso updated this revision to Diff 250020.Mar 12 2020, 12:28 PM
Charusso marked 3 inline comments as done.
Charusso retitled this revision from [analyzer][WIP] add-new-checker.py: Introduction to [analyzer] add-new-checker.py: Introduction.
  • Try to invoke TableGen, if that fails the user need to specify the path to it.
  • The script actually creates a real world (hidden) checker.
    • This checker always made with the build invocation.
    • Its test file always made with the build invocation.
    • Everything else remain as is.
  • (calculated: DummyChecker.cpp (100 lines))
Charusso marked an inline comment as done.Mar 12 2020, 12:31 PM
Charusso added inline comments.
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
24 ↗(On Diff #250020)

The script is allergic to human issues. The followup will be to normalize the necessary files.

clang/utils/analyzer/add-new-checker.py
80–82

Hm, I am not a magician, so my assumption one has the llvm/clang therefore the llvm-tblgen available. If not, it warns you and you need to specify. May if you want to make sure it "just works" please adjust it later.

NoQ added a comment.Mar 15 2020, 8:22 PM
  • The script actually creates a real world (hidden) checker.
    • This checker always made with the build invocation.
    • Its test file always made with the build invocation.

Mm, ok, but this checker is checked in into the repo and therefore can diverge from whatever the script generates, right? Or is each test run updating the repo? Can we simply make the script do cat DummyChecker.cpp | sed s/DummyChecker/$NAME/g > $NAME.cpp and store the checker only once?

clang/utils/analyzer/add-new-checker.py
84

Let's tell the user politely how to specify llvm-tblgen.

715–716

So, --overwrite and add help?

Charusso updated this revision to Diff 253777.Mar 30 2020, 8:43 PM
Charusso marked 3 inline comments as done.
  • Remove the test of creating a live checker, instead copy over the live checker when the script runs.
  • Simplify the script by adding the new package to the end of the file.
  • In case of the checkers.rst a non-alpha package is going to be added before the alpha packages.
  • According to this change simplify the tests.
  • DummyChecker -> ExampleChecker.
In D73521#1923693, @NoQ wrote:

Or is each test run updating the repo? Can we simply make the script do cat DummyChecker.cpp | sed s/DummyChecker/$NAME/g > $NAME.cpp and store the checker only once?

It was updating, yes. The first idea was to copy over the live checker, but I wanted to make it as portable as possible.

clang/utils/analyzer/add-new-checker.py
715–716

It was only made for the test. Removed.

Will this utility affect Visual Studio builds?

Charusso added a comment.EditedMar 31 2020, 4:47 AM

Will this utility affect Visual Studio builds?

The community owns like 50 build-bots, so it should affect all of them by default. I am hoping it will just work.