[clang-tools-extra] r213155 - [clang-tidy] Add a checker for zero-length memset.
Alexander Kornienko
alexfh at google.com
Thu Jul 17 01:10:22 PDT 2014
On Wed, Jul 16, 2014 at 10:32 PM, Benjamin Kramer <benny.kra at gmail.com>
wrote:
>
> On 16.07.2014, at 17:49, Nico Weber <thakis at chromium.org> 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 <
> benny.kra at googlemail.com> 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
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
>
> _______________________________________________
> 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/20140717/721bda46/attachment.html>
More information about the cfe-commits
mailing list