Re: Program-assigned thread names on Windows

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

On 23/07/2016 18:01, LRN wrote:
> On 23.07.2016 19:39, John Baldwin wrote:
>> > On Saturday, July 23, 2016 12:25:15 PM LRN wrote:
>>> >> The attached patch adds thread naming support on Windows.

Nice.  A few comments below.

>>> >>
>>> >> This works as documented[1] on MSDN - by catching a specific
>>> >> exception that the program throws.
>>> >>
>>> >> Setting thread name this way is supported by glib[2] and winpthreads[3]
>>> >> at least, as well as any program developed with MS toolchain (because
>>> >> WinDbg supported this for a long time).
>>> >>
>>> >> [1] https://msdn.microsoft.com/en-us/library/xcb2z8hs.aspx
>>> >> [2] https://git.gnome.org/browse/glib/commit/glib
>>> >> /gthread-win32.c?id=e118856430a798bbc529691ad235fd0b0684439d
>>> >> [3] https://sourceforge.net/p/mingw-w64/mingw-w64/ci
>>> >> /0d95c795b44b76e1b60dfc119fd93cfd0cb35816/
>> >
>
> This is done by catching an exception number 0x406D1388
> (it has no documented name), which is thrown by the program.

The name used in the MSDN article [1] is 'MS_VC_EXCEPTION', so why not 
use that?

> +    case WINDOWS_THREADNAME_EXCEPTION:
> +      DEBUG_EXCEPTION_SIMPLE (WINDOWS_THREADNAME_EXCEPTION_S);
> +      ourstatus->value.sig = GDB_SIGNAL_TRAP;
> +      if (current_event.u.Exception.ExceptionRecord.NumberParameters == 4)
> +	{
> +	  DWORD named_thread_id;
> +	  ptid_t named_thread_ptid;
> +	  struct thread_info *named_thread;
> +	  uintptr_t thread_name_target;
> +	  char *thread_name;
> +

Shouldn't this check for ExceptionInformation[0] = 0x1000, and treat 
this as an unknown exception otherwise?

> +	  named_thread_id = (DWORD) current_event.u.Exception.ExceptionRecord.ExceptionInformation[2];
> +	  thread_name_target = (uintptr_t) current_event.u.Exception.ExceptionRecord.ExceptionInformation[1];

Is this going to be correct for 64-bit builds?

> +
> +	  if (named_thread_id == (DWORD) -1)
> +	    named_thread_id = current_event.dwThreadId;
> +
> +	  named_thread_ptid = ptid_build (current_event.dwProcessId, 0, named_thread_id),
> +	  named_thread = find_thread_ptid (named_thread_ptid);
> +
> +	  thread_name = NULL;
> +	  if (target_read_string ((CORE_ADDR) thread_name_target, &thread_name, 1024, 0))

Does thread_name end up not being null terminated if the thread name 
length really exceeds 1024? (or is improperly not null terminated in the 
target process...)

> +	    {
> +	      if (thread_name != NULL && thread_name[0] != '\0')
> +		{
> +		  xfree (named_thread->name);
> +		  named_thread->name = thread_name;
> +		}
> +	      else if (thread_name != NULL)
> +		{
> +		  /* nothing to do */;
> +		  xfree (thread_name);
> +		}
> +	    }
> +	  result = 2;
> +	}
> +      break;
>      default:
>        /* Treat unhandled first chance exceptions specially.  */
>        if (current_event.u.Exception.dwFirstChance)
> @@ -1153,7 +1194,7 @@ handle_exception (struct target_waitstatus *ourstatus)
>      }
>    exception_count++;
>    last_sig = ourstatus->value.sig;
> -  return 1;
> +  return result;
>  }
>
>  /* Resume thread specified by ID, or all artificially suspended
> @@ -1510,10 +1551,19 @@ get_windows_debug_event (struct target_ops *ops,
>  		     "EXCEPTION_DEBUG_EVENT"));
>        if (saw_create != 1)
>  	break;
> -      if (handle_exception (ourstatus))
> -	thread_id = current_event.dwThreadId;
> -      else
> -	continue_status = DBG_EXCEPTION_NOT_HANDLED;
> +      switch (handle_exception (ourstatus))
> +	{
> +	case 0:
> +	default:
> +	  continue_status = DBG_EXCEPTION_NOT_HANDLED;
> +	  break;
> +	case 1:
> +	  thread_id = current_event.dwThreadId;
> +	  break;
> +	case 2:
> +	  continue_status = DBG_CONTINUE;
> +	  break;
> +	}
>        break;
>
>      case OUTPUT_DEBUG_STRING_EVENT:	/* Message from the kernel.  */
> -- 2.4.0
>