Add type for $_tlb->process_environment_block->process_parameters

  The type then looks like this:

(gdb) pt $_tlb->process_environment_block->process_parameters
type = struct rtl_user_process_parameters {
    DWORD32 maximum_length;
    DWORD32 length;
    DWORD32 flags;
    DWORD32 debug_flags;
    void *console_handle;
    DWORD32 console_flags;
    void *standard_input;
    void *standard_output;
    void *standard_error;
    unicode_string current_directory;
    void *current_directory_handle;
    unicode_string dll_path;
    unicode_string image_path_name;
    unicode_string command_line;
    void *environment;
    DWORD32 starting_x;
    DWORD32 starting_y;
    DWORD32 count_x;
    DWORD32 count_y;
    DWORD32 count_chars_x;
    DWORD32 count_chars_y;
    DWORD32 fill_attribute;
    DWORD32 window_flags;
    DWORD32 show_window_flags;
    unicode_string window_title;
    unicode_string desktop_info;
    unicode_string shell_info;
    unicode_string runtime_data;
} *

It's mainly useful to get the current directory, or the full command line:

(gdb) p $_tlb->process_environment_block->process_parameters->current_directory
$1 = {
  length = 26,
  maximum_length = 520,
  buffer = 0xe36c8 L"C:\\src\\tests\\"
}
(gdb) p $_tlb->process_environment_block->process_parameters->command_line
$2 = {
  length = 94,
  maximum_length = 96,
  buffer = 0xe32aa L"\"C:\\gdb\\build64\\gdb-git\\gdb\\gdb.exe\" access.exe"
}

gdb/ChangeLog:

2019-12-23  Hannes Domani  <ssbssa@yahoo.de>

	* windows-tdep.c (windows_get_tlb_type):
	Add rtl_user_process_parameters type.
---
 gdb/windows-tdep.c | 65 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)
  

I was hoping that our Windows specialist would review this :), but I'll
give it a look.

On 2019-12-23 11:10 a.m., Hannes Domani via gdb-patches wrote:
> The type then looks like this:
> 
> (gdb) pt $_tlb->process_environment_block->process_parameters
> type = struct rtl_user_process_parameters {
>     DWORD32 maximum_length;
>     DWORD32 length;
>     DWORD32 flags;
>     DWORD32 debug_flags;
>     void *console_handle;
>     DWORD32 console_flags;
>     void *standard_input;
>     void *standard_output;
>     void *standard_error;
>     unicode_string current_directory;
>     void *current_directory_handle;
>     unicode_string dll_path;
>     unicode_string image_path_name;
>     unicode_string command_line;
>     void *environment;
>     DWORD32 starting_x;
>     DWORD32 starting_y;
>     DWORD32 count_x;
>     DWORD32 count_y;
>     DWORD32 count_chars_x;
>     DWORD32 count_chars_y;
>     DWORD32 fill_attribute;
>     DWORD32 window_flags;
>     DWORD32 show_window_flags;
>     unicode_string window_title;
>     unicode_string desktop_info;
>     unicode_string shell_info;
>     unicode_string runtime_data;
> } *
> 
> It's mainly useful to get the current directory, or the full command line:
> 
> (gdb) p $_tlb->process_environment_block->process_parameters->current_directory
> $1 = {
>   length = 26,
>   maximum_length = 520,
>   buffer = 0xe36c8 L"C:\\src\\tests\\"
> }
> (gdb) p $_tlb->process_environment_block->process_parameters->command_line
> $2 = {
>   length = 94,
>   maximum_length = 96,
>   buffer = 0xe32aa L"\"C:\\gdb\\build64\\gdb-git\\gdb\\gdb.exe\" access.exe"
> }

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

> 
> gdb/ChangeLog:
> 
> 2019-12-23  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* windows-tdep.c (windows_get_tlb_type):
> 	Add rtl_user_process_parameters type.
> ---
>  gdb/windows-tdep.c | 65 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
> index bb69a79996..83a80f2f80 100644
> --- a/gdb/windows-tdep.c
> +++ b/gdb/windows-tdep.c
> @@ -114,6 +114,8 @@ windows_get_tlb_type (struct gdbarch *gdbarch)
>    struct type *peb_type, *peb_ptr_type, *list_type;
>    struct type *module_list_ptr_type;
>    struct type *tib_type, *seh_type, *tib_ptr_type, *seh_ptr_type;
> +  struct type *word_type, *wchar_type, *wchar_ptr_type;
> +  struct type *uni_str_type, *rupp_type, *rupp_ptr_type;
>  
>    /* Do not rebuild type if same gdbarch as last time.  */
>    if (last_tlb_type && last_gdbarch == gdbarch)
> @@ -123,7 +125,15 @@ windows_get_tlb_type (struct gdbarch *gdbarch)
>  				 1, "DWORD_PTR");
>    dword32_type = arch_integer_type (gdbarch, 32,
>  				 1, "DWORD32");
> +  word_type = arch_integer_type (gdbarch, 16,
> +				 1, "WORD");
> +  wchar_type = arch_integer_type (gdbarch, 16,
> +				  1, "wchar_t");
>    void_ptr_type = lookup_pointer_type (builtin_type (gdbarch)->builtin_void);
> +  wchar_ptr_type = arch_type (gdbarch, TYPE_CODE_PTR,
> +			      TYPE_LENGTH (void_ptr_type) * TARGET_CHAR_BIT,
> +			      NULL);
> +  TYPE_TARGET_TYPE (wchar_ptr_type) = wchar_type;

Could you use arch_pointer_type instead of setting the TYPE_TARGET_TYPE by hand?

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.

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

> +				       TYPE_LENGTH (wchar_ptr_type));
> +
> +  /* struct _RTL_USER_PROCESS_PARAMETERS */
> +  rupp_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
> +  TYPE_NAME (rupp_type) = xstrdup ("rtl_user_process_parameters");

Couldn't you pass the name directly to arch_composite_type?

Simon
  

Am Mittwoch, 15. Januar 2020, 22:27:40 MEZ hat Simon Marchi <simark@simark.ca> Folgendes geschrieben:

> I was hoping that our Windows specialist would review this :), but I'll
> give it a look.
>
> On 2019-12-23 11:10 a.m., Hannes Domani via gdb-patches wrote:
> > The type then looks like this:
> >
> > (gdb) pt $_tlb->process_environment_block->process_parameters
> > type = struct rtl_user_process_parameters {
> >    DWORD32 maximum_length;
> >    DWORD32 length;
> >    DWORD32 flags;
> >    DWORD32 debug_flags;
> >    void *console_handle;
> >    DWORD32 console_flags;
> >    void *standard_input;
> >    void *standard_output;
> >    void *standard_error;
> >    unicode_string current_directory;
> >    void *current_directory_handle;
> >    unicode_string dll_path;
> >    unicode_string image_path_name;
> >    unicode_string command_line;
> >    void *environment;
> >    DWORD32 starting_x;
> >    DWORD32 starting_y;
> >    DWORD32 count_x;
> >    DWORD32 count_y;
> >    DWORD32 count_chars_x;
> >    DWORD32 count_chars_y;
> >    DWORD32 fill_attribute;
> >    DWORD32 window_flags;
> >    DWORD32 show_window_flags;
> >    unicode_string window_title;
> >    unicode_string desktop_info;
> >    unicode_string shell_info;
> >    unicode_string runtime_data;
> > } *
> >
> > It's mainly useful to get the current directory, or the full command line:
> >
> > (gdb) p $_tlb->process_environment_block->process_parameters->current_directory
> > $1 = {
> >  length = 26,
> >  maximum_length = 520,
> >  buffer = 0xe36c8 L"C:\\src\\tests\\"
> > }
> > (gdb) p $_tlb->process_environment_block->process_parameters->command_line
> > $2 = {
> >  length = 94,
> >  maximum_length = 96,
> >  buffer = 0xe32aa L"\"C:\\gdb\\build64\\gdb-git\\gdb\\gdb.exe\" access.exe"
> > }
>
> 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?

>
> >
> > gdb/ChangeLog:
> >
> > 2019-12-23  Hannes Domani  <ssbssa@yahoo.de>
> >
> >     * windows-tdep.c (windows_get_tlb_type):
> >     Add rtl_user_process_parameters type.
> > ---
> >  gdb/windows-tdep.c | 65 +++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 64 insertions(+), 1 deletion(-)
> >
> > diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
> > index bb69a79996..83a80f2f80 100644
> > --- a/gdb/windows-tdep.c
> > +++ b/gdb/windows-tdep.c
> > @@ -114,6 +114,8 @@ windows_get_tlb_type (struct gdbarch *gdbarch)
> >    struct type *peb_type, *peb_ptr_type, *list_type;
> >    struct type *module_list_ptr_type;
> >    struct type *tib_type, *seh_type, *tib_ptr_type, *seh_ptr_type;
> > +  struct type *word_type, *wchar_type, *wchar_ptr_type;
> > +  struct type *uni_str_type, *rupp_type, *rupp_ptr_type;
> >
> >    /* Do not rebuild type if same gdbarch as last time.  */
> >    if (last_tlb_type && last_gdbarch == gdbarch)
> > @@ -123,7 +125,15 @@ windows_get_tlb_type (struct gdbarch *gdbarch)
> >                  1, "DWORD_PTR");
> >    dword32_type = arch_integer_type (gdbarch, 32,
> >                  1, "DWORD32");
> > +  word_type = arch_integer_type (gdbarch, 16,
> > +                1, "WORD");
> > +  wchar_type = arch_integer_type (gdbarch, 16,
> > +                  1, "wchar_t");
> >    void_ptr_type = lookup_pointer_type (builtin_type (gdbarch)->builtin_void);
> > +  wchar_ptr_type = arch_type (gdbarch, TYPE_CODE_PTR,
> > +                  TYPE_LENGTH (void_ptr_type) * TARGET_CHAR_BIT,
> > +                  NULL);
> > +  TYPE_TARGET_TYPE (wchar_ptr_type) = wchar_type;
>
> Could you use arch_pointer_type instead of setting the TYPE_TARGET_TYPE by hand?

I will try that.

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

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

>
>
> > +                      TYPE_LENGTH (wchar_ptr_type));
> > +
> > +  /* struct _RTL_USER_PROCESS_PARAMETERS */
> > +  rupp_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
> > +  TYPE_NAME (rupp_type) = xstrdup ("rtl_user_process_parameters");
>
>
> Couldn't you pass the name directly to arch_composite_type?

I will try that as well.


Regards
Hannes Domani
  

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