Re: [PATCH 1/3] New function value_has_address

[Pedro Alves <palves at redhat dot 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