Add some convenient metadata in Android's heap profiler dump.

tools/android/heap_profiler/heap_dump.c

 // Copyright 2014 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.
 
 // 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:
 // {
@@ skipping 20 matching lines @@
 //       +---------------------> Index of the entry (as for "allocs" xref).
 //                               Indexes are hex and might not be monotonic.
 
 #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/stat.h>
 
 #include "tools/android/heap_profiler/heap_profiler.h"
 
 
 static void lseek_abs(int fd, size_t off);
+static int read_proc_cmdline(char* cmdline, int size);
 
 static int pid;
 
 
-static int dump_process_heap(int mem_fd, FILE* fmaps, bool dump_also_allocs) {
+static int dump_process_heap(
+    int mem_fd,
+    FILE* fmaps,
+    bool dump_also_allocs,
+    char* comment) {
   HeapStats stats;
+  time_t tm;
+  struct tm local_tm;
+  char asc_tm[64];
+  char cmdline[512];
+
+  tm = time(NULL);
+  localtime_r(&tm, &local_tm);
+  strftime(asc_tm, 64, "%Y-%m-%d %H:%M:%S%z", &local_tm);

[Primiano Tucci (use gerrit), 2014/07/15 23:42:34]

Can we avoid extracting the time on the device and push it into the comment by
the host?
I'm considering this for two reasons:
- Most of the times, on testing devices, the timezone is screwed (nobody cares
about setting that correctly). The host instead should have the correct time.
- timer.h in the Android NDK (w.r.t of 64-bit) is sadly in a bad shape for
historical reasons. Take a look to src/base/time/time_posix.cc and all the crazy
ifdef logic there :)

[Dai Mikurube (NOT FULLTIME), 2014/07/16 06:24:57]

Makes sense especially for the latter. Changed it just to dump an epoch time in
seconds.

+
+  read_proc_cmdline(cmdline, 512);
 
   // Look for the mmap which contains the HeapStats in the target process vmem.
   // On Linux/Android, the libheap_profiler mmaps explicitly /dev/zero. The
   // region furthermore starts with a magic marker to disambiguate.
   bool stats_mmap_found = false;
   for (;;) {
     char line[1024];
     if (fgets(line, sizeof(line), fmaps) == NULL)
       break;
 
@@ skipping 18 matching lines @@
   }
 
   if (!stats_mmap_found) {
     fprintf(stderr, "Could not find the HeapStats area. "
                     "It looks like libheap_profiler is not loaded.\n");
     return -1;
   }
 
   // Print JSON-formatted output.
   printf("{\n");
+  printf("  \"pid\": %d,\n", pid);
+  printf("  \"time\": \"%s\",\n", asc_tm);
+  printf("  \"comment\": \"%s\",\n", comment);
+  printf("  \"cmdline\": \"%s\",\n", cmdline);
+  printf("  \"pagesize\": %d,\n", getpagesize());

[Primiano Tucci (use gerrit), 2014/07/15 23:42:34]

I'm curious, Is there any case in which this is expected to return a value !=
4096?
Also, all the units in this file are expressed in bytes. what is this for?

[Dai Mikurube (NOT FULLTIME), 2014/07/16 06:24:57]

Ah, it may not be required here. I used it in dmprof to align with page frames
in /proc/.../pagemap without any assumption, but here, page frames are provided
from memdump in plan.

I kept it as is since it's not big, but I'd remove it if you want.

   printf("  \"total_allocated\": %zu,\n", stats.total_alloc_bytes);
   printf("  \"num_allocs\":      %"PRIu32",\n", stats.num_allocs);
   printf("  \"num_stacks\":      %"PRIu32",\n", stats.num_stack_traces);
 
   uint32_t dbg_counted_allocs = 0;
   size_t dbg_counted_total_alloc_bytes = 0;
   bool prepend_trailing_comma = false;  // JSON syntax, I hate you.
   uint32_t i;
 
   // Dump the optional allocation table.
@@ skipping 139 matching lines @@
   char path[64];
   snprintf(path, sizeof(path), "/proc/%d/maps", pid);
   FILE* fmaps = fopen(path, "r");
   if (fmaps == NULL) {
     fprintf(stderr, "Could not open %s.\n", path);
     perror("fopen");
   }
   return fmaps;
 }
 
