[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

[Tom Roeder via Phabricator via cfe-commits]

tmroeder marked 5 inline comments as done.
tmroeder added a comment.

Thanks to everyone for the comments. I've answered them as best I can, and I'm definitely open to changes or to scrapping this entirely.

I should have prefixed this patch with a discussion on the main list, perhaps. My main use case for this clang-tidy module is twofold: find problems in existing kernel code and checking incoming patches to (some of the) kernel mailing lists. I don't expect all (or even most) kernel developers to use this, just like I don't think most kernel developers use existing checkers (sparse and smatch) that have to be explicitly enabled by build-time options in Kbuild.

You might ask why I would want to implement these checks in clang-tidy at all if there are already static-analysis tools like sparse and smatch, though I expect that this list is generally in favor of expanding the scope of clang-tidy. The answer is that I think that having these checks directly in the compiler is the right way to go; clang-tidy (and clang-check, where I also would like to add some static analysis for the kernel) provide a principled basis for checking C code rather than using custom C parsers for the kernel. And I really like the AST matcher language and think it provides a powerful tool for writing these checks.



================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:12
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <c++/8/bits/c++config.h>
+
----------------
lebedev.ri wrote:
> This looks wrong
Yeah, I'm not sure where that came from. I'll remove it.


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:24
+                              "ERR_CAST", "PTR_ERR_OR_ZERO"));
+  auto NonCheckingStmts = stmt(anyOf(compoundStmt(), labelStmt()));
+  Finder->addMatcher(
----------------
lebedev.ri wrote:
> Are there any rules what kernel considers to be checking and not checking?
> This i think e.g. will accept cast-to-void, assignment to a variable and then ignoring it, etc.
> 
Yes, this check is extremely simplistic. My understanding of clang-tidy checks was that the most value comes from them catching obviously wrong behavior and not having any false positives. I didn't think they were supposed to catch all the ways in which something could be wrong.

But I've never written a clang-tidy check before. What is their expected purpose?


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:39
+  if (MatchedCallExpr &&
+      MatchedCallExpr->hasUnusedResultAttr(*Result.Context)) {
+    diag(MatchedCallExpr->getExprLoc(), "Unused result from error function %0")
----------------
aaron.ballman wrote:
> Is the presence of the attribute actually important, or is it simply expected that the declarations will have the attribute and so this check is benign?
The latter. But I think I could remove it; it seems unlikely that the attribute will be removed from these functions any time, though it could be disabled. It gets set in include/linux/compiler-gcc.h because clang sets the macros __GNUC__ and __GNUC_MINOR__ and __GNUC_PATCHLEVEL__ greater than 3, 4, and 0, respectively.


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:51
+  }
+}
+
----------------
aaron.ballman wrote:
> Is there a way for the user to explicitly disable this warning, or is there no such thing as a false positive as far as the kernel is concerned?
> 
> Should the diagnostic also produce a fix-it, or is there not a sufficiently common replacement pattern to be used?
The basic case (are these error functions being used in any way at all?) is an error and should be fixed. As noted in response to a comment above, the check in that case is so naive that anything it catches is wrong.

With respect to not using the result from functions that return this value, I think the same argument applies: if code calls a function that returns an error like this and literally ignores the result, then clang-tidy should flag it. However, I don't know of a tool that currently checks the kernel for this kind of transitive property with respect to these functions, so it's possible that there are errors like that in the kernel (or idioms that I don't know about).

I thought that the way that you turn off clang-tidy checks is by specifying them at the command line with a minus sign in front of them: -checks=-linuxkernel-must-check-errs.

Or do you mean locally turning it off? In that case, you can just use the result of the function in any trivial way, like casting it to void.

With respect to the fixit; I thought about that, but I'm not sure I know what the right fixit should be. I'd like to leave it without a fixit for now and come back to it later if it becomes clear what to do here.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst:10
+functions are marked with ``__attribute__((warn_unused_result))``, but
+the compiler warning for this attribute is not always enabled.
+
----------------
gribozavr wrote:
> IIRC it is possible to pass through compiler warnings through ClangTidy... WDYT about that instead of reimplementing the warning?
> 
> However we would lose the ability to "infer" `warn_unused_result` on functions that return `ERR_PTR`.  However, since the analysis is not cross-translation-unit, IDK how much value there is.
I don't know exactly how to pass the warnings through, but I'd be interested in learning how to do that. I agree that that would be cleaner than this (partial) reimplementation.

Note that my code above does do something like that: I currently check that the unused-result attribute is set on the return from the call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59963