[clang-tools-extra] r213155 - [clang-tidy] Add a checker for zero-length memset.

[Nico Weber]

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 :-) ).


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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140716/ccb033a4/attachment.html>