Type profiler by intercepting 'new' and 'delete' expressions.

base/allocator/type_profiler.h

Index: base/allocator/type_profiler.h
diff --git a/base/allocator/type_profiler.h b/base/allocator/type_profiler.h
new file mode 100644
index 0000000000000000000000000000000000000000..babc3d8f4482fac82c984ac6b56465b09fee7301
--- /dev/null
+++ b/base/allocator/type_profiler.h
@@ -0,0 +1,59 @@
+// Copyright 2012 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.
+
+#ifndef BASE_ALLOCATOR_TYPE_PROFILER_H_
+#define BASE_ALLOCATOR_TYPE_PROFILER_H_
+
+#if defined(TYPE_PROFILING)
+
+#include <cstddef>  // for size_t
+#include <typeinfo>  // for std::typeinfo
+
+// These two functions wrap all 'new' and 'delete' expressions.
+// They are called by a modified version of Clang checked-in
+// at deps/third_party/llvm-allocated-type.
+// These prototype declarations are required to make them friends of
+// InterceptFunctions below.
+
+void* __op_new_intercept__(void* ptr,
+                           size_t size,
+                           const std::type_info& type);
+
+void* __op_delete_intercept__(void* ptr,
+                              size_t size,
+                              const std::type_info& type);
+

[Ryan Sleevi, 2012/09/12 19:13:34]

I think I may have to disagree with Jim here, unfortunately, in that I think
these two functions above are logically and functionally different than the
remaining functions below.

I still strongly believe that the declarations for
__op_new_intercept__/__op_delete_intercept__ should be provided by the compiler.
At that point, you would not/do not need to inject this header into all targets,
and so the only dependency on 'type_profiler' would be
'allocator_extension_thunks' (eg: you would remove the global injection from
build/common.gypi)

[Ryan Sleevi, 2012/09/12 22:04:18]

Have you checked in the latest version of your clang plugin?

I didn't see the .gypi forcing a -include, but I also didn't see your (current)
clang plugin providing a declaration for these functions. So I'm not sure where
the names are coming from. If the checked in copy is the latest, then I fear it
may be relying on Clang's inherited-from-GCC ability to implicitly assume the
definitions of functions and then let the linker resolve them, which would be
bad.

[Dai Mikurube (NOT FULLTIME), 2012/09/13 01:12:40]

The latest modified compiler has been checked-in.  My extension is implemented
by modifying Clang code.  Declaration of these functions are now instrumented by
the modified Clang binary.

The compiler option is not provided for this file (now, it is not included
automatically).  So, these functions are not declared automatically in this
file.

[Ryan Sleevi, 2012/09/13 01:34:59]

Have you not updated
http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/llvm-allocated-t...
then?

The last update to this file was before we requested you to implement the
declarations, and the current implementation is still reliant on the identifier
table having a declaration existing in the translation unit:

+    IdentifierInfo *NewInterceptInfo =
+        &PP.getIdentifierTable().get("__op_new_intercept__");
+    LookupResult R(*this, NewInterceptInfo, SourceLocation(),
+                   LookupOrdinaryName);
+    LookupQualifiedName(R, Context.getTranslationUnitDecl());

+namespace base {
+namespace type_profiler {
+
+typedef void* InterceptFunction(void*, size_t, const std::type_info&);
+
+class InterceptFunctions {
+ public:
+  // It must be called only once in a process while it is in single-thread.
+  // For now, ContentMainRunnerImpl::Initialize is the only supposed caller
+  // of this function except for single-threaded unit tests.
+  static void SetFunctions(InterceptFunction* new_intercept,
+                           InterceptFunction* delete_intercept);

[Ryan Sleevi, 2012/09/12 19:13:34]

It's unclear to me why this is a static class, given that it only has a single
static function, and it's already within a namespace.

If I were to wager a guess (without having read all of the review feedback with
Jim), I suspect it's to accomodate the friend definitions from lines 41-43.

A different way to accomodate this would be to structure the headers as such:

type_profiler.h
  - Has a single function, SetFunctions(...)
type_profiler_internal.h
  - Has a function ResetFunctions (for unit tests)
  - Has a function IsAvailable (for the implementation of
_new/_delete_intercept__)

type_profiler.cc
  - Actually implements all three functions. The static variables (eg: line 50,
51) are then kept entirely opaque/internal to that .cc file

[Ryan Sleevi, 2012/09/12 22:04:18]

I spoke with Jim, and while we agree both solutions would work, let's keep this
as is for now (eg: do not implement the separation).

Despite the style guide discouraging classes-as-static-method-containers (as
opposed to using namespaces), it does provide a useful check here, in that it
*forces* the compiler to check the access rules, as opposed to _internal.h,
which anybody "could" include.

[Dai Mikurube (NOT FULLTIME), 2012/09/13 01:12:40]

Ok, I keep it as is.

+
+ private:
+  friend class TypeProfilerTest;
+  friend void* ::__op_new_intercept__(void*, size_t, const std::type_info&);
+  friend void* ::__op_delete_intercept__(void*, size_t, const std::type_info&);
+
+  // These functions are not thread safe.
+  // They must be used only from single-threaded unit tests.
+  static void ResetFunctions();
+  static bool IsAvailable();
+
+  static InterceptFunction* new_intercept_;
+  static InterceptFunction* delete_intercept_;

[Ryan Sleevi, 2012/09/12 22:04:18]

However, please remove all static member variables from these classes, as
they're unnecessary.

The 'leak' implementation details to everyone who includes the .h, and, because
there are concrete friends defined, it may allow them to inadvertently modify
these variables.

Instead, move the definition and declaration of these variables to an unnamed
namespace within the .cc file that backs this header, and have
SetFunctions/ResetFunctions modify them.

They should follow the global naming scheme (g_new_intercept,
g_delete_intercept)

Please do this for all static members of classes within this CL.

[Dai Mikurube (NOT FULLTIME), 2012/09/13 04:00:12]

Done.  As a side effect, we could remove forward declarations and friend
declaration for __op_*.

+};
+
+}  // namespace type_profiler
+}  // namespace base
+
+#endif  // defined(TYPE_PROFILING)
+
+#endif  // BASE_ALLOCATOR_TYPE_PROFILER_H_