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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
