Page MenuHomePhabricator

[clang-tidy] Add a module for the Linux kernel.
Needs ReviewPublic

Authored by tmroeder on Mar 28 2019, 2:58 PM.

Details

Summary

Now that clang is going to be able to build the Linux kernel again on
x86, and we have gen_compile_commands.py upstream for generating
compile_commands.json, clang-tidy can be used on the Linux kernel
source.

To that end, this commit adds a new clang-tidy module to be used for
checks specific to Linux kernel source. The Linux kernel follows its own
style of C, and it will be useful to separate those checks into their
own module.

Event Timeline

tmroeder created this revision.Mar 28 2019, 2:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2019, 2:58 PM

Not sure if you are the right reviewers; I just looked back in the commit history to see who reviewed clang-tidy changes recently.

Missing docs/ReleaseNotes.rst and docs/clang-tidy/index.rst changes.

clang-tools-extra/clang-tidy/linux/LinuxTidyModule.cpp
13 ↗(On Diff #192721)

You don't need this here.

I think linuxkernel name will be less ambiguous.

Eugene.Zelenko retitled this revision from Add a clang-tidy module for the Linux kernel. to [clang-tidy] Add module for the Linux kernel..Mar 28 2019, 3:20 PM
Eugene.Zelenko edited reviewers, added: alexfh, hokein, JonasToth; removed: gribozavr.
Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added a subscriber: gribozavr.

Looks reasonable in general, but we usually add modules with at least one check. Let's do the same here.

tmroeder updated this revision to Diff 192860.Mar 29 2019, 10:37 AM

Changed the module name to linuxkernel as suggested and updated the files to match.

tmroeder retitled this revision from [clang-tidy] Add module for the Linux kernel. to [clang-tidy] Add a module for the Linux kernel..Mar 29 2019, 10:37 AM
tmroeder marked 2 inline comments as done.

Looks reasonable in general, but we usually add modules with at least one check. Let's do the same here.

OK, will do.

clang-tools-extra/clang-tidy/linux/LinuxTidyModule.cpp
13 ↗(On Diff #192721)

Thanks. I've removed it now.

alexfh added a comment.Apr 3 2019, 6:21 AM

Can you verify that the add_new_check.py script works fine with this new module?
Thanks!

tmroeder updated this revision to Diff 196148.Apr 22 2019, 3:48 PM
tmroeder marked an inline comment as done.

Updated with an initial check.

Verified that add_new_check.py works (that's how I added the check). Also added some basic docs and a test.

Eugene.Zelenko added inline comments.Apr 22 2019, 3:51 PM
clang-tools-extra/docs/ReleaseNotes.rst
148

Please add short description. Should be same as first statements in documentation.

tmroeder updated this revision to Diff 196150.Apr 22 2019, 3:53 PM

Actually add the documentation in the release notes.

tmroeder updated this revision to Diff 196151.Apr 22 2019, 4:02 PM

Fix a line-length issue in the check code and rewrite the doc text.

tmroeder marked an inline comment as done.Apr 22 2019, 4:04 PM
tmroeder added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
148

Please take a look. I've rewritten the initial documentation lines in the check header and used that text here, as suggested.

lebedev.ri added inline comments.Apr 23 2019, 12:11 AM
clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
13

This looks wrong

25

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.

aaron.ballman added inline comments.Apr 23 2019, 6:50 AM
clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
40

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?

41

clang-tidy diagnostics are nongrammatical in the same way the compiler error messages are: don't capitalize the start of the sentence, no terminating punctuation, etc. I'd probably reword to result from error function %0 is unused

42

You should pass the result from getDirectCallee() rather than converting to a string. The diagnostics engine can handle any NamedDecl * automatically, including proper quoting, etc.

49

Similar comments here. How about result from function %0 is unused but represents an error value?

50

Similar issue here as above.

52

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?

gribozavr added inline comments.Apr 23 2019, 8:08 AM
clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
11

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.

tmroeder marked 5 inline comments as done.Apr 23 2019, 9:17 AM

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.

clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
13

Yeah, I'm not sure where that came from. I'll remove it.

25

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?

40

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.

52

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.

clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
11

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.

tmroeder updated this revision to Diff 196273.Apr 23 2019, 9:48 AM

Remove an unnecessary header and fix the error text.

tmroeder marked 4 inline comments as done.Apr 23 2019, 9:50 AM
gribozavr added inline comments.Apr 23 2019, 11:44 AM
clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
11

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.

It seems like it is done by default, and -Wunused-result is enabled by default:

$ cat /tmp/example.cpp
int __attribute__((warn_unused_result)) foo();

void bar() {
  foo();
}
$ ./bin/clang-tidy /tmp/example.cpp
1 warning generated.
/tmp/example.cpp:4:3: warning: ignoring return value of function declared with 'warn_unused_result' attribute [clang-diagnostic-unused-result]
  foo();
  ^

If it does not work for you, you can enable it on the command line:

clang-tidy -extra-arg=-Wunused-result -checks=clang-diagnostic-unused-result /tmp/example.cpp

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.

Yes, I was saying that we would lose the ability to do that. However, is it that valuable? The analysis in ClangTidy does not cross translation unit boundaries, so unless the caller and the callee are in the same file, this inference does not buy us much.

You could also use an existing check, bugprone-unused-return-value, without even requiring the function to be annotated with the attribute:

$ cat /tmp/example.cpp
int foo();

void bar() {
  foo();
}
$ ./bin/clang-tidy /tmp/example.cpp -config="{Checks: 'bugprone-unused-return-value', CheckOptions: [{key: 'bugprone-unused-return-value.CheckedFunctions', value: 'foo'}]}"
1 warning generated.
/tmp/example.cpp:4:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
  foo();
  ^
/tmp/example.cpp:4:3: note: cast the expression to void to silence this warning
aaron.ballman added inline comments.Apr 23 2019, 12:22 PM
clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
25

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.

40

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.

52

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.

clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
6

Let's come up with another check; __must_check has a bug upstream in the kernel sources (I looked into this maybe a month ago). The kernel disables a warning group that this would be under, if I re-enable the lone warning, then this works properly at compile time. (I forget the warning, and should have filed a bug). Point being, fixing this upstream in kernel sources is preferable to me to a clang tidy check, but it's a good start.