Program-assigned thread names on Windows

  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.
  

On 26/07/2016 07:07, LRN wrote:
> 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:

>> > 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.

Ah yes, I see.

I was thrown off by your references [2], [3], which compute 
nNumberOfArguments for RaiseException() as sizeof (info) / sizeof 
(DWORD), which I think is incorrect on 64-bit, and should be 
sizeof(info) / sizeof(ULONG_PTR), as the MSDN example code has.

>> > 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

I think this is a bug in the code you are testing with, as mentioned 
above, which doubles nNumberOfArguments ...

>> > 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.

It seems on x86_64, the structure is laid out by gcc as:

4 DWORD dwType
4 padding
8 LPCSTR szName
4 DWORD dwThreadID
4 DWORD dwFlags

total size 24, so nNumberOfArguments = 3, so this is passed to the 
debugger as an array of 3 DWORD64s

Of course, the 'correct' layout is defined by how the sample code is 
laid out by MSVC, which I'm guessing is the same, but haven't checked...

So dwThreadID and dwFlags get packed together into 
ExceptionInformation[2].  Fortunately, dwFlags should be set to 0.

Furthermore, accessing dwType as a DWORD64 value via 
ExceptionInformation[0] relies on the padding being zero initialized in 
the debugee to have useful values! I guess you'll have to mask with 0xffff?

> 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.

Great, thanks.  A few minor comments below.

> 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
>
>
> 0001-Support-settings-thread-name-MS-Windows.patch
>
>
> 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;

Since this holds a Win32 thread id, should it be DWORD type?

> +	  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);
> +

	  DEBUG_EXCEPTION_SIMPLE ("MS_VC_EXCEPTION"); ?

> +	  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';

I'd just null-terminate unconditionally.

> +	      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))

Would it be clearer to use an enum to name these return cases from 
handle_exception()?

> +	{
> +	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.  */
  

On 26.07.2016 16:18, Jon Turney wrote:
> On 26/07/2016 07:07, LRN wrote:
>> 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:
> 
>>>> 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.
> 
> Ah yes, I see.
> 
> I was thrown off by your references [2], [3], which compute 
> nNumberOfArguments for RaiseException() as sizeof (info) / sizeof 
> (DWORD), which I think is incorrect on 64-bit, and should be 
> sizeof(info) / sizeof(ULONG_PTR), as the MSDN example code has.

Ah, that must be where '6' came from. Indeed, sizeof (THEADNAME_INFO) is
[4 +4padding] + [8] + [4 + 4] = 24.
24 / 4 = 6.

Also, i stand corrected. I've claimed that the array is realigned when it
passes the 32-bit/64-bit barrier, and it is. However, there's no such thing
when 64-bit process throws an exception and 64-bit debugger caches it. So,
because, as i've shown above, not all fields are 8-byte aligned (first two
fields are aligned, because one of them is a self-aligned pointer, but the
last two fields are packed together into one 8-byte slot), it can look
weird when ExceptionInformation[] is interpreted as an array of
pointer-sized values.

This is concerning, as i want the code that throws the exception to be
compatible with MSVS and WinDbg, and for gdb to support *that* version.

> 
>>>> 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
> 
> I think this is a bug in the code you are testing with, as mentioned 
> above, which doubles nNumberOfArguments ...

Yep.

> 
>>>> 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.
> 
> It seems on x86_64, the structure is laid out by gcc as:
> 
> 4 DWORD dwType
> 4 padding
> 8 LPCSTR szName
> 4 DWORD dwThreadID
> 4 DWORD dwFlags
> 
> total size 24, so nNumberOfArguments = 3, so this is passed to the 
> debugger as an array of 3 DWORD64s
> 
> Of course, the 'correct' layout is defined by how the sample code is 
> laid out by MSVC, which I'm guessing is the same, but haven't checked...
> 
> So dwThreadID and dwFlags get packed together into 
> ExceptionInformation[2].  Fortunately, dwFlags should be set to 0.
> 
> Furthermore, accessing dwType as a DWORD64 value via 
> ExceptionInformation[0] relies on the padding being zero initialized in 
> the debugee to have useful values! I guess you'll have to mask with 0xffff?

