This is an archive of the discontinued LLVM Phabricator instance.

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

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
12

This looks wrong

24

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
39

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?

40

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

41

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.

48

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

49

Similar issue here as above.

51

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
10

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
12

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

24

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?

39

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.

51

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
10

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
10

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
24

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.

39

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.

51

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
7

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.

tmroeder marked an inline comment as done.Jun 20 2019, 11:54 AM
tmroeder added inline comments.
clang-tools-extra/test/clang-tidy/linuxkernel-must-check-errs.c
7

Good point. How about the related smatch checks in https://repo.or.cz/smatch.git/blob_plain/HEAD:/check_err_ptr_deref.c

It looks for cases where possible ERR_PTR values are dereferenced (wrong because it's not a pointer), and passing non-negative values to ERR_PTR.

What do you think?

I assume you tried to run it on the kernel. Please post the current output somewhere.

Re more checks. I guess we can borrow some from the existing checkers:
https://www.kernel.org/doc/html/v4.12/dev-tools/sparse.html
https://bottest.wiki.kernel.org/coccicheck
https://lwn.net/Articles/752408/
https://lwn.net/Articles/691882/

They do some checks very well. But if we could do something better (more true positives, less false positives), that may be useful.
Also figuring out which of the existing checks in clang-tidy/analyzer are relevant/useful for kernel is another direction.
Also making the thread-safety analysis work for kernel may be a big deal.

nickdesaulniers accepted this revision.Jul 24 2019, 2:21 PM

Re more checks. I guess we can borrow some from the existing checkers:
https://www.kernel.org/doc/html/v4.12/dev-tools/sparse.html
https://bottest.wiki.kernel.org/coccicheck
https://lwn.net/Articles/752408/
https://lwn.net/Articles/691882/

They do some checks very well. But if we could do something better (more true positives, less false positives), that may be useful.
Also figuring out which of the existing checks in clang-tidy/analyzer are relevant/useful for kernel is another direction.

I agree, but we need the core module added first so we can start adding more.

Also making the thread-safety analysis work for kernel may be a big deal.

Yes; but there's many many issues there; a GSoC project is being done on this.
https://github.com/himanshujha199640/linux-kernel-analysis/blob/report/gsoc19/reports/clang-thread-safety-analysis.md
I don't expect that to be solved here in this code review.

Since we have additional checks waiting on this to land, LGTM.

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

Thinking more about it; while we've now cleaned this up in upstream mainline Linux, it still will be a fair amount of work to backport all of the fixes to the LTS branches from which most distros base their releases on. So this check still will have value in that it can currently detect things that Clang won't report for LTS kernels which are very very relevant to at least Android and CrOS.

Further, @Nathan-Huckleberry has an additional check that needs the module added here, so let's land this patch so we can start adding more checks to the module. Worst case we remove this check, but for now I do think it will give us more coverage of LTS Linux kernels than we have today with Clang.

This revision is now accepted and ready to land.Jul 24 2019, 2:21 PM
tmroeder updated this revision to Diff 211783.Jul 25 2019, 9:48 AM

Sync'ing to the latest HEAD commit on master.

Eugene.Zelenko added inline comments.Jul 25 2019, 9:50 AM
clang-tools-extra/docs/ReleaseNotes.rst
75

Please highlight linux/err.h with double back-ticks. I think will be good idea to synchronize documentation text.

tmroeder updated this revision to Diff 211836.Jul 25 2019, 3:22 PM

Synchronize the documentation, as requested.

tmroeder marked 2 inline comments as done.Jul 25 2019, 3:24 PM
tmroeder added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
75

Thanks, I've synchronized the documentation now between this file and the header.

nickdesaulniers requested changes to this revision.Jul 25 2019, 3:31 PM
nickdesaulniers added inline comments.
clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
12

Hard to tell, but I don't think this using statement was ever removed as requested?

This revision now requires changes to proceed.Jul 25 2019, 3:31 PM
Eugene.Zelenko added inline comments.Jul 25 2019, 3:32 PM
clang-tools-extra/docs/ReleaseNotes.rst
77

Release Notes should include short description. One sentence is enough, but it'll good idea to keep it same as first statement of documentation.

This revision was not accepted when it landed; it landed in state Needs Revision.Jul 25 2019, 3:33 PM
This revision was automatically updated to reflect the committed changes.
tmroeder marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2019, 3:34 PM
tmroeder marked an inline comment as done.Jul 26 2019, 9:41 AM
tmroeder added inline comments.
clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
12

It's not the "using" statement that was requested to be removed in the initial diff, but rather "#include <c++/8/bits/c++config.h>", which I did remove.

I'm not sure why the "This looks wrong" gets attached to the "using" statement in later diffs.

tmroeder marked an inline comment as done.Jul 26 2019, 9:43 AM
tmroeder added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
77

Would you like me to submit a patch that removes the second paragraph of this description, then?

Eugene.Zelenko added inline comments.Jul 26 2019, 9:52 AM
clang-tools-extra/docs/ReleaseNotes.rst
77

Yes. Documentation is created for details. But you still need to make its first sentence same as in Release Notes. See other checks and their Release Notes as example (in previous release branches).

tmroeder marked an inline comment as done.Jul 26 2019, 9:58 AM
tmroeder added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
77

I'm sorry, I don't understand. What's the referent of "it" in "you still need to make its first sentence same as in Release Notes"? What first sentence needs to match the first sentence of the Release Notes?

If it's the header file documentation (MustCheckErrs.h), then as far as I can tell, the first paragraph of Release Notes is identical to the first paragraph of the .h file documentation, other than the double backquotes, which I didn't think belonged in a .h file comment.

What am I missing?

tmroeder marked an inline comment as done.Jul 26 2019, 12:09 PM
tmroeder added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
77

I have looked back, and I think that https://reviews.llvm.org/D65343 should address the comment here. Please take a look at let me know.

tmroeder marked an inline comment as done.Jul 26 2019, 2:46 PM
tmroeder added inline comments.
clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
12

Nick, can you confirm that this addresses your comment?

nickdesaulniers added inline comments.Aug 7 2019, 3:58 PM
clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp
12

LGTM