Re: [PATCH] Support for Fortran's ASSUMED RANK

[Andrew Burgess <aburgess@redhat.com>]

* Potharla, Rupesh via Gdb-patches <gdb-patches@sourceware.org> [2022-01-28 07:49:54 +0000]:

> [AMD Official Use Only]
> 
> Hi Andrew, 
> 
> Request to review the attached patch file and please find comments inline below ...
> 
> 
> Regards,
> Rupesh P
> 
> > -----Original Message-----
> > From: Andrew Burgess <aburgess@redhat.com>
> > Sent: Saturday, January 22, 2022 12:37 AM
> > > Compiler Version:
> > > gcc (GCC) 12.0.0 20211122 (experimental)
> > 
> > I guess this must be something that was fixed recently, I tried with the 9.3
> > compiler I have locally and the tests failed.  I also tried with some random
> > build from git from early last year, and that also failed.
> > 
> > Ideally the tests wont just fail when using older tools, but instead the test will
> > detect that my compiler isn't good enough and just skip the tests, so you
> > might need to try with some older compilers.
> > 
> 
> The complete support for assumed rank is recently added in gcc. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103315 . Made changes in the testcase for skipping the testcase run for older compilers.  
> 
> > 
> > > From 383c561a1e926b7b65a1d8ffb5d70a047a4d4559 Mon Sep 17 00:00:00
> > 2001
> > > From: rupesh <rupesh.potharla@amd.com>
> > > Date: Fri, 29 Oct 2021 11:32:58 +0530
> > > Subject: [PATCH] Support for Fortran's ASSUMED RANK.
> > >
> > > gdb/ChangeLog:
> > >
> > >       * dwarf2/loc.c: (dwarf2_locexpr_baton_eval): Push array dimension
> > onto the stack.
> > >       * dwarf2/loc.h: Added an additional parameter to the function
> > dwarf2_evaluate_property.
> > >       * dwarf2/read.c: (scan_partial_symbols): Process
> > DW_TAG_generic_subrange.
> > >       * (add_partial_symbol): Process DW_TAG_generic_subrange.
> > >       * (process_die): Process DW_TAG_generic_subrange.
> > >       * (is_type_tag_for_partial) : Check for DW_TAG_generic_subrange type.
> > >       * (load_partial_dies): Load DW_TAG_generic_subrange.
> > >       * (new_symbol): Create entry for DW_TAG_generic_subrange type.
> > >       * (read_type_die_1): Read DW_TAG_generic_subrange type.
> > >       * (set_die_type) : Add dynamic property type for DW_AT_rank.
> > >       * f-typeprint.c: (f_type_print_varspec_suffix): Removed
> > TYPE_DATA_LOCATION.
> > >       * findvar.c: (follow_static_link): Passing new argument to the function
> > call dwarf2_evaluate_property.
> > >       * gdbtypes.c: (resolve_dynamic_range): Passing new argument to the
> > function call dwarf2_evaluate_property.
> > >       * (resolve_dynamic_array_or_string): Handle rank dynamic property by
> > creating and removing types.
> > >       * gdbtypes.h: (DYN_PROP_RANK, TYPE_DYN_PROP,  TYPE_RANK_PROP):
> > New Macros
> > >       * gnu-v3-abi.c: Passing new argument to the function call
> > dwarf2_evaluate_property.
> > >       * testsuite/gdb.fortran/assumedrank.exp: New Testcase
> > >       * testsuite/gdb.fortran/assumedrank.f90: New Testcase
> > 
> > ChangeLog entries are no longer needed for GDB, though you are welcome
> > to have content like this in the commit message if you like.  That said, the
> > preference in GDB is to have the commit described within the main body of
> > the commit message - a ChangeLog formatted block should be in addition.
> > 
> > You should still keep the lines under ~76ish characters though.
> 
> Added commit message describing the main body of the commit message. And also kept lines under ~76ish.  
> 
> > > diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c index
> > > 182f15e7077..60f831c2cff 100644
> > > --- a/gdb/dwarf2/loc.c
> > > +++ b/gdb/dwarf2/loc.c
> > > @@ -1547,7 +1547,8 @@ dwarf2_locexpr_baton_eval (const struct
> > dwarf2_locexpr_baton *dlbaton,
> > >                          const struct property_addr_info *addr_stack,
> > >                          CORE_ADDR *valp,
> > >                          bool push_initial_value,
> > > -                        bool *is_reference)
> > > +                        bool *is_reference,
> > > +                        int rank)
> > >  {
> > >    if (dlbaton == NULL || dlbaton->size == 0)
> > >      return 0;
> > > @@ -1559,6 +1560,10 @@ dwarf2_locexpr_baton_eval (const struct
> > dwarf2_locexpr_baton *dlbaton,
> > >    value *result;
> > >    scoped_value_mark free_values;
> > >
> > > +  /* push rank value to the stack */
> > > +  if (rank)
> > > +     ctx.push_address((rank - 1), false);
> > > +
> > 
> > I wonder, did you consider making use of the push_initial_value mechanism?
> >
>  I considered accommodating changes using push_initial_value
>  mechanism. The push_initial_value is pushing address to the stack
>  the requirement is to push scalar value onto the stack. So I could
>  not make use of this mechanism.

I still think we should investigate a solution that makes use of the
existing push initial value mechanism, even if we end up needing to
improve that first.

It feels (to me) that just adding more random arguments for things we
want to push is going to make for a clunky API.

Just my thoughts, maybe other maintainers will disagree.

I'll take a look at this and see if I can come up with something, but,
I don't know how much time I'll have to work on this over the next
week.

Thanks,
Andrew