<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 16, 2014 at 10:32 PM, Benjamin Kramer <span dir="ltr"><<a href="mailto:benny.kra@gmail.com" target="_blank">benny.kra@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><br>
On 16.07.2014, at 17:49, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>
<br>
> 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 :-) ).<br>
<br>
</div>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.<br>
<br>
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.<br>
</blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
- Ben<br>
<div class=""><div class="h5"><br>
><br>
><br>
> On Wed, Jul 16, 2014 at 7:30 AM, Benjamin Kramer <<a href="mailto:benny.kra@googlemail.com">benny.kra@googlemail.com</a>> wrote:<br>
> 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>
><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>
</div></div></blockquote></div><br>
</div></div>