Re: [PATCH] Cygwin: New tool: cygmon

[Mark Geisert <mark@maxrnd.com>]

Hi Jon,

I appreciate your review.  I will fold the suggestions from your short email plus 
this longer email into a v2 patch fairly soon.

Jon Turney wrote:
> On 12/06/2021 07:46, Mark Geisert wrote:
> 
>> diff --git a/winsup/utils/cygmon.cc b/winsup/utils/cygmon.cc
>> new file mode 100644
>> index 000000000..9156b27d7
>> --- /dev/null
>> +++ b/winsup/utils/cygmon.cc
[...]
>> +#include "../cygwin/include/sys/cygwin.h"
>> +#include "../cygwin/include/cygwin/version.h"
>> +#include "../cygwin/cygtls_padsize.h"
>> +#include "../cygwin/gcc_seh.h"
> 
> The latest Makefile.am sets things up so these relative paths aren't needed.

Oh yes, I saw those go by but have not made use of the changes.  Will do.

>> +typedef unsigned short ushort;
>> +typedef uint16_t u_int16_t; // to work around ancient gmon.h usage
> 
> 'Non-standard sized type needed by ancient gmon.h' might be clearer

Indeed, thanks.  Will update.

>> +#include "../cygwin/gmon.h"
[...]
>> +size_t
>> +sample (HANDLE h)
>> +{
>> +  static CONTEXT *context = NULL;
>> +  size_t status;
>> +
>> +  if (!context)
>> +    {
>> +      context = (CONTEXT *) calloc (1, sizeof (CONTEXT));
>> +      context->ContextFlags = CONTEXT_CONTROL;
>> +    }
> 
> Why isn't this just 'static CONTEXT'?
> 
> But it also shouldn't be static, because this function needs to be thread-safe as 
> it is called by the profiler thread for every inferior process?

Oof, I must've gotten sidetracked off of coding the change from static to auto by 
a squirrel or a shiny disc or something.  Yes, the local context buffer needs to 
be thread-safe in case of multiple children being profiled.  I knew that...

[...]
>> +  else
>> +//TODO this approach might not support 32-bit executables on 64-bit
> 
> It definitely doesn't. But that's fine.

Will make the comment more definitive.

[...]
>> +void
>> +start_profiler (child *c)
>> +{
>> +  DWORD  tid;
>> +
>> +  if (verbose)
>> +    note ("*** start profiler thread on pid %lu\n", c->pid);
>> +  c->hquitevt = CreateEvent (NULL, TRUE, FALSE, NULL);
>> +  if (!c->hquitevt)
>> +    error (0, "unable to create quit event\n");
>> +  c->profiling = 1;
>> +  c->hprofthr = CreateThread (NULL, 0, profiler, (void *) c, 0, &tid);
>> +  if (!c->hprofthr)
>> +    error (0, "unable to create profiling thread\n");
>> +
>> +//SetThreadPriority (c->hprofthr, THREAD_PRIORITY_TIME_CRITICAL); Don't do this!
> 
> But now I want to see what happens when I do!

You'll be sorry, but yeah the warning here is important because there's a real 
temptation here to do something, anything.. I'll make it more descriptive.

[...]
>> +      fd = open (filename, O_CREAT | O_TRUNC | O_WRONLY | O_BINARY);
[...]
>> +      close (fd);
>> +      chmod (filename, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);//XXX ineffective
> 
> ???

For the life of me I could not figure out how to make the output file mode 0644 
with either open() flags or chmod() afterwards.  I keep getting unwanted 'x' bits 
set.  Perhaps a side-effect of this being a native program rather than a Cygwin 
program.  I just flagged it until I can resolve it.

[...]
>> +void
>> +info_profile_file (char *filename)
> 
> I think this should be a separate tool, since it's not really part of the function 
> of this tool.

Yeah, I guess so.  I didn't think of that option.  I just saw this as too big to 
be a Cygwin-specific patch to gprof, where it could plausibly go.  I will split 
this out to a separate tool called 'gmoninfo' or some such.  Suggestions welcome.

[...]
>> +IMAGE_SECTION_HEADER *
>> +find_text_section (LPVOID base, HANDLE h)
>> +{
>> +  static IMAGE_SECTION_HEADER asect;
>> +  DWORD  lfanew;
>> +  WORD   machine;
>> +  WORD   nsects;
>> +  DWORD  ntsig;
>> +  char  *ptr = (char *) base;
>> +
>> +  IMAGE_DOS_HEADER *idh = (IMAGE_DOS_HEADER *) ptr;
>> +  read_child ((void *) &lfanew, sizeof (lfanew), &idh->e_lfanew, h);
>> +  ptr += lfanew;
>> +
>> +  /* Code handles 32- or 64-bit headers depending on compilation environment. */
>> +  /*TODO It doesn't yet handle 32-bit headers on 64-bit Cygwin or v/v.        */
>> +  IMAGE_NT_HEADERS *inth = (IMAGE_NT_HEADERS *) ptr;
>> +  read_child ((void *) &ntsig, sizeof (ntsig), &inth->Signature, h);
>> +  if (ntsig != IMAGE_NT_SIGNATURE)
>> +    error (0, "find_text_section: NT signature not found\n");
>> +
>> +  read_child ((void *) &machine, sizeof (machine),
>> +              &inth->FileHeader.Machine, h);
>> +#ifdef __x86_64__
>> +  if (machine != IMAGE_FILE_MACHINE_AMD64)
>> +#else
>> +  if (machine != IMAGE_FILE_MACHINE_I386)
>> +#endif
>> +    error (0, "target program was built for different machine architecture\n");
>> +
>> +  read_child ((void *) &nsects, sizeof (nsects),
>> +              &inth->FileHeader.NumberOfSections, h);
>> +  ptr += sizeof (*inth);
>> +
>> +  IMAGE_SECTION_HEADER *ish = (IMAGE_SECTION_HEADER *) ptr;
>> +  for (int i = 0; i < nsects; i++)
>> +    {
>> +      read_child ((void *) &asect, sizeof (asect), ish, h);
>> +      if (0 == memcmp (".text\0\0\0", &asect.Name, 8))
>> +        return &asect;
>> +      ish++;
>> +    }
> 
> While this is adequate and correct in 99% of cases, I think what you're perhaps 
> looking for here is sections which are executable?

I suppose so, but since the gmon.out files (and gprof) can't deal with disjoint 
address spans directly that would mean an additional gmon.out file for each span. 
It could work, but I'm vaguely disturbed by the idea.

> (Well, really we want to enumerate executable memory regions in the inferior, to 
> profile dynamically generated code as well, but...)

Even more disturbing :), but yes, could be done.  At some point the linear search 
for which sampling buffer to bump a bucket in might need changing to a hashed 
lookup...

[...]
>> +int
>> +load_cygwin ()
>> +{
> 
> So: I wonder if this tool should just link with the cygwin DLL.
> 
> I think the historical reason for strace to not be a cygwin executable is so that 
> it can be used to debug problems with cygwin executables startup.
> 
> I don't think that reason applies here?

You're correct that that reason doesn't apply here.  Making this a Cygwin program 
would also make it much easier to debug!  IIRC I started writing this as a Cygwin 
program but switched to a native Windows model when I started cribbing code from 
strace.  But I guess in hindsight it wasn't really necessary to switch.  Thanks 
for bringing this up for consideration.

Thanks & Regards,

..mark