[clang-tidy] Fix cppcoreguidelines-pro-type-vararg false positives with __builtin_ms_va_list

[jubnzv]

Summary

This commit fixes cppcoreguidelines-pro-type-vararg false positives on char * variables.

The incorrect warnings generated by clang-tidy can be illustrated with the following minimal example:

void foo(char* in) {
  char *tmp = in;
}

The problem is that __builtin_ms_va_list desugared as char *, which leads to false positives.

This commit fixes the following bugzilla issue: https://bugs.llvm.org/show_bug.cgi?id=48042.

[aaron.ballman]

It looks like the CI pipeline is failing on the tests.

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

69

[jubnzv]

Thanks, I updated my patch and now CI is happy.
I also found a few additional cases that can lead to false positives on the platforms which implements __builtin_va_list as void * or char *, and added additional checks for this.

[njames93]

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

84

We typically avoid cost qualification on local variables and as the type isn't spelled in the initializer, can you replace auto with the actual deduced type.

[aaron.ballman]

Just some minor nits from me, but otherwise LG!

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

66

QualType objects are intended to be super cheap to copy, so no need to do the const ref dance with them.

97–100

[TWeaver]

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

8

Is the missing FileCheck call here on purpose? it seems to me that the CHECK-MESSAGES aren't actually being verified by this test?

unless I'm missing something.

TIA

[njames93]

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

8

check_clang_tidy invokes FileCheck. Does something else make you think these labels are being tested??

[TWeaver]

This comment was removed by TWeaver.

[TWeaver]

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

8

whilst investigating an unrelated issue on our internal branch, I tried editting the check lines in this test and wasn't able to create a failure. but if I add

'| FileCheck %s -check-prefix=CHECK-MESSAGES' to the run line and then edit the checks, I can induce an error.

This could be an issue on our internal branch though... :shrug: thanks for the speedy reply.

[probinson]

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

8

I'm suspicious that our downstream problem is because the test is assuming that the target is Windows, just because the host is. That's not true for us (or anyone with a Windows-hosted cross-compiler). Does clang-tidy accept a target triple?

[probinson]

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

8

Or possibly the test could be set up to require a Windows target, rather than a Windows host.

[jubnzv]

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

8

I'm not sure that there is a way to pass a target triple to clang-tidy.

But llvm-lit should not run this test on non-Windows host because of REQUIRES: system-windows. For example, when I trying to run it on my Debian machine (llvm-lit clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp -v) I got the following result:

-- Testing: 1 tests, 1 workers --
UNSUPPORTED: Clang Tools :: clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp (1 of 1)

Testing Time: 0.01s
  Unsupported: 1

[dyung]

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

8

Our problem is the opposite, we are running a cross compiler hosted on Windows, but targeting a non-Windows platform (PS4 in our case).

[jubnzv]

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

8

Well, maybe it's a good idea to add the option to set target triple to clang-tidy?

I think this makes sense because clang-tidy can be used to check sources for a project built using a cross-toolchain. In the case, when the platform is set incorrectly, the tool can works incorrectly, because now we have platform-dependent checks.
This also will also help to get rid of similar problems if other platform-dependent tests are added.

I will prepare a patch that adds the -target option within a few days. If anyone has any other ideas on how to work around this problem, I'm open to suggestions.

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"
+#include "clang/Basic/TargetInfo.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 const internal::VariadicDynCastAllOfMatcher<Stmt, VAArgExpr> VAArgExpr;
static constexpr StringRef AllowedVariadics[] = {
     "__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;
+  QualType NodeTy = Node.getUnqualifiedType();
+
+  auto CheckVaList = [](QualType NodeTy, QualType Expected,
+                        const ASTContext &Context) {
+    if (NodeTy == Expected)
+      return true;
+    QualType Desugar = NodeTy;
+    QualType Ty;
+    do {
+      Ty = Desugar;
+      Desugar = Ty.getSingleStepDesugaredType(Context);
+      if (Desugar == Expected)
+        return true;
+    } while (Desugar != Ty);
+    return false;
+  };
+
+  // The internal implementation of __builtin_va_list depends on the target
+  // type. Some targets implements va_list as 'char *' or 'void *'.
+  // In these cases we need to remove all typedefs one by one to check this.
+  using BuiltinVaListKind = TargetInfo::BuiltinVaListKind;
+  BuiltinVaListKind VaListKind = Context.getTargetInfo().getBuiltinVaListKind();
+  if (VaListKind == BuiltinVaListKind::CharPtrBuiltinVaList ||
+      VaListKind == BuiltinVaListKind::VoidPtrBuiltinVaList) {
+    if (CheckVaList(NodeTy, Context.getBuiltinVaListType(), Context))
+      return true;
+  } else if (Desugar ==
+             Context.getBuiltinVaListType().getDesugaredType(Context)) {
+    return true;
+  }
+
+  // We also need to check the implementation of __builtin_ms_va_list in the
+  // same way, because it may differ from the va_list implementation.
+  if (Desugar == Context.getBuiltinMSVaListType().getDesugaredType(Context) &&
+      CheckVaList(NodeTy, Context.getBuiltinMSVaListType(), Context)) {
+    return true;
+  }
+
+  return false;
 }
 
 AST_MATCHER_P(AdjustedType, hasOriginalType,
               ast_matchers::internal::Matcher<QualType>, InnerType) {
   return InnerType.matches(Node.getOriginalType(), Finder, Builder);
 }
 } // namespace
 

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

+// Purpose:
+// Ensure that the 'cppcoreguidelines-pro-type-vararg' check works with the
+// built-in va_list on Windows systems.
+
+// REQUIRES: system-windows
+
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-vararg %t
+
+void test_ms_va_list(int a, ...) {
+  __builtin_ms_va_list ap;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare variables of type va_list; use variadic templates instead
+  __builtin_ms_va_start(ap, a);
+  int b = __builtin_va_arg(ap, int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not use va_arg to define c-style vararg functions; use variadic templates instead
+  __builtin_ms_va_end(ap);
+}
+
+void test_typedefs(int a, ...) {
+  typedef __builtin_ms_va_list my_va_list1;
+  my_va_list1 ap1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare variables of type va_list; use variadic templates instead
+
+  using my_va_list2 = __builtin_ms_va_list;
+  my_va_list2 ap2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare variables of type va_list; use variadic templates instead
+}

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

 
 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);
 }
+
+// Some implementations of __builtin_va_list and __builtin_ms_va_list desugared
+// as 'char *' or 'void *'. This test checks whether we are handling this case
+// correctly and not generating false positives.
+void no_false_positive_desugar_va_list(char *in) {
+  char *tmp1 = in;
+  void *tmp2 = in;
+}