[clang-tidy] Adding Fuchsia checker for human-readable logging

[juliehockett]

Summary

Adds a checker to the Fuchsia module to flag instances of attempting to log the system's numerical representation of the status, instead of the more human-friendly string representation. Matches formatting functions (the printf family and a set of user-specified formatting-like ones) that have a zx_status_t parameter and suggests a fixit to replace the zx_status_t parameter with a call to zx_status_get_string(param).

[JonasToth]

clang-tidy/zircon/HumanReadableStatusCheck.cpp

68

Did you consider using forEachArgumentWithParam matcher?

I think you could match any zx_status_t argument for the formatter functions and bind on them.

103

Is this required as member function? You can make that static (or move into an unnamed namespace, not 100% sure about the ruling in llvm)

108

That does not consider \% and would be buggy for such fmt strings

112

I think this replacement strategy is not general enough.

http://www.cplusplus.com/reference/cstdio/printf/

%[flags][width][.precision][length]specifier

If we decide to implement an actual printf-fmt string replacer that could maybe land in utility.

I assume that %d is the format specification used for 99,99% of all relevant calls?! Maybe it is enough anyway, because the check matches on very specific %d arguments.
What about the other integer formats? Any chance they are used?

docs/ReleaseNotes.rst

202

Please add a short describing sentence.

204

This seems to be slipped through.

test/clang-tidy/zircon-human-readable-status.cpp

68

Please add test with escaped % and other crazy format strings.

How are advanced format strings treated? I think it is necessary to do better parsing.

[Eugene.Zelenko]

docs/clang-tidy/checks/zircon-human-readable-status.rst

6

Please highlight zx_status_t and other identifiers with ``.

clang-tidy/zircon/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyZirconModule
+  HumanReadableStatusCheck.cpp
   TemporaryObjectsCheck.cpp
   ZirconTidyModule.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/zircon/HumanReadableStatusCheck.h

