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

[Georgy Komarov via Phabricator via cfe-commits]

jubnzv created this revision.
jubnzv added reviewers: njames93, aaron.ballman.
jubnzv added a project: clang-tools-extra.
Herald added subscribers: shchenz, kbarton, xazax.hun, nemanjai.
jubnzv requested review of this revision.

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.


Repository:
  rG LLVM Github Monorepo

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,7 @@
   (void)__builtin_isinf_sign(0.f);
   (void)__builtin_prefetch(nullptr);
 }
+
+// __builtin_ms_va_list desugared as 'char *'. This test checks whether we are
+// handling this case correctly and not generating false positives.
+void no_false_positive_desugar_ms_va_list(char *in) { char *tmp = 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
@@ -60,8 +60,22 @@
 AST_MATCHER(QualType, isVAList) {
   ASTContext &Context = Finder->getASTContext();
   QualType Desugar = Node.getDesugaredType(Context);
-  return Context.getBuiltinVaListType().getDesugaredType(Context) == Desugar ||
-         Context.getBuiltinMSVaListType().getDesugaredType(Context) == Desugar;
+
+  // __builtin_ms_va_list is internally implemented as 'char *'. This means that
+  // the node desugared as 'char *' potentially can be a __builtin_ms_va_list.
+  // So we need to remove all typedefs one by one to check this.
+  if (Desugar == Context.getBuiltinMSVaListType().getDesugaredType(Context)) {
+    const auto MSVaListType = Context.getBuiltinMSVaListType();
+    QualType Ty;
+    do {
+      Ty = Desugar;
+      Desugar = Ty.getSingleStepDesugaredType(Context);
+      if (Desugar == MSVaListType)
+        return true;
+    } while (Desugar != Ty);
+  }
+
+  return Context.getBuiltinVaListType().getDesugaredType(Context) == Desugar;
 }
 
 AST_MATCHER_P(AdjustedType, hasOriginalType,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D101259.340362.patch
Type: text/x-patch
Size: 3305 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210425/b4d24e9b/attachment.bin>