[AMD Official Use Only] Hi Andrew, > 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. I had made code changes making use of push initial value mechanism. Request to review the attached patch file and let me know your suggestions? Regards, Rupesh P > -----Original Message----- > From: Andrew Burgess > Sent: Sunday, February 6, 2022 7:10 PM > To: Potharla, Rupesh > Cc: Achra, Nitika ; Kaushik, Sharang > ; Natarajan, Kavitha > ; George, Jini Susan > ; Kumar N, Bhuvanendra > ; E, Nagajyothi > ; Parasuraman, Hariharan > ; Sharma, Alok Kumar > ; Balasubrmanian, Vignesh > ; gdb-patches@sourceware.org > Subject: Re: [PATCH] Support for Fortran's ASSUMED RANK > > [CAUTION: External Email] > > * Potharla, Rupesh via Gdb-patches [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 > > > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.g > nu.org%2Fbugzilla%2Fshow_bug.cgi%3Fid%3D103315&data=04%7C01% > 7Crupesh.potharla%40amd.com%7C96b732e5f3144fce251008d9e9762835%7 > C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637797515982891186 > %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL > CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=l10%2Bju6B%2BghE > sLYgT6FDQVQJ6EAr6crdoHl%2FiFrGim8%3D&reserved=0 . Made > changes in the testcase for skipping the testcase run for older compilers. > > > > > > > > > From 383c561a1e926b7b65a1d8ffb5d70a047a4d4559 Mon Sep 17 > 00:00:00 > > > 2001 > > > > From: rupesh > > > > 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