[clang-tidy] Add bugprone-suspicious-memset-usage check.

[rnkovacs]

Summary

Created new module bugprone and placed the check in that.

Finds memset() calls with potential mistakes in their arguments.

Replaces and extends the existing google-runtime-memset-zero-length check. Cases covered:

• Fill value is a character '0'. Integer 0 might have been intended.
• Fill value is out of char range and gets truncated.
• Byte count is zero. Potentially swapped with the fill value argument.

Hits on LLVM codebase:

susp-memset-llvm.txt816 BDownload

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

docs/clang-tidy/checks/misc-suspicious-memset-usage.rst

6 ↗

(On Diff #97279)

Please enclose memset on `` and add ().

25 ↗

(On Diff #97279)

Please enclose memset on `` and add ().

[malcolm.parsons]

The destination is a this pointer within a class that has a virtual function. It might reset the virtual pointer.

Can you change this to match any pointer to a class with a virtual function?

I'd also like a warning for a pointer to a class with a constructor or destructor.

[rnkovacs]

• Fixed function call format in docs.
• Added check to release notes.

[rnkovacs]

In D32700#743178, @malcolm.parsons wrote:

Can you change this to match any pointer to a class with a virtual function?

Well, I have just found a clang flag [[ https://clang.llvm.org/docs/DiagnosticsReference.html#wdynamic-class-memaccess | -Wdynamic-class-memaccess ]] that finds all of these cases and is enabled by default. I guess I need to drop the this case entirely.

I'd also like a warning for a pointer to a class with a constructor or destructor.

This is a nice idea, I'll do it.

[rnkovacs]

• Removed case related to virtual pointers as there is a diagnostic flag for that.
• Added case warning for calls on classes with a constructor or destructor. Changed tests and docs accordingly.

[xazax.hun]

clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp

21 ↗

(On Diff #97917)

I think this might not be the best approach.

For example, if the constructor is compiler generated, but there is a member of the class with non-trivial constructor, we still want to warn.

E.g.:

struct X { X() { /* something nontrivial */ } };

struct Y { X x; };

Maybe we should check instead whether the class is a POD? Other alternative might be something like
CXXRecordDecl::hasNonTrivialDefaultConstructor.

[rnkovacs]

The current proposition could be that we only keep the first two cases, possibly merging in the google check for a third case (with its old name evoking original functionality). Separately, another check could be written that warns when the below mentioned memory management functions are used on not TriviallyCopyable objects.

I wonder if any of the commenters/reviewers/subscribers have any thoughts about this :)

clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp

21 ↗

(On Diff #97917)

So, we had a discussion yesterday that I'll try to sum up here. The root of the problem is not exactly about constructors or the object being a POD, but rather about calling memset() on an object that is not TriviallyCopyable. But as you suggested, this holds for memcpy() and memmove() as well, and might be better placed in another check.

[alexfh]

The existing google-runtime-memset-zero-length check is related. It finds cases when the byte count parameter is zero and offers to swap that with the fill value argument. Perhaps the two could be merged while maintaining backward compatibility through an alias.

I think, the checks should be merged, and there's no need to maintain the old behavior under the google-runtime-memset-zero-length alias. I'm not even sure we want to make this alias, as long as the new check isn't ridiculously noisy.

At this point I'd suggest that we go back to the idea of adding a separate module for checks that target patterns that are likely to be bugs rather than performance, readability or style issues. The name could be "bugprone" or something like this, and we could move many misc- checks there eventually.

clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp

21 ↗

(On Diff #97917)

This logic could be either here or in a separate check (covering 'memcpy', 'memmove' and friends), but it might even be reasonable to create a compiler diagnostic for this eventually (maybe after a test drive of the logic in a clang-tidy check).

[rnkovacs]

• Merged in the google-runtime-memset-zero-length check.
• Added a separate check for memory manipulation functions: [[ https://reviews.llvm.org/D35051 | misc-undefined-memory-manipulation ]].

[rnkovacs]

• Created new module bugprone and moved the check into that.
• Included summary of hits on LLVM in the description.

[whisperity]

Considering the results published in the opening description:

/home/reka/codechecker_dev_env/llvm/lib/Support/NativeFormatting.cpp:55:29: warning: memset fill value is char '0', potentially mistaken for int 0 [bugprone-suspicious-memset-usage]

std::memset(NumberBuffer, '0', sizeof(NumberBuffer));
                          ^~~~
                          0

/home/reka/codechecker_dev_env/llvm/lib/Support/NativeFormatting.cpp:148:26: warning: memset fill value is char '0', potentially mistaken for int 0 [bugprone-suspicious-memset-usage]

::memset(NumberBuffer, '0', llvm::array_lengthof(NumberBuffer));
                       ^~~~
                       0

In these case, the buffer is of char[] type. This memset call initialises the array with the character literal ASCII 0, which is, in this context, the expected behaviour. Maybe you should add an exemption from the checker for these cases, i.e. where we do know that the buffer is a char[].

Tests should be extended with a case for this addition.

[alexfh]

clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp

127–130 ↗

(On Diff #106172)

It looks like this can be replaced with clang::tooling::fixit::getText() or even createReplacement.

[rnkovacs]

• Added char[] exception along with a test case. There are no more false positives on LLVM.
• Simplified fix-its by using clang::tooling::fixit functions.

[whisperity]

docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst

10 ↗

(On Diff #106268)

Shouldn't this '0' be enclosed within backticks?

[rnkovacs]

docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst

10 ↗

(On Diff #106268)

Fixed that and the too short title underline.

[alexfh]

Looks good with a single comment.

Thank you for working on this!

clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp

74–75 ↗

(On Diff #106381)

The formatting is rather misleading here. Let's put the comment inside the if. Same below.

[whisperity]

clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp

57–58 ↗

(On Diff #106381)

@alexfh Your review on putting the comments within their applicable branch bodies applies here too?

89–90 ↗

(On Diff #106381)

Same as L57-58.

[rnkovacs]

Moved comments inside if bodies.

clang-tools-extra/trunk/clang-tidy/CMakeLists.txt

add_clang_library(clangTidy
   clangStaticAnalyzerCore
   clangStaticAnalyzerFrontend
   clangTooling
   clangToolingCore
   )
 
 add_subdirectory(android)
 add_subdirectory(boost)
+add_subdirectory(bugprone)
 add_subdirectory(cert)
 add_subdirectory(cppcoreguidelines)
 add_subdirectory(google)
 add_subdirectory(hicpp)
 add_subdirectory(llvm)
 add_subdirectory(misc)
 add_subdirectory(modernize)
 add_subdirectory(mpi)
 add_subdirectory(performance)
 add_subdirectory(plugin)
 add_subdirectory(readability)
 add_subdirectory(tool)
 add_subdirectory(utils)

clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp

+//===--- BugproneTidyModule.cpp - clang-tidy ------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+#include "SuspiciousMemsetUsageCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+class BugproneModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<SuspiciousMemsetUsageCheck>(
+        "bugprone-suspicious-memset-usage");
+  }
+};
+
+} // namespace bugprone
+
+// Register the BugproneTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<bugprone::BugproneModule>
+    X("bugprone-module", "Adds checks for bugprone code constructs.");
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the BugproneModule.
+volatile int BugproneModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyBugproneModule
+  BugproneTidyModule.cpp
+  SuspiciousMemsetUsageCheck.cpp
+
+  LINK_LIBS
+  clangAnalysis
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyUtils
+  clangTooling
+  )

clang-tools-extra/trunk/clang-tidy/bugprone/SuspiciousMemsetUsageCheck.h

+//===--- SuspiciousMemsetUsageCheck.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_BUGPRONE_SUSPICIOUS_MEMSET_USAGE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUS_MEMSET_USAGE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds memset calls with potential mistakes in their arguments.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-suspicious-memset-usage.html
+class SuspiciousMemsetUsageCheck : public ClangTidyCheck {
+public:
+  SuspiciousMemsetUsageCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUS_MEMSET_USAGE_H

clang-tools-extra/trunk/clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp

+//===--- SuspiciousMemsetUsageCheck.cpp - clang-tidy-----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "SuspiciousMemsetUsageCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void SuspiciousMemsetUsageCheck::registerMatchers(MatchFinder *Finder) {
+  // Note: void *memset(void *buffer, int fill_char, size_t byte_count);
+  // Look for memset(x, '0', z). Probably memset(x, 0, z) was intended.
+  Finder->addMatcher(
+      callExpr(
+          callee(functionDecl(hasName("::memset"))),
+          hasArgument(1, characterLiteral(equals(static_cast<unsigned>('0')))
+                             .bind("char-zero-fill")),
+          unless(
+              eachOf(hasArgument(0, anyOf(hasType(pointsTo(isAnyCharacter())),
+                                          hasType(arrayType(hasElementType(
+                                              isAnyCharacter()))))),
+                     isInTemplateInstantiation()))),
+      this);
+
+  // Look for memset with an integer literal in its fill_char argument.
+  // Will check if it gets truncated.
+  Finder->addMatcher(callExpr(callee(functionDecl(hasName("::memset"))),
+                              hasArgument(1, integerLiteral().bind("num-fill")),
+                              unless(isInTemplateInstantiation())),
+                     this);
+
+  // Look for memset(x, y, 0) as that is most likely an argument swap.
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(hasName("::memset"))),
+               unless(hasArgument(1, anyOf(characterLiteral(equals(
+                                               static_cast<unsigned>('0'))),
+                                           integerLiteral()))),
+               unless(isInTemplateInstantiation()))
+          .bind("call"),
+      this);
+}
+
+void SuspiciousMemsetUsageCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *CharZeroFill =
+          Result.Nodes.getNodeAs<CharacterLiteral>("char-zero-fill")) {
+    // Case 1: fill_char of memset() is a character '0'. Probably an
+    // integer zero was intended.
+
+    SourceRange CharRange = CharZeroFill->getSourceRange();
+    auto Diag =
+        diag(CharZeroFill->getLocStart(), "memset fill value is char '0', "
+                                          "potentially mistaken for int 0");
+
+    // Only suggest a fix if no macros are involved.
+    if (CharRange.getBegin().isMacroID())
+      return;
+    Diag << FixItHint::CreateReplacement(
+        CharSourceRange::getTokenRange(CharRange), "0");
+  }
+
+  else if (const auto *NumFill =
+               Result.Nodes.getNodeAs<IntegerLiteral>("num-fill")) {
+    // Case 2: fill_char of memset() is larger in size than an unsigned char
+    // so it gets truncated during conversion.
+
+    llvm::APSInt NumValue;
+    const auto UCharMax = (1 << Result.Context->getCharWidth()) - 1;
+    if (!NumFill->EvaluateAsInt(NumValue, *Result.Context) ||
+        (NumValue >= 0 && NumValue <= UCharMax))
+      return;
+
+    diag(NumFill->getLocStart(), "memset fill value is out of unsigned "
+                                 "character range, gets truncated");
+  }
+
+  else if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call")) {
+    // Case 3: byte_count of memset() is zero. This is most likely an
+    // argument swap.
+
+    const Expr *FillChar = Call->getArg(1);
+    const Expr *ByteCount = Call->getArg(2);
+
+    // Return if `byte_count` is not zero at compile time.
+    llvm::APSInt Value1, Value2;
+    if (ByteCount->isValueDependent() ||
+        !ByteCount->EvaluateAsInt(Value2, *Result.Context) || Value2 != 0)
+      return;
+
+    // Return if `fill_char` is known to be zero or negative at compile
+    // time. In these cases, swapping the args would be a nop, or
+    // introduce a definite bug. The code is likely correct.
+    if (!FillChar->isValueDependent() &&
+        FillChar->EvaluateAsInt(Value1, *Result.Context) &&
+        (Value1 == 0 || Value1.isNegative()))
+      return;
+
+    // `byte_count` is known to be zero at compile time, and `fill_char` is
+    // either not known or known to be a positive integer. Emit a warning
+    // and fix-its to swap the arguments.
+    auto D = diag(Call->getLocStart(),
+                  "memset of size zero, potentially swapped arguments");
+    StringRef RHSString = tooling::fixit::getText(*ByteCount, *Result.Context);
+    StringRef LHSString = tooling::fixit::getText(*FillChar, *Result.Context);
+    if (LHSString.empty() || RHSString.empty())
+      return;
+
+    D << tooling::fixit::createReplacement(*FillChar, RHSString)
+      << tooling::fixit::createReplacement(*ByteCount, LHSString);
+  }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyGoogleModule
   AvoidCStyleCastsCheck.cpp
   DefaultArgumentsCheck.cpp
   ExplicitConstructorCheck.cpp
   ExplicitMakePairCheck.cpp
   GlobalNamesInHeadersCheck.cpp
   GoogleTidyModule.cpp
   IntegerTypesCheck.cpp
-  MemsetZeroLengthCheck.cpp
   NonConstReferences.cpp
   OverloadedUnaryAndCheck.cpp
   StringReferenceMemberCheck.cpp
   TodoCommentCheck.cpp
   UnnamedNamespaceInHeaderCheck.cpp
   UsingNamespaceDirectiveCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyReadabilityModule
   clangTidyUtils
   )

clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp

 #include "../readability/NamespaceCommentCheck.h"
 #include "../readability/RedundantSmartptrGetCheck.h"
 #include "AvoidCStyleCastsCheck.h"
 #include "DefaultArgumentsCheck.h"
 #include "ExplicitConstructorCheck.h"
 #include "ExplicitMakePairCheck.h"
 #include "GlobalNamesInHeadersCheck.h"
 #include "IntegerTypesCheck.h"
-#include "MemsetZeroLengthCheck.h"
 #include "NonConstReferences.h"
 #include "OverloadedUnaryAndCheck.h"
 #include "StringReferenceMemberCheck.h"
 #include "TodoCommentCheck.h"
 #include "UnnamedNamespaceInHeaderCheck.h"
 #include "UsingNamespaceDirectiveCheck.h"
 
 using namespace clang::ast_matchers;
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<runtime::IntegerTypesCheck>(
         "google-runtime-int");
     CheckFactories.registerCheck<runtime::OverloadedUnaryAndCheck>(
         "google-runtime-operator");
     CheckFactories.registerCheck<runtime::NonConstReferences>(
         "google-runtime-references");
     CheckFactories.registerCheck<runtime::StringReferenceMemberCheck>(
         "google-runtime-member-string-references");
-    CheckFactories.registerCheck<runtime::MemsetZeroLengthCheck>(
-        "google-runtime-memset");
     CheckFactories.registerCheck<readability::AvoidCStyleCastsCheck>(
         "google-readability-casting");
     CheckFactories.registerCheck<readability::TodoCommentCheck>(
         "google-readability-todo");
     CheckFactories
         .registerCheck<clang::tidy::readability::BracesAroundStatementsCheck>(
             "google-readability-braces-around-statements");
     CheckFactories.registerCheck<readability::GlobalNamesInHeadersCheck>(

clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.h

-//===--- 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_MEMSETZEROLENGTHCHECK_H
-#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_MEMSETZEROLENGTHCHECK_H
-
-#include "../ClangTidy.h"
-
-namespace clang {
-namespace tidy {
-namespace google {
-namespace runtime {
-
-/// 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:
-  MemsetZeroLengthCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
-  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
-  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
-};
-
-} // namespace runtime
-} // namespace google
-} // namespace tidy
-} // namespace clang
-
-#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_MEMSETZEROLENGTHCHECK_H

clang-tools-extra/trunk/clang-tidy/google/MemsetZeroLengthCheck.cpp

-//===--- 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/AST/ASTContext.h"
-#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/ASTMatchers/ASTMatchers.h"
-#include "clang/Lex/Lexer.h"
-
-using namespace clang::ast_matchers;
-
-namespace clang {
-namespace tidy {
-namespace google {
-namespace runtime {
-
-void MemsetZeroLengthCheck::registerMatchers(
-    ast_matchers::MatchFinder *Finder) {
-  // 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(isInTemplateInstantiation()))
-                         .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");
-
-  // Note, this is:
-  // void *memset(void *buffer, int fill_char, size_t byte_count);
-  // Arg1 is fill_char, Arg2 is byte_count.
-  const Expr *Arg1 = Call->getArg(1);
-  const Expr *Arg2 = Call->getArg(2);
-
-  // Return if `byte_count` is not zero at compile time.
-  llvm::APSInt Value1, Value2;
-  if (Arg2->isValueDependent() ||
-      !Arg2->EvaluateAsInt(Value2, *Result.Context) || Value2 != 0)
-    return;
-
-  // Return if `fill_char` is known to be zero or negative at compile
-  // time. In these cases, swapping the args would be a nop, or
-  // introduce a definite bug. The code is likely correct.
-  if (!Arg1->isValueDependent() &&
-      Arg1->EvaluateAsInt(Value1, *Result.Context) &&
-      (Value1 == 0 || Value1.isNegative()))
-    return;
-
-  // `byte_count` is known to be zero at compile time, and `fill_char` is
-  // either not known or known to be a positive integer. 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 google
-} // namespace tidy
-} // namespace clang

clang-tools-extra/trunk/clang-tidy/tool/CMakeLists.txt

add_dependencies(clang-tidy
   )
 target_link_libraries(clang-tidy
   clangAST
   clangASTMatchers
   clangBasic
   clangTidy
   clangTidyAndroidModule
   clangTidyBoostModule
+  clangTidyBugproneModule
   clangTidyCERTModule
   clangTidyCppCoreGuidelinesModule
   clangTidyGoogleModule
   clangTidyHICPPModule
   clangTidyLLVMModule
   clangTidyMiscModule
   clangTidyModernizeModule
   clangTidyMPIModule

clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp

 static int LLVM_ATTRIBUTE_UNUSED CERTModuleAnchorDestination =
     CERTModuleAnchorSource;
 
 // This anchor is used to force the linker to link the BoostModule.
 extern volatile int BoostModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED BoostModuleAnchorDestination =
     BoostModuleAnchorSource;
 
+// This anchor is used to force the linker to link the BugproneModule.
+extern volatile int BugproneModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED BugproneModuleAnchorDestination =
+    BugproneModuleAnchorSource;
+
 // This anchor is used to force the linker to link the LLVMModule.
 extern volatile int LLVMModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED LLVMModuleAnchorDestination =
     LLVMModuleAnchorSource;
 
 // This anchor is used to force the linker to link the CppCoreGuidelinesModule.
 extern volatile int CppCoreGuidelinesModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED CppCoreGuidelinesModuleAnchorDestination =

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New `android-cloexec-fopen
   Checks if the required mode ``e`` exists in the mode argument of ``fopen()``.
 
 - New `android-cloexec-socket
   <http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-socket.html>`_ check
 
   Checks if the required file flag ``SOCK_CLOEXEC`` is present in the argument of
   ``socket()``.
 
+- New `bugprone-suspicious-memset-usage
+  <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-suspicious-memset-usage.html>`_ check
+
+  Finds ``memset()`` calls with potential mistakes in their arguments.
+  Replaces and extends the ``google-runtime-memset`` check.
+
 - New `cert-dcl21-cpp
   <http://clang.llvm.org/extra/clang-tidy/checks/cert-dcl21-cpp.html>`_ check
 
   Checks if the overloaded postfix ``operator++/--`` returns a constant object.
 
 - New `cert-dcl58-cpp
   <http://clang.llvm.org/extra/clang-tidy/checks/cert-dcl58-cpp.html>`_ check
 

 - New `readability-misleading-indentation
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-misleading-indentation.html>`_ check
 
   Finds misleading indentation where braces should be introduced or the code should be reformatted.
 
 - Support clang-formatting of the code around applied fixes (``-format-style``
   command-line option).
 
+- New `bugprone` module
+
+  Adds checks that target bugprone code constructs.
+
 - New `hicpp` module
 
   Adds checks that implement the `High Integrity C++ Coding Standard <http://www.codingstandard.com/section/index/>`_ and other safety
   standards. Many checks are aliased to other modules.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst

+.. title:: clang-tidy - bugprone-suspicious-memset-usage
+
+bugprone-suspicious-memset-usage
+================================
+
+This check finds ``memset()`` calls with potential mistakes in their arguments.
+Considering the function as ``void* memset(void* destination, int fill_value,
+size_t byte_count)``, the following cases are covered:
+
+**Case 1: Fill value is a character ``'0'``**
+
+Filling up a memory area with ASCII code 48 characters is not customary,
+possibly integer zeroes were intended instead.
+The check offers a replacement of ``'0'`` with ``0``. Memsetting character
+pointers with ``'0'`` is allowed.
+
+**Case 2: Fill value is truncated**
+
+Memset converts ``fill_value`` to ``unsigned char`` before using it. If
+``fill_value`` is out of unsigned character range, it gets truncated
+and memory will not contain the desired pattern.
+
+**Case 3: Byte count is zero**
+
+Calling memset with a literal zero in its ``byte_count`` argument is likely
+to be unintended and swapped with ``fill_value``. The check offers to swap
+these two arguments.
+
+Corresponding cpplint.py check name: ``runtime/memset``.
+
+
+Examples:
+
+.. code-block:: c++
+
+  void foo() {
+    int i[5] = {1, 2, 3, 4, 5};
+    int *ip = i;
+    char c = '1';
+    char *cp = &c;
+    int v = 0;
+
+    // Case 1
+    memset(ip, '0', 1); // suspicious
+    memset(cp, '0', 1); // OK
+
+    // Case 2
+    memset(ip, 0xabcd, 1); // fill value gets truncated
+    memset(ip, 0x00, 1);   // OK
+
+    // Case 3
+    memset(ip, sizeof(int), v); // zero length, potentially swapped
+    memset(ip, 0, 1);           // OK
+  }

clang-tools-extra/trunk/docs/clang-tidy/checks/google-runtime-memset.rst

-.. title:: clang-tidy - google-runtime-memset
-
-google-runtime-memset
-=====================
-
-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`.

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    android-cloexec-creat
    android-cloexec-fopen
    android-cloexec-open
    android-cloexec-socket
    boost-use-to-string
+   bugprone-suspicious-memset-usage
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl21-cpp
    cert-dcl50-cpp
    cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp>
    cert-dcl58-cpp
    cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp>
    cert-env33-c
    cert-err09-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err09-cpp>
.. toctree::
    google-readability-braces-around-statements (redirects to readability-braces-around-statements) <google-readability-braces-around-statements>
    google-readability-casting
    google-readability-function-size (redirects to readability-function-size) <google-readability-function-size>
    google-readability-namespace-comments (redirects to llvm-namespace-comment) <google-readability-namespace-comments>
    google-readability-redundant-smartptr-get (redirects to readability-redundant-smartptr-get) <google-readability-redundant-smartptr-get>
    google-readability-todo
    google-runtime-int
    google-runtime-member-string-references
-   google-runtime-memset
    google-runtime-operator
    google-runtime-references
    hicpp-explicit-conversions
    hicpp-function-size
    hicpp-invalid-access-moved
    hicpp-member-init
    hicpp-named-parameter
    hicpp-new-delete-operators

clang-tools-extra/trunk/docs/clang-tidy/index.rst

 
 There are currently the following groups of checks:
 
 ====================== =========================================================
 Name prefix            Description
 ====================== =========================================================
 ``android-``           Checks related to Android.
 ``boost-``             Checks related to Boost library.
+``bugprone-``          Checks that target bugprone code constructs.
 ``cert-``              Checks related to CERT Secure Coding Guidelines.
 ``cppcoreguidelines-`` Checks related to C++ Core Guidelines.
 ``clang-analyzer-``    Clang Static Analyzer checks.
 ``google-``            Checks related to Google coding conventions.
 ``hicpp-``             Checks related to High Integrity C++ Coding Standard.
 ``llvm-``              Checks related to the LLVM coding conventions.
 ``misc-``              Checks that we didn't have a better category for.
 ``modernize-``         Checks that advocate usage of modern (currently "modern"

clang-tools-extra/trunk/test/clang-tidy/bugprone-suspicious-memset-usage.cpp

+// RUN: %check_clang_tidy %s bugprone-suspicious-memset-usage %t
+
+void *memset(void *, int, __SIZE_TYPE__);
+
+namespace std {
+  using ::memset;
+}
+
+template <typename T>
+void mtempl(int *ptr) {
+  memset(ptr, '0', sizeof(T));
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: memset fill value is char '0', potentially mistaken for int 0 [bugprone-suspicious-memset-usage]
+// CHECK-FIXES: memset(ptr, 0, sizeof(T));
+  memset(ptr, 256, sizeof(T));
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: memset fill value is out of unsigned character range, gets truncated [bugprone-suspicious-memset-usage]
+  memset(0, sizeof(T), 0);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped arguments [bugprone-suspicious-memset-usage]
+// CHECK-FIXES: memset(0, 0, sizeof(T));
+  memset(0, sizeof(int), 0);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped arguments [bugprone-suspicious-memset-usage]
+// CHECK-FIXES: memset(0, 0, sizeof(int));
+}
+
+void foo(int xsize, int ysize) {
+  int i[5] = {1, 2, 3, 4, 5};
+  char ca[3] = {'a', 'b', 'c'};
+  int *p = i;
+  int l = 5;
+  char z = '1';
+  char *c = &z;
+  int v = 0;
+
+  memset(p, '0', l);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: memset fill value is char '0', potentially mistaken for int 0 [bugprone-suspicious-memset-usage]
+// CHECK-FIXES: memset(p, 0, l);
+
+  memset(p, 0xabcd, l);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: memset fill value is out of unsigned character range, gets truncated [bugprone-suspicious-memset-usage]
+
+  memset(p, sizeof(int), 0);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped arguments [bugprone-suspicious-memset-usage]
+// CHECK-FIXES: memset(p, 0, sizeof(int));
+  std::memset(p, sizeof(int), 0x00);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped arguments [bugprone-suspicious-memset-usage]
+// CHECK-FIXES: std::memset(p, 0x00, sizeof(int));
+
+#define M_CHAR_ZERO memset(p, '0', l);
+  M_CHAR_ZERO
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset fill value is char '0', potentially mistaken for int 0 [bugprone-suspicious-memset-usage]
+
+#define M_OUTSIDE_RANGE memset(p, 0xabcd, l);
+  M_OUTSIDE_RANGE
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset fill value is out of unsigned character range, gets truncated [bugprone-suspicious-memset-usage]
+
+#define M_ZERO_LENGTH memset(p, sizeof(int), 0);
+  M_ZERO_LENGTH
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped arguments [bugprone-suspicious-memset-usage]
+
+  memset(p, '2', l);
+  memset(p, 0, l);
+  memset(c, '0', 1);
+  memset(ca, '0', sizeof(ca));
+
+  memset(p, 0x00, l);
+  mtempl<int>(p);
+
+  memset(p, sizeof(int), v + 1);
+  memset(p, 0xcd, 1);
+
+  // Don't warn when the fill char and the length are both known to be
+  // zero.  No bug is possible.
+  memset(p, 0, v);
+
+  // -1 is clearly not a length by virtue of being negative, so no warning
+  // despite v == 0.
+  memset(p, -1, v);
+}

clang-tools-extra/trunk/test/clang-tidy/google-runtime-memset-zero-length.cpp

-// RUN: %check_clang_tidy %s google-runtime-memset %t
-
-void *memset(void *, int, __SIZE_TYPE__);
-
-namespace std {
-  using ::memset;
-}
-
-template <int i, typename T>
-void memtmpl() {
-  memset(0, sizeof(int), i);
-  memset(0, sizeof(T), sizeof(T));
-  memset(0, sizeof(T), 0);
-// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument
-// CHECK-FIXES: memset(0, 0, sizeof(T));
-  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);
-
-  // Don't warn when the fill char and the length are both known to be
-  // zero.  No bug is possible.
-  memset(a, 0, v);
-  memset(a, v, 0);
-
-  // -1 is clearly not a length by virtue of being negative, so no warning
-  // despite v == 0.
-  memset(a, -1, v);
-
-  memtmpl<0, int>();
-}