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
signature.asc
Description: PGP signature
