Add a clang tool to detect std::move(raw ptr)

tools/clang/move_raw/MoveRaw.cpp

Index: tools/clang/move_raw/MoveRaw.cpp
diff --git a/tools/clang/move_raw/MoveRaw.cpp b/tools/clang/move_raw/MoveRaw.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..a82aa3a7ce90a8bc10d7f75d33af4f7c11c37077
--- /dev/null
+++ b/tools/clang/move_raw/MoveRaw.cpp
@@ -0,0 +1,93 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Clang tool to find where std::move is called on a raw pointer.
+// Calling std::move on a raw pointer has no useful effect and is likely a
+// sign of an error (e.g., mistaking a raw pointer for a smart pointer).
+
+#include <memory>
+#include <string>
+
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/TargetSelect.h"
+
+using namespace clang::ast_matchers;
+using clang::tooling::CommonOptionsParser;
+using clang::tooling::Replacements;
+
+namespace {
+
+class MoveCallCollector : public MatchFinder::MatchCallback {
+ public:
+  explicit MoveCallCollector(Replacements* replacements)
+      : replacements_(replacements) {}
+  virtual void run(const MatchFinder::MatchResult& result) override;
+
+ private:
+  Replacements* const replacements_;
+};
+
+void MoveCallCollector::run(const MatchFinder::MatchResult& result) {
+  const clang::Expr* callsite =
+      result.Nodes.getNodeAs<clang::Expr>("move_call");
+  auto err = replacements_->add(clang::tooling::Replacement(

[vabr (Chromium), 2017/06/04 13:22:24]

It turned out that this hit errors in ~120 files where replacements from
multiple files were combined (a header and the .cc file, or two headers).
Looking at http://clang.llvm.org/docs/LibASTMatchersTutorial.html, checking
headers is actually not so useful and can be avoided by inserting, e.g., the
following block before line 44: 

  if (!callsite ||
      !result.SourceManager->isWrittenInMainFile(
          result.SourceManager->getSpellingLoc(callsite->getLocStart()))) {
    return;
  }

When I did that, I was able to eliminate all errors and find more instances of
std::move(raw) in .cc/.cpp files.

However, checking headers at least once does make sense to me, and I'm not sure
if that happens -- when I ran run_tool.py with a header file as an argument,
there was no output, even though the header itself contained std::move(raw).
Perhaps instead of checking isWrittenInMainFile I should check something else
which also works for headers?

[dcheng, 2017/06/07 19:58:34]

I would suggest just using a std::set<clang::tooling::Replacement> instead of
the built-in Replacements class.

[vabr (Chromium), 2017/06/08 12:37:49]

Done, with vector, because I'm not sure I need the uniqueness or sorting of set.

[dcheng, 2017/06/08 19:40:42]

Well, why emit duplicates? =)

[vabr (Chromium), 2017/06/09 07:02:57]

By "not sure I need the uniqueness" I meant "I cannot imagine how there could be
duplicates". :) But this container has on the order of units of elements, so I
am not concerned with unnecessary overhead coming from set, and I will change it
to set.

[dcheng, 2017/06/09 07:08:32]

I suppose in the context of how these tools are typically run (via the
run_tool.py helper), it won't happen. Generally speaking though, it's possible
to run one invocation of the clang tool across multiple files, and you can end
up with duplicates in that case.

[vabr (Chromium), 2017/06/09 07:19:38]

Got it, thanks! I agree that set is a better choice here for this reason.

+      *result.SourceManager,
+      result.SourceManager->getSpellingLoc(callsite->getLocStart()), 0,
+      "/*This tries to move a raw pointer!*/"));
+  assert(!err);
+}
+
+}  // namespace
+
+static llvm::cl::extrahelp common_help(CommonOptionsParser::HelpMessage);
+
+int main(int argc, const char* argv[]) {
+  // TODO(dcheng): Clang tooling should do this itself.
+  // https://llvm.org/bugs/show_bug.cgi?id=21627
+  llvm::InitializeNativeTarget();
+  llvm::InitializeNativeTargetAsmParser();
+  llvm::cl::OptionCategory category(
+      "Catching red flags: calling std::move on raw pointers");
+  CommonOptionsParser options(argc, argv, category);
+  clang::tooling::ClangTool tool(options.getCompilations(),
+                                 options.getSourcePathList());
+
+  MatchFinder match_finder;
+  Replacements replacements;
+
+  StatementMatcher move_on_raw_matcher =
+      callExpr(argumentCountIs(1), callee(functionDecl(hasName("::std::move"))),
+               hasArgument(0, hasType(pointerType())))
+          .bind("move_call");
+  MoveCallCollector callback(&replacements);
+  match_finder.addMatcher(move_on_raw_matcher, &callback);
+
+  std::unique_ptr<clang::tooling::FrontendActionFactory> factory =
+      clang::tooling::newFrontendActionFactory(&match_finder);
+  int result = tool.run(factory.get());
+  if (result != 0)
+    return result;
+
+  // Serialization format is documented in tools/clang/scripts/run_tool.py
+  llvm::outs() << "==== BEGIN EDITS ====\n";

[vabr (Chromium), 2017/06/04 10:56:17]

I was wondering if I should only emit BEGIN EDITS and END EDITS if there is >0
replacements, to avoid log spam. But the existing tools I saw always emit those,
even if no replacements are found.

[dcheng, 2017/06/07 19:58:34]

I don't think it matters too much, since we need to postprocess these through a
script anyway.

[vabr (Chromium), 2017/06/08 12:37:49]

If it does not matter much, then I propose to make it silent for cases with no
replacements. Dumping tens of thousands of unnecessary lines and then filtering
them again makes processing slow, even if it is done by scripts.

[dcheng, 2017/06/08 19:40:42]

*shrug*

I don't feel strongly. But 2 extra lines in several gigabytes of output is
unlikely to make a real difference.

[vabr (Chromium), 2017/06/09 07:02:57]

It was not 2 extra lines. When I ran the tool, tens thousands of files
contributed just the empty BEGIN/END sequence, with about 10-20 contributing
also some replacements. The file had close to 100000 lines, and only about 100
of them were actually useful.

[dcheng, 2017/06/09 07:06:41]

Sorry, to clarify, I mean 2 extra lines per invocation of the tool. I'm
surprised that there were 100k lines of output though: IIRC, there's only
something like 25k .cc targets, so I would have expected something on the order
of ~50k lines maybe.

I don't feel super strongly about it.

[vabr (Chromium), 2017/06/09 07:19:38]

My apologies, I was sloppy with the estimate (I did not actually count the
lines, because that file is long gone by now). ~50k sounds more plausible for
what it had. 

IMO, even with that update the empty BEGIN/END blocks make a vast majority of
the large result, so given you don't feel strongly, I will keep the early exit
without logging if there are new replacements (introduced in patch set 3).

+  for (const auto& r : replacements) {
+    std::string replacement_text = r.getReplacementText().str();
+    std::replace(replacement_text.begin(), replacement_text.end(), '\n', '\0');
+    llvm::outs() << "r:::" << r.getFilePath() << ":::" << r.getOffset()
+                 << ":::" << r.getLength() << ":::" << replacement_text << "\n";
+  }
+  llvm::outs() << "==== END EDITS ====\n";
+
+  return 0;
+}