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

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

Hi,


On 1/8/21 8:56 AM, Andrew Burgess wrote:
> * 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


This seems to be causing some internal errors on AArch64-Linux Ubuntu 18.04.

FAIL: gdb.fortran/intvar-dynamic-types.exp: print $a (GDB internal error)
FAIL: gdb.fortran/intvar-dynamic-types.exp: print $a: print array_one 
field (GDB internal error)
FAIL: gdb.fortran/intvar-dynamic-types.exp: print $a: print array_two 
field (GDB internal error)
FAIL: gdb.fortran/intvar-dynamic-types.exp: print $a: print full 
contents (GDB internal error)
FAIL: gdb.fortran/intvar-dynamic-types.exp: print $b after a change
FAIL: gdb.fortran/intvar-dynamic-types.exp: print $c after a change
FAIL: gdb.fortran/intvar-dynamic-types.exp: print some_var
FAIL: gdb.fortran/intvar-dynamic-types.exp: set $a%array_one(2,2) = 3
FAIL: gdb.fortran/intvar-dynamic-types.exp: set $a%array_two(3,1) = 4
FAIL: gdb.fortran/intvar-dynamic-types.exp: set $b(2,2) = 3
FAIL: gdb.fortran/intvar-dynamic-types.exp: set $c(3,1) = 4

The internal error is...

print $a
$1 = ( array_one = ../../../repos/binutils-gdb/gdb/value.c:3983: 
internal-error: Unexpected lazy value type.

Any ideas? I can provide the full log as well, if you think that is useful.