Created attachment 301092 [details] [review]
Add g_system_thread_set_name() implementation for W32 threads

This works by using semi-documented[1] exception to tell the debugger
that a thread needs to have its name changed.

If this exception is not caught and handled by something, it will crash
the process, so we need to set up our own handler in case there's no
debugger attached or the debugger can't handle this type of exception.

Since SEH is not supported by gcc on i686 (at the moment), we need to use VEH
instead. For completeness the MSVC-oriented code still uses SEH, although
there is no reason why it shouldn't work with the VEH variant used by MinGW.

VEH handler has to be set up somewhere (g_thread_win32_init () works nicely)
and removed once it's not needed (g_thread_win32_process_detach () is added
expressly for that purpose). Note that g_thread_win32_process_detach() is
only called when glib is unloaded by FreeLibrary(), not when glib-using
process is terminating.

This exception is known to work with WinDbg, and adding support for it into
GDB proved to be feasible (GDB patch will be sent upstream, eventually).

[1] https://msdn.microsoft.com/en-us/library/xcb2z8hs%28v=vs.71%29.aspx

Review of attachment 301092 [details] [review]:

See the style issues.

::: glib/gthread-win32.c
@@ +537,3 @@
+#ifdef _MSC_VER
+   __try
+   {

indentation

@@ +541,3 @@
+   }
+    __except (EXCEPTION_EXECUTE_HANDLER)
+   {

indentation

@@ +1048,3 @@
+
+#ifndef _MSC_VER
+  SetThreadName_VEH_handle =

I'd say put it in one line

@@ +1051,3 @@
+      AddVectoredExceptionHandler (1, &SetThreadName_VEH);
+  if (SetThreadName_VEH_handle == NULL)
+  {

wrong indentation

@@ +1100,3 @@
+#ifndef _MSC_VER
+  if (SetThreadName_VEH_handle != NULL)
+  {

also indentation here

Created attachment 301093 [details] [review]
Add g_system_thread_set_name() implementation for W32 threads v2

v2: Fixed style nitpicks

Review of attachment 301093 [details] [review]:

This looks good to me and was discussed on IRC, though I would prefer desrt to ACK it as well.

In process_detach u missed another indentation. :)

Created attachment 301140 [details] [review]
Add g_system_thread_set_name() implementation for W32 threads v3

v3: Fixed indentation in g_thread_win32_process_detach()

Review of attachment 301093 [details] [review]:

Why can't we just use IsDebuggerPresent to decide if we should set the name or not?

ie: why do we ever need an exception handler here?

Well, IsDebuggerPresent() check only checks for a debugger being attached, and i don't know how it does that. One might imagine that some piece of software would want to handle exceptions, including the threadname-setting exception, without being an attached debugger. Conditioning SetThreadName on IsDebuggerPresent() will prevent that hypothetical piece of software from doing that.

We need an exception handler because:

A) see above (can't be sure that IsDebuggerPresent() will always indicate that there IS/ISN'T something that would catch the exception; not caught -> crash)

B) not all debuggers support this (no support -> exception not caught -> crash)

Hi,

Sorry for not getting to this earlier, as:

-I was working on getting Pango master to be buildable
 out of the box from a tarball generated from git master.
-My main developer's box hard drive went out-of service.

I ran the code against Visual Studio 2013 Community, and the MSVC portion of the code seemed to run fine, with the thread names shown in the debugger, but few things to note here, that you might want to note in comments:

-The Express Editions of Visual Studio do not have
 thread windows in their debuggers, so one cannot see
 the thread name there (one must use windbg in such a
 case, if needed).  The community editions (that we have
 from late last year and on) support this.

-If one is installing a vectored exception handler (VEH) on the
 exception 0x406D1388, the Visual Studio debugger (or windbg) is
 going to catch and handle that exception first, which means the
 self-defined exception handler is not going to be invoked when
 a debugger is attached, unless "Visual C++ exception" are
 disabled in windbg.[1]

[1]: http://blogs.msdn.com/b/stevejs/archive/2005/12/19/505815.aspx

With blessings.

