[clang-tidy] ignore builtin varargs from pro-type-vararg-check

[njames93]

Summary

Disables the check from warning on some built in vararg functions, Address Clang-tidy should not consider __builtin_constant_p a variadic function.

[aaron.ballman]

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

22

How would we know which builtins should or should not trigger this? Can we add that information to this comment (or, better yet, just fix the fixme up front)?

[njames93]

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

22

Good point, its more a case that I don't know all the variadic builtins clang supports. Those ones I included are just the ones you find in the gcc documentation. Shall I just remove the Fixme completely, If there is another one that's been missed we can address that later

[njames93]

Remove unnecessary fixme.

[aaron.ballman]

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

22

Taking a look at Builtins.def shows quite a few more that likely should be added to the list. I think we should probably have the initial commit covering anything that's a C standard library function/macro that ends with "." in its builtin type signature where the C API isn't variadic. For instance, __builtin_isfinite, __builtin_isinf, etc. I'm fine if we don't vet every builtin we support (that's a large amount of work), but we should be able to cover the most common cases where there's a specification to compare against.

[njames93]

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

22

BUILTIN(__builtin_isgreater     , "i.", "Fnct")
BUILTIN(__builtin_isgreaterequal, "i.", "Fnct")
BUILTIN(__builtin_isless        , "i.", "Fnct")
BUILTIN(__builtin_islessequal   , "i.", "Fnct")
BUILTIN(__builtin_islessgreater , "i.", "Fnct")
BUILTIN(__builtin_isunordered   , "i.", "Fnct")
BUILTIN(__builtin_fpclassify, "iiiiii.", "Fnct")
BUILTIN(__builtin_isfinite,   "i.", "Fnct")
BUILTIN(__builtin_isinf,      "i.", "Fnct")
BUILTIN(__builtin_isinf_sign, "i.", "Fnct")
BUILTIN(__builtin_isnan,      "i.", "Fnct")
BUILTIN(__builtin_isnormal,   "i.", "Fnct")
BUILTIN(__builtin_signbit, "i.", "Fnct")
BUILTIN(__builtin_constant_p, "i.", "nctu")
BUILTIN(__builtin_classify_type, "i.", "nctu")
BUILTIN(__builtin_va_start, "vA.", "nt")
BUILTIN(__builtin_stdarg_start, "vA.", "nt")
BUILTIN(__builtin_assume_aligned, "v*vC*z.", "nc")
BUILTIN(__builtin_fprintf, "iP*cC*.", "Fp:1:")
BUILTIN(__builtin_printf, "icC*.", "Fp:0:")
BUILTIN(__builtin_snprintf, "ic*zcC*.", "nFp:2:")
BUILTIN(__builtin___snprintf_chk, "ic*zizcC*.", "Fp:4:")
BUILTIN(__builtin___sprintf_chk, "ic*izcC*.", "Fp:3:")
BUILTIN(__builtin___fprintf_chk, "iP*icC*.", "Fp:2:")
BUILTIN(__builtin___printf_chk, "iicC*.", "Fp:1:")
BUILTIN(__builtin_prefetch, "vvC*.", "nc")
BUILTIN(__builtin_shufflevector, "v."   , "nct")
BUILTIN(__builtin_convertvector, "v."   , "nct")
BUILTIN(__builtin_call_with_static_chain, "v.", "nt")
BUILTIN(__builtin_annotation, "v.", "tn")
BUILTIN(__builtin_add_overflow, "b.", "nt")
BUILTIN(__builtin_sub_overflow, "b.", "nt")
BUILTIN(__builtin_mul_overflow, "b.", "nt")
BUILTIN(__builtin_preserve_access_index, "v.", "t")
BUILTIN(__builtin_nontemporal_store, "v.", "t")
BUILTIN(__builtin_nontemporal_load, "v.", "t")
BUILTIN(__builtin_os_log_format_buffer_size, "zcC*.", "p:0:nut")
BUILTIN(__builtin_os_log_format, "v*v*cC*.", "p:0:nt")
BUILTIN(__builtin_ms_va_start, "vc*&.", "nt")

That's all the variadics inside Builtin.def that are called __builtin....
Would you suggest including all those apart from the va, print or format related ones

[aaron.ballman]

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

22

I would include all of those that are not defined as being variadic in the C, C++, or POSIX standards. So things like __builtin_isgreater make sense to include, but __builtin_isinf_sign is a bit less clear to me. So a large part of that list seems like stuff we'd want to include, but not all of it.

[njames93]

Included more builtins.

[aaron.ballman]

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

40

I think we may want to keep this one as it's the builtin for the va_start macro, which is not variadic.

41

Should remove commented-out code from the list.

42

If it's documented as being variadic, I think we want to remove it from the list so we warn on its use, right?

From here on down look like things that are not part of any standard, also, so we may want to remove them based on that, too.

50

Same here.

64

Except for this one -- I think we may want to keep this one because it's the MS version of va_start, which is not variadic.

[njames93]

Add va_start.

[njames93]

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

41

Once we are happy I will remove it, more a temporary step.

42

This function should have this signature if default arguments were allowed.

extern void* __builtin_assume_aligned(const void* exp, size_t align, size_t misaligned = 0);

50

Likewise this one should have this signature

extern void ____builtin_prefetch(const void* addr, bool rw = false, unsigned locality = 3);

[aaron.ballman]

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

41

SGTM!

42

Ah, I see now. Makes sense to keep it, but maybe update the comment as "documented as variadic to support default parameters".

If you've vetted the other builtins below to see that they're not actually variadic, I'm fine leaving them in the list as, btw.

[njames93]

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

40

Allowing va_start makes sense, but its breaking the current test cases. According to the documentation that is expected behaviour, So shall I remove __builtin_va_start again?

[aaron.ballman]

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

40

I think that test case is wrong and should be changed, tbh. The point to the rule is to not call variadic functions. There's a different rule about not defining a variadic interface, so that warning should be do not use va_start/va_arg to define c-style vararg functions; use variadic templates instead, not this one.

[njames93]

• Detect va_list type VarDecls to warn on

[njames93]

Hopefully fixed windows buildn

[aaron.ballman]

LGTM, thank you for the fixes!

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

 //===--- ProTypeVarargCheck.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 "ProTypeVarargCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 const internal::VariadicDynCastAllOfMatcher<Stmt, VAArgExpr> vAArgExpr;
 
+static constexpr StringRef AllowedVariadics[] = {
+    // clang-format off
+    "__builtin_isgreater", 
+    "__builtin_isgreaterequal", 
+    "__builtin_isless",
+    "__builtin_islessequal", 
+    "__builtin_islessgreater", 
+    "__builtin_isunordered",
+    "__builtin_fpclassify", 
+    "__builtin_isfinite", 
+    "__builtin_isinf",
+    "__builtin_isinf_sign", 
+    "__builtin_isnan", 
+    "__builtin_isnormal",
+    "__builtin_signbit", 
+    "__builtin_constant_p", 
+    "__builtin_classify_type",
+    "__builtin_va_start",
+    "__builtin_assume_aligned", // Documented as variadic to support default 
+                                // parameters.
+    "__builtin_prefetch",       // Documented as variadic to support default
+                                // parameters.
+    "__builtin_shufflevector",  // Documented as variadic but with a defined
+                                // number of args based on vector size.
+    "__builtin_convertvector", 
+    "__builtin_call_with_static_chain",
+    "__builtin_annotation", 
+    "__builtin_add_overflow", 
+    "__builtin_sub_overflow",
+    "__builtin_mul_overflow", 
+    "__builtin_preserve_access_index",
+    "__builtin_nontemporal_store", 
+    "__builtin_nontemporal_load",
+    "__builtin_ms_va_start",
+    // clang-format on
+};
+
+namespace {
+AST_MATCHER(QualType, isVAList) {
+  ASTContext &Context = Finder->getASTContext();
+  QualType Desugar = Node.getDesugaredType(Context);
+  return Context.getBuiltinVaListType().getDesugaredType(Context) == Desugar ||
+         Context.getBuiltinMSVaListType().getDesugaredType(Context) == Desugar;
+}
+
+AST_MATCHER_P(AdjustedType, hasOriginalType,
+              ast_matchers::internal::Matcher<QualType>, InnerType) {
+  return InnerType.matches(Node.getOriginalType(), Finder, Builder);
+}
+} // namespace
+
 void ProTypeVarargCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(vAArgExpr().bind("va_use"), this);
 
   Finder->addMatcher(
-      callExpr(callee(functionDecl(isVariadic()))).bind("callvararg"), this);
+      callExpr(callee(functionDecl(isVariadic(),
+                                   unless(hasAnyName(AllowedVariadics)))))
+          .bind("callvararg"),
+      this);
+
+  Finder->addMatcher(
+      varDecl(unless(parmVarDecl()),
+              hasType(qualType(
+                  anyOf(isVAList(), decayedType(hasOriginalType(isVAList()))))))
+          .bind("va_list"),
+      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 '...'
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; "
+         "do not use 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 cppcoreguidelines
 } // namespace tidy
 } // namespace clang

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

 template <typename T>
 void CallFooIfAvailable(T& t) {
   CallFooIfAvailableImpl(t, 0); // OK to call variadic function when the argument is a literal 0
 }
 
 #include <stdarg.h>
 void my_printf(const char* format, ...) {
   va_list ap;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare variables of type va_list; use variadic templates instead
   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
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare variables of type va_list; use variadic templates instead
+  va_copy(n, ap);
   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
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not use va_arg to define c-style vararg functions; use variadic templates instead
+  va_end(ap);
 }
 
 int my_vprintf(const char* format, va_list arg ); // OK to declare function taking va_list
+
+void ignoredBuiltinsTest() {
+  (void)__builtin_assume_aligned(0, 8);
+  (void)__builtin_constant_p(0);
+  (void)__builtin_fpclassify(0, 0, 0, 0, 0, 0.f);
+  (void)__builtin_isinf_sign(0.f);
+  (void)__builtin_prefetch(nullptr);
+}