This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/3] New function value_has_address
- From: Pedro Alves <palves at redhat dot com>
- To: Ulrich Weigand <uweigand at de dot ibm dot com>
- Cc: Joel Brobecker <brobecker at adacore dot com>, Yao Qi <qiyaoltc at gmail dot com>, gdb-patches at sourceware dot org
- Date: Tue, 22 Nov 2016 18:29:48 +0000
- Subject: Re: [PATCH 1/3] New function value_has_address
- Authentication-results: sourceware.org; auth=none
- References: <20161122181616.118F810B923@oc8523832656.ibm.com>
On 11/22/2016 06:16 PM, Ulrich Weigand wrote:
> Pedro Alves wrote:
>> On 11/22/2016 04:50 PM, Joel Brobecker wrote:
>>> Hey Yao,
>>>
>>>> +/* 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);
>>>
>>> I'm wondering about the function's name. Does a value that
>>> lives in a register, for instance, really have an address?
>>> For me, if there was a function value_has_address, it would
>>> return nonzero only for lval_memory. I'm not too sure if
>>> lval_computed would qualify or not.
>>>
>>> Perhaps, what you were looking for, is something like
>>> value_lives_in_inferior?
>>
>> The intention of the function is to return true if the value
>> uses the value.location.address union field as location:
>>
>> /* 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;
>> ...
>> } location;
>>
>> I think that it's good that the names match. If one is renamed,
>> so should the other, IMO. Maybe call the function
>> value_has_address_location? I think it'd be good if the
>> function's intro comment made this link more explicit.
>> Actually, I see now that patch #3 tweaks the comment.
>
> I think part of the confusion is that the comment above is simply
> no longer true; for lval_register values, address is *not* (any longer)
> used to hold any byte offset into a register structure, as far as I
> can see.
Curious. Even better then.
> Instead, for lval_register values, the register that holds
> the value is identifed solely via the VALUE_REGNUM/VALUE_NEXT_FRAME_ID
> fields, and the address field is ignored.
>
> I think we should reword the comments to reflect the fact that
> "address" is only used for lval_address. On the other hand,
> the regnum/frame_id fields should move into the union and only
> be used for lval_register values ...
That makes sense to me.
Thanks,
Pedro Alves