Re: [PATCH 1/3] New function value_has_address

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

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

Bye,
Ulrich

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