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

[Simon Marchi <simark at simark dot ca>]

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