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; +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits