зеркало из https://github.com/mozilla/gecko-dev.git
Bug 1717076: Add documentation about advanced checker techniques r=andi
Differential Revision: https://phabricator.services.mozilla.com/D118211
This commit is contained in:
Tom Ritter
Родитель
537031b360
Коммит
1b8cf0af95
|
|
@ -50,20 +50,38 @@ The gist of converting your matcher is to take the following pseudo-code and pas
|
|||
this);
|
||||
|
||||
|
||||
It honest to god is usually that easy. Here's a working example where I pasted in straight from Compiler Explorer and then ran through clang-format:
|
||||
It honest to god is usually that easy. Here's a working example where I pasted in straight from Compiler Explorer:
|
||||
|
||||
::
|
||||
|
||||
AstMatcher->addMatcher(
|
||||
traverse(TK_IgnoreUnlessSpelledInSource,
|
||||
ifStmt(allOf(has(binaryOperator(has(declRefExpr(hasType(enumDecl()))))),
|
||||
hasElse(ifStmt(allOf(unless(hasElse(anything())),
|
||||
has(binaryOperator(has(declRefExpr(
|
||||
hasType(enumDecl()))))))))))
|
||||
traverse(TK_IgnoreUnlessSpelledInSource,
|
||||
ifStmt(allOf(
|
||||
has(
|
||||
binaryOperator(
|
||||
has(
|
||||
declRefExpr(hasType(enumDecl().bind("enum")))
|
||||
)
|
||||
)
|
||||
),
|
||||
hasElse(
|
||||
ifStmt(allOf(
|
||||
unless(hasElse(anything())),
|
||||
has(
|
||||
binaryOperator(
|
||||
has(
|
||||
declRefExpr(hasType(enumDecl()))
|
||||
)
|
||||
)
|
||||
)
|
||||
))
|
||||
)
|
||||
))
|
||||
.bind("node")),
|
||||
this);
|
||||
|
||||
|
||||
|
||||
If for some reason you're not using the ``IgnoreUnlessSpelledInSource`` Traversal Mode, remove the call to traverse and the corresponding closing paren. (Also, if you're comparing this code to existing source code, know that because this traversal mode is a new clang feature, most historical clang checks do not use it.)
|
||||
|
||||
Wiring up Warnings and Errors
|
||||
|
|
|
|||
|
|
@ -0,0 +1,148 @@
|
|||
.. _advanced_check_features:
|
||||
|
||||
Advanced Check Features
|
||||
=======================
|
||||
|
||||
This page covers additional ways to improve and extend the check you've added to build/clang-plugin.
|
||||
|
||||
Adding Tests
|
||||
------------
|
||||
|
||||
No doubt you've seen the tests for existing checks in `build/clang-plugin/tests <https://searchfox.org/mozilla-central/source/build/clang-plugin/tests>`_. Adding tests is straightforward; and your reviewer should insist you do so. Simply copying the existing format of any test and how diagnostics are marked as expected.
|
||||
|
||||
One wrinkle - all clang plugin checks are applied to all tests. We try to write tests so that only one check applies to it. If you write a check that triggers on an existing test, try to fix the existing test slightly so the new check does not trigger on it.
|
||||
|
||||
Using Bind To Output More Useful Information
|
||||
--------------------------------------------
|
||||
|
||||
You've probably been wondering what the heck ``.bind()`` is for. You've been seeing it all over the place but never has it actually been explained what it's for and when to use it.
|
||||
|
||||
``.bind()`` is used to give a name to part of the AST discovered through your matcher, so you can use it later. Let's go back to our sample matcher:
|
||||
|
||||
::
|
||||
|
||||
AstMatcher->addMatcher(
|
||||
traverse(TK_IgnoreUnlessSpelledInSource,
|
||||
ifStmt(allOf(
|
||||
has(
|
||||
binaryOperator(
|
||||
has(
|
||||
declRefExpr(hasType(enumDecl()))
|
||||
)
|
||||
)
|
||||
),
|
||||
hasElse(
|
||||
ifStmt(allOf(
|
||||
unless(hasElse(anything())),
|
||||
has(
|
||||
binaryOperator(
|
||||
has(
|
||||
declRefExpr(hasType(enumDecl()))
|
||||
)
|
||||
)
|
||||
)
|
||||
))
|
||||
)
|
||||
))
|
||||
.bind("node")),
|
||||
this);
|
||||
|
||||
Now the ``.bind("node")`` makes more sense. We're naming the If statement we matched, so we can refer to it later when we call ``Result.Nodes.getNodeAs<IfStmt>("node")``.
|
||||
|
||||
Let's say we want to provide the *type* of the enum in our warning message. There are two enums we end up seeing in our matcher - the enum in the first if statement, and the enum in the second. We're going to arbitrarily pick the first and give it the name ``enumType``:
|
||||
|
||||
::
|
||||
|
||||
AstMatcher->addMatcher(
|
||||
traverse(TK_IgnoreUnlessSpelledInSource,
|
||||
ifStmt(allOf(
|
||||
has(
|
||||
binaryOperator(
|
||||
has(
|
||||
declRefExpr(hasType(enumDecl().bind("enumType")))
|
||||
)
|
||||
)
|
||||
),
|
||||
hasElse(
|
||||
ifStmt(allOf(
|
||||
unless(hasElse(anything())),
|
||||
has(
|
||||
binaryOperator(
|
||||
has(
|
||||
declRefExpr(hasType(enumDecl()))
|
||||
)
|
||||
)
|
||||
)
|
||||
))
|
||||
)
|
||||
))
|
||||
.bind("node")),
|
||||
this);
|
||||
|
||||
And in our check() function, we can use it like so:
|
||||
|
||||
::
|
||||
|
||||
void MissingElseInEnumComparisons::check(
|
||||
const MatchFinder::MatchResult &Result) {
|
||||
const auto *MatchedDecl = Result.Nodes.getNodeAs<IfStmt>("node");
|
||||
const auto *EnumType = Result.Nodes.getNodeAs<EnumDecl>("enumType");
|
||||
|
||||
diag(MatchedDecl->getIfLoc(),
|
||||
"Enum comparisons to %0 in an if/else if block without a trailing else.",
|
||||
DiagnosticIDs::Warning) << EnumType->getName();
|
||||
}
|
||||
|
||||
Repeated matcher calls
|
||||
--------------------------
|
||||
|
||||
If you find yourself repeating the same several matchers in several spots, you can turn it into a variable to use.
|
||||
|
||||
::
|
||||
|
||||
auto isTemporaryLifetimeBoundCall =
|
||||
cxxMemberCallExpr(
|
||||
onImplicitObjectArgument(anyOf(has(cxxTemporaryObjectExpr()),
|
||||
has(materializeTemporaryExpr()))),
|
||||
callee(functionDecl(isMozTemporaryLifetimeBound())));
|
||||
|
||||
auto hasTemporaryLifetimeBoundCall =
|
||||
anyOf(isTemporaryLifetimeBoundCall,
|
||||
conditionalOperator(
|
||||
anyOf(hasFalseExpression(isTemporaryLifetimeBoundCall),
|
||||
hasTrueExpression(isTemporaryLifetimeBoundCall))));
|
||||
|
||||
The above example is parameter-less, but if you need to supply a parameter that changes, you can turn it into a lambda:
|
||||
|
||||
::
|
||||
|
||||
auto hasConstCharPtrParam = [](const unsigned int Position) {
|
||||
return hasParameter(
|
||||
Position, hasType(hasCanonicalType(pointsTo(asString("const char")))));
|
||||
};
|
||||
|
||||
auto hasParamOfType = [](const unsigned int Position, const char *Name) {
|
||||
return hasParameter(Position, hasType(asString(Name)));
|
||||
};
|
||||
|
||||
auto hasIntegerParam = [](const unsigned int Position) {
|
||||
return hasParameter(Position, hasType(isInteger()));
|
||||
};
|
||||
|
||||
AstMatcher->addMatcher(
|
||||
callExpr(
|
||||
hasName("fopen"),
|
||||
hasConstCharPtrParam(0))
|
||||
.bind("funcCall"),
|
||||
this);
|
||||
|
||||
|
||||
Allow-listing existing callsites
|
||||
--------------------------------
|
||||
|
||||
While it's not a great situation, you can set up an allow-list of existing callsites if you need to. A simple allow-list is demonstrated in `NoGetPrincipalURI <https://hg.mozilla.org/mozilla-central/rev/fb60b22ee6616521b386d90aec07b03b77905f4e>`_. The `NoNewThreadsChecker <https://hg.mozilla.org/mozilla-central/rev/f400f164b3947b4dd54089a36ea31cca2d72805b>`_ is an example of a more sophisticated way of setting up a larger allow-list.
|
||||
|
||||
|
||||
Custom Annotations
|
||||
------------------
|
||||
It's possible to create custom annotations that will be a no-op when compiled, but can be used by a static analysis check. These can be used to annotate special types of sources and sinks (for example). We have some examples of this in-tree presently (such as ``MOZ_CAN_RUN_SCRIPT``) but currently don't have a detailed walkthrough in this documentation of how to set these up and use them. (Patches welcome.)
|
||||
|
|
@ -11,4 +11,5 @@ or seriously develop one we can land and run internally. While being written fo
|
|||
compiler-explorer.rst
|
||||
writing-matchers.rst
|
||||
adding-a-check.rst
|
||||
advanced-check-features.rst
|
||||
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче