<div dir="ltr">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 :-) ).</div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jul 16, 2014 at 7:30 AM, Benjamin Kramer <span dir="ltr"><<a href="mailto:benny.kra@googlemail.com" target="_blank">benny.kra@googlemail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: d0k<br>
Date: Wed Jul 16 09:30:19 2014<br>
New Revision: 213155<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=213155&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=213155&view=rev</a><br>
Log:<br>
[clang-tidy] Add a checker for zero-length memset.<br>
<br>
If there's memset(x, y, 0) in the code it's most likely a mistake. The<br>
checker suggests a fix-it to swap 'y' and '0'.<br>
<br>
I think this has the potential to be promoted into a general clang warning<br>
after some testing in clang-tidy.<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D4535" target="_blank">http://reviews.llvm.org/D4535</a><br>
<br>
Added:<br>
clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.cpp<br>
clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.h<br>
clang-tools-extra/trunk/test/clang-tidy/google-memset-zero-length.cpp<br>
Modified:<br>
clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt<br>
clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp<br>
<br>
Modified: clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt?rev=213155&r1=213154&r2=213155&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt?rev=213155&r1=213154&r2=213155&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt (original)<br>
+++ clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt Wed Jul 16 09:30:19 2014<br>
@@ -5,6 +5,7 @@ add_clang_library(clangTidyGoogleModule<br>
ExplicitConstructorCheck.cpp<br>
ExplicitMakePairCheck.cpp<br>
GoogleTidyModule.cpp<br>
+ MemsetZeroLengthCheck.cpp<br>
NamedParameterCheck.cpp<br>
OverloadedUnaryAndCheck.cpp<br>
StringReferenceMemberCheck.cpp<br>
<br>
Modified: clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp?rev=213155&r1=213154&r2=213155&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp?rev=213155&r1=213154&r2=213155&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp (original)<br>
+++ clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp Wed Jul 16 09:30:19 2014<br>
@@ -13,6 +13,7 @@<br>
#include "AvoidCStyleCastsCheck.h"<br>
#include "ExplicitConstructorCheck.h"<br>
#include "ExplicitMakePairCheck.h"<br>
+#include "MemsetZeroLengthCheck.h"<br>
#include "NamedParameterCheck.h"<br>
#include "OverloadedUnaryAndCheck.h"<br>
#include "StringReferenceMemberCheck.h"<br>
@@ -46,6 +47,9 @@ public:<br>
"google-runtime-member-string-references",<br>
new ClangTidyCheckFactory<runtime::StringReferenceMemberCheck>());<br>
CheckFactories.addCheckFactory(<br>
+ "google-runtime-memset",<br>
+ new ClangTidyCheckFactory<runtime::MemsetZeroLengthCheck>());<br>
+ CheckFactories.addCheckFactory(<br>
"google-readability-casting",<br>
new ClangTidyCheckFactory<readability::AvoidCStyleCastsCheck>());<br>
CheckFactories.addCheckFactory(<br>
<br>
Added: clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.cpp?rev=213155&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.cpp?rev=213155&view=auto</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.cpp (added)<br>
+++ clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.cpp Wed Jul 16 09:30:19 2014<br>
@@ -0,0 +1,85 @@<br>
+//===--- MemsetZeroLengthCheck.cpp - clang-tidy -------------------*- C++ -*-===//<br>
+//<br>
+// The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+#include "MemsetZeroLengthCheck.h"<br>
+#include "clang/ASTMatchers/ASTMatchFinder.h"<br>
+#include "clang/ASTMatchers/ASTMatchers.h"<br>
+#include "clang/AST/ASTContext.h"<br>
+#include "clang/Lex/Lexer.h"<br>
+<br>
+using namespace clang::ast_matchers;<br>
+<br>
+namespace clang {<br>
+namespace tidy {<br>
+namespace runtime {<br>
+<br>
+void<br>
+MemsetZeroLengthCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {<br>
+ auto InTemplateInstantiation = hasAncestor(<br>
+ decl(anyOf(recordDecl(ast_matchers::isTemplateInstantiation()),<br>
+ functionDecl(ast_matchers::isTemplateInstantiation()))));<br>
+ // Look for memset(x, y, 0) as those is most likely an argument swap.<br>
+ // TODO: Also handle other standard functions that suffer from the same<br>
+ // problem, e.g. memchr.<br>
+ Finder->addMatcher(<br>
+ callExpr(callee(functionDecl(hasName("::memset"))), argumentCountIs(3),<br>
+ unless(InTemplateInstantiation)).bind("decl"),<br>
+ this);<br>
+}<br>
+<br>
+/// \brief Get a StringRef representing a SourceRange.<br>
+static StringRef getAsString(const MatchFinder::MatchResult &Result,<br>
+ SourceRange R) {<br>
+ const SourceManager &SM = *Result.SourceManager;<br>
+ // Don't even try to resolve macro or include contraptions. Not worth emitting<br>
+ // a fixit for.<br>
+ if (R.getBegin().isMacroID() ||<br>
+ !SM.isWrittenInSameFile(R.getBegin(), R.getEnd()))<br>
+ return StringRef();<br>
+<br>
+ const char *Begin = SM.getCharacterData(R.getBegin());<br>
+ const char *End = SM.getCharacterData(Lexer::getLocForEndOfToken(<br>
+ R.getEnd(), 0, SM, Result.Context->getLangOpts()));<br>
+<br>
+ return StringRef(Begin, End - Begin);<br>
+}<br>
+<br>
+void MemsetZeroLengthCheck::check(const MatchFinder::MatchResult &Result) {<br>
+ const auto *Call = Result.Nodes.getNodeAs<CallExpr>("decl");<br>
+<br>
+ const Expr *Arg1 = Call->getArg(1);<br>
+ const Expr *Arg2 = Call->getArg(2);<br>
+<br>
+ // Try to evaluate the second argument so we can also find values that are not<br>
+ // just literals. We don't emit a warning if the second argument also<br>
+ // evaluates to zero.<br>
+ llvm::APSInt Value1, Value2;<br>
+ if (!Arg2->EvaluateAsInt(Value2, *Result.Context) || Value2 != 0 ||<br>
+ (Arg1->EvaluateAsInt(Value1, *Result.Context) && Value1 == 0))<br>
+ return;<br>
+<br>
+ // Emit a warning and fix-its to swap the arguments.<br>
+ auto D = diag(Call->getLocStart(),<br>
+ "memset of size zero, potentially swapped arguments");<br>
+ SourceRange LHSRange = Arg1->getSourceRange();<br>
+ SourceRange RHSRange = Arg2->getSourceRange();<br>
+ StringRef RHSString = getAsString(Result, RHSRange);<br>
+ StringRef LHSString = getAsString(Result, LHSRange);<br>
+ if (LHSString.empty() || RHSString.empty())<br>
+ return;<br>
+<br>
+ D << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(LHSRange),<br>
+ RHSString)<br>
+ << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(RHSRange),<br>
+ LHSString);<br>
+}<br>
+<br>
+} // namespace runtime<br>
+} // namespace tidy<br>
+} // namespace clang<br>
<br>
Added: clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.h?rev=213155&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.h?rev=213155&view=auto</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.h (added)<br>
+++ clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.h Wed Jul 16 09:30:19 2014<br>
@@ -0,0 +1,35 @@<br>
+//===--- MemsetZeroLengthCheck.h - clang-tidy ---------------------*- C++ -*-===//<br>
+//<br>
+// The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_MEMSET_ZERO_LENGTH_CHECK_H<br>
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_MEMSET_ZERO_LENGTH_CHECK_H<br>
+<br>
+#include "../ClangTidy.h"<br>
+<br>
+namespace clang {<br>
+namespace tidy {<br>
+namespace runtime {<br>
+<br>
+/// \brief Finds calls to memset with a literal zero in the length argument.<br>
+///<br>
+/// This is most likely unintended and the length and value arguments are<br>
+/// swapped.<br>
+///<br>
+/// Corresponding cpplint.py check name: 'runtime/memset'.<br>
+class MemsetZeroLengthCheck : public ClangTidyCheck {<br>
+public:<br>
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;<br>
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;<br>
+};<br>
+<br>
+} // namespace runtime<br>
+} // namespace tidy<br>
+} // namespace clang<br>
+<br>
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_MEMSET_ZERO_LENGTH_CHECK_H<br>
<br>
Added: clang-tools-extra/trunk/test/clang-tidy/google-memset-zero-length.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/google-memset-zero-length.cpp?rev=213155&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/google-memset-zero-length.cpp?rev=213155&view=auto</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/test/clang-tidy/google-memset-zero-length.cpp (added)<br>
+++ clang-tools-extra/trunk/test/clang-tidy/google-memset-zero-length.cpp Wed Jul 16 09:30:19 2014<br>
@@ -0,0 +1,51 @@<br>
+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s google-runtime-memset %t<br>
+// REQUIRES: shell<br>
+<br>
+void *memset(void *, int, __SIZE_TYPE__);<br>
+<br>
+namespace std {<br>
+ using ::memset;<br>
+}<br>
+<br>
+template <int i><br>
+void memtmpl() {<br>
+ memset(0, sizeof(int), i);<br>
+ memset(0, sizeof(int), 0);<br>
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument<br>
+// CHECK-FIXES: memset(0, 0, sizeof(int));<br>
+}<br>
+<br>
+void foo(void *a, int xsize, int ysize) {<br>
+ memset(a, sizeof(int), 0);<br>
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument<br>
+// CHECK-FIXES: memset(a, 0, sizeof(int));<br>
+#define M memset(a, sizeof(int), 0);<br>
+ M<br>
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument<br>
+// CHECK-FIXES: #define M memset(a, sizeof(int), 0);<br>
+ ::memset(a, xsize *<br>
+ ysize, 0);<br>
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: memset of size zero, potentially swapped argument<br>
+// CHECK-FIXES: ::memset(a, 0, xsize *<br>
+// CHECK-FIXES-NEXT: ysize);<br>
+ std::memset(a, sizeof(int), 0x00);<br>
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument<br>
+// CHECK-FIXES: std::memset(a, 0x00, sizeof(int));<br>
+<br>
+ const int v = 0;<br>
+ memset(a, sizeof(int), v);<br>
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument<br>
+// CHECK-FIXES: memset(a, v, sizeof(int));<br>
+<br>
+ memset(a, sizeof(int), v + v);<br>
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument<br>
+// CHECK-FIXES: memset(a, v + v, sizeof(int));<br>
+<br>
+ memset(a, sizeof(int), v + 1);<br>
+<br>
+ memset(a, -1, sizeof(int));<br>
+ memset(a, 0xcd, 1);<br>
+ memset(a, v, 0);<br>
+<br>
+ memtmpl<0>();<br>
+}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>