Re: Program-assigned thread names on Windows

[LRN <lrn1986@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4814 bytes --]

On 26.07.2016 0:32, LRN wrote:
> On 25.07.2016 17:23, LRN wrote:
>> On 25.07.2016 17:06, Jon Turney wrote:
>>> On 25/07/2016 14:34, LRN wrote:
>>>> On 25.07.2016 15:17, Jon Turney wrote:
>>>>> On 23/07/2016 18:01, LRN wrote:
>>>>>> +	  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?
>>>>
>>>> I've only tested this on i686.
>>>>
>>>> Which variable are you concerned about - named_thread_id or thread_name_target?
>>>
>>> Both.  The ExceptionInformation isn't actually array of DWORDs, it's a 
>>> THREADNAME_INFO structure, which contains a LPCSTR pointer (which has a 
>>> different size on x86 and x86_64) *before* the thread id.
>>>
>>> So, I think this should check that NumbersParameters * sizeof(DWORD) is 
>>> equal to or greater than sizeof(THREADNAME_INFO), then cast 
>>> ExceptionInformation to a THREADNAME_INFO.
>>>
>>>> Tough this is a good point. MSDN says that i686 and x86_64 EXCEPTION_RECORD
>>>> structures have different layout (well, to-be-pointer struct fields are
>>>> DWORD64 on x86_64).
>>>
>>> I don't think gdb currently supports 32/64 bit interworking on Windows, 
>>> so perhaps that is all moot (although if that is the case, perhaps it 
>>> should diagnose attempts to do that)
>>>
>>
>> Yep, just tried to attach to a 64-bit process from a 32-bit gdb, and gdb
>> failed to attach.
>>
>> I'll try to come up with a way to build 64-bit gdb... it might take a while
>> though.
>>
> 
> 1) 64-bit gdb can attach to 32-bit debugees.
> 64-bit gdb sure throws a number of warnings when attaching to a 32-bit
> debugee, but still attaches. However, it quickly gets into a tailspin, if i
> do anything other than "run" (set breakpoints, step through functions).
> 
> 2) EXCEPTION_RECORD does not need to be casted into EXCEPTION_RECORD32 or
> EXCEPTION_RECORD64 for native processes, as it's correctly aligned in
> either way ("2x4, 2 pointers, 4, pointer" - for 32-bit case everything is
> tightly packed and 4-byte aligned, for 64-bit case the last pointer moves 4
> bytes further to be self-aligned to 8 bytes, while everything else remains
> the same), so we can keep accessing stuff via EXCEPTION_RECORD natively.
> That is, EXCEPTION_RECORD64 is how EXCEPTION_RECORD normally looks in
> 64-bit process.
> 
> 3) EXCEPTION_RECORD that we receive is sized to *gdb* bitness. That is,
> casing it to EXCEPTION_RECORD32 in 64-bit gdb will always lead to bad
> interpretation, even if debugee is 32-bit.
> 
> 4) ExceptionInfromation array that we receive as part of EXCEPTION_RECORD
> is *also natively aligned for gdb*. I've made 32-bit debugee print out the
> addresses of fields of the THEADNAME_INFO structure, and it's aligned to 4
> bytes (as expected), but examining the EXCEPTION_RECORD structure that
> 64-bit gdb receives shows that the ExceptionInformation array is aligned to
> 8 bytes. Therefore, it's safe to always use EXCEPTION_RECORD as-is, without
> worrying about alignment of the ExceptionInformation data.
> 
> 5) 64-bit gdb receives an EXCEPTION_RECORD with NumberParameters == 6 when
> debugee is 64-bit. The contents of the extra 2 elements are a mystery (they
> seem to point to the stack, but that's all i can tell). Also, the 4-th
> element (which is "Reserved for future use, must be zero") is not zero when
> the exception is caught.
> In light of this, we should probably check for NumberParameters >= 4. Or
> even NumberParameters >= 3, given that we don't really look at the 4th
> parameter.
> 

Attaching the latest version of the patch:

* Treats ExceptionInformation[0] != 0x1000 or NumberParameters < 3 as
unknown exception.
* Uses (hopefully) correct datatypes for thread_name_target and
named_thread_id.
* Ensures thread name is 0-terminated, doesn't leak.
* Uses "MS_VC_EXCEPTION" as the exception name.

By the way, the realignment of the ExceptionInformation when it is passed
from a 32-bit process to a 64-bit one suggests that RaiseException()
documentation is actually precise: ExceptionInformation is an array of
pointer-sized values, and is treated as such. As a test, i've tried to pass
a struct with 12 separate char fields initialized into consecutive numbers
(and packed tightly, i've checked), and by the time gdb got it, the
"struct" was chopped into groups of 4 bytes, each of which was padded by 4
empty extra bytes.
MS uses THREADNAME_INFO struct in its example, but it really should have
used an array of ULONG_PTR, because that is what is being actually sent.

-- 
O< ascii ribbon - stop html email! - www.asciiribbon.org

[-- Attachment #1.1.2: 0001-Support-settings-thread-name-MS-Windows.patch --]
[-- Type: text/plain, Size: 4303 bytes --]

From 141c4ff8f185dd2ee1a8ffbf4d26a21e16c852bd 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: Sun, 26 Jun 2016 11:14:49 +0000
Subject: [PATCH 1/3] Support settings thread name (MS-Windows)

This is done by catching an exception number 0x406D1388
(it has no documented name), 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 code
2 means that gdb should just continue, without returning
thread ID up the stack (which will result in further handling
of the exception, which is not what we want).
---
 gdb/windows-nat.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 5 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 3f67486..084d5a9 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -174,6 +174,9 @@ static int debug_registers_used;
 static int windows_initialization_done;
 #define DR6_CLEAR_VALUE 0xffff0ff0
 
+#define MS_VC_EXCEPTION 0x406D1388
+#define MS_VC_EXCEPTION_S "0x406D1388"
+
 /* The string sent by cygwin when it processes a signal.
    FIXME: This should be in a cygwin include file.  */
 #ifndef _CYGWIN_SIGNAL_STRING
@@ -1035,6 +1038,7 @@ static int
 handle_exception (struct target_waitstatus *ourstatus)
 {
   DWORD code = current_event.u.Exception.ExceptionRecord.ExceptionCode;
+  int result = 1;
 
   ourstatus->kind = TARGET_WAITKIND_STOPPED;
 
@@ -1140,6 +1144,49 @@ 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] == 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_S);
+
+	  named_thread_id = (long) current_event.u.Exception.ExceptionRecord.ExceptionInformation[2];
+	  thread_name_target = current_event.u.Exception.ExceptionRecord.ExceptionInformation[1];
+
+	  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);
+	  if (thread_name_len > 0 && thread_name != NULL)
+	    {
+	      if (thread_name[thread_name_len - 1] != '\0')
+		thread_name[thread_name_len - 1] = '\0';
+	      if (thread_name[0] != '\0')
+		{
+		  xfree (named_thread->name);
+		  named_thread->name = thread_name;
+		}
+	      else
+		{
+		  xfree (thread_name);
+		}
+	    }
+	  ourstatus->value.sig = GDB_SIGNAL_TRAP;
+	  result = 2;
+	  break;
+	}
+	/* treat improperly formed exception as unknown, fallthrough */
     default:
       /* Treat unhandled first chance exceptions specially.  */
       if (current_event.u.Exception.dwFirstChance)
@@ -1153,7 +1200,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 +1557,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


[-- Attachment #1.1.3: 0x6759BA74.asc --]
[-- Type: application/pgp-keys, Size: 3540 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]