New function value_has_address

  This patch is to move duplicated condition checking in three functions
to a single new function, value_has_address.

The patch is obvious.  I'll push it in later today, in case that people
think function value_has_address is not named properly.

gdb:

2016-10-27  Yao Qi  <yao.qi@linaro.org>

	* value.c (value_has_address): New function.
	(value_address): Call value_has_address.
	(value_raw_address): Likewise.
	(set_value_address): Likewise.
---
 gdb/value.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)
  

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?
  

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.
  

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
  

On Thu, Oct 27, 2016 at 2:03 PM, Pedro Alves <palves@redhat.com> wrote:
>
> 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.

So looks we can restrict value_has_address,

/* Return true if VALUE->location.address is valid, otherwise return
   false.  */

static int
value_has_address (const struct value *value)
{
  return (value->lval == lval_memory || value->lval == lval_register);
}

This triggers some GDB internal errors.  I fixed some of them, but
still don't know how to fix one in gdbpy_apply_val_pretty_printer,

  set_value_component_location (value, val);
  /* 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);

still can't figure out why do we call set_value_component_location
here.  These code was added in
https://sourceware.org/ml/gdb-patches/2010-06/msg00132.html
I'll ask Tom.

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

value_address () return value is not always passed to val_print.
We pass zero to val_print in some places in ada-valprint.c.
I don't see how to eliminate tat parameter, if I understand you
correctly.

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

Yes, I agree, but I don't know how much memory usage increase
if "struct value" becomes "class value".  We have such comments
to "struct value",

/* Note that the fields in this structure are arranged to save a bit
   of memory.  */

I am not ready converting value to C++ as I still don't think I fully
understand it.
  

On 10/28/2016 12:49 PM, Yao Qi wrote:
>> > 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.
>> >
> Yes, I agree, but I don't know how much memory usage increase
> if "struct value" becomes "class value".  

Maybe one pointer.  Probably doesn't matter.
It'd be the equivalent of moving the const struct lval_funcs *funcs field
out of the union (If you add virtual methods to a class, it adds one
hidden pointer to a table of function pointers -- the vtable, so
that calls to virtual methods can dynamically dispatch, just like
lval_funcs).  If some fields move to subclasses that are seldom
instantiated, then you may actually save memory.  Or not, because
the heap may already be returning memory blocks larger than
we ask it, and we might simply waste less.  Depends on current size
of the structure, and malloc being used, etc.  Not a clear cut.

> We have such comments to "struct value",
> 
> /* Note that the fields in this structure are arranged to save a bit
>    of memory.  */

I think that's referring to the maybe-surprising ordering of members
of the struct -- they're layed out to avoid holes due to alignment.

> 
> I am not ready converting value to C++ as I still don't think I fully
> understand it.

I wasn't suggesting you'd do it yourself.  Just saying that if people
want to try it, they have my support.  :-)

Thanks,
Pedro Alves
  

On 10/28/2016 12:49 PM, Yao Qi wrote:
> 
>> >
>> > 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.
> value_address () return value is not always passed to val_print.
> We pass zero to val_print in some places in ada-valprint.c.
> I don't see how to eliminate tat parameter, if I understand you
> correctly.
> 

That suggests that it's calling val_print on values that can't
even have an address.  Those cases would be adjusted to address,
as the others.  Then at the points that actually do need
a value's address somewhere inside val_print and callees, we'd get
the address from the struct value * pointer passed around
as well, maybe adjusted with the sliding embedded_offset
passed down too.  Currently, it's not easy to see which are
those places exactly because we always pass around some
address, even if bogus.

Thanks,
Pedro Alves