[clang-tools-extra] r212535 - [clang-tidy] Add a little checker for Twine locals in LLVM.
Alexander Kornienko
alexfh at google.com
Tue Jul 8 09:09:58 PDT 2014
On Tue, Jul 8, 2014 at 4:49 PM, Benjamin Kramer <benny.kra at gmail.com> wrote:
> On Tue, Jul 8, 2014 at 5:33 PM, Alexander Kornienko <alexfh at google.com>
> 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 <
> benny.kra at googlemail.com>
> > 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
> >> cfe-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140708/26695235/attachment.html>
More information about the cfe-commits
mailing list