+static int read_proc_cmdline(char* cmdline, int size) {
+  char path[64];
+  snprintf(path, sizeof(path), "/proc/%d/cmdline", pid);
+  FILE* fcmdline = fopen(path, "r");

[Primiano Tucci (use gerrit), 2014/07/15 23:42:34]

I'd personally just use raw open and read in these simple cases, just to avoid
introducing dependencies if there isn't an actual benefit (you don't need to 
use any actual stdio facility, just read at most N bytes from a file).
But I'm fine either ways, choose the one you prefer.
I did use fopen and fgets only for the maps because I needed to read the file
line-by-line there.

[Dai Mikurube (NOT FULLTIME), 2014/07/16 06:24:57]

Okay, switched to open/read.

[Primiano Tucci (use gerrit), 2014/07/16 16:36:18]

Thanks :)

+  if (fcmdline == NULL) {
+    fprintf(stderr, "Could not open %s.\n", path);
+    perror("fopen");
+    return -1;
+  }
+  if (NULL == fgets(cmdline, size, fcmdline)) {
+    fprintf(stderr, "Could not read %s.\n", path);
+    perror("fgets");
+    return -1;
+  }
+  fclose(fcmdline);
+  return 0;
+}
+
 int main(int argc, char** argv) {
   char c;
   int ret = 0;
   bool should_freeze_process = true;
   bool dump_also_allocs = false;
+  char comment[1024];

[Primiano Tucci (use gerrit), 2014/07/15 23:42:33]

What about  just:
char comment[1024] = { '\0' };
and remove the l ine on 297?

[Dai Mikurube (NOT FULLTIME), 2014/07/16 06:24:57]

Done.

 
-
-  while (((c = getopt(argc, argv, "nx")) & 0x80) == 0) {
+  comment[0] = '\0';
+  while (((c = getopt(argc, argv, "nxc:")) & 0x80) == 0) {
    switch (c) {
       case 'n':
         should_freeze_process = false;
         break;
       case 'x':
         dump_also_allocs = true;
         break;
+      case 'c':
+        strncpy(comment, optarg, 1024);

[Primiano Tucci (use gerrit), 2014/07/15 23:42:34]

s/strncpy/strlcpy/.
strlcpy is safer and nowadays is supported on all the platforms we care.
Also you might want s/1024/sizeof(comment)/.
 

[Dai Mikurube (NOT FULLTIME), 2014/07/16 06:24:57]

Done.

+        comment[1023] = '\0';

[Primiano Tucci (use gerrit), 2014/07/15 23:42:34]

Precisely. By using strlcpy you don't need this anymore :)

[Dai Mikurube (NOT FULLTIME), 2014/07/16 06:24:57]

Acknowledged.

+        break;
      }
   }
 
   if (optind >= argc) {
-    printf("Usage: %s [-n] [-x] pid\n", argv[0]);
+    printf("Usage: %s [-n] [-x] [-c comment] pid\n", argv[0]);
     return -1;
   }
 
   pid = atoi(argv[optind]);
 
   if (should_freeze_process && !freeze_process())
     return -1;
 
   // Wait for the process to actually freeze.
   waitpid(pid, NULL, 0);
 
   int mem_fd = open_proc_mem_fd();
   if (mem_fd < 0)
     ret = -1;
 
   FILE* fmaps = open_proc_maps();
   if (fmaps == NULL)
     ret = -1;
 
   if (ret == 0)
-    ret = dump_process_heap(mem_fd, fmaps, dump_also_allocs);
+    ret = dump_process_heap(mem_fd, fmaps, dump_also_allocs, comment);
 
   if (should_freeze_process)
     kill(pid, SIGCONT);
 
   // Cleanup.
   fflush(stdout);
   close(mem_fd);
   fclose(fmaps);
   return ret;
 }