[clang-tidy] Add linux kernel log functions checker

[trixirt]

Summary

The linux kernel's checkpatch.pl 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);

checkpatch.pl 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
https://lore.kernel.org/lkml/[e-mail]/

[njames93]

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.

clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp

42–45

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 (...)
  printf(...);

58

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

59

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");

64

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]

65

Can this be refactored to use an early exit instead.

79

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);
         ^

90

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

clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.h

13–14

These includes seem redundant in this file.

22

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.

clang-tools-extra/test/clang-tidy/checkers/linuxkernel-logfunctions-h.c

3

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.

[Eugene.Zelenko]

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

[lebedev.ri]

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

[trixirt]

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.

[nickdesaulniers]

In D93182#2451509, @lebedev.ri wrote:

Out of curiocity, are all checkpatch diagnostics (and something else)?
are planned to be ported into being clang-tidy checks?

Not at this time, no.

How many will checks will that be?

N/A

I'll be slightly worried if linuxkernel module
ends up having more checks than all other modules combined..

I mean, is clang-tidy NOT for codebase-specific lints?

[void]

In D93182#2451509, @lebedev.ri wrote:

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

In my opinion, we shouldn't be concerned about the number of checks, especially with a code base as large and complex as Linux. Linux also has security issues that other pieces of software don't have, which need detailed checks.

If the number of checks is considered too much, what other options should we go with?

[void]

clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp

51

For future reference, Clang / LLVM normally likes to limit the amount of indentation that's due to checks like these. So early exit like this:

void LogFunctionsCheck::check(const MatchFinder::MatchResult &Result) {
  if (FixerKind != LFFK_H)
    return;

  ... // Fixer code here.

But I agree with @njames93 that you probably don't need the enum right now.

53

It might be worthwhile to name Function something like Printk so that the code can be more self-documenting. (Also change the bound variable from func to printk.)

clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyLinuxKernelModule
   ExtraSemiCheck.cpp
   LinuxKernelTidyModule.cpp
+  LogFunctionsCheck.cpp
   MustCheckErrsCheck.cpp
 
   LINK_LIBS
   clangTidy
   clangTidyUtils
 
   DEPENDS
   omp_gen

clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp

 //===--- LinuxKernelTidyModule.cpp - clang-tidy----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ExtraSemiCheck.h"
+#include "LogFunctionsCheck.h"
 #include "MustCheckErrsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace linuxkernel {
 
 /// This module is for checks specific to the Linux kernel.
 class LinuxKernelModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ExtraSemiCheck>("linuxkernel-extra-semi");
+    CheckFactories.registerCheck<LogFunctionsCheck>(
+        "linuxkernel-log-functions");
     CheckFactories.registerCheck<MustCheckErrsCheck>(
         "linuxkernel-must-check-errs");
   }
 };
 // Register the LinuxKernelTidyModule using this statically initialized
 // variable.
 static ClangTidyModuleRegistry::Add<LinuxKernelModule>
     X("linux-module", "Adds checks specific to the Linux kernel.");
 } // namespace linuxkernel
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the LinuxKernelModule.
 volatile int LinuxKernelModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.h

