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

base/process_util_posix.cc

 // Copyright (c) 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.
 
 #include <dirent.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <signal.h>
 #include <stdlib.h>
 #include <sys/resource.h>
@@ skipping 15 matching lines @@
 #include "base/files/dir_reader_posix.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/process_util.h"
 #include "base/stringprintf.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #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
 
 #if defined(OS_FREEBSD)
 #include <sys/event.h>
 #include <sys/ucontext.h>
 #endif
 
 #if defined(OS_MACOSX)
@@ skipping 588 matching lines @@
     if (options.new_process_group) {
       // Instead of inheriting the process group ID of the parent, the child
       // starts off a new process group with pgid equal to its process ID.
       if (setpgid(0, 0) < 0) {
         RAW_LOG(ERROR, "setpgid failed");
         _exit(127);
       }
     }
 
     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();
            resource != options.maximize_rlimits->end();
            ++resource) {
         struct rlimit limit;
         if (getrlimit(*resource, &limit) < 0) {
           RAW_LOG(WARNING, "getrlimit failed");
         } else if (limit.rlim_cur < limit.rlim_max) {
           limit.rlim_cur = limit.rlim_max;
           if (setrlimit(*resource, &limit) < 0) {
             RAW_LOG(WARNING, "setrlimit failed");
           }
         }
       }
+#ifdef USE_ALLOCATED_TYPE
+      ResumeAllocatedTypeIntercept();
+#endif
     }
 
 #if defined(OS_MACOSX)
     RestoreDefaultExceptionHandler();
 #endif  // defined(OS_MACOSX)
 
     ResetChildSignalHandlersToDefaults();
 
 #if defined(OS_MACOSX)
     if (options.synchronize) {
@@ skipping 22 matching lines @@
                 ioctl(options.ctrl_terminal_fd, TIOCSCTTY, NULL)) == -1) {
           RAW_LOG(WARNING, "ioctl(TIOCSCTTY), ctrl terminal not set");
         }
       } else {
         RAW_LOG(WARNING, "setsid failed, ctrl terminal not set");
       }
     }
 #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) {
       // Do a blocking read to wait until the parent says it's OK to proceed.
       // The byte that's read here is written by LaunchSynchronize.
       char read_char;
       int read_result =
           HANDLE_EINTR(read(*synchronization_read_fd.get(), &read_char, 1));
       if (read_result != 1) {
         RAW_LOG(ERROR, "LaunchProcess: synchronization read: error");
         _exit(127);
       }
 
       // The pipe is no longer useful. Don't let it live on in the new process
       // after exec.
       synchronization_read_fd.reset();  // closes synchronization_pipe_fds[0]
     }
 #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:");
     RAW_LOG(ERROR, argv_cstr[0]);
     _exit(127);
   } else {
     // Parent process
     if (options.wait) {
       // While this isn't strictly disk IO, waiting for another process to
       // finish is the sort of thing ThreadRestrictions is trying to prevent.
@@ skipping 342 matching lines @@
 
         // Obscure fork() rule: in the child, if you don't end up doing exec*(),
         // you call _exit() instead of exit(). This is because _exit() does not
         // call any previously-registered (in the parent) exit handlers, which
         // might do things like block waiting for threads that don't even exist
         // in the child.
         int dev_null = open("/dev/null", O_WRONLY);
         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));
         // Adding another element here? Remeber to increase the argument to
         // reserve(), above.
 
         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
           execve(argv_cstr[0], argv_cstr.get(), envp);
         _exit(127);
       }
     default:  // parent
       {
         // Close our writing end of pipe now. Otherwise later read would not
         // be able to detect end of child's output (in theory we could still
@@ skipping 190 matching lines @@
   if (IsChildDead(process))
     return;
 
   BackgroundReaper* reaper = new BackgroundReaper(process, 0);
   PlatformThread::CreateNonJoinable(0, reaper);
 }
 
 #endif  // !defined(OS_MACOSX)
 
 }  // namespace base