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

[Georgy Komarov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc2e9baf2e8da: [clang-tidy] Fix 
cppcoreguidelines-pro-type-vararg false positives with… (authored by jubnzv).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101259/new/

https://reviews.llvm.org/D101259

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


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg.cpp
@@ -58,3 +58,11 @@
   (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;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp
===================================================================
--- /dev/null
+++ 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
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
+++ 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 @@
 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,

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits