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

base/process_util_posix.cc

Index: base/process_util_posix.cc
diff --git a/base/process_util_posix.cc b/base/process_util_posix.cc
index a6e816af4484eeeb545cded16f9e4dd25533a62b..394b1600767b2a57cc8ba1719ccf7f45cfceec20 100644
--- a/base/process_util_posix.cc
+++ b/base/process_util_posix.cc
@@ -33,6 +33,10 @@
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread_restrictions.h"
 
+#ifdef USE_ALLOCATED_TYPE
+#include "base/allocator/allocated_type_tcmalloc.h"
+#endif
+
 #if defined(OS_CHROMEOS)
 #include <sys/ioctl.h>
 #endif
@@ -641,6 +645,9 @@ bool LaunchProcess(const std::vector<std::string>& argv,
     }
 
     if (options.maximize_rlimits) {
+#ifdef USE_ALLOCATED_TYPE
+      SuspendAllocatedTypeIntercept();
+#endif

[Dai Mikurube (NOT FULLTIME), 2012/08/02 07:43:55]

These are to avoid the DANGER of http://crbug.com/36678.

This change acquires locks in 'new' and 'delete'. As a result, some STL
operations cause lock acquisitions.  These SuspendAllocatedTypeIntercept stops
lock acquisitions.

I know they make the code very unreadable...  It is bad.  If someone has good
ideas to fix it, please tell me that.

[Nico, 2012/08/02 15:35:03]

As far as I understand the bug, any code that does allocation between the fork
above and the exec below is wrong. So the allocations should be removed
(independent of your change), and then you don't need to change this file (?)

[Dai Mikurube (NOT FULLTIME), 2012/08/02 16:14:35]

In my understanding, they don't allocate (malloc) additional memory here.  "new"
is really called in STL, but they are placement news.

My interceptor catches also placement news, and it instruments lock
acquisitions.  That is the problem here.


In fact, I'm not completely sure that additional memory
is not allocated. It may sometimes happen by <allocator>?
Removing all STL operations may contribute to stability.

[jar (doing other things), 2012/08/02 23:38:30]

I don't follow this motivation.

...but... if your goal is to make this code more readable by not injecting the
ifdefs, consider always calling the functions, but having the functions become
no-ops (in the implementation file) when !defined(USE_ALLOCATED_TYPE).

The compiler will generally optimize out the no-op function... so the impact is
identical on released code (and readability is higher).

On 2012/08/02 07:43:55, Dai Mikurube wrote:

[Dai Mikurube (NOT FULLTIME), 2012/08/03 10:01:50]

Thank you.  That is a good idea.

For now, I keep it as is since we should consider more (including removing STL
operations) around this issue.  I added a temporary comment on #include
"base/allocator/allocated_type_tcmalloc.h".

[jar (doing other things), 2012/08/04 01:13:47]

Your original comment was "...unreadable. It is bad. ...ideas to fix, please
tell me".

My suggestion to make it readable was to remove the ifdef here, and put it in
the called function. 

Why not do it? Why leave the code harder to read?


On 2012/08/03 10:01:50, Dai Mikurube wrote:

[Dai Mikurube (NOT FULLTIME), 2012/08/07 10:39:18]

I'm still thinking also about removing all STL operations from this function. 
With that, we don't need to call Suspend... and Resume... anywhere.

Since Suspend... and Resume... are the only changes in this file, I decided to
change this file after the final decision.

On 2012/08/04 01:13:47, jar wrote:

       // Some resource limits need to be maximal in this child.
       std::set<int>::const_iterator resource;
       for (resource = options.maximize_rlimits->begin();
@@ -656,6 +663,9 @@ bool LaunchProcess(const std::vector<std::string>& argv,
           }
         }
       }
+#ifdef USE_ALLOCATED_TYPE
+      ResumeAllocatedTypeIntercept();
+#endif
     }
 
 #if defined(OS_MACOSX)
