[PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[Mark Geisert]

[-- Attachment #1: Type: text/plain, Size: 1154 bytes --]

Version 2 incorporating review comments of version 1.

Change log relative to winsup/cygwin:

         * include/sys/cygwin.h: Add CW_CYGHEAP_PROFTHR_ALL.
         * cygheap.cc (cygheap_profthr_all): New C-callable function that
         runs cygheap's threadlist handing each pthread's thread handle in
         turn to profthr_byhandle().
         * external.cc (cygwin_internal): Add case CW_CYGHEAP_PROFTHR_ALL.
         * gmon.c (_mcleanup): Add support for multiple simultaneous
         gmon.out* files named via environment variable GMON_OUT_PREFIX.
         * gmon.h (struct gmonparam): Make state decl volatile.
         * mcount.c (_MCOUNT_DECL): Change stores into gmonparam.state to use
         Interlocked operations. Add #include "winsup.h", update commentary.
         * profil.c (profthr_byhandle): New function abstracting out the
         updating of profile counters based on a thread handle.
         (profthr_func): Update to call profthr_byhandle() to sample the main
         thread then call cygheap_profthr_all() indirectly through
         cygwin_internal(CW_CYGHEAP_PROFTHR_ALL) to sample all other threads.

Thanks,

..mark

[-- Attachment #2: 0001-gprof-profiling-of-multi-threaded-Cygwin-programs.patch --]
[-- Type: text/plain, Size: 8539 bytes --]


---
 winsup/cygwin/cygheap.cc           | 12 ++++++
 winsup/cygwin/external.cc          | 11 ++++++
 winsup/cygwin/gmon.c               | 81 ++++++++++++++++----------------------
 winsup/cygwin/gmon.h               |  2 +-
 winsup/cygwin/include/sys/cygwin.h |  2 +
 winsup/cygwin/mcount.c             | 12 +++---
 winsup/cygwin/profil.c             | 38 +++++++++++++-----
 7 files changed, 94 insertions(+), 64 deletions(-)

diff --git a/winsup/cygwin/cygheap.cc b/winsup/cygwin/cygheap.cc
index 6493485..4932cf0 100644
--- a/winsup/cygwin/cygheap.cc
+++ b/winsup/cygwin/cygheap.cc
@@ -744,3 +744,15 @@ init_cygheap::find_tls (int sig, bool& issig_wait)
     WaitForSingleObject (t->mutex, INFINITE);
   return t;
 }
+
+/* Called from profil.c to sample all non-main thread PC values for profiling */
+extern "C" void
+cygheap_profthr_all (void (*profthr_byhandle) (HANDLE))
+{
+  for (uint32_t ix = 0; ix < nthreads; ix++)
+    {
+      _cygtls *tls = cygheap->threadlist[ix].thread;
+      if (tls->tid)
+	profthr_byhandle (tls->tid->win32_obj_id);
+    }
+}
diff --git a/winsup/cygwin/external.cc b/winsup/cygwin/external.cc
index e379df1..02335eb 100644
--- a/winsup/cygwin/external.cc
+++ b/winsup/cygwin/external.cc
@@ -702,6 +702,17 @@ cygwin_internal (cygwin_getinfo_types t, ...)
 	}
 	break;
 
+      case CW_CYGHEAP_PROFTHR_ALL:
+	{
+	  typedef void (*func_t) (HANDLE);
+	  extern void cygheap_profthr_all (func_t);
+
+	  func_t profthr_byhandle = va_arg(arg, func_t);
+	  cygheap_profthr_all (profthr_byhandle);
+	  res = 0;
+	}
+	break;
+
       default:
 	set_errno (ENOSYS);
     }
