[clang-tools-extra] r213155 - [clang-tidy] Add a checker for zero-length memset.
Benjamin Kramer
benny.kra at gmail.com
Wed Jul 16 13:32:51 PDT 2014
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.
- 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
>
More information about the cfe-commits
mailing list