+//===--- LogFunctionsCheck.h - clang-tidy -----------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_LOGFUNCTIONSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_LOGFUNCTIONSCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/Lex/MacroInfo.h"
+#include <vector>
+
+namespace clang {
+namespace tidy {
+namespace linuxkernel {
+
+class LogFunctionsCheck : public ClangTidyCheck {
+
+  enum LogFunctionsFixerKind { LFFK_None, LFFK_H };
+
+public:
+  LogFunctionsCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  enum LogFunctionsFixerKind FixerKind;
+  const std::string LogFunctionsFixerKindName;
+};
+
+} // namespace linuxkernel
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_LOGFUNCTIONSCHECK_H

clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp

+//===--- LogFunctionsCheck.cpp - clang-tidy -------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "LogFunctionsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace linuxkernel {
+
+LogFunctionsCheck::LogFunctionsCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context), FixerKind(LFFK_H) {}
+
+void LogFunctionsCheck::registerMatchers(MatchFinder *Finder) {
+  if (FixerKind == LFFK_H) {
+    // From the reproducer
+    // extern int printk(const char *, ...) __attribute__((format(printf, 1, 2)));
+    // void f(unsigned short a) { printk("%hx\n", a); }
+    //
+    // The AST
+    // |-FunctionDecl printk 'int (const char *, ...)' extern
+    // | |-ParmVarDecl 'const char *'
+    // | `-FormatAttr printf 1 2
+    // ...
+    //
+    //  `-CompoundStmt
+    //    `-CallExpr
+    //     |-ImplicitCastExpr
+    //     | `-DeclRefExpr Function 'printk' 'int (const char *, ...)'
+    //     |-ImplicitCastExpr
+    //     | `-ImplicitCastExpr
+    //     |   `-StringLiteral
+    Finder->addMatcher(
+        compoundStmt(has(
+            callExpr(has(ignoringParenImpCasts(stringLiteral().bind("lit"))),
+                     callee(functionDecl(hasAttr(attr::Format)).bind("func")))
+                .bind("call"))),
+        this);
+  }
+}
+
+void LogFunctionsCheck::check(const MatchFinder::MatchResult &Result) {
+  if (FixerKind == LFFK_H) {
+    const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
+    const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("func");
+    const auto *Literal = Result.Nodes.getNodeAs<StringLiteral>("lit");
+    FormatAttr *Format = Function->getAttr<FormatAttr>();
+    unsigned ArgNum = Format->getFormatIdx() - 1;
+    if (Format->getType()->getName() == "printf" &&
+        ArgNum < Call->getNumArgs() &&
+        Literal == Call->getArg(ArgNum)->IgnoreCasts() && Literal->isAscii() &&
+        Literal->getCharByteWidth() == 1) {
+      StringRef LiteralString = Literal->getString();
+      StringRef LookForH[] = {"%hi",  "%hd",  "%hu",  "%hx",
+                              "%hhi", "%hhd", "%hhu", "%hhx"};
+      for (auto FoundH : LookForH) {
+        if (LiteralString.contains(FoundH)) {
+          size_t HBegin = LiteralString.find(FoundH) + 1;
+          size_t HEnd = HBegin + 1;
+          StringRef ErrorString = "h";
+          if (FoundH.contains("hh")) {
+            HEnd++;
+            ErrorString = "hh";
+          }
+          SourceLocation BeginLoc = Literal->getLocationOfByte(
+              HBegin, Result.Context->getSourceManager(),
+              Result.Context->getLangOpts(), Result.Context->getTargetInfo());
+          SourceLocation EndLoc = Literal->getLocationOfByte(
+              HEnd, Result.Context->getSourceManager(),
+              Result.Context->getLangOpts(), Result.Context->getTargetInfo());
+          diag(EndLoc, "Integer promotion: Using '%0' in '%1' is unnecessay")
+              << ErrorString << FoundH
+              << FixItHint::CreateRemoval(
+                     CharSourceRange::getCharRange(BeginLoc, EndLoc));
+        }
+      }
+    }
+  }
+}
+
+void LogFunctionsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "Fixer", "");
+}
+
+} // namespace linuxkernel
+} // namespace tidy
+} // namespace clang

clang-tools-extra/test/clang-tidy/checkers/linuxkernel-logfunctions-h.c

+// RUN: %check_clang_tidy %s linuxkernel-log-functions %t
+
+extern printk(const char *, ...) __attribute__((format(printf, 1, 2)));
+
+// scripts/checkpatch.pl produces
+// WARNING: Integer promotion: Using 'h' in '%hx' is unnecessary
+// ... linuxkernel-logfunctions-h.c:10:
+// +  printk("%hx\n", a);
+//
+// with --fix-inplace, the change is
+//  void f(unsigned char a)
+// {
+// -  printk("%hx\n", a);
+// +  printk("%x\n", a);
+// }
+void f(unsigned char a) {
+  printk("%hx\n", a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Integer promotion: Using 'h' in '%hx' is unnecessay
+  // CHECK-FIXES: printk("%x\n", a);{{$}}
+}