[clang-tools-extra] r212535 - [clang-tidy] Add a little checker for Twine locals in LLVM.
Benjamin Kramer
benny.kra at gmail.com
Tue Jul 8 08:49:59 PDT 2014
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.
> 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.
>
>> +
>> + 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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
More information about the cfe-commits
mailing list