Clang plugin to check that unstable types are not used in IPC.

tools/clang/plugins/CheckIPCVisitor.h

Index: tools/clang/plugins/CheckIPCVisitor.h
diff --git a/tools/clang/plugins/CheckIPCVisitor.h b/tools/clang/plugins/CheckIPCVisitor.h
new file mode 100644
index 0000000000000000000000000000000000000000..d1b4d47fa2db85a1dc6cf031f6cc197e31fd2603
--- /dev/null
+++ b/tools/clang/plugins/CheckIPCVisitor.h
@@ -0,0 +1,97 @@
+// Copyright (c) 2016 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.
+
+// This plugin ensures that 32/64-bit unstable types are not used in IPC.

[dcheng, 2016/02/23 19:46:47]

s/plugin/check/

[Dmitry Skiba, 2016/02/24 14:39:04]

Done.

+//
+// Type (or typedef) is unstable if it changes size between 32/ 64-bit

[dcheng, 2016/02/23 19:46:47]

Nit: s/Type/A type/

[Dmitry Skiba, 2016/02/24 14:39:04]

Done.

+// platforms. However, it's impossible to accurately identify unstable
+// typedefs, because their definitions rely on the preprocessor. For
+// example uintptr_t is either unsigned int or unsigned long.
+//
+// So we're not trying to be accurate, and just blacklisting some types
+// that are known to be unstable:
+// 1. Types: long / unsigned long (but not typedefs to)
+// 2. Typedefs: intmax_t, uintmax_t, intptr_t, uintptr_t, wint_t,
+//    size_t, rsize_t, ssize_t, ptrdiff_t, dev_t, off_t, clock_t,
+//    time_t, suseconds_t (including typedefs to)
+//
+// The plugin checks for blacklisted types in:
+// 1. IPC::WriteParam() calls; additionally:
+//   a. WriteParam() can be used inside a template only if that template
+//      is IPC::ParamTraits<> specialization

[dcheng, 2016/02/23 19:46:46]

s/is IPC/is an IPC/

[Dmitry Skiba, 2016/02/24 14:39:04]

Done.

+//   b. When used in a template on a dependent type WriteParam() must
+//      explicitly specify template argument (WriteParam<T>(...)) because
+//      template arguments are checked by #2

[dcheng, 2016/02/23 19:46:47]

Hmm... is it possible to fix this shortcoming? It's kind of unfortunate, the
whole point of WriteParam is to be a type-deducing helper.

[Dmitry Skiba, 2016/02/24 14:39:04]

Yes, but that only for templates. Everywhere else WriteParam() is fully
permitted and still deduces types as usual.

The issue with templates is that we're blacklisting typedefs, but that
information exists only at the "top level". I.e. 

template <class T>
void DoWrite(const T& value) {
  WriteParam(value);
}

DoWrite<size_t>()
DoWrite<unsigned int>()

Both DoWrite calls will use the same unsigned int specialization (on 32-bit
Android), but we must fail on WriteParam() inside the first one. The proper way
is to build some system to infer types back to their typedefs outside of
templates, but that's hard and probably not doable in a plugin where we can only
observe final AST.

So I went different path - I prohibit usage of WriteParam() in templates, with
the exception of ParamTraits<> specializations. And inside ParamTraits<> I
simply don't check WriteParam() arguments at all. That mostly works, because
usually for a given IPC message WriteParam() ends up being called on everything
that was listed, i.e. for

IPC_MESSAGE((bool, MyClass, std::vector<char>))

we expect WriteParam() calls on bool, MyClass, std::vector<> and char.
IPC_MESSAGE uses IPC::CheckedTuple<> which recursively checks arguments, so
IPC_MESSAGE((std::vector<std::pair<bool, size_t>>)) fails.

However, there is a chance that ParamTraits<> might call WriteParam() on
something that was not checked by IPC::CheckedTuple<>. For example on size_type:

