Index: clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt +++ clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt @@ -5,6 +5,7 @@ AssertSideEffectCheck.cpp AssignOperatorSignatureCheck.cpp BoolPointerImplicitConversionCheck.cpp + DanglingHandleCheck.cpp DefinitionsInHeadersCheck.cpp ForwardDeclarationNamespaceCheck.cpp InaccurateEraseCheck.cpp Index: clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.h +++ clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.h @@ -0,0 +1,43 @@ +//===--- DanglingHandleCheck.h - clang-tidy----------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DANGLING_HANDLE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DANGLING_HANDLE_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// Detect dangling references in value handlers like +/// std::experimental::string_view. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-dangling-handle.html +class DanglingHandleCheck : public ClangTidyCheck { +public: + DanglingHandleCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + void registerMatchersForVariables(ast_matchers::MatchFinder *Finder); + void registerMatchersForReturn(ast_matchers::MatchFinder *Finder); + + const std::vector HandleClasses; + const ast_matchers::internal::Matcher IsAHandle; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DANGLING_HANDLE_H Index: clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.cpp @@ -0,0 +1,185 @@ +//===--- DanglingHandleCheck.cpp - clang-tidy------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "DanglingHandleCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +namespace { + +static const char HandleClassesDelimiter[] = ";"; + +std::vector parseClasses(StringRef Option) { + SmallVector Classes; + Option.split(Classes, HandleClassesDelimiter); + std::vector Result; + for (StringRef &Class : Classes) { + Class = Class.trim(); + if (!Class.empty()) + Result.push_back(Class); + } + return Result; +} + +ast_matchers::internal::BindableMatcher handleFrom( + ast_matchers::internal::Matcher IsAHandle, + ast_matchers::internal::Matcher Arg) { + return cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))), + hasArgument(0, Arg)); +} + +ast_matchers::internal::Matcher handleFromTemporaryValue( + ast_matchers::internal::Matcher IsAHandle) { + // If a ternary operator returns a temporary value, then both branches hold a + // temporary value. If one of them is not a temporary then it must be copied + // into one to satisfy the type of the operator. + const auto TemporaryTernary = + conditionalOperator(hasTrueExpression(cxxBindTemporaryExpr()), + hasFalseExpression(cxxBindTemporaryExpr())); + + return handleFrom(IsAHandle, anyOf(cxxBindTemporaryExpr(), TemporaryTernary)); +} + +ast_matchers::internal::Matcher isASequence() { + return hasAnyName("::std::deque", "::std::forward_list", "::std::list", + "::std::vector"); +} + +ast_matchers::internal::Matcher isASet() { + return hasAnyName("::std::set", "::std::multiset", "::std::unordered_set", + "::std::unordered_multiset"); +} + +ast_matchers::internal::Matcher isAMap() { + return hasAnyName("::std::map", "::std::multimap", "::std::unordered_map", + "::std::unordered_multimap"); +} + +ast_matchers::internal::BindableMatcher +makeContainerMatcher(ast_matchers::internal::Matcher IsAHandle) { + // This matcher could be expanded to detect: + // - Constructors: eg. vector(3, string("A")); + // - emplace*(): This requires a different logic to determine that + // the conversion will happen inside the container. + // - map's insert: This requires detecting that the pair conversion triggers + // the bug. A little more complicated than what we have now. + return callExpr( + hasAnyArgument(handleFromTemporaryValue(IsAHandle)), + anyOf( + // For sequences: assign, push_back, resize. + cxxMemberCallExpr( + callee(functionDecl(hasAnyName("assign", "push_back", "resize"))), + on(expr(hasType(recordDecl(isASequence()))))), + // For sequences and sets: insert. + cxxMemberCallExpr( + callee(functionDecl(hasName("insert"))), + on(expr(hasType(recordDecl(anyOf(isASequence(), isASet())))))), + // For maps: operator[]. + cxxOperatorCallExpr(callee(cxxMethodDecl(ofClass(isAMap()))), + hasOverloadedOperatorName("[]")))); +} + +} // anonymous namespace + +DanglingHandleCheck::DanglingHandleCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + HandleClasses(parseClasses(Options.get( + "HandleClasses", + "std::basic_string_view;std::experimental::basic_string_view"))), + IsAHandle(cxxRecordDecl(hasAnyName(std::vector( + HandleClasses.begin(), HandleClasses.end()))) + .bind("handle")) {} + +void DanglingHandleCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "HandleClasses", + llvm::join(HandleClasses.begin(), HandleClasses.end(), + HandleClassesDelimiter)); +} + +void DanglingHandleCheck::registerMatchersForVariables(MatchFinder* Finder) { + const auto ConvertedHandle = handleFromTemporaryValue(IsAHandle); + + // Find 'Handle foo(ReturnsAValue());' + Finder->addMatcher( + varDecl(hasType(cxxRecordDecl(IsAHandle)), + hasInitializer( + exprWithCleanups(has(ConvertedHandle)).bind("bad_stmt"))), + this); + + // Find 'Handle foo = ReturnsAValue();' + Finder->addMatcher( + varDecl(hasType(cxxRecordDecl(IsAHandle)), unless(parmVarDecl()), + hasInitializer( + exprWithCleanups(has(handleFrom(IsAHandle, ConvertedHandle))) + .bind("bad_stmt"))), + this); + // Find 'foo = ReturnsAValue(); // foo is Handle' + Finder->addMatcher( + cxxOperatorCallExpr(callee(cxxMethodDecl(ofClass(IsAHandle))), + hasOverloadedOperatorName("="), + hasArgument(1, ConvertedHandle)) + .bind("bad_stmt"), + this); + + // Container insertions that will dangle. + Finder->addMatcher(makeContainerMatcher(IsAHandle).bind("bad_stmt"), this); +} + +void DanglingHandleCheck::registerMatchersForReturn(MatchFinder* Finder) { + // Return a local. + Finder->addMatcher( + returnStmt( + // The AST contains two constructor calls: + // 1. Value to Handle conversion. + // 2. Handle copy construction. + // We have to match both. + has(handleFrom( + IsAHandle, + handleFrom(IsAHandle, declRefExpr(to(varDecl( + // Is function scope ... + hasAutomaticStorageDuration(), + // ... and it is a local array or Value. + anyOf(hasType(arrayType()), + hasType(recordDecl( + unless(IsAHandle)))))))))), + // Temporary fix for false positives inside lambdas. + unless(hasAncestor(lambdaExpr()))) + .bind("bad_stmt"), + this); + + // Return a temporary. + Finder->addMatcher( + returnStmt(has(exprWithCleanups(has(handleFrom( + IsAHandle, handleFromTemporaryValue(IsAHandle)))))) + .bind("bad_stmt"), + this); +} + +void DanglingHandleCheck::registerMatchers(MatchFinder* Finder) { + registerMatchersForVariables(Finder); + registerMatchersForReturn(Finder); +} + +void DanglingHandleCheck::check(const MatchFinder::MatchResult& Result) { + auto *Handle = Result.Nodes.getNodeAs("handle"); + diag(Result.Nodes.getNodeAs("bad_stmt")->getLocStart(), + "%0 outlives its value") + << Handle->getQualifiedNameAsString(); +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp +++ clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp @@ -14,6 +14,7 @@ #include "AssertSideEffectCheck.h" #include "AssignOperatorSignatureCheck.h" #include "BoolPointerImplicitConversionCheck.h" +#include "DanglingHandleCheck.h" #include "DefinitionsInHeadersCheck.h" #include "ForwardDeclarationNamespaceCheck.h" #include "InaccurateEraseCheck.h" @@ -54,6 +55,8 @@ "misc-assign-operator-signature"); CheckFactories.registerCheck( "misc-bool-pointer-implicit-conversion"); + CheckFactories.registerCheck( + "misc-dangling-handle"); CheckFactories.registerCheck( "misc-definitions-in-headers"); CheckFactories.registerCheck( Index: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst @@ -50,6 +50,7 @@ misc-assert-side-effect misc-assign-operator-signature misc-bool-pointer-implicit-conversion + misc-dangling-handle misc-definitions-in-headers misc-forward-declaration-namespace misc-inaccurate-erase Index: clang-tools-extra/trunk/docs/clang-tidy/checks/misc-dangling-handle.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/misc-dangling-handle.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/misc-dangling-handle.rst @@ -0,0 +1,34 @@ +.. title:: clang-tidy - misc-dangling-handle + +misc-dangling-handle +==================== + +Detect dangling references in value handlers like +`std::experimental::string_view`. +These dangling references can come from constructing handles from temporary +values, where the temporary is destroyed soon after the handle is created. + +By default only `std::experimental::basic_string_view` is considered. +This list can be modified by passing a ; separated list of class names using +the HandleClasses option. + +Examples: + +.. code-block:: c++ + + string_view View = string(); // View will dangle. + string A; + View = A + "A"; // still dangle. + + vector V; + V.push_back(string()); // V[0] is dangling. + V.resize(3, string()); // V[1] and V[2] will also dangle. + + string_view f() { + // All these return values will dangle. + return string(); + string S; + return S; + char Array[10]{}; + return Array; + } Index: clang-tools-extra/trunk/test/clang-tidy/misc-dangling-handle.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/misc-dangling-handle.cpp +++ clang-tools-extra/trunk/test/clang-tidy/misc-dangling-handle.cpp @@ -0,0 +1,191 @@ +// RUN: %check_clang_tidy %s misc-dangling-handle %t -- \ +// RUN: -config="{CheckOptions: \ +// RUN: [{key: misc-dangling-handle.HandleClasses, \ +// RUN: value: 'std::basic_string_view; ::llvm::StringRef;'}]}" \ +// RUN: -- -std=c++11 + +namespace std { + +template +class vector { + public: + using const_iterator = const T*; + using iterator = T*; + using size_type = int; + + void assign(size_type count, const T& value); + iterator insert(const_iterator pos, const T& value); + iterator insert(const_iterator pos, T&& value); + iterator insert(const_iterator pos, size_type count, const T& value); + void push_back(const T&); + void push_back(T&&); + void resize(size_type count, const T& value); +}; + +template +class pair {}; + +template +class set { + public: + using const_iterator = const T*; + using iterator = T*; + + std::pair insert(const T& value); + std::pair insert(T&& value); + iterator insert(const_iterator hint, const T& value); + iterator insert(const_iterator hint, T&& value); +}; + +template +class map { + public: + using value_type = pair; + value_type& operator[](const Key& key); + value_type& operator[](Key&& key); +}; + +class basic_string { + public: + basic_string(); + basic_string(const char*); + ~basic_string(); +}; + +typedef basic_string string; + +class basic_string_view { + public: + basic_string_view(const char*); + basic_string_view(const basic_string&); +}; + +typedef basic_string_view string_view; + +} // namespace std + +namespace llvm { + +class StringRef { + public: + StringRef(); + StringRef(const char*); + StringRef(const std::string&); +}; + +} // namespace llvm + +std::string ReturnsAString(); + +void Positives() { + std::string_view view1 = std::string(); + // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [misc-dangling-handle] + + std::string_view view_2 = ReturnsAString(); + // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives + + view1 = std::string(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives + + const std::string& str_ref = ""; + std::string_view view3 = true ? "A" : str_ref; + // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives + view3 = true ? "A" : str_ref; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives + + std::string_view view4(ReturnsAString()); + // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives +} + +void OtherTypes() { + llvm::StringRef ref = std::string(); + // CHECK-MESSAGES: [[@LINE-1]]:19: warning: llvm::StringRef outlives its value +} + +const char static_array[] = "A"; +std::string_view ReturnStatements(int i, std::string value_arg, + const std::string &ref_arg) { + const char array[] = "A"; + const char* ptr = "A"; + std::string s; + static std::string ss; + switch (i) { + // Bad cases + case 0: + return array; // refers to local + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv + case 1: + return s; // refers to local + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv + case 2: + return std::string(); // refers to temporary + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv + case 3: + return value_arg; // refers to by-value arg + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv + + // Ok cases + case 100: + return ss; // refers to static + case 101: + return static_array; // refers to static + case 102: + return ptr; // pointer is ok + case 103: + return ref_arg; // refers to by-ref arg + } + + struct S { + std::string_view view() { return value; } + std::string value; + }; + + (void)[&]()->std::string_view { + // This should not warn. The string is bound by reference. + return s; + }; + (void)[=]() -> std::string_view { + // This should not warn. The reference is valid as long as the lambda. + return s; + }; + (void)[=]() -> std::string_view { + // FIXME: This one should warn. We are returning a reference to a local + // lambda variable. + std::string local; + return local; + }; + return ""; +} + +void Containers() { + std::vector v; + v.assign(3, std::string()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives + v.insert(nullptr, std::string()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives + v.insert(nullptr, 3, std::string()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives + v.push_back(std::string()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives + v.resize(3, std::string()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives + + std::set s; + s.insert(std::string()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives + s.insert(nullptr, std::string()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives + + std::map m; + m[std::string()]; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives +} + +void TakesAStringView(std::string_view); + +void Negatives(std::string_view default_arg = ReturnsAString()) { + std::string str; + std::string_view view = str; + + TakesAStringView(std::string()); +}