Re: Program-assigned thread names on Windows

[LRN <lrn1986@gmail.com>]

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

On 25.07.2016 15:17, Jon Turney wrote:
> 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:
>>>>>>
>>>>>> 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?

No reason. If you want, run sed -e
's/WINDOWS_THREADNAME_EXCEPTION/MS_VC_EXCEPTION' over the patch file prior
to committing it.

That said, i think "MS_VC_EXCEPTION" does not offer a good enough
description (doesn't mention threads, does mention VisualC).

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

Yes, it should.

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

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

On the other hand, the example code for throwing the exception uses 32-bit
DWORD fields explicitly. I don't know what the OS does between the
exception being thrown and given to gdb.

I'll try to use i686 gdb to debug an x86_64 process, but the reverse would
be difficult, as i lack an established buildsystem for building x86_64 gdb.

EXCEPTION_RECORD layout aside, casting thread ID into 32-bit DWORD should
be OK, because thread IDs are 32-bit even on 64-bit Windows.

Casting thread_name_target to uintptr_t is less clear. On one hand, it
could be x86_64 pointer in debugee address space. On the other hand, there
are some calls in windows-nat.c (to WriteProcessMemory(), for example) that
do this kind of casting. Most likely the correct way to do this is to cast
it to CORE_ADDR...

This will most likely produce extra warnings after EXCEPTION_RECORD ->
EXCEPTION_RECORD32/64 change, i'll see what gcc has to say about this.

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

Good point. I think it's best to check the last byte in the string to be 0,
using the value returned by target_read_string(), and set it to 0 if it
isn't. Give it 1025 instead of 1024 as the maximum (although either is
arbitrary, really).


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