Thanks for writing the check! It would be better though, if you sent the code for pre-commit review, especially as you are adding a new check.
Did you run the check over the whole LLVM/Clang source code? Could you share the results? (Number of warnings and false-positive rate in a sample of a few tens of instances.) On Tue, Jul 8, 2014 at 3:32 PM, Benjamin Kramer <[email protected]> wrote: > Author: d0k > Date: Tue Jul 8 09:32:17 2014 > New Revision: 212535 > > URL: http://llvm.org/viewvc/llvm-project?rev=212535&view=rev > Log: > [clang-tidy] Add a little checker for Twine locals in LLVM. > > Those often cause use after free bugs and should be generally avoided. > Technically it is safe to have a Twine with >=2 components in a variable > but I don't think it is a good pattern to follow. The almost trivial > checker > comes with elaborated fix-it hints that turn the Twine into a std::string > if necessary and otherwise fall back to the original type if the Twine > is created from a single value. > > Added: > clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.cpp > clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.h > clang-tools-extra/trunk/test/clang-tidy/llvm-twine-local.cpp > Modified: > clang-tools-extra/trunk/clang-tidy/llvm/CMakeLists.txt > clang-tools-extra/trunk/clang-tidy/llvm/LLVMTidyModule.cpp > > Modified: clang-tools-extra/trunk/clang-tidy/llvm/CMakeLists.txt > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/llvm/CMakeLists.txt?rev=212535&r1=212534&r2=212535&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/clang-tidy/llvm/CMakeLists.txt (original) > +++ clang-tools-extra/trunk/clang-tidy/llvm/CMakeLists.txt Tue Jul 8 > 09:32:17 2014 > @@ -4,6 +4,7 @@ add_clang_library(clangTidyLLVMModule > IncludeOrderCheck.cpp > LLVMTidyModule.cpp > NamespaceCommentCheck.cpp > + TwineLocalCheck.cpp > > LINK_LIBS > clangAST > > Modified: clang-tools-extra/trunk/clang-tidy/llvm/LLVMTidyModule.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/llvm/LLVMTidyModule.cpp?rev=212535&r1=212534&r2=212535&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/clang-tidy/llvm/LLVMTidyModule.cpp (original) > +++ clang-tools-extra/trunk/clang-tidy/llvm/LLVMTidyModule.cpp Tue Jul 8 > 09:32:17 2014 > @@ -12,6 +12,7 @@ > #include "../ClangTidyModuleRegistry.h" > #include "IncludeOrderCheck.h" > #include "NamespaceCommentCheck.h" > +#include "TwineLocalCheck.h" > > namespace clang { > namespace tidy { > @@ -24,6 +25,9 @@ public: > CheckFactories.addCheckFactory( > "llvm-namespace-comment", > new ClangTidyCheckFactory<NamespaceCommentCheck>()); > + CheckFactories.addCheckFactory( > + "llvm-twine-local", > + new ClangTidyCheckFactory<TwineLocalCheck>()); > } > }; > > > Added: clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.cpp?rev=212535&view=auto > > ============================================================================== > --- clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.cpp (added) > +++ clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.cpp Tue Jul 8 > 09:32:17 2014 > @@ -0,0 +1,64 @@ > +//===--- TwineLocalCheck.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 "TwineLocalCheck.h" > +#include "clang/AST/ASTContext.h" > +#include "clang/ASTMatchers/ASTMatchers.h" > +#include "clang/Lex/Lexer.h" > +#include "llvm/Support/raw_ostream.h" > Is this header needed? > + > +using namespace clang::ast_matchers; > + > +namespace clang { > +namespace tidy { > + > +TwineLocalCheck::TwineLocalCheck() {} > + > +void TwineLocalCheck::registerMatchers(MatchFinder *Finder) { > + auto TwineType = > + qualType(hasDeclaration(recordDecl(hasName("::llvm::Twine")))); > + Finder->addMatcher(varDecl(hasType(TwineType)).bind("variable"), this); > +} > + > +void TwineLocalCheck::check(const MatchFinder::MatchResult &Result) { > + const VarDecl *VD = Result.Nodes.getNodeAs<VarDecl>("variable"); > + auto Diag = diag(VD->getLocation(), > + "twine variables are prone to use after free bugs"); > I'm not a native speaker, but "use-after-free" looks more correct to me. > + > + // If this VarDecl has an initializer try to fix it. > + if (VD->hasInit()) { > + // Peel away implicit constructors and casts so we can see the actual > type > + // of the initializer. > + const Expr *C = VD->getInit(); > + while (isa<CXXConstructExpr>(C)) > + C = cast<CXXConstructExpr>(C)->getArg(0)->IgnoreParenImpCasts(); > Maybe while (auto Ctor = dyn_cast<CXXConstructExpr>(C)) C = Ctor->getArg(0)->IgnoreParenImpCasts(); ? + > + SourceRange TypeRange = > + VD->getTypeSourceInfo()->getTypeLoc().getSourceRange(); > + > + // A real Twine, turn it into a std::string. > + if (VD->getType()->getCanonicalTypeUnqualified() == > + C->getType()->getCanonicalTypeUnqualified()) { > + SourceLocation EndLoc = Lexer::getLocForEndOfToken( > + VD->getInit()->getLocEnd(), 0, *Result.SourceManager, > + Result.Context->getLangOpts()); > + Diag << FixItHint::CreateReplacement(TypeRange, "std::string") > + << FixItHint::CreateInsertion(VD->getInit()->getLocStart(), > "(") > + << FixItHint::CreateInsertion(EndLoc, ").str()"); > + } else { > + // Just an implicit conversion. Insert the real type. > + Diag << FixItHint::CreateReplacement( > + TypeRange, > + C->getType().getAsString(Result.Context->getPrintingPolicy())); > + } > + } > +} > + > +} // namespace tidy > +} // namespace clang > > Added: clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.h > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.h?rev=212535&view=auto > > ============================================================================== > --- clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.h (added) > +++ clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.h Tue Jul 8 > 09:32:17 2014 > @@ -0,0 +1,31 @@ > +//===--- TwineLocalCheck.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_LLVM_TWINE_LOCAL_CHECK_H > +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_TWINE_LOCAL_CHECK_H > + > +#include "../ClangTidy.h" > +#include "llvm/Support/Regex.h" > Do you need this include? > + > +namespace clang { > +namespace tidy { > + > +/// \brief Looks for local Twine variables which are prone to use after > frees > +/// and should be generally avoided. > +class TwineLocalCheck : public ClangTidyCheck { > +public: > + TwineLocalCheck(); > + void registerMatchers(ast_matchers::MatchFinder *Finder) override; > + void check(const ast_matchers::MatchFinder::MatchResult &Result) > override; > +}; > + > +} // namespace tidy > +} // namespace clang > + > +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_TWINE_LOCAL_CHECK_H > > Added: clang-tools-extra/trunk/test/clang-tidy/llvm-twine-local.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/llvm-twine-local.cpp?rev=212535&view=auto > > ============================================================================== > --- clang-tools-extra/trunk/test/clang-tidy/llvm-twine-local.cpp (added) > +++ clang-tools-extra/trunk/test/clang-tidy/llvm-twine-local.cpp Tue Jul > 8 09:32:17 2014 > @@ -0,0 +1,35 @@ > +// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp > +// RUN: clang-tidy %t.cpp -checks='-*,llvm-twine-local' -fix -- > %t.msg > 2>&1 > +// RUN: FileCheck -input-file=%t.cpp %s +// RUN: FileCheck -input-file=%t.msg -check-prefix=CHECK-MESSAGES %s > It's a nice idea to test both messages and fixes simultaneously. Maybe you can update check_clang_tidy_fix.sh and tests that use it? > + > +namespace llvm { > +class Twine { > +public: > + Twine(const char *); > + Twine(int); > + Twine &operator+(const Twine &); > +}; > +} > + > +using namespace llvm; > + > +void foo(const Twine &x); > + > +static Twine Moo = Twine("bark") + "bah"; > +// CHECK-MASSAGES: twine variables are prone to use after free bugs > +// CHECK-MESSAGES: note: FIX-IT applied suggested code changes > +// CHECK: static std::string Moo = (Twine("bark") + "bah").str(); > + > +int main() { > + const Twine t = Twine("a") + "b" + Twine(42); > +// CHECK-MASSAGES: twine variables are prone to use after free bugs > +// CHECK-MESSAGES: note: FIX-IT applied suggested code changes > +// CHECK: std::string t = (Twine("a") + "b" + Twine(42)).str(); > + foo(Twine("a") + "b"); > + > + Twine Prefix = false ? "__INT_FAST" : "__UINT_FAST"; > +// CHECK-MASSAGES: twine variables are prone to use after free bugs > +// CHECK-MESSAGES: note: FIX-IT applied suggested code changes > +// CHECK: const char * Prefix = false ? "__INT_FAST" : "__UINT_FAST"; > +} > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