+//===--- HumanReadableStatusCheck.h - clang-tidy---------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_ZXHUMANREADABLESTATUSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_ZXHUMANREADABLESTATUSCHECK_H
+
+#include "../ClangTidy.h"
+#include "../utils/OptionsUtils.h"
+
+namespace clang {
+namespace tidy {
+namespace zircon {
+
+/// Suggests fix for printf-family functions with a zx_status_t argument to
+/// convert status argument to a human-readable string.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/zircon-human-readable-status.html
+class HumanReadableStatusCheck : public ClangTidyCheck {
+public:
+  HumanReadableStatusCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        FormatLikeFunctions(Options.get("FormatLikeFunctions", "")) {}
+
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  std::string ReplaceFmtString(int Pos, StringRef FmtString);
+
+  const std::string FormatLikeFunctions;
+};
+
+} // namespace zircon
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_ZXHUMANREADABLESTATUSCHECK_H

clang-tidy/zircon/HumanReadableStatusCheck.cpp

+//===--- HumanReadableStatusCheck.cpp - clang-tidy-------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "HumanReadableStatusCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace zircon {
+
+// Semicolon separated list of known format-like functions. The list must end
+// with a semicolon.
+static const char KnownFormatFunctions[] = "printf;"
+                                           "fprintf;"
+                                           "sprintf;"
+                                           "snprintf;"
+                                           "printf_s;"
+                                           "fprintf_s;"
+                                           "sprintf_s;"
+                                           "snprintf_s;"
+                                           "wprintf;"
+                                           "fwprintf;"
+                                           "swprintf;"
+                                           "wprintf_s;"
+                                           "fwprintf_s;"
+                                           "swprintf_s;"
+                                           "snwprintf_s;"
+                                           "vprintf;"
+                                           "vfprintf;"
+                                           "vsprintf;"
+                                           "vsnprintf;"
+                                           "vprintf_s;"
+                                           "vfprintf_s;"
+                                           "vsprintf_s;"
+                                           "vsnprintf_s;";
+
+void HumanReadableStatusCheck::registerMatchers(MatchFinder *Finder) {
+  std::vector<std::string> FunctionNames = utils::options::parseStringList(
+      (llvm::Twine(KnownFormatFunctions) + FormatLikeFunctions).str());
+  Finder->addMatcher(
+      callExpr(
+          allOf(hasAnyArgument(ignoringParenImpCasts(declRefExpr(
+                    hasType(typedefNameDecl(hasName("zx_status_t")))))),
+                callee(functionDecl(hasAnyName(llvm::SmallVector<StringRef, 32>(
+                    FunctionNames.begin(), FunctionNames.end()))))))
+          .bind("formatter"),
+      this);
+}
+
+void HumanReadableStatusCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *D = Result.Nodes.getNodeAs<CallExpr>("formatter")) {
+    auto Diag = diag(D->getLocStart(), "non-human-readable log message");
+    // Check each argument (after the first string arg) for zx_status_t
+    int ArgCount = 0;
+    StringRef FmtString;
+    SourceRange FmtStringRange;
+    for (const auto *Arg : D->arguments()) {
+      if (const auto *ICE = dyn_cast<ImplicitCastExpr>(Arg)) {
+        // Parse the first argument.
+        if (ArgCount == 0) {
+          if (const auto *Str = dyn_cast<StringLiteral>(ICE->getSubExpr())) {
+            FmtString = Str->getString();
+            FmtStringRange = Str->getSourceRange();
+            ++ArgCount;
+            continue;
+          }
+          return;
+        }
+
+        // Check subsequent arguments for zx_status_t args.
+        if (const auto *DRE = dyn_cast<DeclRefExpr>(ICE->getSubExpr())) {
+          if (DRE->getDecl()->getType().getAsString() == "zx_status_t") {
+            Diag << FixItHint::CreateReplacement(
+                FmtStringRange,
+                "\"" + ReplaceFmtString(ArgCount, FmtString) + "\"");
+            Diag << FixItHint::CreateReplacement(
+                DRE->getSourceRange(),
+                "zx_status_get_string(" +
+                    DRE->getNameInfo().getName().getAsString() + ")");
+          }
+        }
+        ++ArgCount;
+      }
+    }
+  }
+}
+
+void HumanReadableStatusCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "FormatLikeFunctions", FormatLikeFunctions);
+}
+
+std::string HumanReadableStatusCheck::ReplaceFmtString(int Pos,
+                                                       StringRef FmtString) {
+  int Current = 1;
+  size_t Loc = 0;
+  while (Current <= Pos && Loc < FmtString.size() - 1) {
+    Loc = FmtString.find('%', Loc);
+    ++Current;
+    ++Loc;
+  }
+  std::string ReplacementString = FmtString;
+  if (Loc != StringRef::npos && ReplacementString[Loc] == 'd')
+    ReplacementString[Loc] = 's';
+  return ReplacementString;
+}
+
+} // namespace zircon
+} // namespace tidy
+} // namespace clang

clang-tidy/zircon/ZirconTidyModule.cpp

 //===--- ZirconTidyModule.cpp - clang-tidy---------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "HumanReadableStatusCheck.h"
 #include "TemporaryObjectsCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace zircon {
 
 /// This module is for Zircon-specific checks.
 class ZirconModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<HumanReadableStatusCheck>(
+        "zircon-human-readable-status");
     CheckFactories.registerCheck<TemporaryObjectsCheck>(
         "zircon-temporary-objects");
   }
 };
 
 // Register the ZirconTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<ZirconModule>
     X("zircon-module", "Adds Zircon kernel checks.");
 } // namespace zircon
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the ZirconModule.
 volatile int ZirconModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

docs/ReleaseNotes.rst

 - The 'misc-undelegated-constructor' check was renamed to :doc:`bugprone-undelegated-constructor
   <clang-tidy/checks/bugprone-undelegated-constructor>`
 
 - The 'misc-unused-raii' check was renamed to :doc:`bugprone-unused-raii
   <clang-tidy/checks/bugprone-unused-raii>`
 
 - The 'google-runtime-member-string-references' check was removed.
 
