[PATCH] D93182: [clang-tidy] Add linux kernel log functions checker

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

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.



================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:42-45
+        compoundStmt(has(
+            callExpr(has(ignoringParenImpCasts(stringLiteral().bind("lit"))),
+                     callee(functionDecl(hasAttr(attr::Format)).bind("func")))
+                .bind("call"))),
----------------
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`.
```lang=c
if (...)
  printf(...);```


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:58
+    if (Format->getType()->getName() == "printf" &&
+        ArgNum < Call->getNumArgs() &&
+        Literal == Call->getArg(ArgNum)->IgnoreCasts() && Literal->isAscii() &&
----------------
Should this be an assert, I can't think of a reason why the FormatIDx would be out of range.


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:59
+        ArgNum < Call->getNumArgs() &&
+        Literal == Call->getArg(ArgNum)->IgnoreCasts() && Literal->isAscii() &&
+        Literal->getCharByteWidth() == 1) {
----------------
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
```lang=c
printf("fmtString", "formatArg");```


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:64
+                              "%hhi", "%hhd", "%hhu", "%hhx"};
+      for (auto FoundH : LookForH) {
+        if (LiteralString.contains(FoundH)) {
----------------
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]`


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:65
+      for (auto FoundH : LookForH) {
+        if (LiteralString.contains(FoundH)) {
+          size_t HBegin = LiteralString.find(FoundH) + 1;
----------------
Can this be refactored to use an early exit instead.


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:79
+              Result.Context->getLangOpts(), Result.Context->getTargetInfo());
+          diag(EndLoc, "Integer promotion: Using '%0' in '%1' is unnecessay")
+              << ErrorString << FoundH
----------------
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`.
```lang=c
printf("%hx, a);
        ^
printf("%hx, a);
         ^
``` 



================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:90
+void LogFunctionsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "Fixer", "");
+}
----------------
There is no option retrieved called `Fixer`, so there should be no option stored call `Fixer`.


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.h:13-14
+#include "../ClangTidyCheck.h"
+#include "clang/Lex/MacroInfo.h"
+#include <vector>
+
----------------
These includes seem redundant in this file.


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.h:22
+
+  enum LogFunctionsFixerKind { LFFK_None, LFFK_H };
+
----------------
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.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/linuxkernel-logfunctions-h.c:3
+
+extern printk(const char *, ...) __attribute__((format(printf, 1, 2)));
+
----------------
Can you add a check with a function
```lang=c
extern printf(const char *, ...);
```
Just to demonstrate it will only flag functions that have the printf format attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93182