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