+- New `zircon-human-readable-status
+  <http://clang.llvm.org/extra/clang-tidy/checks/zircon-human-readable-status.html>`_ check
+
+  FIXME: add release notes.
+
+- New `zircon-temporary-objects
+  <http://clang.llvm.org/extra/clang-tidy/checks/zircon-temporary-objects.html>`_ check
+
+  Warns on construction of specific temporary objects in the Zircon kernel.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/list.rst

.. toctree::
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-static-accessed-through-instance
    readability-static-definition-in-anonymous-namespace
    readability-string-compare
    readability-uniqueptr-delete-release
+   zircon-human-readable-status
    zircon-temporary-objects

docs/clang-tidy/checks/zircon-human-readable-status.rst

+.. title:: clang-tidy - zircon-human-readable-status
+
+zircon-human-readable-status
+============================
+
+Suggests fix for printf-family functions with a zx_status_t argument to convert
+status argument to a human-readable string (zx_status_t is a Zircon kernel
+status message, and zx_status_get_string() is a function that retrieves the
+associated string value.
+
+For example:
+
+.. code-block:: c++
+
+  zx_status_t status;
+  printf("%d", status);   // Suggests printf("%s", zx_status_get_string(status))
+  
+  unsigned num;
+  printf("%d", num)       // No warning
+
+
+Options
+-------
+
+.. option:: Names
+
+   A semi-colon-separated list of fully-qualified names of functions that 
+   are printf-like (i.e. are variadic, with the first argument being a
+   formatting string and subsequent argments being the printf replacements.).
+   Default is empty.
 No newline at end of file

test/clang-tidy/zircon-human-readable-status.cpp

+// RUN: %check_clang_tidy %s zircon-human-readable-status %t -- \
+// RUN:   -config="{CheckOptions: [{key: zircon-human-readable-status.FormatLikeFunctions, value: 'my_printer'}]}" \
+// RUN:   -header-filter=.* \
+// RUN: -- -std=c++11
+
+#include <stdio.h>
+
+#define PRINT(fmt...) printf(fmt);
+
+void my_printer(const char *fmt, ...) {
+  va_list args;
+  va_start(args, fmt);
+  printf(fmt, args);
+  va_end(args);
+}
+
+#define WRAP(fmt...) wrapper(fmt);
+void wrapper(const char *fmt, ...) {
+  va_list args;
+  va_start(args, fmt);
+  printf(fmt, args);
+  va_end(args);
+}
+
+typedef int zx_status_t;
+
+const char *zx_status_get_string(zx_status_t status) { return "status"; }
+
+int main() {
+  zx_status_t status = 0;
+  printf("%d", status);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-human-readable log message
+  // CHECK-FIXES: printf("%s", zx_status_get_string(status));
+  printf("%s", zx_status_get_string(status));
+  my_printer("%d", status);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-human-readable log message
+  // CHECK-FIXES: my_printer("%s", zx_status_get_string(status));
+  PRINT("%d", status);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-human-readable log message
+  // CHECK-FIXES: PRINT("%s", zx_status_get_string(status));
+  
+  WRAP("%d", status);
+  wrapper("%d", status);
+  
+  unsigned notstatus = 0;
+  printf("%d", notstatus);
+  printf("%s", zx_status_get_string(status));
+  my_printer("%d", notstatus);
+  PRINT("%d", notstatus);
+  WRAP("%d", notstatus);
+  wrapper("%d", notstatus);
+  
+  printf("%d, %d", status, notstatus);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-human-readable log message
+  // CHECK-FIXES: printf("%s, %d", zx_status_get_string(status), notstatus);
+  printf("%s, %d", zx_status_get_string(status), notstatus);
+  my_printer("%d, %d", status, notstatus);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-human-readable log message
+  // CHECK-FIXES: my_printer("%s, %d", zx_status_get_string(status), notstatus);
+  printf("%d, %d", notstatus, status);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-human-readable log message
+  // CHECK-FIXES: printf("%d, %s", notstatus, zx_status_get_string(status));
+  PRINT("%d, %d", status, notstatus);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-human-readable log message
+  // CHECK-FIXES: PRINT("%s, %d", zx_status_get_string(status), notstatus);
+  
+  WRAP("%d, %d", notstatus, status);
+  wrapper("%d, %d", notstatus, status);
+
+}