Page MenuHomePhabricator

[clang-tidy] Add linux kernel log functions checker
Needs ReviewPublic

Authored by trixirt on Dec 13 2020, 11:00 AM.



The linux kernel's script does several checks
and fixits on the log functions. This clang-tidy checker repeats
these checks so the fixits can be applied tree wide.

The first fixer looks for unneeded 'h' in format strings.

From this reproducer

printk("%hx\n", a); produces this warning

WARNING: Integer promotion: Using 'h' in '%hx' is unnecessary + printk("%hx\n", a);

and this fix

  • printk("%hx\n", a);

+ printk("%x\n", a);

LKML discussion

Diff Detail

Event Timeline

trixirt created this revision.Dec 13 2020, 11:00 AM
trixirt requested review of this revision.Dec 13 2020, 11:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2020, 11:00 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This check needs documentation adding in clang-tools-extra/docs/clang-tidy/checks/linux-kernel-logfunctions-h.rst
Also needs to add this in clang-tools-extra/docs/clang-tidy/checks/list.rst, keeping it in alphabetical order.
Also need to add an entry to clang-tools-extra/docs/ReleaseNotes.rst

You may also need to check the state of your local branch as the pre merge build bot can't apply this cleanly in order to test.


Why are you matching on the compoundStmt, surely the callExpr should be the top level matcher?
Doing this will miss cases where 2 occurance of printf occur in the same compoundStmt and miss the cases where printf isn't in a compoundStmt.

if (...)

Should this be an assert, I can't think of a reason why the FormatIDx would be out of range.


Why bother binding the lit node if you can just get it from the FormatArgs. This is also potentially safer also as it won't get confused by

printf("fmtString", "formatArg");

This whole loop looks wrong.
It will only report the first instance of FoundH and its doing a lot of duplicated work
It would be better to scan across the string for %h then check the next chars for [idux] or h[idux]


Can this be refactored to use an early exit instead.


Why is the diag location reported as the end of the format specifier, It should really be pointed to the start of the format specifier or location of the h.

printf("%hx, a);
printf("%hx, a);

There is no option retrieved called Fixer, so there should be no option stored call Fixer.


These includes seem redundant in this file.


Right now you are only using LFFK_H. So for simplicity, this enum should be removed from this patch. If there is a follow up where this actually used, the enum should be added there.


Can you add a check with a function

extern printf(const char *, ...);

Just to demonstrate it will only flag functions that have the printf format attribute.

Maybe Clang's -Wformat should be extended to cover Linux kernel-specific format options?

Out of curiocity, are all checkpatch diagnostics (and something else)?
are planned to be ported into being clang-tidy checks?
How many will checks will that be?
I'll be slightly worried if linuxkernel module
ends up having more checks than all other modules combined..

Yes, this change has problems, I will resubmit.
There are many log function checks in checkpatch, and i need to write another one to find the unattibuted decls.
I don't think adding linux specific warnings is -Wformat is needed, that doesn't scale with all the other projects wanting special attention
There may be a lot of checks and fixits added to linuxkernel/ . checkpatch only checks for new problems.