[PATCH 0/1] Fix variable length strings array for FORTRAN

[PATCH 0/1] Fix variable length strings array for FORTRAN

[abdul.b.ijaz]

Hi All,

GDB is not able to print FORTRAN variable length string array value
when dwarf info contains DW_AT_string_length only regarding the string
length information. So patch update handing of dynamic array for such
cases.

GDB Testsuite is executed using the patch and there is no regression
seen as compare to master on Ubuntu 18.04 machine.

Best Regards,
Abdul Basit

Abdul Basit Ijaz (1):
  gdb: Fix arrays of variable length strings for FORTRAN

 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

-- 
2.31.1

[PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN

[abdul.b.ijaz]

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.

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)

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;
 
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))
+    elt_type = resolve_dynamic_array_or_string (ary_dim, addr_stack);
   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);
+  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
+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

Re: [PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN

[Andrew Burgess]

* abdul.b.ijaz via Gdb-patches <gdb-patches@sourceware.org> [2021-08-20 13:06:38 +0200]:

> 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...

> 
> 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.

> 
> 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.

>  
> 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))
> +    elt_type = resolve_dynamic_array_or_string (ary_dim, addr_stack);
>    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 ....




> +  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.

Thanks,
Andrew


> +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
> 

RE: [PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN

[Ijaz, Abdul B]

> 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.


> 
> 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.



>  
> 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);
>    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. 


> +  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

