[PATCH] Support profiling of multi-threaded apps.

Mark Geisert mark@maxrnd.com
Thu Mar 10 00:54:00 GMT 2016


On Wed, 9 Mar 2016, Corinna Vinschen wrote:
> Hi Mark,
>
> On Mar  9 00:39, Mark Geisert wrote:
>> This is Version 3 incorporating review comments of Version 2.  This is just
>> the code patch; a separate doc patch is forthcoming.
>
> The patch looks fine to me code-wise.  I just have a few style requests:
>
>> +	if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
>> +		char *buf;
>> +		long divisor = 1000*1000*1000;	// covers positive pid_t values
>
> Why "long"?  It's safe to use here, but it doesn't match the incoming
> datatype.  pid_t is 4 bytes, but long is 8 bytes on x86_64.  If you
> like it better that way we can keep it in but wouldn't, say, int32_t
> be a better match?  Also, can you convert the TAB to a space preceeding
> the comment so it's within 80 columns, please?

The "long" was a dumb mistake (and malingering 32-bit orientation) on my 
part.  It shall be made int32_t of course.

> I'm also a bit unhappy with some of the comments not trailing on a code
> line.  E.g.:
>
> // sample the pc of the thread indicated by thr, but bail if anything amiss
>
> // record profiling samples for other pthreads, if any
>
> Ideally that would be /**/ style comments, starting in uppercase and
> as full sentences ending with a dot, e.g.:
>
> /* Sample the pc of the thread indicated by thr, but bail if anything amiss. */
>
> /* Record profiling samples for other pthreads, if any. */

I'll go over the whole patch set to get rid of nonleading TABs and to fix 
the comments as you suggest.  The latter was just me not realizing there 
was a convention to follow so I didn't :-).  All shall be fixed.

gmon.c/.h need to be reformatted to GNU style like the rest of Cygwin but 
I'm leaving that to a future patch (NO I AM NOT COMMITTING TO THAT!! :-).

> With these changes I'll apply the patch.
>
>
> Thanks,
> Corinna
>
> -- 
> Corinna Vinschen                  Please, send mails regarding Cygwin to
> Cygwin Maintainer                 cygwin AT cygwin DOT com
> Red Hat
>

Cheers,

..mark



More information about the Cygwin-patches mailing list