Re: [PATCH 2/4] gdb/types: Resolve pointer types dynamically

[Simon Marchi <simark@simark.ca>]

On 2022-09-20 03:26, Nils-Christian Kempke via Gdb-patches wrote:
> From: Bernhard Heckel <bernhard.heckel@intel.com>
> 
> This commit allows pointers to be dynamic types (on the outmost
> level).  Similar to references, a pointer is considered a dynamic type
> if its target type is a dynamic type and it is on the outmost level.
> 
> When resolving a dynamic pointer in resolve_dynamic_pointer
> we a) try and resolve its target type (in a similar fashion as it is
> done for references) and b) resolve the dynamic DW_AT_associated
> property, which is emitted by ifx and ifort for pointers.
> 
> This generally makes the GDB output more verbose.  We are able to print
> more details about a pointer's target like the dimension of an array.
> 
> In Fortran, if we have a pointer to a dynamic type
> 
>   type buffer
>     real, dimension(:), pointer :: ptr
>   end type buffer
>   type(buffer), pointer :: buffer_ptr
>   allocate (buffer_ptr)
>   allocate (buffer_ptr%ptr (5))
> 
> which then gets allocated, we now resolve the dynamic type before
> printing the pointer's type:
> 
> Before:
> 
>   (gdb) ptype buffer_ptr
>   type = PTR TO -> ( Type buffer
>     real(kind=4) :: alpha(:)
>   End Type buffer )
> 
> After:
> 
>   (gdb) ptype buffer_ptr
>   type = PTR TO -> ( Type buffer
>     real(kind=4) :: alpha(5)
>   End Type buffer )
> 
> Similarly in C++ we can dynamically resolve e.g. pointers to arrays:
> 
>   int len = 3;
>   int arr[len];
>   int (*ptr)[len];
>   int ptr = &arr;
> 
> Once the pointer is assigned one gets:
> 
> Before:
> 
>   (gdb) p ptr
>   $1 = (int (*)[variable length]) 0x123456
>   (gdb) ptype ptr
>   type = int (*)[variable length]
> 
> After:
> 
>   (gdb) p ptr
>   $1 = (int (*)[3]) 0x123456
>   (gdb) ptype ptr
>   type = int (*)[3]
> 
> For more examples see the modified/added test cases.

Hi,

That all looks nice to me, that seems like some concrete improvements
for the user.  I noted some nits below.  I am not really familiar with
Fortran or the dynamic types thing, so I'd really like if someone more
familiar with those could take a look.

> ---
>  gdb/gdbtypes.c                                |  53 +++++-
>  gdb/testsuite/gdb.cp/vla-cxx.cc               |   4 +
>  gdb/testsuite/gdb.cp/vla-cxx.exp              |  33 ++++
>  gdb/testsuite/gdb.dwarf2/dynarr-ptr.exp       |  16 +-
>  .../gdb.fortran/pointer-to-pointer.exp        |   2 +-
>  gdb/testsuite/gdb.fortran/pointers.exp        | 178 ++++++++++++++++++
>  gdb/testsuite/gdb.fortran/pointers.f90        |  29 +++
>  gdb/valprint.c                                |   6 -
>  8 files changed, 303 insertions(+), 18 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.fortran/pointers.exp
> 
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index c458b204157..a4d79a64e95 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -2083,9 +2083,15 @@ is_dynamic_type_internal (struct type *type, int top_level)
>  {
>    type = check_typedef (type);
>  
> -  /* We only want to recognize references at the outermost level.  */
> -  if (top_level && type->code () == TYPE_CODE_REF)
> -    type = check_typedef (TYPE_TARGET_TYPE (type));
> +  /* We only want to recognize references and pointers at the outermost
> +     level.  */
> +  if (type->code () == TYPE_CODE_REF || type->code () == TYPE_CODE_PTR)
> +  {
> +    if (top_level != 0)
> +      type = check_typedef (TYPE_TARGET_TYPE (type));
> +    else
> +      return 0;
> +  }

Can you explain this change here, specifically the addition of the
"return 0"?

My understanding is that for REFs and PTRs, nothing below will match and
we will end up returning 0 anyway, so this is just a new shortcut for
when the type is a REF or PTR and top_level is false.  But I'd like to
confirm.

>  
>    /* Types that have a dynamic TYPE_DATA_LOCATION are considered
>       dynamic, even if the type itself is statically defined.
> @@ -2787,6 +2793,43 @@ resolve_dynamic_struct (struct type *type,
>    return resolved_type;
>  }
>  
> +/* Worker for pointer types.  */
> +
> +static struct type *
> +resolve_dynamic_pointer (struct type *type,
> +			 struct property_addr_info *addr_stack)
> +{
> +  struct dynamic_prop *prop;
> +  CORE_ADDR value;
> +
> +  /* Resolve the target type of this type.  */
> +  struct property_addr_info pinfo;
> +  pinfo.type = check_typedef (TYPE_TARGET_TYPE (type));
> +  pinfo.valaddr = {};
> +  if (addr_stack->valaddr.data () != NULL)

NULL -> nullptr

> +    pinfo.addr = extract_typed_address (addr_stack->valaddr.data (),
> +					type);

"type" would fit on the same line

> +  else
> +    pinfo.addr = read_memory_typed_address (addr_stack->addr, type);
> +  pinfo.next = addr_stack;
> +
> +  struct type* resolved_type = copy_type (type);

Space before *

> +
> +  /* Resolve associated property.  */
> +  prop = TYPE_ASSOCIATED_PROP (resolved_type);
> +  if (prop != nullptr
> +      && dwarf2_evaluate_property (prop, nullptr, addr_stack, &value))
> +    prop->set_const_val (value);
> +
> +  if (pinfo.addr != 0x0 && !type_not_associated (resolved_type))
> +    TYPE_TARGET_TYPE (resolved_type)
> +      = resolve_dynamic_type_internal (TYPE_TARGET_TYPE (type),
> +				       &pinfo, 0);
> +
> +

Remove one newline here.

> diff --git a/gdb/testsuite/gdb.cp/vla-cxx.exp b/gdb/testsuite/gdb.cp/vla-cxx.exp
> index 3494b5e8b77..e2bb8989212 100644
> --- a/gdb/testsuite/gdb.cp/vla-cxx.exp
> +++ b/gdb/testsuite/gdb.cp/vla-cxx.exp
> @@ -23,6 +23,36 @@ if ![runto_main] {
>      return -1
>  }
>  
> +gdb_breakpoint [gdb_get_line_number "Before pointer assignment"]
> +gdb_continue_to_breakpoint "Before pointer assignment"
> +
> +set test_name "ptype ptr, Before pointer assignment"
> +gdb_test_multiple "ptype ptr" $test_name {
> +  # gcc/icx
> +  -re -wrap "= int \\(\\*\\)\\\[variable length\\\]" {
> +    pass $test_name
> +  }
> +  # icc
> +  -re -wrap "= int \\(\\*\\)\\\[3\\\]" {
> +    pass $test_name
> +  }

No need to use the test_name variable nowadays, you can use the magic
$gdb_test_name inside the gdb_test_multiple body to access the name that
was passed to it.

Simon