[clang-tidy] add check cppcoreguidelines-pro-type-vararg-use

[mgehre]

Summary

This check flags all calls to c-style vararg functions.

Passing to varargs assumes the correct type will be read. This is
fragile because it cannot generally be enforced to be safe in the
language and so relies on programmer discipline to get it right.

This rule is part of the "Type safety" profile of the C++ Core
Guidelines, see
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-type8-avoid-reading-from-varargs-or-passing-vararg-arguments-prefer-variadic-template-parameters-instead

[sbenza]

clang-tidy/cppcoreguidelines/ProTypeVarargUseCheck.cpp

20 ↗

(On Diff #37520)

The guideline says that we should also issue a diagnostics for uses of va_list/va_start/va_arg.

test/clang-tidy/cppcoreguidelines-pro-type-vararg-use.cpp

14 ↗

(On Diff #37520)

how does this handle SFINAE style ... uses?
The guideline mentions this case as "useful" so we should try to avoid warning on it.

[mgehre]

clang-tidy/cppcoreguidelines/ProTypeVarargUseCheck.cpp

20 ↗

(On Diff #37520)

This is handled by http://reviews.llvm.org/D13785

test/clang-tidy/cppcoreguidelines-pro-type-vararg-use.cpp

14 ↗

(On Diff #37520)

I saw the note in the guidelines, but frankly I don't quite get the use case.
Do you have an example or reference for me?

[sbenza]

clang-tidy/cppcoreguidelines/ProTypeVarargUseCheck.cpp

20 ↗

(On Diff #37520)

I see. They are split between vararg "def" and "use".

test/clang-tidy/cppcoreguidelines-pro-type-vararg-use.cpp

14 ↗

(On Diff #37520)

... has the lowest rank for overload resolution.
You can use it as a default case.
Example:

template <typename T>
void CallFooIfAvailableImpl(T& t, decltype(t.foo())*) {
  t->foo();
}
template <typename T>
void CallFooIfAvailableImpl(T& t, ...) {
  // nothing
}
template <typename T>
void CallFooIfAvailable(T& t) {
  CallFooIfAvailableImpl(t, 0);
}

It is also useful when making traits.
Eg: https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Member_Detector#Solution_and_Sample_Code

[mgehre]

Revert "[clang-tidy] add cert's VariadicFunctionDefCheck as cppcoreguidelines-pro-type-vararg-def"; warn about va_* in this check (to allow SFINAE)

[sbenza]

clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp

26 ↗

(On Diff #37790)

Is there a way to look for the standard type (and not the implementation defined __xxx type name)?

test/clang-tidy/cppcoreguidelines-pro-type-vararg.cpp

38 ↗

(On Diff #37790)

isn't va_arg as useless as va_copy (without va_start)?

Actually, given that va_arg is the only one we can look for without using the private __va_xxx name, maybe we should only warn on that one.

[sbenza]

test/clang-tidy/cppcoreguidelines-pro-type-vararg.cpp

27 ↗

(On Diff #37790)

You would still warn on the callers of this.
Maybe we should ignore any call if the variadic part is a single argument with value 0, which is what is commonly used for SFINAE checks.

[mgehre]

Only flag va_arg and variadic call. Suppress warning if the only vararg argument is literal 0.

[sbenza]

Minor comments only.

clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp

29 ↗

(On Diff #37814)

No need to mention __builtin_va_start anymore.

test/clang-tidy/cppcoreguidelines-pro-type-vararg.cpp

31 ↗

(On Diff #37814)

nit: t.foo() here vs t->foo() in the next line.

[mgehre]

removed __builtin_va_start; change f->foo() to f.foo()

[sbenza]

Thanks for check.

[mgehre]

Thanks for the good review!

clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp

26 ↗

(On Diff #37790)

I don't see it in the AST, because the other names are macros. But maybe there is some magic I don't know of.

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   CppCoreGuidelinesTidyModule.cpp
   ProBoundsPointerArithmeticCheck.cpp
   ProTypeConstCastCheck.cpp
   ProTypeReinterpretCastCheck.cpp
   ProTypeStaticCastDowncastCheck.cpp
   ProTypeUnionAccessCheck.cpp
+  ProTypeVarargCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
-  clangTidyCERTModule
   clangTidyMiscModule
   clangTidyUtils
   clangTooling
   )

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===--- CppCoreGuidelinesModule.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 "../cert/VariadicFunctionDefCheck.h"
 #include "../misc/AssignOperatorSignatureCheck.h"
 #include "ProBoundsPointerArithmeticCheck.h"
 #include "ProTypeConstCastCheck.h"
 #include "ProTypeReinterpretCastCheck.h"
 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
+#include "ProTypeVarargCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ProBoundsPointerArithmeticCheck>(
         "cppcoreguidelines-pro-bounds-pointer-arithmetic");
     CheckFactories.registerCheck<ProTypeConstCastCheck>(
         "cppcoreguidelines-pro-type-const-cast");
-    CheckFactories.registerCheck<VariadicFunctionDefCheck>(
-        "cppcoreguidelines-pro-type-vararg-def");
     CheckFactories.registerCheck<ProTypeReinterpretCastCheck>(
         "cppcoreguidelines-pro-type-reinterpret-cast");
     CheckFactories.registerCheck<ProTypeStaticCastDowncastCheck>(
         "cppcoreguidelines-pro-type-static-cast-downcast");
     CheckFactories.registerCheck<ProTypeUnionAccessCheck>(
         "cppcoreguidelines-pro-type-union-access");
+    CheckFactories.registerCheck<ProTypeVarargCheck>(
+        "cppcoreguidelines-pro-type-vararg");
     CheckFactories.registerCheck<misc::AssignOperatorSignatureCheck>(
         "cppcoreguidelines-c-copy-assignment-signature");
   }
 };
 
 // Register the LLVMTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<CppCoreGuidelinesModule>
     X("cppcoreguidelines-module", "Adds checks for the C++ Core Guidelines.");

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h

+//===--- ProTypeVarargCheck.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_CPPCOREGUIDELINES_PRO_TYPE_VARARG_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_VARARG_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// This check flags all calls to c-style variadic functions and all use
+/// of va_arg.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-pro-type-vararg.html
+class ProTypeVarargCheck : public ClangTidyCheck {
+public:
+  ProTypeVarargCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_VARARG_H

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp

+//===--- ProTypeVarargCheck.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 "ProTypeVarargCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+const internal::VariadicDynCastAllOfMatcher<Stmt, VAArgExpr> vAArgExpr;
+
+void ProTypeVarargCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(vAArgExpr().bind("va_use"), this);
+
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(isVariadic())))
+          .bind("callvararg"),
+      this);
+}
+
+static bool hasSingleVariadicArgumentWithValue(const CallExpr *C, uint64_t I) {
+  const auto *FDecl = dyn_cast<FunctionDecl>(C->getCalleeDecl());
+  if (!FDecl)
+    return false;
+
+  auto N = FDecl->getNumParams(); // Number of parameters without '...'
+  if (C->getNumArgs() != N + 1)
+    return false; // more/less than one argument passed to '...'
+
+  const auto *IntLit =
+      dyn_cast<IntegerLiteral>(C->getArg(N)->IgnoreParenImpCasts());
+  if (!IntLit)
+    return false;
+
+  if (IntLit->getValue() != I)
+    return false;
+
+  return true;
+}
+
+void ProTypeVarargCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *Matched = Result.Nodes.getNodeAs<CallExpr>("callvararg")) {
+    if (hasSingleVariadicArgumentWithValue(Matched, 0))
+      return;
+    diag(Matched->getExprLoc(), "do not call c-style vararg functions");
+  }
+
+  if (const auto *Matched = Result.Nodes.getNodeAs<Expr>("va_use")) {
+    diag(Matched->getExprLoc(),
+         "do not use va_start/va_arg to define c-style vararg functions; "
+         "use variadic templates instead");
+  }
+
+  if (const auto *Matched = Result.Nodes.getNodeAs<VarDecl>("va_list")) {
+    auto SR = Matched->getSourceRange();
+    if (SR.isInvalid())
+      return; // some implicitly generated builtins take va_list
+    diag(SR.getBegin(), "do not declare variables of type va_list; "
+                        "use variadic templates instead");
+  }
+}
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-pro-type-vararg.rst

+cppcoreguidelines-pro-type-vararg
+=====================================
+
+This check flags all calls to c-style vararg functions and all use
+of va_arg.
+To allow for SFINAE use of vararg functions, a call is not flagged if
+a literal 0 is passed as the only vararg argument.
+
+Passing to varargs assumes the correct type will be read. This is fragile because it cannot generally be enforced to be safe in the language and so relies on programmer discipline to get it right.
+
+This rule is part of the "Type safety" profile of the C++ Core Guidelines, see
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-type8-avoid-reading-from-varargs-or-passing-vararg-arguments-prefer-variadic-template-parameters-instead

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

 List of clang-tidy Checks
 =========================
 
 .. toctree::
    cert-setlongjmp
    cert-variadic-function-def
    cppcoreguidelines-pro-bounds-pointer-arithmetic
    cppcoreguidelines-pro-type-const-cast
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
+   cppcoreguidelines-pro-type-vararg
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
    google-explicit-constructor
    google-global-names-in-headers
    google-readability-braces-around-statements
    google-readability-casting
    google-readability-function-size

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-vararg.cpp

+// RUN: %python %S/check_clang_tidy.py %s cppcoreguidelines-pro-type-vararg %t
+
+void f(int i);
+void f_vararg(int i, ...);
+
+struct C {
+  void g_vararg(...);
+  void g(const char*);
+} c;
+
+template<typename... P>
+void cpp_vararg(P... p);
+
+void check() {
+  f_vararg(1, 7, 9);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
+  c.g_vararg("foo");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not call c-style vararg functions
+
+  f(3); // OK
+  c.g("foo"); // OK
+  cpp_vararg(1, 7, 9); // OK
+}
+
+// ... as a parameter is allowed (e.g. for SFINAE)
+template <typename T>
+void CallFooIfAvailableImpl(T& t, ...) {
+  // nothing
+}
+template <typename T>
+void CallFooIfAvailableImpl(T& t, decltype(t.foo())*) {
+  t.foo();
+}
+template <typename T>
+void CallFooIfAvailable(T& t) {
+  CallFooIfAvailableImpl(t, 0); // OK to call variadic function when the argument is a literal 0
+}
+
+#include <cstdarg>
+void my_printf(const char* format, ...) {
+  va_list ap;
+  va_start(ap, format);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not call c-style vararg functions
+  va_list n;
+  va_copy(n, ap); // Don't warn, va_copy is anyway useless without va_start
+  int i = va_arg(ap, int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not use va_start/va_arg to define c-style vararg functions; use variadic templates instead
+  va_end(ap); // Don't warn, va_end is anyway useless without va_start
+}
+
+int my_vprintf(const char* format, va_list arg ); // OK to declare function taking va_list