ParamTraits<std::vector<P>>::Write(vec) {
  WriteParam(vec.size());
}

To protect from that I require WriteParam() calls to explicitly reference
template argument. Or to explicitly cast to something that doesn't depend on
template argument, in which case it's checked by a standard WriteParam() check
(ran on template itself).

As you can see from ipc_message_utils.h most explicit WriteParam specializations
are straightforward. And for corner cases we have WriteParamUnchecked() (see
SmallMap specialization). 

[dcheng, 2016/02/24 22:57:42]

So assume I have a clang::CallExpr representing the WriteParam() call above.
Does that mean that write_param_call->getArg(0)->getType() returns a QualType
like "unsigned int" instead of "size_t"?
Alternatively, can we wait until the template is instantiated and we have
concrete types and try to emit the warning then?
It's simple, but still an anti-pattern. And it has the very real possibility
that the explicitly-specified template arg might differ from the actual type of
the argument. If there's any way to avoid this, we should try to find it.

+// 2. IPC::CheckedTuple<> specializations, both in CheckedTuple<> and
+//    all specializations it references (i.e. std::vector<Set<size_t>> is
+//    not allowed because size_t is blacklisted)
+
+#ifndef TOOLS_CLANG_PLUGINS_CHECKIPC_VISITOR_H_
+#define TOOLS_CLANG_PLUGINS_CHECKIPC_VISITOR_H_
+
+#include <vector>
+
+#include "clang/AST/AST.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Frontend/CompilerInstance.h"
+
+namespace chrome_checker {
+
+class CheckIPCVisitor {
+ public:
+  explicit CheckIPCVisitor(clang::CompilerInstance& compiler);
+
+  void set_context(clang::ASTContext* context) { context_ = context; }
+
+  bool shouldVisitTemplateInstantiations() const { return true; }

[dcheng, 2016/02/23 19:46:47]

NameLikeThis or name_like_this please.

[Dmitry Skiba, 2016/02/24 14:39:04]

Done.

+
+  void BeginDecl(clang::Decl*);
+  void EndDecl();
+  void VisitTemplateSpecializationType(clang::TemplateSpecializationType*);
+  void VisitCallExpr(clang::CallExpr*);
+
+ private:
+  /* ValidateXXX functions return false if validation failed and diagnostic

[dcheng, 2016/02/23 19:46:47]

Nit: Use C++ style // comments for consistency.

[Dmitry Skiba, 2016/02/24 14:39:04]

Done.

+     was reported. They return true otherwise (not applicable / validation
+     succeeded).
+  */
+
+  bool ValidateWriteParam(const clang::CallExpr* call_expr) const;
+
+  bool ValidateWriteParamSignature(const clang::CallExpr* call_expr) const;
+
+  bool ValidateWriteParamInsideTemplate(
+      const clang::CallExpr* call_expr,
+      const clang::UnresolvedLookupExpr* lookup_expr) const;
+
+  bool ValidateWriteParamArgument(const clang::Expr* arg_expr) const;
+
+  bool ValidateWriteParamCaller(const clang::CallExpr* call_expr) const;
+
+  bool ValidateCheckedTuple(
+      const clang::TemplateSpecializationType* spec) const;
+
+  bool IsInsideParamTraits() const;
+
+  template <typename T>
+  const T* GetParentDecl() const;
+
+  clang::CompilerInstance& compiler_;
+  clang::ASTContext* context_;
+
+  unsigned error_write_param_bad_type_;
+  unsigned error_tuple_bad_type_;
+
+  unsigned error_write_param_template_arg_;
+  unsigned error_write_param_wrong_template_;
+  unsigned error_write_param_bad_signature_;
+  unsigned note_see_here_;
+
+  std::vector<const clang::Decl*> decl_stack_;
+};
+
+}  // namespace chrome_checker
+
+#endif  // TOOLS_CLANG_PLUGINS_CHECKIPC_VISITOR_H_