On Tue, Jul 8, 2014 at 4:49 PM, Benjamin Kramer <[email protected]> wrote:
> 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. > I don't insist, but I'd appreciate if you sent clang-tidy patches for pre-commit review. > > > 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. > Feel free to change this if you agree ;) > > > >> + > >> + 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. > That's fine. Thanks! > > 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
