This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] New function value_has_address
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>, Simon Marchi <simon dot marchi at polymtl dot ca>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Thu, 27 Oct 2016 14:03:34 +0100
- Subject: Re: [PATCH] New function value_has_address
- Authentication-results: sourceware.org; auth=none
- References: <1477557571-17122-1-git-send-email-yao.qi@linaro.org> <329976bc32481db3faff9288f777431d@polymtl.ca> <CAH=s-PPXzimpQ_OCruLfqgOik5siJ568Pr6kA9+UwwCaM7A8SA@mail.gmail.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