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

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

Hi,

Sorry for this first message, something went wrong there..
Again.

First of all, thanks for the feedback!

> -----Original Message-----
> From: Gdb-patches <gdb-patches-bounces+nils-
> christian.kempke=intel.com@sourceware.org> On Behalf Of Kempke, Nils-
> Christian via Gdb-patches
> Sent: Tuesday, April 19, 2022 8:59 AM
> To: Tom Tromey <tom@tromey.com>; Kempke, Nils-Christian via Gdb-
> patches <gdb-patches@sourceware.org>
> Subject: RE: [PATCH 2/2] gdb: Resolve dynamic target types of pointers.
> 
> >
> > >> > +	  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?
> >
> > > This if else has been introduced because of the way ifort/icc describe
> > pointers
> > > vs. gcc/gfortran and icx/ifx.
> >
> > > <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
> >
> > > 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 still do not understand, sorry.
> >
> > For me this looks like two different types.  In one situation, the type
> > is a pointer to an array.  In the second, it's just an array.  So, I
> > would expect them to print differently.
> >
> > Are you saying these should print identically?  If so, that's fine --
> > but then it seems like something to do in the Fortran code, not in
> > c-valprint.c.
> 
> Yes, that is true. So, in Fortran pointers are just aliases for the underlying
> objects

First about the pointers in general:

They are emitted as two different types but represent the same object in
the code. The fact that ifort emits pointer-to-array here is, I think, less
ideal than a plain array type.
In Fortran a pointer is more like an alias to the underlying object.  It should be
thought of as a type with the pointer attribute, rather than a c pointer. It is
handled the exact same way the as object itself.  E.g. printing an associated
pointer-to-array would just print the array - as if one had printed that
instead. There is some hidden dereferencing going on when using them
(and as compared To C pointers) - so yes, imo ideally the print should be
the same.

But this would not be addressed with the sent patch.  The changes in the
patch only affect the resolution of the target dynamic type (thus the cases
in the tests).

I think the explanation I gave above was quite bad as well and in my initial
submission of this patch I am not sure whether I had 100% understood this
part as I do think of it now.  I will try again:

In GDB when we end up at the point where the dynamic type is to be resolved,
the address we will get from

   CORE_ADDR addr = value_as_address (val);

will, if the underlying type had a DW_AT_data_location, be the result of
the evaluation of that DW_AT_data_location.  This is fine for pure arrays,
but with pointer to arrays this is problematic.  The reason for this is, as described
in the DWARF5 5.18.1:

1  5.18.1 Data location
   ...
5  The DW_AT_data_location attribute may be used with any type that provides
6  one or more levels of hidden indirection and/or run-time parameters in its
7  representation. ...
   ...
10 This location description will typically begin with DW_OP_push_object_address which
11 loads the address of the object which can then serve as a descriptor in subsequent
12 calculation. For an example using DW_AT_data_location for a Fortran 90 array, see
13 Appendix D.2.1 on page 292.

(the example on 292 I think is quite helpful)

In the example on page 292, not only the DW_TAG_data_location but also
the DW_TAG_subrange_type require the evaluation of an expression containing
the DW_OP_push_object_address.  The object address that should be used for
these evaluations is the one of the original variable, in the example the variable
'arrayvar', which has its own DW_AT_location attribute.  It is especially not the
result of the array type's DW_AT_data_location. 

The example describes this as

Page 295:
   7 For b), for each dimension of the array (only one in this case), go interpret the
   8 usual lower bound attribute. Again this is an expression, which again begins
   9 with DW_OP_push_object_address. This object is still arrayvar, from step 1,
   10 because we have not begun to actually perform any indexing yet

where b) is evaluation of the lower bounds of the array. Here the 'still arrayvar'
tells to use the DW_AT_location.

Going back to ifort:

<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 using DW_OP_push_object_address)
  <> DW_AT_allocated: (using DW_OP_push_object_address) 
<2><EEE> DW_TAG_subrange_type
  <> DW_AT_TYPE: <FFF>
  <> DW_AT_lower_bound (using DW_OP_push_object_address)
  <> DW_AT_upper_bound (using DW_OP_push_object_address)

With such a location description we'd not be able to correctly evaluate the
DW_TAG_subrange_type as GDB would try to use the DW_AT_data_location
(gotten when evaluating value_as_address) instead of the DW_AT_location of
the original variable (which would be value_address (..)).  Thus, when we see
that the underlying type has a DW_AT_data_location we use the value_address
instead the value_as_address here.

I don't think this is a perfect solution really.. But I am not sure how to handle this
better atm.  I would be ok with removing this check from the patch for now, and
just always use value_as_address.. This part though for sure would need a way
better commit message I feel like.
Ifx, gfortran and flang do not emit these kind of types as far as I know (removing
the check for DW_AT_data_location will also only mess with the icc/ifort passrate
of the tests).  I guess, in order to properly resolve a target type of a pointer one
would have to got through all its dynamic attributes that are about to be resolved
and check, whether these need the address of the object or not.

> > Maybe in the code the check for TYPE_DATA_LOCATION is a proxy for
> > checking that the pointer is a pointer to a Fortran array?  I guess I
> > could understand that, but it doesn't seem like a good approach, because
> > what if some C compiler starts emitting this?

If a C compiler would start emitting this, I think we would also get problems
when trying to resolve the dynamic target type - as the location of the actual array
is not the location of the pointer. But that also assumes again that using
DW_AT_data_location for the target type will in general come with using
DW_ OP_push_object_address for the target type, too.

So, this checking is somehow a proxy, but for realizing that the resolving the
target type will need the object_address of the original pointer.
 
> > >> 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.
> >
> > If Fortran-specific printing is needed, this will have to change.

That is true. Dereferencing pointers before printing them would be such a thing,
but I think handling the pointer-to-array DWARF construct above should apply to
more than just the ifort Fortran pointers - any compiler could start emitting the same
construct and we'd get problems at the same spots when trying to resolve the types.
At least I don't see how DWARF spec would stand in the way of compilers emitting
this.
Currently I only know of ifort emitting this construct where the resolving of the
subrange upper and lower bounds actually requires the original pointer address.
I know that icx also would emit pointers to arrays in the same way ifort does, but
here, the subrange (afaik) does not depend on the object address and does not
use DW_OP_push_object_address.

So overall, this currently only happens with ifrot and in Fortran.

Sorry for the bad first explanation.

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