Re: [PATCH] New function value_has_address

[Pedro Alves <palves at redhat dot com>]

On 10/27/2016 01:12 PM, Yao Qi wrote:
> On Thu, Oct 27, 2016 at 12:29 PM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>> That help clarifies the code, I like it.
>>
>>> diff --git a/gdb/value.c b/gdb/value.c
>>> index b825aec..4b5cbde 100644
>>> --- a/gdb/value.c
>>> +++ b/gdb/value.c
>>> @@ -1538,12 +1538,20 @@ value_lval_const (const struct value *value)
>>>    return value->lval;
>>>  }
>>>
>>> +/* Return true if VALUE has address, otherwise return false.  */
>>
>>
>> This comment is really obvious, it's really just stating the function name.
>> Could you expand it a bit, perhaps saying what it means for a value to "have
>> address"?  I suppose it means that it has a memory address on the target?
> 
> It is intended to avoid expanding the comments because I can't give a
> clear meaning, like "has a memory address on the target", unless I fully
> understand "value" in gdb.  That is why I only make value_has_address
> static.
> 
> I find some inconsistencies across the code,
> 
>   /* The only lval kinds which do not live in target memory.  */
>   if (VALUE_LVAL (val) != not_lval
>       && VALUE_LVAL (val) != lval_internalvar
>       && VALUE_LVAL (val) != lval_xcallable)
>     return 0;
> 
> lval_internalvar_component is not checked,
> 
>   /* set_value_component_location resets the address, so we may
>      need to set it again.  */
>   if (VALUE_LVAL (value) != lval_internalvar
>       && VALUE_LVAL (value) != lval_internalvar_component
>       && VALUE_LVAL (value) != lval_computed)
>     set_value_address (value, address + embedded_offset);
> 
> lval_xcallable is not checked, but lval_computed is checked.  Before
> we clearly document the meaning of value_has_address, we need to
> figure out these things above first.
> 

I think the answer is here:

  /* Location of value (if lval).  */
  union
  {
    /* If lval == lval_memory, this is the address in the inferior.
       If lval == lval_register, this is the byte offset into the
       registers structure.  */
    CORE_ADDR address;

    /* Pointer to internal variable.  */
    struct internalvar *internalvar;

    /* Pointer to xmethod worker.  */
    struct xmethod_worker *xm_worker;

    /* If lval == lval_computed, this is a set of function pointers
       to use to access and describe the value, and a closure pointer
       for them to use.  */
    struct
    {
      /* Functions to call.  */
      const struct lval_funcs *funcs;

      /* Closure for those functions to use.  */
      void *closure;
    } computed;
  } location;


That's a union.  We should only ever access the "address"
on lval types that use the "address" field above.

Note that we call value_address on lval_computed values and that
incorrectly returns "value->location.address + value->offset",
which is completely bogus.  (Try running to main, and then doing
p $_siginfo).

The value printing routines want to pass around a value address,
but since we actually pass the value pointer around nowadays too,
I think that parameter could be eliminated, and the result is
likely to be that value_address ends up called is much fewer
places where it doesn't really make sense.

IMO, struct value and its lval types would be nice candidates
for converting to a class hierarchy, with lval_funcs
expanded into virtual methods in struct value directly, that are
then used by all value lval types, including lval_memory/lval_register.

Thanks,
Pedro Alves