Re: Program-assigned thread names on Windows

[Pedro Alves <palves at redhat dot com>]

On 08/10/2016 08:11 AM, LRN wrote:
> No one seems to object.
> 

I have some comments.  See below.

> I've attached the version of the patch with the named_thread == NULL issue
> fixed, and also the ChangeLog and NEWS patches just for the hell of it.
> 

>  #define DR6_CLEAR_VALUE 0xffff0ff0
>  
> +#define MS_VC_EXCEPTION 0x406D1388
> +

This should have a describing comment.  There's one in the git commit
log we can reuse.

> @@ -1140,10 +1150,52 @@ handle_exception (struct target_waitstatus *ourstatus)
>        DEBUG_EXCEPTION_SIMPLE ("EXCEPTION_NONCONTINUABLE_EXCEPTION");
>        ourstatus->value.sig = GDB_SIGNAL_ILL;
>        break;
> +    case MS_VC_EXCEPTION:
> +      if (current_event.u.Exception.ExceptionRecord.NumberParameters >= 3
> +          && (current_event.u.Exception.ExceptionRecord.ExceptionInformation[0] & 0xFFFFFFFF) == 0x1000)
> +	{
> +	  long named_thread_id;
> +	  ptid_t named_thread_ptid;
> +	  struct thread_info *named_thread;
> +	  CORE_ADDR thread_name_target;
> +	  char *thread_name;
> +	  int thread_name_len;
> +
> +	  DEBUG_EXCEPTION_SIMPLE ("MS_VC_EXCEPTION");
> +
> +	  named_thread_id = (long) (0xFFFFFFFF & current_event.u.Exception.ExceptionRecord.ExceptionInformation[2]);
> +	  thread_name_target = current_event.u.Exception.ExceptionRecord.ExceptionInformation[1];
> +

Lines too long.  Easy to sort out by adding:

  EXCEPTION_RECORD *rec = &current_event.u.Exception.ExceptionRecord;

at the top, and then using rec instead.

Hex constants are written in lowercase.

> +	  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;
> +	  thread_name_len = target_read_string (thread_name_target, &thread_name, 1025, 0);

Overly long lines here too.

> +	  if (thread_name_len > 0 && thread_name != NULL)
> +	    {
> +	      thread_name[thread_name_len - 1] = '\0';
> +	      if (thread_name[0] != '\0' && named_thread != NULL)
> +		{
> +		  xfree (named_thread->name);
> +		  named_thread->name = thread_name;

This doesn't look correct.  This overwrites a thread name a user
has specified manually, with "thread name".  Instead, store the
name somewhere else, and then implement a target_thread_name target
method that returns that name.  Turns out there's a seemingly unused
name field in windows_thread_info that we can borrow.

> +		}
> +	      else
> +		{
> +		  xfree (thread_name);

Why bother to read the string at all if named_thread is NULL?

> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -77,6 +77,12 @@
>    The "catch syscall" command now supports catching a group of related
>    syscalls using the 'group:' or 'g:' prefix.
>  
> +* Support for thread names on MS-Windows.
> +
> +  GDB will catch and correctly handle the special exception that
> +  programs running on MS-Windows use to assign names to threads
> +  in the debugger.
> +

Should be moved to the "after 7.12" section.  I did that for you.
(Slightly edited to avoid future tense, while at it.)

I was going to apply your patches, so I squashed them all, and
only after did I notice the problems, so I went ahead and did
the changes.  I also fixed a couple typos in the commit log.

Could you give this updated patch a try?  I build-tested it using
a cross compiler, but haven't tried it out on Windows.

--------------------------------------
>From 4da942cfa605c82c292f08b0787378a0a2f82fcb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=D0=A0=D1=83=D1=81=D0=BB=D0=B0=D0=BD=20=D0=98=D0=B6=D0=B1?=
 =?UTF-8?q?=D1=83=D0=BB=D0=B0=D1=82=D0=BE=D0=B2?= <lrn1986@gmail.com>
Date: Wed, 10 Aug 2016 12:56:37 +0100
Subject: [PATCH] Support setting thread names (MS-Windows)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is done by catching an exception number 0x406D1388 (it has no
documented name, though MSDN dubs it "MS_VC_EXCEPTION" in one code
example), which is thrown by the program.  The exception record
contains an ID of a thread and a name to give it.

This requires rolling back some changes in handle_exception(), which
now again returns more than two distinct values.  The value
HANDLE_EXCEPTION_IGNORED means that gdb should just continue, without
returning thread ID up the stack (which would result in further
handling of the exception, which is not what we want).

gdb/ChangeLog:
2016-08-10  Руслан Ижбулатов  <lrn1986@gmail.com>

	* windows-nat.c (handle_exception): Handle exception 0x406D1388
	that is used to set thread name.
	* NEWS: Mention the thread naming support on MS-Windows.
---
 gdb/NEWS          |  6 ++++
 gdb/windows-nat.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 85 insertions(+), 11 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index b08d8a0..6f5feb1 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,12 @@
 
 *** Changes since GDB 7.12
 
+* Support for thread names on MS-Windows.
+
+  GDB now catches and handles the special exception that programs
+  running on MS-Windows use to assign names to threads in the
+  debugger.
+
 *** Changes in GDB 7.12
 
 * GDB and GDBserver now build with a C++ compiler by default.
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 3f67486..3b0b3e3 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -174,6 +174,18 @@ static int debug_registers_used;
 static int windows_initialization_done;
 #define DR6_CLEAR_VALUE 0xffff0ff0
 
+/* The exception number which is thrown by the program to set a
+   thread's name in the debugger.  The exception record contains an ID
+   of a thread and a name to give it.  */
+#define MS_VC_EXCEPTION 0x406d1388
+
+typedef enum
+{
+  HANDLE_EXCEPTION_UNHANDLED = 0,
+  HANDLE_EXCEPTION_HANDLED,
+  HANDLE_EXCEPTION_IGNORED
+} handle_exception_result;
+
 /* The string sent by cygwin when it processes a signal.
    FIXME: This should be in a cygwin include file.  */
 #ifndef _CYGWIN_SIGNAL_STRING
@@ -441,6 +453,7 @@ windows_delete_thread (ptid_t ptid, DWORD exit_code)
     {
       windows_thread_info *here = th->next;
       th->next = here->next;
+      xfree (here->name);
       xfree (here);
     }
 }
@@ -1031,10 +1044,12 @@ display_selectors (char * args, int from_tty)
     host_address_to_string (\
       current_event.u.Exception.ExceptionRecord.ExceptionAddress))
 
-static int
+static handle_exception_result
 handle_exception (struct target_waitstatus *ourstatus)
 {
-  DWORD code = current_event.u.Exception.ExceptionRecord.ExceptionCode;
+  EXCEPTION_RECORD *rec = &current_event.u.Exception.ExceptionRecord;
+  DWORD code = rec->ExceptionCode;
+  handle_exception_result result = HANDLE_EXCEPTION_HANDLED;
 
   ourstatus->kind = TARGET_WAITKIND_STOPPED;
 
@@ -1057,14 +1072,13 @@ handle_exception (struct target_waitstatus *ourstatus)
 	   cygwin-specific-signal.  So, ignore SEGVs if they show up
 	   within the text segment of the DLL itself.  */
 	const char *fn;
-	CORE_ADDR addr = (CORE_ADDR) (uintptr_t)
-	  current_event.u.Exception.ExceptionRecord.ExceptionAddress;
+	CORE_ADDR addr = (CORE_ADDR) (uintptr_t) rec->ExceptionAddress;
 
 	if ((!cygwin_exceptions && (addr >= cygwin_load_start
 				    && addr < cygwin_load_end))
 	    || (find_pc_partial_function (addr, &fn, NULL, NULL)
 		&& startswith (fn, "KERNEL32!IsBad")))
-	  return 0;
+	  return HANDLE_EXCEPTION_UNHANDLED;
       }
 #endif
       break;
@@ -1140,10 +1154,46 @@ handle_exception (struct target_waitstatus *ourstatus)
       DEBUG_EXCEPTION_SIMPLE ("EXCEPTION_NONCONTINUABLE_EXCEPTION");
       ourstatus->value.sig = GDB_SIGNAL_ILL;
       break;
+    case MS_VC_EXCEPTION:
+      if (rec->NumberParameters >= 3
+	  && (rec->ExceptionInformation[0] & 0xffffffff) == 0x1000)
+	{
+	  long named_thread_id;
+	  windows_thread_info* named_thread;
+	  CORE_ADDR thread_name_target;
+
+	  DEBUG_EXCEPTION_SIMPLE ("MS_VC_EXCEPTION");
+
+	  thread_name_target = rec->ExceptionInformation[1];
+	  named_thread_id = (long) (0xffffffff & rec->ExceptionInformation[2]);
+
+	  if (named_thread_id == (DWORD) -1)
+	    named_thread_id = current_event.dwThreadId;
+
+	  named_thread = thread_rec (named_thread_id, 0);
+	  if (named_thread != NULL)
+	    {
+	      int thread_name_len;
+	      char *thread_name;
+
+	      thread_name_len = target_read_string (thread_name_target,
+						    &thread_name, 1025, NULL);
+	      if (thread_name_len > 0)
+		{
+		  thread_name[thread_name_len - 1] = '\0';
+		  xfree (named_thread->name);
+		  named_thread->name = thread_name;
+		}
+	    }
+	  ourstatus->value.sig = GDB_SIGNAL_TRAP;
+	  result = HANDLE_EXCEPTION_IGNORED;
+	  break;
+	}
+	/* treat improperly formed exception as unknown, fallthrough */
     default:
       /* Treat unhandled first chance exceptions specially.  */
       if (current_event.u.Exception.dwFirstChance)
-	return 0;
+	return HANDLE_EXCEPTION_UNHANDLED;
       printf_unfiltered ("gdb: unknown target exception 0x%08x at %s\n",
 	(unsigned) current_event.u.Exception.ExceptionRecord.ExceptionCode,
 	host_address_to_string (
@@ -1153,7 +1203,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 +1560,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 HANDLE_EXCEPTION_UNHANDLED:
+	default:
+	  continue_status = DBG_EXCEPTION_NOT_HANDLED;
+	  break;
+	case HANDLE_EXCEPTION_HANDLED:
+	  thread_id = current_event.dwThreadId;
+	  break;
+	case HANDLE_EXCEPTION_IGNORED:
+	  continue_status = DBG_CONTINUE;
+	  break;
+	}
       break;
 
     case OUTPUT_DEBUG_STRING_EVENT:	/* Message from the kernel.  */
@@ -2514,6 +2573,14 @@ windows_get_ada_task_ptid (struct target_ops *self, long lwp, long thread)
   return ptid_build (ptid_get_pid (inferior_ptid), 0, lwp);
 }
 
+/* Implementation of the to_thread_name method.  */
+
+static const char *
+windows_thread_name (struct target_ops *self, struct thread_info *thr)
+{
+  return thread_rec (ptid_get_tid (thr->ptid), 0)->name;
+}
+
 static struct target_ops *
 windows_target (void)
 {
@@ -2538,6 +2605,7 @@ windows_target (void)
   t->to_pid_to_exec_file = windows_pid_to_exec_file;
   t->to_get_ada_task_ptid = windows_get_ada_task_ptid;
   t->to_get_tib_address = windows_get_tib_address;
+  t->to_thread_name = windows_thread_name;
 
   return t;
 }
-- 
2.5.5