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

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 d07b52a428bb8aa9c7da554f8dbdaf2bec0ee66d..177edd08cd8f9d462dce043b17b38b43dfe97da4 100644
--- a/tools/android/heap_profiler/heap_dump.c
+++ b/tools/android/heap_profiler/heap_dump.c
@@ -38,6 +38,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <time.h>
 #include <unistd.h>
 #include <sys/stat.h>
 
@@ -45,12 +46,27 @@
 
 
 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
@@ -89,6 +105,11 @@ static int dump_process_heap(int mem_fd, FILE* fmaps, bool dump_also_allocs) {
 
   // 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);
@@ -248,14 +269,33 @@ static FILE* open_proc_maps() {
   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;
@@ -263,11 +303,15 @@ int main(int argc, char** argv) {
       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;
   }
 
@@ -288,7 +332,7 @@ int main(int argc, char** argv) {
     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);