RE: [PATCH 2/2] gdb: Resolve dynamic target types of pointers.

["Kempke, Nils-Christian" <nils-christian.kempke@intel.com>]

Hi Tom,
Thanks for the feedback. Sorry that it took me so long to reply - the mail
somehow got lost..

> -----Original Message-----
> From: Tom Tromey <tom@tromey.com>
> Sent: Thursday, February 10, 2022 8:47 PM
> To: Nils-Christian Kempke via Gdb-patches <gdb-patches@sourceware.org>
> Cc: Kempke, Nils-Christian <nils-christian.kempke@intel.com>
> Subject: Re: [PATCH 2/2] gdb: Resolve dynamic target types of pointers.
> 
> >>>>> Nils-Christian Kempke via Gdb-patches <gdb-
> patches@sourceware.org> writes:
> 
> Hi.  Thanks for the patch.
> 
> > +	  if (type->code () == TYPE_CODE_PTR && is_dynamic_type (type))
> > +	    {
> > +	      CORE_ADDR addr;
> > +	      if (nullptr != TYPE_DATA_LOCATION (TYPE_TARGET_TYPE (type)))
> > +		addr = value_address (val);
> > +	      else
> > +		addr = value_as_address (val);
> 
> This seems weird to me.  When does value_as_address fail?
> It seems to me that a value whose type is TYPE_CODE_PTR should be more
> easily convertible to a CORE_ADDR without examining
> TYPE_DATA_LOCATION.
> 
> The same thing applies in a couple more spots in the patch.
> 

This if else has been introduced because of the way ifort/icc describe pointers
vs. gcc/gfortran and icx/ifx.  For the simplified DWARF below I'll try and omit
non-useful entries.  As an example I'll take a pointer to an array (which is what
we want in the situation of dynamic target types I guess).  Different from 
gfortran and ifx the DWARF entry for a pointer in ifort is a

<1><AAA> DW_TAG_variable
    <> DW_AT_type: <BBB>
    <> DW_AT_location: (location of the pointer)
<1><BBB> DW_TAG_pointer_type
    DW_AT_type: <CCC>
<1><CCC> DW_TAG_array_type
    <> DW_AT_type: <DDD>
    <> DW_AT_data_location: (location of the array)
<2><EEE> DW_TAG_subrange_type

So, in ifort the DW_AT_location of the DW_TAG_variable is actually the
location of the pointer variable and not the location of the array.
Dereferencing this location then actually yields the location of the array. And
this location is then used in the DW_AT_data_location expression to
retrieve the actual array.
When printing such a pointer in gdb one will end up in the c_value_print
function in c-valprint.c. Taking the value_as_address now fails - since
it is not the address of the target_type but the address of the address of the
target_type. The same thing holds for dereferencing such a DWARF
construct in value_ind in valops.c. 
To resolve this, an additional check is added for the target_type's
data_location. If the target type of a pointer does have a data_location, then
that one is being used as location of the underlying type.
For gfortran and ifx the same pointer to array in Fortran is described as

Fortran:

<1><AAA> DW_TAG_variable
    <>   DW_AT_type: <BBB>
    <>   DW_AT_location: (location of the pointer) 
<1><BBB> DW_TAG_array_type
    <> DW_AT_type: <CCC>
    <> DW_AT_data_location: (location of the array)
<2><DDD> DW_TAG_subrange_type

When printing this pointer to array in gfortran/ifx the pointer print function is
not actually called - since the DWARF does not emit a pointer here.  Instead,
the normal array print is invoked which will use the DW_AT_data_location.
So we do not run into this same problem

> I'm curious why the code added here is not also needed in f-valprint.c.

The reason here is simply that Fortran does not have this implemented for
Itself.  Instead the fortran f_langugage inherits from language_defn (the all
languages base class) which simply delegates the value-print to 
c_value_print / so changing it there suffices.

> 
> > --- 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 {
> > +  # gfortran
> > +  -re "= int \\(\\*\\)\\\[variable length\\\]\r\n$gdb_prompt $" {
> > +    pass $test_name
> > +  }
> > +  # ifort/ifx
> 
> This references fortran compilers but is a c++ test.
> 

Thanks, good spot - corrected in v2.

> > +gdb_test "ptype ptr" "int \\(\\*\\)\\\[3\\\]"
> > +gdb_test "print ptr" "\\(int \\(\\*\\)\\\[3\\\]\\) $hex"
> > +gdb_test "print *ptr" " = \\{5, 7, 9\\}"
> 
> Do these pass with all compilers or do they also need to be conditional?
> 

These pass with all combinations I tried. The test has another failure for
ifort but this one is not related to the actual ptype but some issue in the
DWARF from ifort (print vlaref and print vlaref2 fail).

But actually I noticed an error on my side - in fact (and as pointed out above)
ifx and gfortran use similar DWARF to describe the above compiled test
while ifort differs.  So, the two cases for the conditional tests would be
gfortran/ifx and ifort. I'll correct this in my second version.

Maybe a word about what I tested:
The compiler stacks I tested were gcc + gfortran, icc + icpc + ifort and
icx + icpx + ifx (here a non-released version with some more bugfixes).

My main focus laid on the GNU stack and here all test are passing. For any
testing with ifx I had to also apply a not-yet-upstreamed patch since ifx
uses a slightly different format for its emitted dwarf type name (so, e.g.
the $int for ifx is different from ifort and gfortran). I intend to upstream
this one soon to actually improve ifx pass rates.

All modified test are passing for the GNU and any of the Intel stacks. For
GNU I could not detect new issues in the rest of the testsuite - for the
Intel stacks I only really tested the directly affected gdb.fortran and
gdb.cp parts of the testsuite.

I just rebased this patch to master but there were (nearly) no conflicts so I'd
want to resolve this discussion before sending a v2.

Thanks,
Nils
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928