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

[Jon Turney <jon dot turney at dronecode dot org dot uk>]

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) ?