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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
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(
----------------
tmroeder wrote:
> 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?
We are more forgiving of false positives in clang-tidy than we are in the compiler proper, so we're okay with checks being more chatty, within reason. Obviously, the less false positives (and false negatives), the better because it's more useful for the user. 


================
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")
----------------
tmroeder wrote:
> 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.
I'd probably remove it -- the check is useful even if the function is not marked with the attribute, but if the function is expected to be marked with the attribute in all circumstances anyway, then this is just doing needless work.


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:51
+  }
+}
+
----------------
tmroeder wrote:
> 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.
> 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.

That sounds great to me.

> 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.

That's mostly what I was trying to understand. We usually want checks to have some way to be silenced locally (cast to void, NOLINT comments, whatever makes sense for the construct), though it sounds like your situation probably doesn't need it because every instance caught is truly a bug.

> 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.

Sounds fine to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59963