Re: [PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN

[Andrew Burgess]

* 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

RE: [PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN

[Ijaz, Abdul B]

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

Re: [PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

>> case TYPE_CODE_ARRAY:
>> +    case TYPE_CODE_STRING:
>> c_value_print_array (val, stream, recurse, options);
>> break;

Andrew> I don't understand what part this change plays in this patch.  I can
Andrew> see below how you're now creating values with TYPE_CODE_STRING instead
Andrew> of TYPE_CODE_ARRAY, but then I'd expect these to be covered by the
Andrew> existing handling of TYPE_CODE_STRING in
Andrew> f_language::value_print_inner.

I wonder if this should go in generic_value_print instead.

Tom

Re: [PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2021-08-23 14:54:53 -0600]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> >> case TYPE_CODE_ARRAY:
> >> +    case TYPE_CODE_STRING:
> >> c_value_print_array (val, stream, recurse, options);
> >> break;
> 
> Andrew> I don't understand what part this change plays in this patch.  I can
> Andrew> see below how you're now creating values with TYPE_CODE_STRING instead
> Andrew> of TYPE_CODE_ARRAY, but then I'd expect these to be covered by the
> Andrew> existing handling of TYPE_CODE_STRING in
> Andrew> f_language::value_print_inner.
> 
> I wonder if this should go in generic_value_print instead.

generic_val_print_array doesn't print arrays of character type things
as a string, so I think having this change in c_value_print_inner
makes sense.

I did wonder if we should _also_ change generic_val_print though, as
this would catch any language that wasn't Fortran, C, or C++ that
called into generic_val_print and didn't already handle
TYPE_CODE_STRING.

However, that's only Modula2 and Pascal, both of which already handle
TYPE_CODE_ARRAY and do something similar to C's print character types
as a string, which leads me to think that maybe both of these
languages should be handling TYPE_CODE_STRING as an alias for
TYPE_CODE_ARRAY.

Given that language_defn::value_print_inner actually calls
c_value_print_inner, rather than generic_val_print, I do wonder if
some/all of c_value_print_inner could be moved into generic_val_print,
though that's clearly out of scope of this patch.

I'm also tempted to say that the Modula2 and Pascal changes are
optional, as that seems like an edge case (user debugging Fortran code
with the language forced to one of those two choices).

Thanks,
Andrew

Re: [PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN

[Tom Tromey]

>> >> case TYPE_CODE_ARRAY:
>> >> +    case TYPE_CODE_STRING:
>> >> c_value_print_array (val, stream, recurse, options);
>> >> break;
>> 
Andrew> I don't understand what part this change plays in this patch.  I can
Andrew> see below how you're now creating values with TYPE_CODE_STRING instead
Andrew> of TYPE_CODE_ARRAY, but then I'd expect these to be covered by the
Andrew> existing handling of TYPE_CODE_STRING in
Andrew> f_language::value_print_inner.

>> I wonder if this should go in generic_value_print instead.

Andrew> generic_val_print_array doesn't print arrays of character type things
Andrew> as a string, so I think having this change in c_value_print_inner
Andrew> makes sense.

In this case, though, the patch is adding new treatment for
TYPE_CODE_STRING.  It seems to me that it would make sense to handle
this in the generic code.

Andrew> I did wonder if we should _also_ change generic_val_print though, as
Andrew> this would catch any language that wasn't Fortran, C, or C++ that
Andrew> called into generic_val_print and didn't already handle
Andrew> TYPE_CODE_STRING.

Yeah.

Andrew> However, that's only Modula2 and Pascal, both of which already handle
Andrew> TYPE_CODE_ARRAY and do something similar to C's print character types
Andrew> as a string, which leads me to think that maybe both of these
Andrew> languages should be handling TYPE_CODE_STRING as an alias for
Andrew> TYPE_CODE_ARRAY.

That would be fine as well.  It's still often useful for the generic
code to handle a case, because users can language-switch and wind up
printing an "unexpected" type via the "wrong" language, like:

(gdb) print value_of_type_code_string
(gdb) set lang c
(gdb) print $

Andrew> I'm also tempted to say that the Modula2 and Pascal changes are
Andrew> optional, as that seems like an edge case (user debugging Fortran code
Andrew> with the language forced to one of those two choices).

This should do something sensible when possible.  It doesn't always --
sometimes the languages have special code to decode things.  But, that
seems like a not very good way to do it to me; better instead to unify
the type system and then the fallback code can do something useful, even
if not fully correct for the current language (which isn't always
possible).

One question for me is, if TYPE_CODE_STRING is a string, can we avoid
language-specific code for it at all?  And just always handle it in the
generic code.

Tom

RE: [PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN

[Ijaz, Abdul B]

Hi Tom and Andrew,

I do not feel so comfortable to modify generic code and my break things because of not having full overview. Will it be fine to merge this change if it is fine and not breaking anything or how shall we proceed on this. Thanks

Best Regards
Abdul Basit 

-----Original Message-----
From: Tom Tromey <tom@tromey.com> 
Sent: Friday, August 27, 2021 7:03 PM
To: Andrew Burgess <andrew.burgess@embecosm.com>
Cc: Tom Tromey <tom@tromey.com>; Ijaz, Abdul B <abdul.b.ijaz@intel.com>; 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

>> >> case TYPE_CODE_ARRAY:
>> >> +    case TYPE_CODE_STRING:
>> >> c_value_print_array (val, stream, recurse, options); break;
>> 
Andrew> I don't understand what part this change plays in this patch.  I 
Andrew> can see below how you're now creating values with 
Andrew> TYPE_CODE_STRING instead of TYPE_CODE_ARRAY, but then I'd expect 
Andrew> these to be covered by the existing handling of TYPE_CODE_STRING 
Andrew> in f_language::value_print_inner.

>> I wonder if this should go in generic_value_print instead.

Andrew> generic_val_print_array doesn't print arrays of character type 
Andrew> things as a string, so I think having this change in 
Andrew> c_value_print_inner makes sense.

In this case, though, the patch is adding new treatment for TYPE_CODE_STRING.  It seems to me that it would make sense to handle this in the generic code.

Andrew> I did wonder if we should _also_ change generic_val_print 
Andrew> though, as this would catch any language that wasn't Fortran, C, 
Andrew> or C++ that called into generic_val_print and didn't already 
Andrew> handle TYPE_CODE_STRING.

Yeah.

Andrew> However, that's only Modula2 and Pascal, both of which already 
Andrew> handle TYPE_CODE_ARRAY and do something similar to C's print 
Andrew> character types as a string, which leads me to think that maybe 
Andrew> both of these languages should be handling TYPE_CODE_STRING as 
Andrew> an alias for TYPE_CODE_ARRAY.

That would be fine as well.  It's still often useful for the generic code to handle a case, because users can language-switch and wind up printing an "unexpected" type via the "wrong" language, like:

(gdb) print value_of_type_code_string
(gdb) set lang c
(gdb) print $

Andrew> I'm also tempted to say that the Modula2 and Pascal changes are 
Andrew> optional, as that seems like an edge case (user debugging 
Andrew> Fortran code with the language forced to one of those two choices).

This should do something sensible when possible.  It doesn't always -- sometimes the languages have special code to decode things.  But, that seems like a not very good way to do it to me; better instead to unify the type system and then the fallback code can do something useful, even if not fully correct for the current language (which isn't always possible).

One question for me is, if TYPE_CODE_STRING is a string, can we avoid language-specific code for it at all?  And just always handle it in the generic code.

Tom
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

RE: [PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN

[Ijaz, Abdul B]

Hi Tom,

Reminder to the last query. If it is fine can we merge this patch since it is not breaking existing functionality.  Thanks

Best Regards
Abdul Basit

-----Original Message-----
From: Ijaz, Abdul B <abdul.b.ijaz@intel.com> 
Sent: Wednesday, December 29, 2021 12:31 PM
To: Tom Tromey <tom@tromey.com>; Andrew Burgess <andrew.burgess@embecosm.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

Hi Tom and Andrew,

I do not feel so comfortable to modify generic code and my break things because of not having full overview. Will it be fine to merge this change if it is fine and not breaking anything or how shall we proceed on this. Thanks

Best Regards
Abdul Basit 

-----Original Message-----
From: Tom Tromey <tom@tromey.com>
Sent: Friday, August 27, 2021 7:03 PM
To: Andrew Burgess <andrew.burgess@embecosm.com>
Cc: Tom Tromey <tom@tromey.com>; Ijaz, Abdul B <abdul.b.ijaz@intel.com>; 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

>> >> case TYPE_CODE_ARRAY:
>> >> +    case TYPE_CODE_STRING:
>> >> c_value_print_array (val, stream, recurse, options); break;
>> 
Andrew> I don't understand what part this change plays in this patch.  I 
Andrew> can see below how you're now creating values with 
Andrew> TYPE_CODE_STRING instead of TYPE_CODE_ARRAY, but then I'd expect 
Andrew> these to be covered by the existing handling of TYPE_CODE_STRING 
Andrew> in f_language::value_print_inner.

>> I wonder if this should go in generic_value_print instead.

Andrew> generic_val_print_array doesn't print arrays of character type 
Andrew> things as a string, so I think having this change in 
Andrew> c_value_print_inner makes sense.

In this case, though, the patch is adding new treatment for TYPE_CODE_STRING.  It seems to me that it would make sense to handle this in the generic code.

Andrew> I did wonder if we should _also_ change generic_val_print 
Andrew> though, as this would catch any language that wasn't Fortran, C, 
Andrew> or C++ that called into generic_val_print and didn't already 
Andrew> handle TYPE_CODE_STRING.

Yeah.

Andrew> However, that's only Modula2 and Pascal, both of which already 
Andrew> handle TYPE_CODE_ARRAY and do something similar to C's print 
Andrew> character types as a string, which leads me to think that maybe 
Andrew> both of these languages should be handling TYPE_CODE_STRING as 
Andrew> an alias for TYPE_CODE_ARRAY.

That would be fine as well.  It's still often useful for the generic code to handle a case, because users can language-switch and wind up printing an "unexpected" type via the "wrong" language, like:

(gdb) print value_of_type_code_string
(gdb) set lang c
(gdb) print $

Andrew> I'm also tempted to say that the Modula2 and Pascal changes are 
Andrew> optional, as that seems like an edge case (user debugging 
Andrew> Fortran code with the language forced to one of those two choices).

This should do something sensible when possible.  It doesn't always -- sometimes the languages have special code to decode things.  But, that seems like a not very good way to do it to me; better instead to unify the type system and then the fallback code can do something useful, even if not fully correct for the current language (which isn't always possible).

One question for me is, if TYPE_CODE_STRING is a string, can we avoid language-specific code for it at all?  And just always handle it in the generic code.

Tom
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

Re: [PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN

[Tom Tromey]

> I do not feel so comfortable to modify generic code and my break
> things because of not having full overview. Will it be fine to merge
> this change if it is fine and not breaking anything or how shall we
> proceed on this. Thanks

I think it's best to address Andrew's review.
IIRC my comments can probably be deferred.

thanks,
Tom

Re: [PATCH 1/1] gdb: Fix arrays of variable length strings for FORTRAN

[Andrew Burgess]

* abdul.b.ijaz via Gdb-patches <gdb-patches@sourceware.org> [2021-08-20 13:06:38 +0200]:

> 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.
> 
> 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)
> 
> 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;
>  
> 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))

Sorry, I forgot to say, the indentation is not correct here.  The '||'
should align like:

  if (ary_dim != NULL && (ary_dim->code () == TYPE_CODE_ARRAY
                          || ary_dim->code () == TYPE_CODE_STRING))

Thanks,
Andrew

> +    elt_type = resolve_dynamic_array_or_string (ary_dim, addr_stack);
>    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);
> +  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
> +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
>