I'll play a bit with the 64-bit exception-throwing example and see how
WinDbg reacts to various combinations of alignment and argument counting,
and will make gdb support the layout that WinDbg supports.

>> +	{
>> +	  long named_thread_id;
> 
> Since this holds a Win32 thread id, should it be DWORD type?

I've changed it into long, because long is what ptid_build() takes. If
there's some kind of typecast going on, it'll happen when we assign things
to named_thread_id, not when we pass them to ptid_build().

>> +	  DEBUG_EXCEPTION_SIMPLE (MS_VC_EXCEPTION_S);
>> +
> 
> 	  DEBUG_EXCEPTION_SIMPLE ("MS_VC_EXCEPTION"); ?

I would actually prefer:
DEBUG_EXCEPTION_SIMPLE (STRINGIFY(MS_VC_EXCEPTION))
, but i don't know if gdb has a stringifying macro somewhere, and haven't
bothered to look. As i've said previously, MS_VC_EXCEPTION is not a
fully-documented name. It certainly is not in any SDK. But if you *want* to
show "MS_VC_EXCEPTION", that's easily done, obviously.

>> +	  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';
> 
> I'd just null-terminate unconditionally.

Okay.

>> @@ -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))
> 
> Would it be clearer to use an enum to name these return cases from 
> handle_exception()?

It would be. Where should id put its definition and how do i name it and
its values?
  

On 26.07.2016 17:17, LRN wrote:
> On 26.07.2016 16:18, Jon Turney wrote:
>> On 26/07/2016 07:07, LRN wrote:
>>> 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:
>>>>> 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.
>>
>> It seems on x86_64, the structure is laid out by gcc as:
>>
>> 4 DWORD dwType
>> 4 padding
>> 8 LPCSTR szName
>> 4 DWORD dwThreadID
>> 4 DWORD dwFlags
>>
>> total size 24, so nNumberOfArguments = 3, so this is passed to the 
>> debugger as an array of 3 DWORD64s
>>
>> Of course, the 'correct' layout is defined by how the sample code is 
>> laid out by MSVC, which I'm guessing is the same, but haven't checked...
>>
>> So dwThreadID and dwFlags get packed together into 
>> ExceptionInformation[2].  Fortunately, dwFlags should be set to 0.
>>
>> Furthermore, accessing dwType as a DWORD64 value via 
>> ExceptionInformation[0] relies on the padding being zero initialized in 
>> the debugee to have useful values! I guess you'll have to mask with 0xffff?
> 
> I'll play a bit with the 64-bit exception-throwing example and see how
> WinDbg reacts to various combinations of alignment and argument counting,
> and will make gdb support the layout that WinDbg supports.

Played around with 64-bit WinDbg.
It worked with the code that i've used originally (from MSDN with no
significant changes). Also:

1) WinDbg (of either bitness) doesn't care what the argument count is, as
long as it's at least 3 (0x1000, string pointer and thread ID); giving it 2
makes it silently drop the exception and not set the thread name

2) WinDbg (of either bitness) currently doesn't care what you put in
dwFlags. I've tried filling dwFlags with garbage (a copy of the dwThreadID
value, for example), and WinDbg still set the thread name correctly,
without misidentifying the thread.
This leads me to believe that, as you've suggested, 64-bit WinDbg does &
0x00000000FFFFFFFF on ExceptionInformation[2] (32-bit WinDbg doesn't have to).

Also of note, 32-bit WinDbg can't debug 64-bit executables, but 64-bit
WinDbg can debug 32-bit executables.

Maybe they foresaw the use of 64-bit architectures (i can't dig deeper into
the MSDN than MSVC 2003, not sure how the thread-name example looked in
MSVC 6.0 era) and padded the struct size to be a multiple of 8, reserving
the last DWORD for future use; later realized that due to struct packing a
64-bit debugger would get 3 64-bit pointer-sized values, with the last one
being a combination of threadid and flags, and adapted their debugger to
handle exactly this case.

Anyway, for gdb:
1) Look for at least 3 arguments.
2) In 64-bit gdb do the & 0xFFFFFFFF on the 3rd member to get thread id.

And that's it.