diff --git a/winsup/cygwin/gmon.c b/winsup/cygwin/gmon.c
index 96b1189..553f3b7 100644
--- a/winsup/cygwin/gmon.c
+++ b/winsup/cygwin/gmon.c
@@ -151,7 +151,6 @@ void _mcleanup (void);
 void
 _mcleanup(void)
 {
-	static char gmon_out[] = "gmon.out";
 	int fd;
 	int hz;
 	int fromindex;
@@ -161,7 +160,8 @@ _mcleanup(void)
 	struct rawarc rawarc;
 	struct gmonparam *p = &_gmonparam;
 	struct gmonhdr gmonhdr, *hdr;
-	const char *proffile;
+	char *filename = (char *) "gmon.out";
+	char *prefix;
 #ifdef DEBUG
 	int log, len;
 	char dbuf[200];
@@ -173,58 +173,43 @@ _mcleanup(void)
 	hz = PROF_HZ;
 	moncontrol(0);
 
-#ifdef nope
-	if ((profdir = getenv("PROFDIR")) != NULL) {
-		extern char *__progname;
-		char *s, *t, *limit;
-		pid_t pid;
-		long divisor;
-
-		/* If PROFDIR contains a null value, no profiling
-		   output is produced */
-		if (*profdir == '\0') {
-			return;
-		}
-
-		limit = buf + sizeof buf - 1 - 10 - 1 -
-		    strlen(__progname) - 1;
-		t = buf;
-		s = profdir;
-		while((*t = *s) != '\0' && t < limit) {
-			t++;
-			s++;
-		}
-		*t++ = '/';
-
-		/*
-		 * Copy and convert pid from a pid_t to a string.  For
-		 * best performance, divisor should be initialized to
-		 * the largest power of 10 less than PID_MAX.
-		 */
-		pid = getpid();
-		divisor=10000;
-		while (divisor > pid) divisor /= 10;	/* skip leading zeros */
-		do {
-			*t++ = (pid/divisor) + '0';
+	/* We copy an undocumented glibc feature: customizing the profiler's
+	   output file name somewhat, depending on the env var GMON_OUT_PREFIX.
+	   if GMON_OUT_PREFIX is unspecified, the file's name is "gmon.out".
+
+	   if GMON_OUT_PREFIX is specified with at least one character, the
+	   file's name is computed as "$GMON_OUT_PREFIX.$pid".
+
+	   if GMON_OUT_PREFIX is specified but contains no characters, the
+	   file's name is computed as "gmon.out.$pid".  Cygwin-specific.
+	*/
+	if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
+		char *buf;
+		long divisor = 100000;	// the power of 10 bigger than PID_MAX
+		pid_t pid = getpid();
+		size_t len = strlen(prefix);
+
+		if (len == 0)
+			len = strlen(prefix = filename);
+		buf = alloca(len + 8);	// allows for '.', 5-digit pid, NUL, +1
+
+		memcpy(buf, prefix, len);
+		buf[len++] = '.';
+
+		while (divisor > pid)	// skip leading zeroes
+			divisor /= 10;
+		do {			// convert pid to digits and store 'em
+			buf[len++] = (pid / divisor) + '0';
 			pid %= divisor;
 		} while (divisor /= 10);
-		*t++ = '.';
-
-		s = __progname;
-		while ((*t++ = *s++) != '\0')
-			;
 
-		proffile = buf;
-	} else {
-		proffile = gmon_out;
+		buf[len] = '\0';
+		filename = buf;
 	}
-#else
-	proffile = gmon_out;
-#endif
 
-	fd = open(proffile , O_CREAT|O_TRUNC|O_WRONLY|O_BINARY, 0666);
+	fd = open(filename, O_CREAT|O_TRUNC|O_WRONLY|O_BINARY, 0666);
 	if (fd < 0) {
-		perror( proffile );
+		perror(filename);
 		return;
 	}
 #ifdef DEBUG
diff --git a/winsup/cygwin/gmon.h b/winsup/cygwin/gmon.h
index 0932ed9..b0fb479 100644
--- a/winsup/cygwin/gmon.h
+++ b/winsup/cygwin/gmon.h
@@ -153,7 +153,7 @@ struct rawarc {
  * The profiling data structures are housed in this structure.
  */
 struct gmonparam {
-	int		state;
+	volatile int	state;
 	u_short		*kcount;
 	size_t		kcountsize;
 	u_short		*froms;
diff --git a/winsup/cygwin/include/sys/cygwin.h b/winsup/cygwin/include/sys/cygwin.h
index 5b7da58..8c7128c 100644
--- a/winsup/cygwin/include/sys/cygwin.h
+++ b/winsup/cygwin/include/sys/cygwin.h
@@ -160,6 +160,7 @@ typedef enum
     CW_GETNSS_PWD_SRC,
     CW_GETNSS_GRP_SRC,
     CW_EXCEPTION_RECORD_FROM_SIGINFO_T,
+    CW_CYGHEAP_PROFTHR_ALL,
   } cygwin_getinfo_types;
 
 #define CW_LOCK_PINFO CW_LOCK_PINFO
@@ -221,6 +222,7 @@ typedef enum
 #define CW_GETNSS_PWD_SRC CW_GETNSS_PWD_SRC
 #define CW_GETNSS_GRP_SRC CW_GETNSS_GRP_SRC
 #define CW_EXCEPTION_RECORD_FROM_SIGINFO_T CW_EXCEPTION_RECORD_FROM_SIGINFO_T
+#define CW_CYGHEAP_PROFTHR_ALL CW_CYGHEAP_PROFTHR_ALL
 
 /* Token type for CW_SET_EXTERNAL_TOKEN */
 enum
diff --git a/winsup/cygwin/mcount.c b/winsup/cygwin/mcount.c
index fad6728..6111b35 100644
--- a/winsup/cygwin/mcount.c
+++ b/winsup/cygwin/mcount.c
@@ -41,6 +41,7 @@ static char rcsid[] = "$OpenBSD: mcount.c,v 1.6 1997/07/23 21:11:27 kstailey Exp
 #endif
 #include <sys/types.h>
 #include "gmon.h"
+#include "winsup.h"
 
 /*
  * mcount is called on entry to each function compiled with the profiling
@@ -70,11 +71,12 @@ _MCOUNT_DECL (size_t frompc, size_t selfpc)
 	p = &_gmonparam;
 	/*
 	 * check that we are profiling
-	 * and that we aren't recursively invoked.
+	 * and that we aren't recursively invoked by this thread
+	 * or entered anew by any other thread.
 	 */
-	if (p->state != GMON_PROF_ON)
+	if (InterlockedCompareExchange (
+		    &p->state, GMON_PROF_BUSY, GMON_PROF_ON) != GMON_PROF_ON)
 		return;
-	p->state = GMON_PROF_BUSY;
 	/*
 	 * check that frompcindex is a reasonable pc value.
 	 * for example:	signal catchers get called from the stack,
@@ -162,10 +164,10 @@ _MCOUNT_DECL (size_t frompc, size_t selfpc)
 		}
 	}
 done:
-	p->state = GMON_PROF_ON;
+	InterlockedExchange (&p->state, GMON_PROF_ON);
 	return;
 overflow:
-	p->state = GMON_PROF_ERROR;
+	InterlockedExchange (&p->state, GMON_PROF_ERROR);
 	return;
 }
 
diff --git a/winsup/cygwin/profil.c b/winsup/cygwin/profil.c
index eb41c08..9f183af 100644
--- a/winsup/cygwin/profil.c
+++ b/winsup/cygwin/profil.c
@@ -18,6 +18,7 @@
 #endif
 #include <windows.h>
 #include <stdio.h>
+#include <sys/cygwin.h>
 #include <sys/types.h>
 #include <errno.h>
 #include <math.h>
@@ -65,25 +66,42 @@ print_prof (struct profinfo *p)
 }
 #endif
 
-/* Everytime we wake up use the main thread pc to hash into the cell in the
-   profile buffer ARG. */
+/* Every time we wake up sample the main thread's pc to hash into the cell
+   in the profile buffer ARG.  Then all other pthreads' pc's are sampled.  */
 
-static void CALLBACK profthr_func (LPVOID);
+static void
+profthr_byhandle (HANDLE thr)
+{
+  size_t pc;
+
+  // sample the pc of the thread indicated by thr, but bail if anything amiss
+  if (thr == INVALID_HANDLE_VALUE)
+    return;
+  pc = get_thrpc (thr);
+  if (pc == -1)
+    return;
+
+  // code assumes there is only one profinfo in play: the static prof up top
+  if (pc >= prof.lowpc && pc < prof.highpc)
+    {
+      size_t idx = PROFIDX (pc, prof.lowpc, prof.scale);
+      prof.counter[idx]++;
+    }
+}
 
 static void CALLBACK
 profthr_func (LPVOID arg)
 {
   struct profinfo *p = (struct profinfo *) arg;
-  size_t pc, idx;
 
   for (;;)
     {
-      pc = (size_t) get_thrpc (p->targthr);
-      if (pc >= p->lowpc && pc < p->highpc)
-	{
-	  idx = PROFIDX (pc, p->lowpc, p->scale);
-	  p->counter[idx]++;
-	}
+      // record profiling sample for main thread
+      profthr_byhandle (p->targthr);
+
+      // record profiling samples for other pthreads, if any
+      cygwin_internal (CW_CYGHEAP_PROFTHR_ALL, profthr_byhandle);
+
 #if 0
       print_prof (p);
 #endif
-- 
2.7.0

Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

Hi Mark,

On Feb 20 00:16, Mark Geisert wrote:
> Version 2 incorporating review comments of version 1.

I'm afraid we need a v3, two points.

One is, for completeness it would be nice if you could add a
description to the git comment along the lines of your original
comment so we have a description in the log.

The other point is:

> +	/* We copy an undocumented glibc feature: customizing the profiler's
> +	   output file name somewhat, depending on the env var GMON_OUT_PREFIX.
> +	   if GMON_OUT_PREFIX is unspecified, the file's name is "gmon.out".
> +
> +	   if GMON_OUT_PREFIX is specified with at least one character, the
> +	   file's name is computed as "$GMON_OUT_PREFIX.$pid".
> +
> +	   if GMON_OUT_PREFIX is specified but contains no characters, the
> +	   file's name is computed as "gmon.out.$pid".  Cygwin-specific.
> +	*/
> +	if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
> +		char *buf;
> +		long divisor = 100000;	// the power of 10 bigger than PID_MAX
> +		pid_t pid = getpid();
> +		size_t len = strlen(prefix);
> +
> +		if (len == 0)
> +			len = strlen(prefix = filename);
> +		buf = alloca(len + 8);	// allows for '.', 5-digit pid, NUL, +1

I've seen 6 digit PIDs.  In fact, we're not that tight on space here
so we should err on the side of caution and leave room for the entire
possible size of a Windows PID.  That's a LONG, 32 bit, 10 decimal
digits.

Other than that, the patch looks good to me.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[Jon Turney]

Thanks for this.  A few comments inline.

On 20/02/2016 08:16, Mark Geisert wrote:
> diff --git a/winsup/cygwin/cygheap.cc b/winsup/cygwin/cygheap.cc
> index 6493485..4932cf0 100644
> --- a/winsup/cygwin/cygheap.cc
> +++ b/winsup/cygwin/cygheap.cc
> @@ -744,3 +744,15 @@ init_cygheap::find_tls (int sig, bool& issig_wait)
>       WaitForSingleObject (t->mutex, INFINITE);
>     return t;
>   }
> +
> +/* Called from profil.c to sample all non-main thread PC values for profiling */
> +extern "C" void
> +cygheap_profthr_all (void (*profthr_byhandle) (HANDLE))
> +{
> +  for (uint32_t ix = 0; ix < nthreads; ix++)
> +    {
> +      _cygtls *tls = cygheap->threadlist[ix].thread;
> +      if (tls->tid)
> +	profthr_byhandle (tls->tid->win32_obj_id);
> +    }
> +}

There doesn't seem to be anything specific to profiling about this, so 
it could be written in a more generic way, as "call a callback function 
for each thread".

> diff --git a/winsup/cygwin/external.cc b/winsup/cygwin/external.cc
> index e379df1..02335eb 100644
> --- a/winsup/cygwin/external.cc
> +++ b/winsup/cygwin/external.cc
> @@ -702,6 +702,17 @@ cygwin_internal (cygwin_getinfo_types t, ...)
>   	}
>   	break;
>
> +      case CW_CYGHEAP_PROFTHR_ALL:
> +	{
> +	  typedef void (*func_t) (HANDLE);
> +	  extern void cygheap_profthr_all (func_t);
> +
> +	  func_t profthr_byhandle = va_arg(arg, func_t);
> +	  cygheap_profthr_all (profthr_byhandle);
> +	  res = 0;
> +	}
> +	break;
> +

Is this exposed via cygwin_internal() operation just for testing 
purposes?  Or is there some other use envisioned?

> +	/* We copy an undocumented glibc feature: customizing the profiler's
> +	   output file name somewhat, depending on the env var GMON_OUT_PREFIX.
> +	   if GMON_OUT_PREFIX is unspecified, the file's name is "gmon.out".
> +
> +	   if GMON_OUT_PREFIX is specified with at least one character, the
> +	   file's name is computed as "$GMON_OUT_PREFIX.$pid".
> +
> +	   if GMON_OUT_PREFIX is specified but contains no characters, the
> +	   file's name is computed as "gmon.out.$pid".  Cygwin-specific.
> +	*/
> +	if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {

setup-env.xml might be an appropriate place to mention this environment 
variable.

>   static void CALLBACK
>   profthr_func (LPVOID arg)
>   {
>     struct profinfo *p = (struct profinfo *) arg;
> -  size_t pc, idx;
>
>     for (;;)
>       {
> -      pc = (size_t) get_thrpc (p->targthr);
> -      if (pc >= p->lowpc && pc < p->highpc)
> -	{
> -	  idx = PROFIDX (pc, p->lowpc, p->scale);
> -	  p->counter[idx]++;
> -	}
> +      // record profiling sample for main thread
> +      profthr_byhandle (p->targthr);
> +
> +      // record profiling samples for other pthreads, if any
> +      cygwin_internal (CW_CYGHEAP_PROFTHR_ALL, profthr_byhandle);
> +

Hmm.. so why isn't this written as cygheap_profthr_all (profthr_byhandle) ?

Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

On Feb 22 11:44, Jon Turney wrote:
> Thanks for this.  A few comments inline.
> [...]
> On 20/02/2016 08:16, Mark Geisert wrote:
> >+      // record profiling samples for other pthreads, if any
> >+      cygwin_internal (CW_CYGHEAP_PROFTHR_ALL, profthr_byhandle);
> >+
> 
> Hmm.. so why isn't this written as cygheap_profthr_all (profthr_byhandle) ?

I asked for it:
https://cygwin.com/ml/cygwin-patches/2016-q1/msg00028.html


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[Jon Turney]

On 22/02/2016 14:25, Corinna Vinschen wrote:
> On Feb 22 11:44, Jon Turney wrote:
>> Thanks for this.  A few comments inline.
>> [...]
>> On 20/02/2016 08:16, Mark Geisert wrote:
>>> +      // record profiling samples for other pthreads, if any
>>> +      cygwin_internal (CW_CYGHEAP_PROFTHR_ALL, profthr_byhandle);
>>> +
>>
>> Hmm.. so why isn't this written as cygheap_profthr_all (profthr_byhandle) ?
>
> I asked for it:
> https://cygwin.com/ml/cygwin-patches/2016-q1/msg00028.html

Ok.  I guess I don't understand why it's exported at all in that version 
of the patch.

Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

On Feb 22 16:14, Jon Turney wrote:
> On 22/02/2016 14:25, Corinna Vinschen wrote:
> >On Feb 22 11:44, Jon Turney wrote:
> >>Thanks for this.  A few comments inline.
> >>[...]
> >>On 20/02/2016 08:16, Mark Geisert wrote:
> >>>+      // record profiling samples for other pthreads, if any
> >>>+      cygwin_internal (CW_CYGHEAP_PROFTHR_ALL, profthr_byhandle);
> >>>+
> >>
> >>Hmm.. so why isn't this written as cygheap_profthr_all (profthr_byhandle) ?
> >
> >I asked for it:
> >https://cygwin.com/ml/cygwin-patches/2016-q1/msg00028.html
> 
> Ok.  I guess I don't understand why it's exported at all in that version of
> the patch.

I don't understand the question.  cygheap_profthr_all isn't exported
anymore in v2.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[Jon Turney]

On 22/02/2016 16:55, Corinna Vinschen wrote:
> On Feb 22 16:14, Jon Turney wrote:
>> On 22/02/2016 14:25, Corinna Vinschen wrote:
>>> On Feb 22 11:44, Jon Turney wrote:
>>>> Thanks for this.  A few comments inline.
>>>> [...]
>>>> On 20/02/2016 08:16, Mark Geisert wrote:
>>>>> +      // record profiling samples for other pthreads, if any
>>>>> +      cygwin_internal (CW_CYGHEAP_PROFTHR_ALL, profthr_byhandle);
>>>>> +
>>>>
>>>> Hmm.. so why isn't this written as cygheap_profthr_all (profthr_byhandle) ?
>>>
>>> I asked for it:
>>> https://cygwin.com/ml/cygwin-patches/2016-q1/msg00028.html
>>
>> Ok.  I guess I don't understand why it's exported at all in that version of
>> the patch.
>
> I don't understand the question.  cygheap_profthr_all isn't exported
> anymore in v2.

I wasn't taking into account the fact that profil.c gets built into a 
separate library, libgmon.a.

So that clears up my misunderstanding as to why this operation needs to 
be exposed outside the DLL at all.

Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[Mark Geisert]

On Mon, 22 Feb 2016, Corinna Vinschen wrote:
> One is, for completeness it would be nice if you could add a
> description to the git comment along the lines of your original
> comment so we have a description in the log.

Sorry, can't parse this; git newbie here.  Did you mean the 'git commit' 
I'm doing to my private repository and the message associated with the 
commit?  And by "original comment" do you mean what I called the change 
log in the text of my v2 email we're discussing (i.e., not the patch 
attachment but the email body)?

> The other point is:
>> +		long divisor = 100000;	// the power of 10 bigger than PID_MAX
>
> I've seen 6 digit PIDs.  In fact, we're not that tight on space here
> so we should err on the side of caution and leave room for the entire
> possible size of a Windows PID.  That's a LONG, 32 bit, 10 decimal
> digits.

Yikes.  I'd seen large 5-digit pids but could not find a definitive symbol 
defining Windows' maximum pid value.  So I will change divisor's init 
value to 1000*1000*1000 which will allow the conversion loop to support 
10-digit pids.

> Other than that, the patch looks good to me.

Great!  I'll follow up with Jon separately (to the list) on his comments.

..mark

Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[Mark Geisert]

Hi Jon,

On Mon, 22 Feb 2016, Jon Turney wrote:
> Thanks for this.  A few comments inline.
>
> On 20/02/2016 08:16, Mark Geisert wrote:
>> +/* Called from profil.c to sample all non-main thread PC values for 
>> profiling */
>> +extern "C" void
>> +cygheap_profthr_all (void (*profthr_byhandle) (HANDLE))
>> +{
>> +  for (uint32_t ix = 0; ix < nthreads; ix++)
>> +    {
>> +      _cygtls *tls = cygheap->threadlist[ix].thread;
>> +      if (tls->tid)
>> +	profthr_byhandle (tls->tid->win32_obj_id);
>> +    }
>> +}
>
> There doesn't seem to be anything specific to profiling about this, so it 
> could be written in a more generic way, as "call a callback function for each 
> thread".

I saw your later conversation with Corinna on the list re why 
cygwin_internal() is involved now.  (I too had stumbled over the 
cygwin1.dll/libgmon.a gap when I started this work.)  Given the necessity 
of the separation, does it still make sense to write a generic per-thread 
callback mechanism and then make use of it for this patch, or is that 
overkill?  I can't tell.

>> +	if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
>
> setup-env.xml might be an appropriate place to mention this environment 
> variable.

I am now writing a gprof.xml that will be tied into the existing 
programming.xml.  I plan to document GMON_OUT_PREFIX in gprof.xml.  Do you 
think that's sufficient?

..mark

Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1731 bytes --]

On Feb 22 22:58, Mark Geisert wrote:
> On Mon, 22 Feb 2016, Corinna Vinschen wrote:
> >One is, for completeness it would be nice if you could add a
> >description to the git comment along the lines of your original
> >comment so we have a description in the log.
> 
> Sorry, can't parse this; git newbie here.  Did you mean the 'git commit' I'm
> doing to my private repository and the message associated with the commit?

Yes, exactly.

> And by "original comment" do you mean what I called the change log in the
> text of my v2 email we're discussing (i.e., not the patch attachment but the
> email body)?

No, I mean the first patch submission.  Your v1 patch submission had a
nice explaining text.  It might be helpful to have this text (tweaked to
the v2 changes) in the git log, together with the ChangeLog.

> >The other point is:
> >>+		long divisor = 100000;	// the power of 10 bigger than PID_MAX
> >
> >I've seen 6 digit PIDs.  In fact, we're not that tight on space here
> >so we should err on the side of caution and leave room for the entire
> >possible size of a Windows PID.  That's a LONG, 32 bit, 10 decimal
> >digits.
> 
> Yikes.  I'd seen large 5-digit pids but could not find a definitive symbol
> defining Windows' maximum pid value.  So I will change divisor's init value
> to 1000*1000*1000 which will allow the conversion loop to support 10-digit
> pids.

ACK.

> >Other than that, the patch looks good to me.
> 
> Great!  I'll follow up with Jon separately (to the list) on his comments.

Yup.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1932 bytes --]

On Feb 22 23:36, Mark Geisert wrote:
> Hi Jon,
> 
> On Mon, 22 Feb 2016, Jon Turney wrote:
> >Thanks for this.  A few comments inline.
> >
> >On 20/02/2016 08:16, Mark Geisert wrote:
> >>+/* Called from profil.c to sample all non-main thread PC values for
> >>profiling */
> >>+extern "C" void
> >>+cygheap_profthr_all (void (*profthr_byhandle) (HANDLE))
> >>+{
> >>+  for (uint32_t ix = 0; ix < nthreads; ix++)
> >>+    {
> >>+      _cygtls *tls = cygheap->threadlist[ix].thread;
> >>+      if (tls->tid)
> >>+	profthr_byhandle (tls->tid->win32_obj_id);
> >>+    }
> >>+}
> >
> >There doesn't seem to be anything specific to profiling about this, so it
> >could be written in a more generic way, as "call a callback function for
> >each thread".
> 
> I saw your later conversation with Corinna on the list re why
> cygwin_internal() is involved now.  (I too had stumbled over the
> cygwin1.dll/libgmon.a gap when I started this work.)  Given the necessity of
> the separation, does it still make sense to write a generic per-thread
> callback mechanism and then make use of it for this patch, or is that
> overkill?  I can't tell.

One problem with a generic solution is to generalize the arguments to
the called function.  IMHO, keep it as is for now.  If we ever need to
make this generic we can still do it.

> >>+	if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
> >
> >setup-env.xml might be an appropriate place to mention this environment
> >variable.
> 
> I am now writing a gprof.xml that will be tied into the existing
> programming.xml.  I plan to document GMON_OUT_PREFIX in gprof.xml.  Do you
> think that's sufficient?

A single paragraph in setup-env.xml pointing to gprof.xml might be
helpful, I think.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[Mark Geisert]

On Tue, 23 Feb 2016, Corinna Vinschen wrote:
> On Feb 22 23:36, Mark Geisert wrote:
>> On Mon, 22 Feb 2016, Jon Turney wrote:
>>> There doesn't seem to be anything specific to profiling about this, so it
>>> could be written in a more generic way, as "call a callback function for
>>> each thread".
>>
>> I saw your later conversation with Corinna on the list re why
>> cygwin_internal() is involved now.  (I too had stumbled over the
>> cygwin1.dll/libgmon.a gap when I started this work.)  Given the necessity of
>> the separation, does it still make sense to write a generic per-thread
>> callback mechanism and then make use of it for this patch, or is that
>> overkill?  I can't tell.
>
> One problem with a generic solution is to generalize the arguments to
> the called function.  IMHO, keep it as is for now.  If we ever need to
> make this generic we can still do it.

OK.

>>>> +	if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
>>>
>>> setup-env.xml might be an appropriate place to mention this environment
>>> variable.
>>
>> I am now writing a gprof.xml that will be tied into the existing
>> programming.xml.  I plan to document GMON_OUT_PREFIX in gprof.xml.  Do you
>> think that's sufficient?
>
> A single paragraph in setup-env.xml pointing to gprof.xml might be
> helpful, I think.

Alright, I can do that as part of the separate doc patch that I'm working.

I ran into an issue while testing the profiling of programs that fork() 
but don't exec().  So it may be a short while before I can send ver 3. 
Just FYI.

..mark

Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1946 bytes --]

On Feb 25 00:47, Mark Geisert wrote:
> On Tue, 23 Feb 2016, Corinna Vinschen wrote:
> >On Feb 22 23:36, Mark Geisert wrote:
> >>On Mon, 22 Feb 2016, Jon Turney wrote:
> >>>There doesn't seem to be anything specific to profiling about this, so it
> >>>could be written in a more generic way, as "call a callback function for
> >>>each thread".
> >>
> >>I saw your later conversation with Corinna on the list re why
> >>cygwin_internal() is involved now.  (I too had stumbled over the
> >>cygwin1.dll/libgmon.a gap when I started this work.)  Given the necessity of
> >>the separation, does it still make sense to write a generic per-thread
> >>callback mechanism and then make use of it for this patch, or is that
> >>overkill?  I can't tell.
> >
> >One problem with a generic solution is to generalize the arguments to
> >the called function.  IMHO, keep it as is for now.  If we ever need to
> >make this generic we can still do it.
> 
> OK.
> 
> >>>>+	if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
> >>>
> >>>setup-env.xml might be an appropriate place to mention this environment
> >>>variable.
> >>
> >>I am now writing a gprof.xml that will be tied into the existing
> >>programming.xml.  I plan to document GMON_OUT_PREFIX in gprof.xml.  Do you
> >>think that's sufficient?
> >
> >A single paragraph in setup-env.xml pointing to gprof.xml might be
> >helpful, I think.
> 
> Alright, I can do that as part of the separate doc patch that I'm working.
> 
> I ran into an issue while testing the profiling of programs that fork() but
> don't exec().  So it may be a short while before I can send ver 3. Just FYI.

Ok, no worries.  If you want to discuss something, we also have the
#cygwin-developers IRC channel on freenode.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]