[Android] libheap_profiler/heap_dump: use ptrace instead of SIGSTOP.

tools/android/heap_profiler/heap_dump.c

Index: tools/android/heap_profiler/heap_dump.c
diff --git a/tools/android/heap_profiler/heap_dump.c b/tools/android/heap_profiler/heap_dump.c
index 731833ec13710e3d2df3b27a68dd1192e6d861e1..7fea55fa8808308c567a48bd9deb57a510d49be0 100644
--- a/tools/android/heap_profiler/heap_dump.c
+++ b/tools/android/heap_profiler/heap_dump.c
@@ -4,8 +4,6 @@
 
 // The client dump tool for libheap_profiler. It attaches to a process (given
 // its pid) and dumps all the libheap_profiler tracking information in JSON.
-// The target process is frozen (SIGSTOP) while dumping, unless the -n opt.
-// is passed (in which case the caller is responsible for un/freezing).
 // The JSON output looks like this:
 // {
 //   "total_allocated": 908748493,   # Total bytes allocated and not freed.
@@ -33,13 +31,13 @@
 
 #include <fcntl.h>
 #include <inttypes.h>
-#include <signal.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <time.h>
 #include <unistd.h>
+#include <sys/ptrace.h>
 #include <sys/stat.h>
 
 #include "tools/android/heap_profiler/heap_profiler.h"