@@ -698,32 +708,54 @@ bool LaunchProcess(const std::vector<std::string>& argv,
 #endif  // defined(OS_CHROMEOS)
 
     if (options.fds_to_remap) {
+#ifdef USE_ALLOCATED_TYPE
+      SuspendAllocatedTypeIntercept();
+#endif
       for (FileHandleMappingVector::const_iterator
                it = options.fds_to_remap->begin();
            it != options.fds_to_remap->end(); ++it) {
         fd_shuffle1.push_back(InjectionArc(it->first, it->second, false));
         fd_shuffle2.push_back(InjectionArc(it->first, it->second, false));
       }
+#ifdef USE_ALLOCATED_TYPE
+      ResumeAllocatedTypeIntercept();
+#endif
     }
 
 #if defined(OS_MACOSX)
     if (options.synchronize) {
+#ifdef USE_ALLOCATED_TYPE
+      SuspendAllocatedTypeIntercept();
+#endif
       // Remap the read side of the synchronization pipe back onto itself,
       // ensuring that it won't be closed by CloseSuperfluousFds.
       int keep_fd = *synchronization_read_fd.get();
       fd_shuffle1.push_back(InjectionArc(keep_fd, keep_fd, false));
       fd_shuffle2.push_back(InjectionArc(keep_fd, keep_fd, false));
+#ifdef USE_ALLOCATED_TYPE
+      ResumeAllocatedTypeIntercept();
+#endif
     }
 #endif  // defined(OS_MACOSX)
 
     if (options.environ)
       SetEnvironment(new_environ.get());
 
+#ifdef USE_ALLOCATED_TYPE
+    SuspendAllocatedTypeIntercept();
+#endif
     // fd_shuffle1 is mutated by this call because it cannot malloc.
-    if (!ShuffleFileDescriptors(&fd_shuffle1))
+    if (!ShuffleFileDescriptors(&fd_shuffle1)) {
+#ifdef USE_ALLOCATED_TYPE
+      ResumeAllocatedTypeIntercept();
+#endif
       _exit(127);
+    }
 
     CloseSuperfluousFds(fd_shuffle2);
+#ifdef USE_ALLOCATED_TYPE
+      ResumeAllocatedTypeIntercept();
+#endif
 
 #if defined(OS_MACOSX)
     if (options.synchronize) {
@@ -743,9 +775,15 @@ bool LaunchProcess(const std::vector<std::string>& argv,
     }
 #endif  // defined(OS_MACOSX)
 
+#ifdef USE_ALLOCATED_TYPE
+    SuspendAllocatedTypeIntercept();
+#endif
     for (size_t i = 0; i < argv.size(); i++)
       argv_cstr[i] = const_cast<char*>(argv[i].c_str());
     argv_cstr[argv.size()] = NULL;
+#ifdef USE_ALLOCATED_TYPE
+    ResumeAllocatedTypeIntercept();

[Alexander Potapenko, 2012/08/02 14:57:12]

There's no point in resuming the interception if the child process is known to
call exec() later.

[Dai Mikurube (NOT FULLTIME), 2012/08/03 10:01:50]

Ah, right.  For now, I kept it as is, but I added a comment.  (I don't change
the code before decision.)

[jar (doing other things), 2012/08/04 01:13:47]

It strikes me that when you fork, you don't want to get copies of duplicate data
(from the two sides of the fork).  As a result, I'd expect you to permanently
stop collecting in the child (that will exec), and abandoning your data (that
was collected pre-fork) from the child.

What am I missing?

On 2012/08/03 10:01:50, Dai Mikurube wrote:

[Dai Mikurube (NOT FULLTIME), 2012/08/07 10:39:18]

Ahhh, maybe, you are right.  guessing we can suspend in the beginning of a child
process, and no resume...  I'll try it just in case.  (It's not fixed in the
uploaded patch, but I'm trying it locally.  I'll upload it later soon.)

On 2012/08/04 01:13:47, jar wrote:

+#endif
     execvp(argv_cstr[0], argv_cstr.get());
 
     RAW_LOG(ERROR, "LaunchProcess: failed to execvp:");
@@ -1108,6 +1146,9 @@ static GetAppOutputInternalResult GetAppOutputInternal(
         if (dev_null < 0)
           _exit(127);
 
+#ifdef USE_ALLOCATED_TYPE
+        SuspendAllocatedTypeIntercept();
+#endif
         fd_shuffle1.push_back(InjectionArc(pipe_fd[1], STDOUT_FILENO, true));
         fd_shuffle1.push_back(InjectionArc(dev_null, STDERR_FILENO, true));
         fd_shuffle1.push_back(InjectionArc(dev_null, STDIN_FILENO, true));
@@ -1117,14 +1158,21 @@ static GetAppOutputInternalResult GetAppOutputInternal(
         std::copy(fd_shuffle1.begin(), fd_shuffle1.end(),
                   std::back_inserter(fd_shuffle2));
 
-        if (!ShuffleFileDescriptors(&fd_shuffle1))
+        if (!ShuffleFileDescriptors(&fd_shuffle1)) {
+#ifdef USE_ALLOCATED_TYPE
+          ResumeAllocatedTypeIntercept();
+#endif
           _exit(127);
+        }
 
         CloseSuperfluousFds(fd_shuffle2);
 
         for (size_t i = 0; i < argv.size(); i++)
           argv_cstr[i] = const_cast<char*>(argv[i].c_str());
         argv_cstr[argv.size()] = NULL;
+#ifdef USE_ALLOCATED_TYPE
+        ResumeAllocatedTypeIntercept();
+#endif
         if (do_search_path)
           execvp(argv_cstr[0], argv_cstr.get());
         else