Re: [PATCH] gdb: user variables with components of dynamic type

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Joel Brobecker <brobecker@adacore.com> [2020-11-15 18:24:58 +0400]:

> Hi Andrew,
> 
> > -  /* If type has a dynamic resolved location property
> > -     update it's value address.  */
> > +  /* If either the WHOLE value, or the COMPONENT value has a dynamic
> > +     resolved location property then update the address of the COMPONENT.
> > +
> > +     If the COMPONENT itself has a dynamic location, and was an
> > +     lval_internalvar_component, then we change this to lval_memory.
> > +     Usually a component of an internalvar is created non-lazy, and has its
> > +     content immediately copied from the parent internalvar.  However,
> > +     for components with a dynamic location, the content of the component
> > +     is not contained within the parent, but is instead accessed
> > +     indirectly.  Further, the component will be created as a lazy value.
> > +
> > +     By changing the type of the component to lval_memory we ensure that
> > +     value_fetch_lazy can successfully load the component.  */
> >    type = value_type (whole);
> >    if (NULL != TYPE_DATA_LOCATION (type)
> >        && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST)
> >      set_value_address (component, TYPE_DATA_LOCATION_ADDR (type));
> > +
> > +  type = value_type (component);
> > +  if (NULL != TYPE_DATA_LOCATION (type)
> > +      && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST)
> > +    {
> > +      if (VALUE_LVAL (component) == lval_internalvar_component)
> > +	{
> > +	  gdb_assert (value_lazy (component));
> > +	  VALUE_LVAL (component) = lval_memory;
> > +	}
> > +      else
> > +	gdb_assert (VALUE_LVAL (component) == lval_memory);
> > +      set_value_address (component, TYPE_DATA_LOCATION_ADDR (type));
> > +    }
> 
> I have a suggestion, but I am not sure it might be right for
> everyone, as perhaps other people's brains might be thinking
> differently.
> 
> In your patch you architected it with one large comment at beginning
> followed by a number of conditinal branches, with the comment explaining
> the various scenarios that we're about to handle. If your brain works
> like mine, I would find the following approach to make it easier for me
> to understand the code:
> 
>     type = value_type (component);
>     if (NULL != TYPE_DATA_LOCATION (type)
>         && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST)
>       {
>         /* COMPONENT's type has a dynamic location, so we need
>            to update our component's address to reflect the actual
>            location after resolution.  */
>         if (VALUE_LVAL (component) == lval_internalvar_component)
>           {
>             /* This happens when [...].
>                We have to change the lval to lval_memory because ... */
>             gdb_assert (value_lazy (component));
>             VALUE_LVAL (component) = lval_memory;
>           }
> 
>         /* At this point, we assume that COMPONENT is now an lval_memory,
>            and we can now set it address.  */
>         gdb_assert (VALUE_LVAL (component) == lval_memory);
>         set_value_address (component, TYPE_DATA_LOCATION_ADDR (type));
>       }
> 
> As I mentioned, maybe you don't think the read code the same way
> as I do, and so it would be absolutely fine with me if you don't
> agree with the suggestion ;-).

After rereading the our other discussion of this patch I believe the
conclusion was that you don't object to this patch.

As nobody else has commented I went ahead and pushed the version
below.

The only changes are:

  - Split the single big comment up as you suggested, and
  - Tweaked some wording in the commit log.

Thanks,
Andrew

---

