[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

21

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

21

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

21

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

21

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

21

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

39

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

40

Should remove commented-out code from the list.

41

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.

49

Same here.

63

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

40

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

41

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

49

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

40

SGTM!

41

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

39

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

39

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

 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 const internal::VariadicDynCastAllOfMatcher<Stmt, VAArgExpr> vAArgExpr;
 
+// FIXME: Add any more builtin variadics that shouldn't trigger this
+static constexpr StringRef AllowedVariadics[] = {
+    "__builtin_constant_p", "__builtin_isinf_sign", "__builtin_assume_aligned",
+    "__builtin_prefetch",   "__builtin_fpclassify",
+};
+
 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);
 }
 
 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();

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

void my_printf(const char* format, ...) {
   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
+
+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);
+}