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

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

On 1/11/21 11:30 AM, Luis Machado wrote:
> 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.

I think this has been addressed already in a later change. I just 
noticed some libiberty change broke the HEAD build and I was sitting at 
a GDB from Jan 4th. This is a non-issue then.