Re: gprof profiling of multi-threaded Cygwin programs

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

Hi Mark,


thanks for the patch.  Generally the patch is fine, I have just a few
nits.

On Feb 16 21:28, Mark Geisert wrote:
> I've attached a patch set modifying Cygwin's profiling support to sample PC
> values of all an application's threads, not just the main thread.  There is
> no change to how profiling is requested: just compile and link the app with
> "-pg" as usual.  The profiling info is dumped into file gmon.out as usual.
> 
> There is a behavioral change that ought to be documented somewhere:  If a

If it ought to be documented, what about providing the doc patch, too?
Any chance you could come up with a short section about profiling in the
context of winsup/doc/programming.xml?  Otherwise there's basically only
the description of the ssp tool in winsup/doc/utils.xml yet, which is a
bit ... disappointing.

> diff --git a/winsup/cygwin/common.din b/winsup/cygwin/common.din
> index 9584d09..243fd01 100644
> --- a/winsup/cygwin/common.din
> +++ b/winsup/cygwin/common.din
> @@ -269,6 +269,7 @@ ctime SIGFE
>  ctime_r SIGFE
>  cuserid NOSIGFE
>  cwait SIGFE
> +cygheap_profthr_all NOSIGFE

I would like to avoid exporting new cygwin-internal symbols.  Could you
please wrap the new functionality in a call to cygwin_internal?  Just
add a new CW_xxx symbol to include/sys/cygwin.h and use that from
profthr_func.

> +extern "C" void
> +cygheap_profthr_all (void (*profthr_byhandle) (HANDLE))
> +{
> +  int ix = -1;
> +  while (++ix < (int) nthreads)

Why the cast?  Why not stick to the type of nthreads, e.g.

     for (uint32_t ix = 0; ix < nthreads; ++ix)

> +    {
> +      _cygtls *tls = cygheap->threadlist[ix].thread;
> +      if (tls->tid)
> +	profthr_byhandle (tls->tid->win32_obj_id);
> +    }
> +}
> @@ -49,6 +50,7 @@ static char rcsid[] = "$OpenBSD: gmon.c,v 1.8 1997/07/23 21:11:27 kstailey Exp $
>  
>  /* XXX needed? */
>  //extern char *minbrk __asm ("minbrk");
> +extern int _setmode(int, int);
>  
>  #ifdef _WIN64
>  #define MINUS_ONE_P (-1LL)
> @@ -152,6 +154,7 @@ void
>  _mcleanup(void)
>  {
>  	static char gmon_out[] = "gmon.out";
> +	static char gmon_template[] = "gmon.outXXXXXX";
>  	int fd;
>  	int hz;
>  	int fromindex;
> @@ -222,7 +225,14 @@ _mcleanup(void)
>  	proffile = gmon_out;
>  #endif
>  
> -	fd = open(proffile , O_CREAT|O_TRUNC|O_WRONLY|O_BINARY, 0666);
> +	fd = open(proffile, O_CREAT|O_EXCL|O_TRUNC|O_WRONLY|O_BINARY, 0666);
> +	if (fd < 0 && errno == EEXIST) {
> +		fd = mkstemp(gmon_template);
> +		if (fd >= 0) {
> +			_setmode(fd, O_BINARY);

You don't have to call _setmode here.  Files created with mkstemp are
O_BINARY anyway.  And if you don't trust it, use mkostemp with an
explicit O_BINARY flag.

>  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
> +      cygheap_profthr_all (profthr_byhandle);

As outlined above, please call `cygwin_internal (CW_foo, profthr_byhandle)'
from here.


Thanks,
Corinna

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

Attachment: signature.asc
Description: PGP signature