@@ -55,6 +53,7 @@ static int dump_process_heap(
     int mem_fd,
     FILE* fmaps,
     bool dump_also_allocs,
+    bool pedantic,  // Enable pedantic consistency checks on memory counters.
     char* comment) {
   HeapStats stats;
   time_t tm;
@@ -83,8 +82,15 @@ static int dump_process_heap(
 
     // The mmap looks promising. Let's check for the magic marker.
     lseek_abs(mem_fd, start);
-    if (read(mem_fd, &stats, sizeof(stats)) < sizeof(stats))
-      continue;
+    ssize_t rsize = read(mem_fd, &stats, sizeof(stats));
+
+    if (rsize == -1) {
+      perror("read");
+      return -1;
+    }
+
+    if (rsize < sizeof(stats))
+      continue;  // A false positive.

[pasko, 2014/09/10 12:09:18]

this could be a short read, please check for EINTR and repeat the read, just
like we do in base/posix/eintr_wrapper.h

Please do that for all read() in this file.

The term "false positive" in the comment is a bit too generic. Can we just say
"This mapping has a too narrow of a margin to contain HeapStats", or something
like that? I would be OK with removing the comment whatsoever, because the code
speaks for itself.

[Primiano Tucci (use gerrit), 2014/09/10 13:17:35]

Very hard this will happen (The Android framework is oblivious of the existance
of this process, as it is a shell process, and I don't see who could send a
signal here).
But adding the wrapper should be pretty straightforward.


I would be OK with removing the comment whatsoever, because the code
Me too. Removing the comment.

 
     if (stats.magic_start == HEAP_PROFILER_MAGIC_MARKER) {
       stats_mmap_found = true;
@@ -150,14 +156,14 @@ static int dump_process_heap(
     }
     printf("},\n");
 
-    if (dbg_counted_allocs != stats.num_allocs) {
+    if (pedantic && dbg_counted_allocs != stats.num_allocs) {
       fprintf(stderr,
               "ERROR: inconsistent alloc count (%"PRIu32" vs %"PRIu32").\n",
               dbg_counted_allocs, stats.num_allocs);
       return -1;
     }
 
-    if (dbg_counted_total_alloc_bytes != stats.total_alloc_bytes) {
+    if (pedantic && dbg_counted_total_alloc_bytes != stats.total_alloc_bytes) {
       fprintf(stderr, "ERROR: inconsistent alloc totals (%zu vs %zu).\n",
               dbg_counted_total_alloc_bytes, stats.total_alloc_bytes);
       return -1;
@@ -201,7 +207,7 @@ static int dump_process_heap(
   }
   printf("}\n}\n");
 
-  if (dbg_counted_total_alloc_bytes != stats.total_alloc_bytes) {
+  if (pedantic && dbg_counted_total_alloc_bytes != stats.total_alloc_bytes) {
     fprintf(stderr, "ERROR: inconsistent stacks totals (%zu vs %zu).\n",
             dbg_counted_total_alloc_bytes, stats.total_alloc_bytes);
     return -1;
@@ -211,25 +217,6 @@ static int dump_process_heap(
   return 0;
 }
 
-// If the dump is interrupted, resume the target process before exiting.
-static void exit_handler() {
-  kill(pid, SIGCONT);
-  waitpid(pid, NULL, 0);
-  exit(-1);
-}
-
-static bool freeze_process() {
-  if (kill(pid, SIGSTOP) != 0) {
-    fprintf(stderr, "Could not freeze the target process.\n");
-    perror("kill");
-    return false;
-  }
-
-  signal(SIGPIPE, exit_handler);
-  signal(SIGINT, exit_handler);
-  return true;
-}
-
 // Unfortunately lseek takes a *signed* offset, which is unsuitable for large
 // files like /proc/X/mem on 64-bit.
 static void lseek_abs(int fd, size_t off) {
@@ -287,18 +274,18 @@ static void read_proc_cmdline(char* cmdline, int size) {
 int main(int argc, char** argv) {
   char c;
   int ret = 0;
-  bool should_freeze_process = true;
   bool dump_also_allocs = false;
+  bool pedantic = true;
   char comment[1024] = { '\0' };
 
-  while (((c = getopt(argc, argv, "nxc:")) & 0x80) == 0) {
+  while (((c = getopt(argc, argv, "xnc:")) & 0x80) == 0) {

[pasko, 2014/09/10 12:09:18]

Please provide a short human-readable description of these individual options. I
think having them in this file's comments is OK, but I would prefer a --help
printout of course.

[Primiano Tucci (use gerrit), 2014/09/10 13:17:35]

OK. I'll expand the usage string.

    switch (c) {
-      case 'n':
-        should_freeze_process = false;
-        break;
       case 'x':
         dump_also_allocs = true;
         break;
+      case 'n':
+        pedantic = false;
+        break;
       case 'c':
         strlcpy(comment, optarg, sizeof(comment));
         break;
@@ -312,8 +299,10 @@ int main(int argc, char** argv) {
 
   pid = atoi(argv[optind]);
 
-  if (should_freeze_process && !freeze_process())
+  if (ptrace(PTRACE_ATTACH, pid, NULL, NULL) == -1) {

[pasko, 2014/09/10 12:09:18]

ptrace interface is tricky and I am not a good enough expert here. I think it's
a good idea to ask eugenis@ to have a quick look at this, since I remember him
playing with ptrace extensively.

Will PTRACE_ATTACH continue to be allowed on L+? Some distributions require
PR_SET_PTRACER followed by PTRACE_TRACEME.

[Primiano Tucci (use gerrit), 2014/09/10 13:17:35]

Very hard to tell. However PTRACE_ATTACH is the way gdb --attach works, and I
hope Android will keep supporting gdb :)

Some distributions require
That is different and must be done by the traced process (child), not by the
tracer.

In general, AFAICT, as long as the tracer is root everything should be fine.
Adding euginins@ to doublecheck btw.

+    perror("ptrace");
     return -1;
+  }
 
   // Wait for the process to actually freeze.
   waitpid(pid, NULL, 0);
@@ -327,10 +316,9 @@ int main(int argc, char** argv) {
     ret = -1;
 
   if (ret == 0)
-    ret = dump_process_heap(mem_fd, fmaps, dump_also_allocs, comment);
+    ret = dump_process_heap(mem_fd, fmaps, dump_also_allocs, pedantic, comment);
 
-  if (should_freeze_process)
-    kill(pid, SIGCONT);
+  ptrace(PTRACE_DETACH, pid, NULL, NULL);

[pasko, 2014/09/10 12:09:18]

ptrace manual page says:
"""
While being traced, the tracee will stop each time a signal is delivered, even
if the signal is being ignored.
"""

It seems the code as it is now ignores the signal:
"""
After signal-delivery-stop is observed by the tracer, the tracer
should restart the tracee with the call:
    ptrace(PTRACE_restart, pid, 0, sig)
If sig is 0, then a signal is not delivered.
"""

[pasko, 2014/09/10 12:45:53]

please disregard this comment, the quote from above should apply to the signals
that occurred while the process runs and is being traced. We have a simpler
case, the program is either traced or running.

(I am not 100% sure that I read the manual correctly though)

[Primiano Tucci (use gerrit), 2014/09/10 13:17:35]

That is for the case of fork + TRACEME. In the case of PTRACE_ATTACH, the tracee
is already stopped, so it's very hard it will be stopped more than that. :)
Which signal? The signal eventually goes to the tracee, not to this process.
Again, this is for the case of the tracee being alive and the tracer wanting to
be notified when the tracee gets a signal / exception. This is not the case
here.

 
   // Cleanup.
   fflush(stdout);