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 

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, 

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: 

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

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.

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to