On Wed, Jul 16, 2014 at 10:32 PM, Benjamin Kramer <[email protected]> wrote:
> > On 16.07.2014, at 17:49, Nico Weber <[email protected]> wrote: > > > gcc already has this warning ("memset used with constant zero length > parameter; this could be due to transposed parameters"), so I think this > can just become a real warning immediately. (One problem with the gcc > warning is that they emit it after optimizations, so if you do memset(ptr, > value, A - B) and A and B are known at compile time (due to them being > template arguments, for example) it'll warn too. So don't do that :-) ). > > That one is coming from glibc using a horrible attribute hack and relying > on inlining. I've seen it produce vast amounts of false positives, probably > due to templates or just optimizer getting lucky. The tidy check is > suppressing warnings inside of template instantiations which makes it > immune to 0 sneaking in via template arguments and uses the constexpr > machinery to figure out if the length is zero. > > Porting it to clang proper is a bit of work as we don't have AST matchers > there and there is some non-obvious compile time sink because it visits all > ancestors of the call just to figure out if it's in a template instance, > have to find a better way of doing that. > Visiting all the ancestors is the only facility available for this purpose in the AST matchers. This could be done in a more efficient way (e.g. RAV has shouldVisitTemplateInstantiations() to cut the recursion to these subtrees). However, I guess, Sema::CheckMemaccessArguments() also operates on template instantiations, so implementing the warning there may require to propagate the information on whether we're looking into a template instantiation from wherever it is available, which may be not completely trivial. > > - Ben > > > > > > > On Wed, Jul 16, 2014 at 7:30 AM, Benjamin Kramer < > [email protected]> wrote: > > Author: d0k > > Date: Wed Jul 16 09:30:19 2014 > > New Revision: 213155 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=213155&view=rev > > Log: > > [clang-tidy] Add a checker for zero-length memset. > > > > If there's memset(x, y, 0) in the code it's most likely a mistake. The > > checker suggests a fix-it to swap 'y' and '0'. > > > > I think this has the potential to be promoted into a general clang > warning > > after some testing in clang-tidy. > > > > Differential Revision: http://reviews.llvm.org/D4535 > > > > Added: > > clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.cpp > > clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.h > > clang-tools-extra/trunk/test/clang-tidy/google-memset-zero-length.cpp > > Modified: > > clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt > > clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp > > > > Modified: clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt > > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt?rev=213155&r1=213154&r2=213155&view=diff > > > ============================================================================== > > --- clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt (original) > > +++ clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt Wed Jul 16 > 09:30:19 2014 > > @@ -5,6 +5,7 @@ add_clang_library(clangTidyGoogleModule > > ExplicitConstructorCheck.cpp > > ExplicitMakePairCheck.cpp > > GoogleTidyModule.cpp > > + MemsetZeroLengthCheck.cpp > > NamedParameterCheck.cpp > > OverloadedUnaryAndCheck.cpp > > StringReferenceMemberCheck.cpp > > > > Modified: clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp > > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp?rev=213155&r1=213154&r2=213155&view=diff > > > ============================================================================== > > --- clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp > (original) > > +++ clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp Wed > Jul 16 09:30:19 2014 > > @@ -13,6 +13,7 @@ > > #include "AvoidCStyleCastsCheck.h" > > #include "ExplicitConstructorCheck.h" > > #include "ExplicitMakePairCheck.h" > > +#include "MemsetZeroLengthCheck.h" > > #include "NamedParameterCheck.h" > > #include "OverloadedUnaryAndCheck.h" > > #include "StringReferenceMemberCheck.h" > > @@ -46,6 +47,9 @@ public: > > "google-runtime-member-string-references", > > new > ClangTidyCheckFactory<runtime::StringReferenceMemberCheck>()); > > CheckFactories.addCheckFactory( > > + "google-runtime-memset", > > + new ClangTidyCheckFactory<runtime::MemsetZeroLengthCheck>()); > > + CheckFactories.addCheckFactory( > > "google-readability-casting", > > new > ClangTidyCheckFactory<readability::AvoidCStyleCastsCheck>()); > > CheckFactories.addCheckFactory( > > > > Added: > clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.cpp > > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.cpp?rev=213155&view=auto > > > ============================================================================== > > --- clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.cpp > (added) > > +++ clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.cpp > Wed Jul 16 09:30:19 2014 > > @@ -0,0 +1,85 @@ > > +//===--- MemsetZeroLengthCheck.cpp - clang-tidy -------------------*- > C++ -*-===// > > +// > > +// The LLVM Compiler Infrastructure > > +// > > +// This file is distributed under the University of Illinois Open Source > > +// License. See LICENSE.TXT for details. > > +// > > > +//===----------------------------------------------------------------------===// > > + > > +#include "MemsetZeroLengthCheck.h" > > +#include "clang/ASTMatchers/ASTMatchFinder.h" > > +#include "clang/ASTMatchers/ASTMatchers.h" > > +#include "clang/AST/ASTContext.h" > > +#include "clang/Lex/Lexer.h" > > + > > +using namespace clang::ast_matchers; > > + > > +namespace clang { > > +namespace tidy { > > +namespace runtime { > > + > > +void > > +MemsetZeroLengthCheck::registerMatchers(ast_matchers::MatchFinder > *Finder) { > > + auto InTemplateInstantiation = hasAncestor( > > + decl(anyOf(recordDecl(ast_matchers::isTemplateInstantiation()), > > + > functionDecl(ast_matchers::isTemplateInstantiation())))); > > + // Look for memset(x, y, 0) as those is most likely an argument swap. > > + // TODO: Also handle other standard functions that suffer from the > same > > + // problem, e.g. memchr. > > + Finder->addMatcher( > > + callExpr(callee(functionDecl(hasName("::memset"))), > argumentCountIs(3), > > + unless(InTemplateInstantiation)).bind("decl"), > > + this); > > +} > > + > > +/// \brief Get a StringRef representing a SourceRange. > > +static StringRef getAsString(const MatchFinder::MatchResult &Result, > > + SourceRange R) { > > + const SourceManager &SM = *Result.SourceManager; > > + // Don't even try to resolve macro or include contraptions. Not worth > emitting > > + // a fixit for. > > + if (R.getBegin().isMacroID() || > > + !SM.isWrittenInSameFile(R.getBegin(), R.getEnd())) > > + return StringRef(); > > + > > + const char *Begin = SM.getCharacterData(R.getBegin()); > > + const char *End = SM.getCharacterData(Lexer::getLocForEndOfToken( > > + R.getEnd(), 0, SM, Result.Context->getLangOpts())); > > + > > + return StringRef(Begin, End - Begin); > > +} > > + > > +void MemsetZeroLengthCheck::check(const MatchFinder::MatchResult > &Result) { > > + const auto *Call = Result.Nodes.getNodeAs<CallExpr>("decl"); > > + > > + const Expr *Arg1 = Call->getArg(1); > > + const Expr *Arg2 = Call->getArg(2); > > + > > + // Try to evaluate the second argument so we can also find values > that are not > > + // just literals. We don't emit a warning if the second argument also > > + // evaluates to zero. > > + llvm::APSInt Value1, Value2; > > + if (!Arg2->EvaluateAsInt(Value2, *Result.Context) || Value2 != 0 || > > + (Arg1->EvaluateAsInt(Value1, *Result.Context) && Value1 == 0)) > > + return; > > + > > + // Emit a warning and fix-its to swap the arguments. > > + auto D = diag(Call->getLocStart(), > > + "memset of size zero, potentially swapped arguments"); > > + SourceRange LHSRange = Arg1->getSourceRange(); > > + SourceRange RHSRange = Arg2->getSourceRange(); > > + StringRef RHSString = getAsString(Result, RHSRange); > > + StringRef LHSString = getAsString(Result, LHSRange); > > + if (LHSString.empty() || RHSString.empty()) > > + return; > > + > > + D << > FixItHint::CreateReplacement(CharSourceRange::getTokenRange(LHSRange), > > + RHSString) > > + << > FixItHint::CreateReplacement(CharSourceRange::getTokenRange(RHSRange), > > + LHSString); > > +} > > + > > +} // namespace runtime > > +} // namespace tidy > > +} // namespace clang > > > > Added: clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.h > > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.h?rev=213155&view=auto > > > ============================================================================== > > --- clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.h > (added) > > +++ clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.h > Wed Jul 16 09:30:19 2014 > > @@ -0,0 +1,35 @@ > > +//===--- MemsetZeroLengthCheck.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_GOOGLE_MEMSET_ZERO_LENGTH_CHECK_H > > +#define > LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_MEMSET_ZERO_LENGTH_CHECK_H > > + > > +#include "../ClangTidy.h" > > + > > +namespace clang { > > +namespace tidy { > > +namespace runtime { > > + > > +/// \brief Finds calls to memset with a literal zero in the length > argument. > > +/// > > +/// This is most likely unintended and the length and value arguments > are > > +/// swapped. > > +/// > > +/// Corresponding cpplint.py check name: 'runtime/memset'. > > +class MemsetZeroLengthCheck : public ClangTidyCheck { > > +public: > > + void registerMatchers(ast_matchers::MatchFinder *Finder) override; > > + void check(const ast_matchers::MatchFinder::MatchResult &Result) > override; > > +}; > > + > > +} // namespace runtime > > +} // namespace tidy > > +} // namespace clang > > + > > +#endif // > LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_MEMSET_ZERO_LENGTH_CHECK_H > > > > Added: > clang-tools-extra/trunk/test/clang-tidy/google-memset-zero-length.cpp > > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/google-memset-zero-length.cpp?rev=213155&view=auto > > > ============================================================================== > > --- > clang-tools-extra/trunk/test/clang-tidy/google-memset-zero-length.cpp > (added) > > +++ > clang-tools-extra/trunk/test/clang-tidy/google-memset-zero-length.cpp Wed > Jul 16 09:30:19 2014 > > @@ -0,0 +1,51 @@ > > +// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s google-runtime-memset > %t > > +// REQUIRES: shell > > + > > +void *memset(void *, int, __SIZE_TYPE__); > > + > > +namespace std { > > + using ::memset; > > +} > > + > > +template <int i> > > +void memtmpl() { > > + memset(0, sizeof(int), i); > > + memset(0, sizeof(int), 0); > > +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, > potentially swapped argument > > +// CHECK-FIXES: memset(0, 0, sizeof(int)); > > +} > > + > > +void foo(void *a, int xsize, int ysize) { > > + memset(a, sizeof(int), 0); > > +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, > potentially swapped argument > > +// CHECK-FIXES: memset(a, 0, sizeof(int)); > > +#define M memset(a, sizeof(int), 0); > > + M > > +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, > potentially swapped argument > > +// CHECK-FIXES: #define M memset(a, sizeof(int), 0); > > + ::memset(a, xsize * > > + ysize, 0); > > +// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: memset of size zero, > potentially swapped argument > > +// CHECK-FIXES: ::memset(a, 0, xsize * > > +// CHECK-FIXES-NEXT: ysize); > > + std::memset(a, sizeof(int), 0x00); > > +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, > potentially swapped argument > > +// CHECK-FIXES: std::memset(a, 0x00, sizeof(int)); > > + > > + const int v = 0; > > + memset(a, sizeof(int), v); > > +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, > potentially swapped argument > > +// CHECK-FIXES: memset(a, v, sizeof(int)); > > + > > + memset(a, sizeof(int), v + v); > > +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, > potentially swapped argument > > +// CHECK-FIXES: memset(a, v + v, sizeof(int)); > > + > > + memset(a, sizeof(int), v + 1); > > + > > + memset(a, -1, sizeof(int)); > > + memset(a, 0xcd, 1); > > + memset(a, v, 0); > > + > > + memtmpl<0>(); > > +} > > > > > > _______________________________________________ > > 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 >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
