On Tue, Jul 8, 2014 at 5:33 PM, Alexander Kornienko <[email protected]> wrote: > 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.
I'll keep this in mind for later commits, thanks. This one is really trivial though and only applies to LLVM. > 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.) 3 cases in LLVM, 2 of them real bugs. 4 in Clang, all harmless. Even though some of them are technically false positives I consider them all to be bad style. Twine is designed to be used in parameter lists *only*, let's keep it that way. > 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? Nope. > >> >> + >> +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. OK. I found the wording a bit difficult here because we're not dealing with a classic use after free but with a use after a stack temporary has been destroyed. >> >> + >> + // 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(); > ? That's indeed nicer. > >> + >> + 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? Nope. > >> >> + >> +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? For now I went with the easy route and just augmented check_clang_tidy_fix.sh to check messages if there are CHECK-MESSAGES check lines in the input. Thanks for the review, fixes submitted in r212540! - Ben > >> >> + >> +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