(In reply to Fan, Chun-wei from comment #9)
> -If one is installing a vectored exception handler (VEH) on the
>  exception 0x406D1388, the Visual Studio debugger (or windbg) is
>  going to catch and handle that exception first, which means the
>  self-defined exception handler is not going to be invoked when
>  a debugger is attached, unless "Visual C++ exception" are
>  disabled in windbg.[1]
> 
> [1]: http://blogs.msdn.com/b/stevejs/archive/2005/12/19/505815.aspx

Same behaviour as gdb (when gdb is patched to support exception 0x406D1388). This is, presumably, also how SEH works too, and why there's an (empty) SEH handler in MSVC version. VEH version is the same thing, just more verbose (because no compiler support).

I really don't care for this approach and I'd like to see another (if any).  Before that, though -- how is this dealt with in other similar frameworks?  What does Qt do, or Mozilla, for example?

Mozilla does this - https://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/windows/ntthread.c#305 , which is basically the same technique, although they do check for IsDebuggerPresent() (citing performance concerns - it's faster to check for a debugger than to throw and catch an exception when it's not present) and have no need to use VEH, because Firefox is built with MSVC only, apparently.

A possibility of monitoring for debugger attaching and firing the thread-naming exceptions *then* (thus supporting situations when debugged is attached after the threads are created, in which case it would normally miss out on the exceptions) was mentioned in the bug report https://bugzilla.mozilla.org/show_bug.cgi?id=720778, but apparently never made it through.

Qt does this - http://code.qt.io/cgit/qt/qt.git/tree/src/corelib/thread/qthread_win.cpp#n303 , which is basically the same technique, they don't check for debugger and don't use VEH (thread naming support is only available on W32 when building with MSVC).

I leave this entirely in the hands of Chun-wei Fan to decide, then...

Review of attachment 301140 [details] [review]:

Hi,

I'd say go ahead for the Visual Studio part, and please add the comments on VEH in comment 9, in case people try to do such a thing, though it shouldn't be common.  For the GCC (!_MSC_VER) part, perhaps keep it deactivated until the GDB patch gets upstream and released, as it is probably not going to work until so, as the VEH variant, however, does not work in Visual Studio (possibly due to the special handling of 0x406D1388), for records, when I tested it.

Thanks LRN for getting this done.

With blessings, thank you!

(In reply to Fan, Chun-wei from comment #14)
> Review of attachment 301140 [details] [review] [review]:
> 
> I'd say go ahead for the Visual Studio part, and please add the comments on
> VEH in comment 9, in case people try to do such a thing, though it shouldn't
> be common.

I don't think this is necessary.
MSVC (pseudo)code:
> set_thread_name (name) {
>   try {
>     raise (name)
>   }
>   except (CONTINUE) {
>   }
> }
means "raise the exception. If first-chance exception handler (which would be an attached debugger) doesn't handle it, run the "except" handler, which (because CONTINUE) just continues the execution, ignoring the exception"

VEH (pseudo)code:

> set_thread_name () {
>   raise (name)
> }
>
> handler() {
>   continue
> }
means "raise the exception. If first-chance exception handler (which would be an attached debugger) doesn't handle it, run the installed handler, which (because it returns CONTINUE) just continues the execution, ignoring the exception".

If it sounds very similar, that's because it is similar.
The note on the blog post you've linked in comment 9 was meant for the case when an application wants to handle the setthreadname exception internally, before the debugger can catch it. As the post (correctly) says, that's impossible, because debugger gets first-chance exception handling and does handle it, preventing app-installed handler from running.

Being MSVC-centric, the poster assumed that VEH is used by everyone to handle arbitrary exceptions anywhere in the code (because you install handler in one place, and it gets invoked for any exception, unlike SEH, which is tied to a scope), but for MinGW VEH is the only way to handle exceptions (because no i686 SEH support), so we're using it for everything, such as ignoring particular exceptions.

App-installed handler (VEH handler returning CONTINUE in VEH case, SEH CONTINUE handler in SEH case) is there not to do any real work, it's there to make sure that the exception, if not first-chance-handled (because no debugger), would be ignored (instead of going unhandled and crashing the program).


> For the GCC (!_MSC_VER) part, perhaps keep it deactivated until
> the GDB patch gets upstream and released, as it is probably not going to
> work until so, as the VEH variant, however, does not work in Visual Studio
> (possibly due to the special handling of 0x406D1388), for records, when I
> tested it.

That is not what i'm getting. I've tested my VEH version (although i admit that i tested my minimal VEH setthreadname example program, not the glib patch i've posted; maybe i screwed up somewhere there) and it worked with WinDbg, and thus should work with MSVS. I've also asked another person to test my minimal VEH setthreadname example with MSVC, and it worked there as well. So i'd say that lack of support in GDB shouldn't prevent this code from being deployed, bugs should prevent this code from being deployed.

Created attachment 301500 [details] [review]
Extend POSIX g_system_thread_set_name() impl with 2-arg pthread_setname_np

This is how it looks when the same code is buried deep in pthreads
implementation. Looks nicer, right? And the only reason any change to glib
is necessary at all is that i don't want to add the single-argument version
of the function to winpthreads (compatibility with Linux > compatibility with
OSX).

This patch complements attachment 301140 [details] [review]

Review of attachment 301140 [details] [review]:

Hello LRN,

I think if that is the case, I would be OK at least for the _MSC_VER part.  If the !_MSC_VER part of this patch really worked for you, assuming Ryan has given me the consent, please feel free to push this.

I can't say much about the other patch, since I am not that familiar with the pthreads case on Windows...

With blessings, thank you!

(In reply to Fan, Chun-wei from comment #17)
> Review of attachment 301140 [details] [review] [review]:
> 
> I can't say much about the other patch, since I am not that familiar with
> the pthreads case on Windows...

That part is mostly for desrt to chew on - he seems unhappy with all the exception stuff being in glib; stuffing it inside of pthreads implementation and pretending that no exception handling is done might make him happy again.

Speaking of which, i ran a winpthreads patch past ktietz, he OK'ed it (i hope to resolve the remaining issues today and push). I might even get it there in time for 4.0.2 mingw-w64 release.

Pushed http://sourceforge.net/p/mingw-w64/mingw-w64/ci/0d95c795b44b76e1b60dfc119fd93cfd0cb35816

Created attachment 301843 [details] [review]
Extend POSIX g_system_thread_set_name() impl with 2-arg pthread_setname_np

* Fixed nitpicks

Review of attachment 301500 [details] [review]:

This looks good, but since we can use this on Linux now, let's get rid of the Linux branch (and associated configure checks and #includes).  That will be a nice cleanup.

(In reply to Ryan Lortie (desrt) from comment #21)
> Review of attachment 301500 [details] [review] [review]:
> 
> This looks good, but since we can use this on Linux now, let's get rid of
> the Linux branch (and associated configure checks and #includes).  That will
> be a nice cleanup.

You do that. I'm not prepared to test this on Linux, and i don't want to submit patches that i can't even test for compiling correctly.

Created attachment 324565 [details] [review]
Patch to support 2 arg form of pthread_setname_np on Solaris

I was about to file a new bug to add support for the 2 argument form of
pthread_setname_np() that's in Solaris 11.3 & later, but it appears my
patch is almost identical to what was submitted above as attachment 301843 [details] [review].

This is tested & working for me on Solaris.

Attachment 301140 [details] was pushed as e118856

Other attachments were made obsolete by 99bdfd1