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

[Georgy Komarov via cfe-commits]

Author: Georgy Komarov
Date: 2021-05-04T13:49:20+03:00
New Revision: c2e9baf2e8dafe92f57fe4171d4b6a5f50d5999e

URL: https://github.com/llvm/llvm-project/commit/c2e9baf2e8dafe92f57fe4171d4b6a5f50d5999e
DIFF: https://github.com/llvm/llvm-project/commit/c2e9baf2e8dafe92f57fe4171d4b6a5f50d5999e.diff

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

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:

```
goid foo(char* in) {
  char *tmp = in;
}
```

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

Fixes bugzilla issue 48042.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D101259

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

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

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
index ec0e87ae22c05..b26cd59fd672a 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
@@ -10,6 +10,7 @@
 #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;
 
@@ -60,8 +61,45 @@ 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 
diff er 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,

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp
new file mode 100644
index 0000000000000..9cb82497a3aec
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp
@@ -0,0 +1,26 @@
+// 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
+}

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg.cpp
index 1e3b5ee036ca1..e5bf98f02d8b3 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg.cpp
@@ -58,3 +58,11 @@ void ignoredBuiltinsTest() {
   (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;
+}