Re: [PATCH 1/3] Cygwin: New tool: profiler

[Jon Turney <jon.turney@dronecode.org.uk>]

On 19/07/2021 11:04, Corinna Vinschen wrote:
> Hi Matt,
> 
> On Jul 15 21:49, Mark Geisert wrote:
>> The new tool formerly known as cygmon is renamed to 'profiler'.  For the
>> name I considered 'ipsampler' and could not think of any others.  I'm open
>> to a different name if any is suggested.
>>
>> I decided that a discussion of the pros and cons of this profiler vs the
>> existing ssp should probably be in the "Profiling Cygwin Programs" section
>> of the Cygwin User's Guide rather than in the help for either.  That
>> material will be supplied at some point.
>>
>> CONTEXT buffers are made child-specific and thus thread-specific since
>> there is one profiler thread for each child program being profiled.
>>
>> The SetThreadPriority() warning comment has been expanded.
>>
>> chmod() works on Cygwin so the "//XXX ineffective" comment is gone.
>>
>> I decided to make the "sample all executable sections" and "sample
>> dynamically generated code" suggestions simply expanded comments for now.
>>
>> The profiler program is now a Cygwin exe rather than a native exe.
> 
> The patchset LGTM, but for the details I'd like jturney to have a look
> and approve it eventually.

Thanks.  I applied these patches.

A few small issues you might consider addressing in follow-ups.

> +
> +/* Set up interfaces to Cygwin internal funcs and path.cc helper funcs. */
> +extern "C" {
> +uintptr_t cygwin_internal (int, ...);

Since this is a now cygwin application, you can include <sys/cygwin.h> 
for this prototype.

> +WCHAR cygwin_dll_path[32768];
> +}

This is unused.

> +int
> +main (int argc, char **argv)
> +{
> +  /* Make sure to have room for the _cygtls area *and* to initialize it.
> +   * This is required to make sure cygwin_internal calls into Cygwin work
> +   * reliably.  This problem has been noticed under AllocationPreference
> +   * registry setting to 0x100000 (TOP_DOWN).
> +   */
> +  char buf[CYGTLS_PADSIZE];
> +
> +  RtlSecureZeroMemory (buf, sizeof (buf));

Since you aren't dynamically loading cygwin1.dll, none of this guff is 
needed.

> +    <refsynopsisdiv>
> +    <screen>
> +gmondump [OPTION]... FILENAME...
> +    </screen>
> +    </refsynopsisdiv>

This says that 1 or more FILENAME are expected, but actually 0 is handled.

So perhaps either document the actual behaviour, or make providing no 
filenames to gmondump an error.

> +    <para>The <command>profiler</command> utility executes a given program, and
> +      optionally the children of that program, collecting the location of the
> +      CPU instruction pointer (IP) many times per second.

I don't think this says what you mean.  It's not optional that the child 
processes are executed, it's optional that they are profiled.