From: "Ijaz, Abdul B" <abdul.b.ijaz@intel.com>
To: Andrew Burgess <andrew.burgess@embecosm.com>
Cc: abdul.b.ijaz <abijaz@ecsmtp.iul.intel.com>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: RE: [PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN
Date: Fri, 27 Aug 2021 09:11:49 +0000 [thread overview]
Message-ID: <CO1PR11MB47886A595CEA0A9B43D3F10FCBC89@CO1PR11MB4788.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20210825083650.GF2581@embecosm.com>
Thanks Andrew for the feedback.
> You should definitely mention this in the commit message I think, ideally, just mention which test showed the regression and why (e.g. printing a Fortran variable while the language is forced to C).
Sure Will do.
> I noticed you dropped `resolve_p` from the end of the argument list here. Is this intentional?
Thanks for pointing it out. No it was not intentional, I think change introduce while fixing conflicts. Will revert it.
> I think we should do _something_ with the stride, given how closely related arrays and strings seem to be, maybe just this would do:
> prop = type->dyn_prop (DYN_PROP_BYTE_STRIDE);
> if (prop != nullptr && type->code () == TYPE_CODE_STRING)
> {
> prop = nullptr;
> warning (_("byte stride property ignored on string type"));
> }
Will add it and will hopefully try to test and then push the changes for review today.
Thanks & Best Regards,
Abdul Basit
-----Original Message-----
From: Andrew Burgess <andrew.burgess@embecosm.com>
Sent: Wednesday, August 25, 2021 10:37 AM
To: Ijaz, Abdul B <abdul.b.ijaz@intel.com>
Cc: abdul.b.ijaz <abijaz@ecsmtp.iul.intel.com>; gdb-patches@sourceware.org
Subject: Re: [PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN
* Ijaz, Abdul B <abdul.b.ijaz@intel.com> [2021-08-23 08:47:46 +0000]:
> > GDB is not able to print arrays of FORTRAN variable length strings
> > when dwarf info contains DW_AT_string_length only regarding the
> > string length information. So handing of dynamic array is updated
> > to handle such cases.
>
> Thanks for working on this. I have some feedback, see below...
> >> Thanks Andrew for the feedback. I added my response below. Can you please review if it looks fine to you so I will update it accordingly.
>
> >
> > Suppose we have
> >
> > subroutine vla_array (arr1, arr2)
> > character (len=*):: arr1 (:)
> > character (len=5):: arr2 (:)
> >
> > print *, arr1 ! break-here
> > print *, arr2
> > end subroutine vla_array
> >
> > The "print arr1" and "print arr2" command at the "break-here" line
> > gives the following output:
> >
> > (gdb) print arr1
> > $1 = (<error reading variable>
> > (gdb) print arr2
> > $2 = ('abcde', 'abcde', 'abcde')
> > (gdb) ptype arr1
> > type = character*(*) (5)
> > (gdb) ptype arr2
> > type = character*5 (3)
> >
> > So GDB is able to print the array of string with static length but
> > for variable length it fail to do so. For this case now improve
> > handling of the TYPE_CODE_STRING code in dynamic array length
> > resolving function to set the static length of strings as well when
> > only DW_AT_string_length is given in the dwarf info.
> >
> > Dwarf info using Intel® Fortran Compiler for such case contains following:
> > <1><fd>: Abbrev Number: 12 (DW_TAG_string_type)
> > <fe> DW_AT_name : (indirect string, offset: 0xd2): .str.ARR1
> > <102> DW_AT_string_length: 3 byte block: 97 23 8 (DW_OP_push_object_address; DW_OP_plus_uconst: 8)
>
> I don't think the extra indentation before "DW_AT_string_length" is correct here, this attribute is a child of the DW_TAG_string_type, right? Not a child of the DW_AT_name.
>
> >> This is output of readelf also there is no extra indentation before
> >> "DW_AT_string_length". DW_AT_name has 2 digits in front <fe> while
> >> DW_AT_string_lengt has 3 "<102>" so this is 1 more to right. Yes
> >> "DW_AT_string_length" is a child of the DW_TAG_string_type.
Ah, OK that makes sense. No need to change anything then.
>
>
> >
> > After fixing it and compiling using Intel® Fortran Compiler now gdb
> > shows
> > following:
> >
> > (gdb) p arr1
> > $1 = ('abddefghij', 'abddefghij', 'abddefghij', 'abddefghij',
> > 'abddefghij')
> > (gdb) p arr2
> > $2 = ('abcde', 'abcde', 'abcde')
> > (gdb) ptype arr1
> > type = character*10 (5)
> > (gdb) ptype arr2
> > type = character*5 (3)
> >
> > gdb/ChangeLog:
> > 2021-08-20 Abdul Basit Ijaz <abdul.b.ijaz@intel.com>
> >
> > * gdbtypes.c (resolve_dynamic_array_or_string): Improve handling
> > of TYPE_CODE_STRING code to use return value of create_string_type
> > outcome for this case.
> > * c-valprint.c (c_value_print_inner): Handle String type code
> > in the same way as the Array type code.
> >
> > gdb/testsuite/ChangeLog:
> > 2021-08-20 Abdul Basit Ijaz <abdul.b.ijaz@intel.com>
> >
> > * gdb.fortran/vla-array.f90: New fie.
> > * gdb.fortran/vla-array.exp: New fie.
> >
> > 2021-08-20 Abdul Basit Ijaz <abdul.b.ijaz@intel.com>
> > ---
> > gdb/c-valprint.c | 1 +
> > gdb/gdbtypes.c | 12 ++++--
> > gdb/testsuite/gdb.fortran/vla-array.exp | 57
> > +++++++++++++++++++++++++
> > gdb/testsuite/gdb.fortran/vla-array.f90 | 44 +++++++++++++++++++
> > 4 files changed, 110 insertions(+), 4 deletions(-) create mode
> > 100644 gdb/testsuite/gdb.fortran/vla-array.exp
> > create mode 100644 gdb/testsuite/gdb.fortran/vla-array.f90
> >
> > diff --git a/gdb/c-valprint.c b/gdb/c-valprint.c index
> > 9c82869525f..e5a8e122676 100644
> > --- a/gdb/c-valprint.c
> > +++ b/gdb/c-valprint.c
> > @@ -426,6 +426,7 @@ c_value_print_inner (struct value *val, struct ui_file *stream, int recurse,
> > switch (type->code ())
> > {
> > case TYPE_CODE_ARRAY:
> > + case TYPE_CODE_STRING:
> > c_value_print_array (val, stream, recurse, options);
> > break;
>
> I don't understand what part this change plays in this patch. I can see below how you're now creating values with TYPE_CODE_STRING instead of TYPE_CODE_ARRAY, but then I'd expect these to be covered by the existing handling of TYPE_CODE_STRING in f_language::value_print_inner.
>
> Of course, if you forced the language to C while inside the Fortran frame and tried to print the string value then I guess maybe you'd hit this case, but I don't think your test does this, and if that is the case you're covering here it might be worth splitting this into a separate commit to make the split crystal clear.
>
> >> Actually this part was only modified since after fixig the issue
> >> test "gdb.fortran/mixed-lang-stack.exp" has shown regression for
> >> exactly the same scenario you mentioned "forced the language to C
> >> while inside the Fortran frame" so that is why it was updated in
> >> the same patch and this change is cover by this test
> >> "gdb.fortran/mixed-lang-stack.exp" already. So please let me know
> >> shall we move it to separate patch.
You should definitely mention this in the commit message I think, ideally, just mention which test showed the regression and why (e.g. printing a Fortran variable while the language is forced to C).
>
>
>
> >
> > diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c index
> > 74ad5d6f7fe..1444c5feb6c 100644
> > --- a/gdb/gdbtypes.c
> > +++ b/gdb/gdbtypes.c
> > @@ -2312,8 +2312,9 @@ resolve_dynamic_array_or_string (struct type *type,
> > range_type = resolve_dynamic_range (range_type, addr_stack,
> > resolve_p);
> >
> > ary_dim = check_typedef (TYPE_TARGET_TYPE (type));
> > - if (ary_dim != NULL && ary_dim->code () == TYPE_CODE_ARRAY)
> > - elt_type = resolve_dynamic_array_or_string (ary_dim, addr_stack, resolve_p);
> > + if (ary_dim != NULL && (ary_dim->code () == TYPE_CODE_ARRAY
> > + || ary_dim->code () == TYPE_CODE_STRING))
>
> >> Will update the indentation like you pointed out in second email.
>
>
> > + elt_type = resolve_dynamic_array_or_string (ary_dim,
> > + addr_stack);
I noticed you dropped `resolve_p` from the end of the argument list here. Is this intentional?
> > else
> > elt_type = TYPE_TARGET_TYPE (type);
> >
> > @@ -2337,8 +2338,11 @@ resolve_dynamic_array_or_string (struct type *type,
> > else
> > bit_stride = TYPE_FIELD_BITSIZE (type, 0);
> >
> > - return create_array_type_with_stride (type, elt_type, range_type, NULL,
> > - bit_stride);
> > + if (type->code () == TYPE_CODE_STRING)
> > + return create_string_type (type, elt_type, range_type);
>
> I wonder if we should be doing anything with the bit_stride here? I'm not sure if it makes sense for the bit_stride to be anything other than zero, but at the very least it feels like we should throw an error if the stride is not zero ....
>
> >> Regarding bit_stride it is not needed for string type. So shall we
> >> can put a check to try reading it only for array type only since it
> >> will not be needed for string type instead of throwing error.
I think we should do _something_ with the stride, given how closely related arrays and strings seem to be, maybe just this would do:
prop = type->dyn_prop (DYN_PROP_BYTE_STRIDE);
if (prop != nullptr && type->code () == TYPE_CODE_STRING)
{
prop = nullptr;
warning (_("byte stride property ignored on string type"));
}
if this ever does crop up we can figure out what's going on then.
Thanks,
Andrew
>
>
> > + else
> > + return create_array_type_with_stride (type, elt_type, range_type, NULL,
> > + bit_stride);
> > }
> >
> > /* Resolve dynamic bounds of members of the union TYPE to static
> > diff --git a/gdb/testsuite/gdb.fortran/vla-array.exp
> > b/gdb/testsuite/gdb.fortran/vla-array.exp
> > new file mode 100644
> > index 00000000000..a9223576bbd
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.fortran/vla-array.exp
> > @@ -0,0 +1,57 @@
> > +# Copyright 2021 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or
> > +modify # it under the terms of the GNU General Public License as
> > +published by # the Free Software Foundation; either version 3 of
> > +the License, or # (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful, #
> > +but WITHOUT ANY WARRANTY; without even the implied warranty of #
> > +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the # GNU
> > +General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> > +
> > +standard_testfile ".f90"
> > +load_lib "fortran.exp"
> > +
> > +if {[skip_fortran_tests]} { return -1 }
> > +
> > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> > + {debug f90 quiet}] } {
> > + return -1
> > +}
> > +
> > +if ![fortran_runto_main] {
> > + untested "could not run to main"
> > + return -1
> > +}
> > +
> > +# Try to access vla string / vla string array / string array values
>
> Full stop at the end of the comment.
> >> Will do
>
> Thanks & Best Regards,
> Abdul Basit
>
>
> > +gdb_breakpoint [gdb_get_line_number "arr_vla1-print"]
> > +gdb_continue_to_breakpoint "arr_vla1-print"
> > +
> > +# GFortran does not emit DW_TAG_string_type for array of variable
> > +length # string.
> > +if [test_compiler_info "gcc*"] { setup_xfail *-*-* gcc/101826 }
> > +gdb_test "print arr_vla1" \
> > + " = \\\('vlaaryvlaary', 'vlaaryvlaary', 'vlaaryvlaary', 'vlaaryvlaary', 'vlaaryvlaary'\\\)" \
> > + "print vla string array"
> > +
> > +if [test_compiler_info "gcc*"] { setup_xfail *-*-* gcc/101826 }
> > +gdb_test "ptype arr_vla1" \
> > + "type = character\\*12 \\(5\\)" \
> > + "print variable length string array type"
> > +gdb_test "print arr_vla2" \
> > + " = 'vlaary'" \
> > + "print variable length string"
> > +gdb_test "ptype arr_vla2" \
> > + "type = character\\*6" \
> > + "print variable length string type"
> > +gdb_test "print arr2" \
> > + " = \\\('vlaaryvla', 'vlaaryvla', 'vlaaryvla'\\\)" \
> > + "print string array"
> > +gdb_test "ptype arr2" \
> > + "type = character\\*9 \\(3\\)" \
> > + "print string array type"
> > diff --git a/gdb/testsuite/gdb.fortran/vla-array.f90
> > b/gdb/testsuite/gdb.fortran/vla-array.f90
> > new file mode 100644
> > index 00000000000..612e84fe213
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.fortran/vla-array.f90
> > @@ -0,0 +1,44 @@
> > +! Copyright 2021 Free Software Foundation, Inc.
> > +!
> > +! This program is free software; you can redistribute it and/or
> > +modify ! it under the terms of the GNU General Public License as
> > +published by ! the Free Software Foundation; either version 3 of
> > +the License, or ! (at your option) any later version.
> > +!
> > +! This program is distributed in the hope that it will be useful, !
> > +but WITHOUT ANY WARRANTY; without even the implied warranty of !
> > +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the ! GNU
> > +General Public License for more details.
> > +!
> > +! You should have received a copy of the GNU General Public License !
> > +along with this program. If not, see <http://www.gnu.org/licenses/>.
> > +
> > +subroutine vla_array_func (arr_vla1, arr_vla2, arr2)
> > + character (len=*):: arr_vla1 (:)
> > + character (len=*):: arr_vla2
> > + character (len=9):: arr2 (:)
> > +
> > + print *, arr_vla1 ! arr_vla1-print
> > + print *, arr_vla2
> > + print *, arr2
> > +end subroutine vla_array_func
> > +
> > +program vla_array_main
> > +interface
> > + subroutine vla_array_func (arr_vla1, arr_vla2, arr2)
> > + character (len=*):: arr_vla1 (:)
> > + character (len=*):: arr_vla2
> > + character (len=9):: arr2 (:)
> > + end subroutine vla_array_func
> > +end interface
> > + character (len=9) :: arr1 (3)
> > + character (len=6) :: arr2
> > + character (len=12) :: arr3 (5)
> > +
> > + arr1 = 'vlaaryvla'
> > + arr2 = 'vlaary'
> > + arr3 = 'vlaaryvlaary'
> > +
> > + call vla_array_func (arr3, arr2, arr1)
> > +
> > +end program vla_array_main
> > --
> > 2.31.1
> >
>
> 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
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
next prev parent reply other threads:[~2021-08-27 9:11 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-20 11:06 [PATCH 0/1] Fix variable length strings array " abdul.b.ijaz
2021-08-20 11:06 ` [PATCH 1/1] gdb: Fix arrays of variable length strings " abdul.b.ijaz
2021-08-20 15:52 ` Andrew Burgess
2021-08-23 8:47 ` Ijaz, Abdul B
2021-08-25 8:36 ` Andrew Burgess
2021-08-27 9:11 ` Ijaz, Abdul B [this message]
2021-08-23 20:54 ` Tom Tromey
2021-08-25 8:56 ` Andrew Burgess
2021-08-27 17:02 ` Tom Tromey
2021-12-29 11:31 ` Ijaz, Abdul B
2022-04-11 14:51 ` Ijaz, Abdul B
2022-04-15 16:33 ` Tom Tromey
2021-08-20 17:02 ` Andrew Burgess
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CO1PR11MB47886A595CEA0A9B43D3F10FCBC89@CO1PR11MB4788.namprd11.prod.outlook.com \
--to=abdul.b.ijaz@intel.com \
--cc=abijaz@ecsmtp.iul.intel.com \
--cc=andrew.burgess@embecosm.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).