[PATCH] New function value_has_address

[PATCH] New function value_has_address

[Yao Qi]

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(-)

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.  */
+
+static int
+value_has_address (const struct value *value)
+{
+  return (value->lval != lval_internalvar
+	  && value->lval != lval_internalvar_component
+	  && value->lval != lval_xcallable);
+}
+
 CORE_ADDR
 value_address (const struct value *value)
 {
-  if (value->lval == lval_internalvar
-      || value->lval == lval_internalvar_component
-      || value->lval == lval_xcallable)
+  if (!value_has_address (value))
     return 0;
   if (value->parent != NULL)
     return value_address (value->parent) + value->offset;
@@ -1559,9 +1567,7 @@ value_address (const struct value *value)
 CORE_ADDR
 value_raw_address (const struct value *value)
 {
-  if (value->lval == lval_internalvar
-      || value->lval == lval_internalvar_component
-      || value->lval == lval_xcallable)
+  if (!value_has_address (value))
     return 0;
   return value->location.address;
 }
@@ -1569,9 +1575,7 @@ value_raw_address (const struct value *value)
 void
 set_value_address (struct value *value, CORE_ADDR addr)
 {
-  gdb_assert (value->lval != lval_internalvar
-	      && value->lval != lval_internalvar_component
-	      && value->lval != lval_xcallable);
+  gdb_assert (value_has_address (value));
   value->location.address = addr;
 }
 
-- 
1.9.1

Re: [PATCH] New function value_has_address

[Simon Marchi]

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?

Re: [PATCH] New function value_has_address

[Yao Qi]

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.

-- 
Yao (齐尧)

Re: [PATCH] New function value_has_address

[Pedro Alves]

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

Re: [PATCH] New function value_has_address

[Yao Qi]

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.

-- 
Yao (齐尧)

Re: [PATCH] New function value_has_address

[Pedro Alves]

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

Re: [PATCH] New function value_has_address

[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

Re: [PATCH] New function value_has_address

[Ulrich Weigand]

Pedro Alves wrote:

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

Agreed.  I've done a quick check, and it seems except from
recursively passing around the address value throughout
the printing routines, there only a small number of actual
uses of the address parameter:

- print_unpacked_pointer, where we actually want to show that
  address to the user

- cp_print_value routines (and similar code in p-valprint),
  where we want to load the contents of the superclass
  surrounding the current subclass from memory

- Ada needs the address to call resolve_dynamic_type

- The extension language printer (guile, python), where we
  want to create a subobject value at the embedded offset

- A few more places in Ada also want to create the subobject
  value at the embedded offset

I think for the first three, we should simply check right
at the place where it is needed whether the original value
has an address in inferior memory, and if not, just fail.

For the cases where the subobject value is needed, I think
we should provide a generic helper to compute the location
at the embedded offset from the original value's location,
depending on the location type.  (And in those cases where
this may not be possible, just default to unknown location.)

B.t.w. the valaddr parameter likewise seems to be redundant
now and probably ought to be eliminated as well.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] New function value_has_address

[Pedro Alves]

On 10/28/2016 03:09 PM, Ulrich Weigand wrote:
> Pedro Alves wrote:
> 
>> 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.
> 
> Agreed.  I've done a quick check, and it seems except from
> recursively passing around the address value throughout
> the printing routines, there only a small number of actual
> uses of the address parameter:
> 
> - print_unpacked_pointer, where we actually want to show that
>   address to the user
> 
> - cp_print_value routines (and similar code in p-valprint),
>   where we want to load the contents of the superclass
>   surrounding the current subclass from memory
> 
> - Ada needs the address to call resolve_dynamic_type
> 
> - The extension language printer (guile, python), where we
>   want to create a subobject value at the embedded offset
> 
> - A few more places in Ada also want to create the subobject
>   value at the embedded offset
> 
> I think for the first three, we should simply check right
> at the place where it is needed whether the original value
> has an address in inferior memory, and if not, just fail.
> 
> For the cases where the subobject value is needed, I think
> we should provide a generic helper to compute the location
> at the embedded offset from the original value's location,
> depending on the location type.  (And in those cases where
> this may not be possible, just default to unknown location.)
> 

Thanks for investigating!

> B.t.w. the valaddr parameter likewise seems to be redundant
> now and probably ought to be eliminated as well.

Agreed.  Getting rid of that one would allow creating
sparse value contents, btw.  Currently we're forced to
always allocate a huge buffer for huge inferior values
exactly because these routines assume the value contents
are one single byte array.

Thanks,
Pedro Alves