commit 3c8c6de21daaae6450860af6b0df3b464ccb19f6
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Thu Oct 22 11:34:52 2020 +0100

    gdb: user variables with components of dynamic type
    
    Consider this Fortran type:
    
      type :: some_type
         integer, allocatable :: array_one (:,:)
         integer :: a_field
         integer, allocatable :: array_two (:,:)
      end type some_type
    
    And a variable declared:
    
      type(some_type) :: some_var
    
    Now within GDB we try this:
    
      (gdb) set $a = some_var
      (gdb) p $a
      $1 = ( array_one =
      ../../src/gdb/value.c:3968: internal-error: Unexpected lazy value type.
    
    Normally, when an internalvar ($a in this case) is created, it is
    non-lazy, the value is immediately copied out of the inferior into
    GDB's memory.
    
    When printing the internalvar ($a) GDB will extract each field in
    turn, so in this case `array_one`.  As the original internalvar is
    non-lazy then the extracted field will also be non-lazy, with its
    contents immediately copied from the parent internalvar.
    
    However, when the field has a dynamic type this is not the case, in
    value_primitive_field we see that any field with dynamic type is
    always created lazy.  Further, the content of this field will usually
    not have been captured in the contents buffer of the original value, a
    field with dynamic location is effectively a pointer value contained
    within the parent value, with rules in the DWARF for how to
    dereference the pointer.
    
    So, we end up with a lazy lval_internalvar_component representing a
    field within an lval_internalvar.  This eventually ends up in
    value_fetch_lazy, which currently does not support
    lval_internalvar_component, and we see the error above.
    
    My original plan for how to handle this involved extending
    value_fetch_lazy to handle lval_internalvar_component.  However, when
    I did this I ran into another error:
    
      (gdb) set $a = some_var
      (gdb) p $a
      $1 = ( array_one = ((1, 1) (1, 1) (1, 1)), a_field = 5, array_two = ((0, 0, 0) (0, 0, 0)) )
      (gdb) p $a%array_one
      $2 = ((1, 1) (1, 1) (1, 1))
      (gdb) p $a%array_one(1,1)
      ../../src/gdb/value.c:1547: internal-error: void set_value_address(value*, CORE_ADDR): Assertion `value->lval == lval_memory' failed.
    
    The problem now is inside set_value_component_location, where we
    attempt to set the address for a component if the original parent
    value has a dynamic location.  GDB does not expect to ever set the
    address on anything other than an lval_memory value (which seems
    reasonable).
    
    In order to resolve this issue I initially thought about how an
    internalvar should "capture" the value of a program variable at the
    moment the var is created.  In an ideal world (I think) GDB would be
    able to do this even for values with dynamic type.  So in our above
    example doing `set $a = some_var` would capture the content of
    'some_var', but also the content of 'array_one', and also 'array_two',
    even though these content regions are not contained within the region
    of 'some_var'.
    
    Supporting this would require GDB values to be able to carry around
    multiple non-contiguous regions of memory as content in some way,
    which sounds like a pretty huge change to a core part of GDB.
    
    So, I wondered if there was some other solution that wouldn't require
    such a huge change.
    
    What if values with a dynamic location were though of like points with
    automatic dereferencing?  Given this C structure:
    
      struct foo_t {
        int *val;
      }
    
      struct foo_t my_foo;
    
    Then in GDB:
    
      (gdb) $a = my_foo
    
    We would expect GDB to capture the pointer value in '$a', but not the
    value pointed at by the pointer.  So maybe it's not that unreasonable
    to think that given a dynamically typed field GDB will capture the
    address of the content, but not the actual content itself.
    
    That's what this patch does.
    
    The approach is to catch this case in set_value_component_location.
    When we create a component location (of an lval_internalvar) that has
    a dynamic data location, the lval_internalvar_component is changed
    into an lval_memory.  After this, both of the above issues are
    resolved.  In the first case, the lval_memory is still lazy, but
    value_fetch_lazy knows how to handle that.  In the second case, when
    we access an element of the array we are now accessing an element of
    an lval_memory, not an lval_internalvar_component, and calling
    set_value_address on an lval_memory is fine.
    
    gdb/ChangeLog:
    
            * value.c (set_value_component_location): Adjust the VALUE_LVAL
            for internalvar components that have a dynamic location.
    
    gdb/testsuite/ChangeLog:
    
            * gdb.fortran/intvar-dynamic-types.exp: New file.
            * gdb.fortran/intvar-dynamic-types.f90: New file.

diff --git a/gdb/testsuite/gdb.fortran/intvar-dynamic-types.exp b/gdb/testsuite/gdb.fortran/intvar-dynamic-types.exp
new file mode 100644
index 00000000000..16dc603cb47
--- /dev/null
+++ b/gdb/testsuite/gdb.fortran/intvar-dynamic-types.exp
@@ -0,0 +1,97 @@
+# Copyright 2020-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/>.
+
+# Places a value with components that have dynamic type into a GDB
+# user variable, and then prints the user variable.
+
+standard_testfile ".f90"
+load_lib "fortran.exp"
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
+    {debug f90 quiet}] } {
+    return -1
+}
+
+if ![fortran_runto_main] {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_breakpoint [gdb_get_line_number "Break here"]
+gdb_continue_to_breakpoint "Break here"
+
+gdb_test_no_output "set \$a=some_var" "set \$a internal variable"
+
+foreach var { "\$a" "some_var" } {
+    with_test_prefix "print $var" {
+	gdb_test "print $var" \
+	    " = \\( array_one = \\(\\(1, 1\\) \\(1, 1\\) \\(1, 1\\)\\), a_field = 5, array_two = \\(\\(2, 2, 2\\) \\(2, 2, 2\\)\\) \\)" \
+	    "print full contents"
+
+	gdb_test "print $var%array_one" \
+	    " = \\(\\(1, 1\\) \\(1, 1\\) \\(1, 1\\)\\)" \
+	    "print array_one field"
+
+	gdb_test "print $var%a_field" \
+	    " = 5" \
+	    "print a_field field"
+
+	gdb_test "print $var%array_two" \
+	    " = \\(\\(2, 2, 2\\) \\(2, 2, 2\\)\\)" \
+	    "print array_two field"
+    }
+}
+
+# Create new user variables for the fields of some_var, and show that
+# modifying these variables does not change the original value from
+# the program.
+gdb_test_no_output "set \$b = some_var%array_one"
+gdb_test_no_output "set \$c = some_var%array_two"
+gdb_test "print \$b" \
+    " = \\(\\(1, 1\\) \\(1, 1\\) \\(1, 1\\)\\)"
+gdb_test "print \$c" \
+    " = \\(\\(2, 2, 2\\) \\(2, 2, 2\\)\\)"
+gdb_test_no_output "set \$b(2,2) = 3"
+gdb_test_no_output "set \$c(3,1) = 4"
+gdb_test "print \$b" \
+    " = \\(\\(1, 1\\) \\(1, 3\\) \\(1, 1\\)\\)" \
+    "print \$b after a change"
+gdb_test "print \$c" \
+    " = \\(\\(2, 2, 4\\) \\(2, 2, 2\\)\\)" \
+    "print \$c after a change"
+gdb_test "print some_var%array_one" \
+    " = \\(\\(1, 1\\) \\(1, 1\\) \\(1, 1\\)\\)"
+gdb_test "print some_var%array_two" \
+    " = \\(\\(2, 2, 2\\) \\(2, 2, 2\\)\\)"
+
+# Now modify the user variable '$a', which is a copy of 'some_var',
+# and then check how this change is reflected in the original
+# 'some_var', and the user variable $a.
+#
+# When we change 'a_field' which is a non-dynamic field within the
+# user variable, the change is only visible within the user variable.
+#
+# In contrast, when we change 'array_one' and 'array_two', which are
+# both fields of dynanic type, then the change is visible in both the
+# user variable and the original program variable 'some_var'.  This
+# makes sense if you consider the dynamic type as if it was a C
+# pointer with automatic indirection.
+gdb_test_no_output "set \$a%a_field = 3"
+gdb_test_no_output "set \$a%array_one(2,2) = 3"
+gdb_test_no_output "set \$a%array_two(3,1) = 4"
+gdb_test "print \$a" \
+    " = \\( array_one = \\(\\(1, 1\\) \\(1, 3\\) \\(1, 1\\)\\), a_field = 3, array_two = \\(\\(2, 2, 4\\) \\(2, 2, 2\\)\\) \\)"
+gdb_test "print some_var" \
+    " = \\( array_one = \\(\\(1, 1\\) \\(1, 3\\) \\(1, 1\\)\\), a_field = 5, array_two = \\(\\(2, 2, 4\\) \\(2, 2, 2\\)\\) \\)"
diff --git a/gdb/testsuite/gdb.fortran/intvar-dynamic-types.f90 b/gdb/testsuite/gdb.fortran/intvar-dynamic-types.f90
new file mode 100644
index 00000000000..ef51a32ec77
--- /dev/null
+++ b/gdb/testsuite/gdb.fortran/intvar-dynamic-types.f90
@@ -0,0 +1,32 @@
+! Copyright 2020-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/>.
+
+program internal_var_test
+  type :: some_type
+     integer, allocatable :: array_one (:,:)
+     integer :: a_field
+     integer, allocatable :: array_two (:,:)
+  end type some_type
+
+  type(some_type) :: some_var
+
+  allocate (some_var%array_one (2,3))
+  allocate (some_var%array_two (3,2))
+  some_var%array_one (:,:) = 1
+  some_var%a_field = 5
+  some_var%array_two (:,:) = 2
+  deallocate (some_var%array_one) ! Break here.
+  deallocate (some_var%array_two)
+end program internal_var_test
diff --git a/gdb/value.c b/gdb/value.c
index a20ae5af4f0..c84698d25e0 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1784,12 +1784,45 @@ set_value_component_location (struct value *component,
 	component->location.computed.closure = funcs->copy_closure (whole);
     }
 
-  /* If type has a dynamic resolved location property
-     update it's value address.  */
+  /* If the WHOLE value has a dynamically resolved location property then
+     update the address of the COMPONENT.  */
   type = value_type (whole);
   if (NULL != TYPE_DATA_LOCATION (type)
       && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST)
     set_value_address (component, TYPE_DATA_LOCATION_ADDR (type));
+
+  /* Similarly, if the COMPONENT value has a dynamically resolved location
+     property then update its address.  */
+  type = value_type (component);
+  if (NULL != TYPE_DATA_LOCATION (type)
+      && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST)
+    {
+      /* If the COMPONENT has a dynamic location, and is an
+	 lval_internalvar_component, then we change it to a lval_memory.
+
+	 Usually a component of an internalvar is created non-lazy, and has
+	 its content immediately copied from the parent internalvar.
+	 However, for components with a dynamic location, the content of
+	 the component is not contained within the parent, but is instead
+	 accessed indirectly.  Further, the component will be created as a
+	 lazy value.
+
+	 By changing the type of the component to lval_memory we ensure
+	 that value_fetch_lazy can successfully load the component.
+
+         This solution isn't ideal, but a real fix would require values to
+         carry around both the parent value contents, and the contents of
+         any dynamic fields within the parent.  This is a substantial
+         change to how values work in GDB.  */
+      if (VALUE_LVAL (component) == lval_internalvar_component)
+	{
+	  gdb_assert (value_lazy (component));
+	  VALUE_LVAL (component) = lval_memory;
+	}
+      else
+	gdb_assert (VALUE_LVAL (component) == lval_memory);
+      set_value_address (component, TYPE_DATA_LOCATION_ADDR (type));
+    }
 }
 
 /* Access to the value history.  */