Re: [PATCH] Add type for $_tlb->process_environment_block->process_parameters

[Simon Marchi <simark at simark dot ca>]

On 2020-01-16 6:29 a.m., Hannes Domani via gdb-patches wrote:
>> Could you add links in the commit message to the relevant documentation that describes these types?
>>
>> I am guessing this, for example:
>>
>> https://docs.microsoft.com/en-us/windows/win32/api/ntdef/ns-ntdef-_unicode_string
> 
> Yes, that's the one.
> 
> But for rtl_user_process_parameters only very little is officially documented:
> https://docs.microsoft.com/en-us/windows/win32/api/winternl/ns-winternl-rtl_user_process_parameters
> 
> So I used this instead:
> https://www.nirsoft.net/kernel_struct/vista/RTL_USER_PROCESS_PARAMETERS.html
> 
> Should I use that for the commit message?

Sure, you can say exactly that, "The official documentation [1] is limited so I've
used this other page [2]."  Put yourself in the shoes of somebody who is trying to
understand your code and doesn't have the context you have, all these references
can be useful.

>> This function (windows_get_tlb_type) appears to be called everytime the $_tlb variable
>> is used.  The arch_*_type functions allocate new types every time.  This would mean
>> that new types are allocate every time $_tlb is used.  Am I reading this right?
>>
>> If so, it's a problem that predates your patch.  But I think the right way of doing this
>> would be to create the types when the gdbarch is created, store them in the
>> gdbarch_tdep structure, and use them here.
> 
> Actually, this is at the top of windows_get_tlb_type:
>   /* Do not rebuild type if same gdbarch as last time.  */
>   if (last_tlb_type && last_gdbarch == gdbarch)
>     return last_tlb_type;

Ah, I missed that.  Well, that's not ideal because if you repeatedly print
tlb values of two inferiors that have different gdbarches, then it will
allocate new types.  But the probability of this happening is probably very
low, and there are more important problems with the debugging on Windows,
so I'm fine with leaving that as-is.

>>>     /* list entry */
>>>
>>> @@ -168,6 +178,59 @@ windows_get_tlb_type (struct gdbarch *gdbarch)
>>>                   NULL);
>>>     TYPE_TARGET_TYPE (peb_ldr_ptr_type) = peb_ldr_type;
>>>
>>> +  /* struct UNICODE_STRING */
>>> +  uni_str_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
>>> +  TYPE_NAME (uni_str_type) = xstrdup ("unicode_string");
>>> +
>>> +  append_composite_type_field (uni_str_type, "length", word_type);
>>> +  append_composite_type_field (uni_str_type, "maximum_length", word_type);
>>> +  append_composite_type_field_aligned (uni_str_type, "buffer",
>>> +                      wchar_ptr_type,
>>
>> The actual name in the Windows API UNICODE_STRING (in capital letters), and
>> the fields are in camel case, such as "MaximumLength".  Logically, I would
>> expect GDB to name the types and fields the same way.
>>
>> However, I see that the existing types created by this function (that predate
>> your patch) don't do this.  Do you have any idea why it was done like that?
>>
>> I suppose it's better to continue using the same format as the existing types
>> (as you did), but still I find this odd.
> 
> I actually wondered about this myself, but I also just used the existing format.

Ok, it would be good to mention that in the commit message, to explain your choice.

Simon