Re: Program-assigned thread names on Windows

[LRN <lrn1986@gmail.com>]

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

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?

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

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

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