[clang-tools-extra] r212535 - [clang-tidy] Add a little checker for Twine locals in LLVM.
Alexander Kornienko
alexfh at google.com
Tue Jul 8 08:33:25 PDT 2014
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 <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?
> +
> +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
> 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/c48b48ea/attachment.html>
More information about the cfe-commits
mailing list