profile filename doesn't include pid if child doesn't call exec()

[GoogleCodeExporter]

What steps will reproduce the problem?
Our application has a parent process that forks several children, but some of 
these children don't call exec().

What is the expected output? What do you see instead?
When the application exits, the children that didn't call exec all get dumped 
to the same file overwriting each other:

Dumping heap profile to normal.0001.heap (Exiting)
Dumping heap profile to normal.0001.heap (Exiting)
Dumping heap profile to normal.0001.heap (Exiting)

What version of the product are you using? On what operating system?
2.1 on linux.

Please provide any additional information below.

The problem is that GetUniquePathFromEnv() doesn't seem to be called if the 
child process doesn't call exec().  So why not just always include the pid in 
the filename?   This patch works for me:

--- gperftools-2.1.orig/src/base/sysinfo.cc     2013-07-30 10:12:11.000000000 
+0100
+++ gperftools-2.1/src/base/sysinfo.cc  2014-01-13 17:42:53.634101186 +0000
@@ -192,13 +192,7 @@
   char* envval = getenv(env_name);
   if (envval == NULL || *envval == '\0')
     return false;
-  if (envval[0] & 128) {                  // high bit is set
-    snprintf(path, PATH_MAX, "%c%s_%u",   // add pid and clear high bit
-             envval[0] & 127, envval+1, (unsigned int)(getpid()));
-  } else {
     snprintf(path, PATH_MAX, "%s", envval);
-    envval[0] |= 128;                     // set high bit for kids to see
-  }
   return true;
 }

diff -wur gperftools-2.1.orig/src/heap-profiler.cc 
gperftools-2.1/src/heap-profiler.cc
--- gperftools-2.1.orig/src/heap-profiler.cc    2013-07-30 10:12:11.000000000 
+0100
+++ gperftools-2.1/src/heap-profiler.cc 2014-01-14 12:40:14.886714613 +0000
@@ -228,8 +228,8 @@
   // Make file name
   char file_name[1000];
   dump_count++;
-  snprintf(file_name, sizeof(file_name), "%s.%04d%s",
-           filename_prefix, dump_count, HeapProfileTable::kFileExt);
+  snprintf(file_name, sizeof(file_name), "%s_%u.%04d%s",
+           filename_prefix, (unsigned int)getpid(), dump_count, 
HeapProfileTable::kFileExt);

   // Dump the profile
   RAW_VLOG(0, "Dumping heap profile to %s (%s)", file_name, reason);

That gives the following output:

Dumping heap profile to normal_7880.0001.heap (Exiting)
Dumping heap profile to normal_7879.0001.heap (Exiting)
Dumping heap profile to normal_7874.0001.heap (Exiting)

Alternatively pthread_atfork could be used? 

Original issue reported on code.google.com by Laurence...@riverbed.com on 14 Jan 2014 at 12:47

[GoogleCodeExporter]

(This was profiling an existing executable that wasn't built with tcmalloc by 
using LD_PRELOAD, so changing the behaviour of the child after the fork wasn't 
an option. (I work with the reporter.))

Original comment by alan.bra...@gmail.com on 14 Jan 2014 at 3:17

[GoogleCodeExporter]

Good idea. But I believe patch requires a bit more work. Particularly 
GetUniquePathFromEnv is also used by libprofiler and your change breaks old 
behavior but does not add new behavior.

How about moving gitpid logic into that function and making it optional based 
on other environment variable (like GPERFTOOLS_INCLUDE_PID or maybe 
GPERFTOOLS_DONT_INCLUDE_PID if default is to include pids).

Additionally, I'm not sure if we need to keep old behavior by default or not. 
I.e. some folks might have scripts that rely on old names without pids at the 
end.

Original comment by alkondratenko on 18 Jan 2014 at 9:45

[GoogleCodeExporter]

This may be the proper fix:

--- gperftools-2.1.orig/src/heap-profiler.cc    2013-07-30 10:12:11.000000000 
+0100
+++ gperftools-2.1/src/heap-profiler.cc 2014-01-24 19:01:34.337479000 +0000
@@ -228,6 +228,18 @@
   // Make file name
   char file_name[1000];
   dump_count++;
+  
+  char* envval_ = getenv("HEAPPROFILE");
+  if (envval_[0] & 128) {
+         //High bit is set, so we're a child, so re-inititialise
+         //filename_prefix with the pid.  See
+         //https://code.google.com/p/gperftools/issues/detail?id=598
+
+         //TODO: This only needs to be called if the child hasn't called
+         //exec().
+         GetUniquePathFromEnv("HEAPPROFILE", filename_prefix);
+  } 
+ 
   snprintf(file_name, sizeof(file_name), "%s.%04d%s",
            filename_prefix, dump_count, HeapProfileTable::kFileExt);

That should result in no behaviour change, and only adding the pid to child 
processes that didn't have them before.  However, I'm having trouble testing it 
because I'm running into an issue where my shell's HEAPPROFILE keeps getting 
its high bit set (i.e. HEAPPROFILE=/logs/normal becomes 
HEAPPROFILE=¯logs/normal, i.e. the first / turned into a non-ascii char), i.e. 
my shell now looks like the parent, so the parent process of the application 
now gets the pid in the filename. I can't see why that's happening because my 
shell wasn't started with LD_PRELOAD=libtcmalloc.

Well, that's as far as I've got, sorry but I don't think I'll be able to find 
any more time to look at this.

Original comment by Laurence...@riverbed.com on 24 Jan 2014 at 7:07