[PATCH 0/3] New function value_has_address

[PATCH 0/3] New function value_has_address

[Yao Qi]

This patch series is inspired by the discussion in this thread
https://sourceware.org/ml/gdb-patches/2016-10/msg00741.html in which
I only added value_has_address.  During the discussion, looks
value_has_address should be true only for lval_memory and lval_register,
then, I restrict value_has_address (done in patch #3), and trigger a
lot of assertions, because VALUE_VAL is not set properly before
set_value_address.  Patch #2 fixes these problems.

Patch series is regression tested on x86_64-linux.

Note that, I also find that value_has_address may only return true
for lval_memory, because I can't see how VALUE->location.address is valid
to lval_register.  Of course, I can be wrong, so further analysis
is needed.

*** BLURB HERE ***

Yao Qi (3):
  New function value_has_address
  Set VALUE_VAL before set_value_address
  Restrict value_has_address

 gdb/ada-lang.c |  2 +-
 gdb/elfread.c  |  2 ++
 gdb/value.c    | 27 ++++++++++++++++-----------
 3 files changed, 19 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH 2/3] Set VALUE_VAL before set_value_address

[Yao Qi]

Since we have a check on VALUE_VAL in set_value_address, we need to
set VALUE_VAL properly before set_value_address.

gdb:

2016-11-21  Yao Qi  <yao.qi@linaro.org>

	* ada-lang.c (ensure_lval): Call set_value_address after setting
	VALUE_LVAL.
	* elfread.c (elf_gnu_ifunc_resolve_addr): Set VALUE_LVAL to
	lval_memory.
	(elf_gnu_ifunc_resolver_return_stop): Likewise.
	* value.c (value_fn_field): Likewise.
	(value_from_contents_and_address_unresolved): Likewise.
	(value_from_contents_and_address): Likewise.
---
 gdb/ada-lang.c | 2 +-
 gdb/elfread.c  | 2 ++
 gdb/value.c    | 5 +++--
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 0647a9b..33591af 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -4473,8 +4473,8 @@ ensure_lval (struct value *val)
       const CORE_ADDR addr =
         value_as_long (value_allocate_space_in_inferior (len));
 
-      set_value_address (val, addr);
       VALUE_LVAL (val) = lval_memory;
+      set_value_address (val, addr);
       write_memory (addr, value_contents (val), len);
     }
 
diff --git a/gdb/elfread.c b/gdb/elfread.c
index e49af6d..c6d0fdb 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -879,6 +879,7 @@ elf_gnu_ifunc_resolve_addr (struct gdbarch *gdbarch, CORE_ADDR pc)
     name_at_pc = NULL;
 
   function = allocate_value (func_func_type);
+  VALUE_LVAL (function) = lval_memory;
   set_value_address (function, pc);
 
   /* STT_GNU_IFUNC resolver functions usually receive the HWCAP vector as
@@ -992,6 +993,7 @@ elf_gnu_ifunc_resolver_return_stop (struct breakpoint *b)
   gdb_assert (b->loc->next == NULL);
 
   func_func = allocate_value (func_func_type);
+  VALUE_LVAL (func_func) = lval_memory;
   set_value_address (func_func, b->loc->related_address);
 
   value = allocate_value (value_type);
diff --git a/gdb/value.c b/gdb/value.c
index a8ab5db..a093a9a 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3280,6 +3280,7 @@ value_fn_field (struct value **arg1p, struct fn_field *f,
     }
 
   v = allocate_value (ftype);
+  VALUE_LVAL (v) = lval_memory;
   if (sym)
     {
       set_value_address (v, BLOCK_START (SYMBOL_BLOCK_VALUE (sym)));
@@ -3666,8 +3667,8 @@ value_from_contents_and_address_unresolved (struct type *type,
     v = allocate_value_lazy (type);
   else
     v = value_from_contents (type, valaddr);
-  set_value_address (v, address);
   VALUE_LVAL (v) = lval_memory;
+  set_value_address (v, address);
   return v;
 }
 
@@ -3692,8 +3693,8 @@ value_from_contents_and_address (struct type *type,
   if (TYPE_DATA_LOCATION (resolved_type_no_typedef) != NULL
       && TYPE_DATA_LOCATION_KIND (resolved_type_no_typedef) == PROP_CONST)
     address = TYPE_DATA_LOCATION_ADDR (resolved_type_no_typedef);
-  set_value_address (v, address);
   VALUE_LVAL (v) = lval_memory;
+  set_value_address (v, address);
   return v;
 }
 
-- 
1.9.1

[PATCH 3/3] Restrict value_has_address

[Yao Qi]

gdb:

2016-11-21  Yao Qi  <yao.qi@linaro.org>

	* value.c (value_has_address): Return true if lval is
	either lval_memory or lval_register.
---
 gdb/value.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/value.c b/gdb/value.c
index a093a9a..5ac9123 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1540,14 +1540,14 @@ value_lval_const (const struct value *value)
   return value->lval;
 }
 
-/* Return true if VALUE has address, otherwise return false.  */
+/* Return true if VALUE->location.address is valid, 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);
+  return (value->lval == lval_memory
+	  || value->lval == lval_register);
 }
 
 CORE_ADDR
-- 
1.9.1

[PATCH 1/3] 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.

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 8d33501..a8ab5db 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1540,12 +1540,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;
@@ -1561,9 +1569,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;
 }
@@ -1571,9 +1577,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 1/3] New function value_has_address

[Joel Brobecker]

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?

-- 
Joel

Re: [PATCH 2/3] Set VALUE_VAL before set_value_address

[Luis Machado]

On 11/22/2016 09:48 AM, Yao Qi wrote:
> Since we have a check on VALUE_VAL in set_value_address, we need to
> set VALUE_VAL properly before set_value_address.
>
> gdb:
>
> 2016-11-21  Yao Qi  <yao.qi@linaro.org>
>
> 	* ada-lang.c (ensure_lval): Call set_value_address after setting
> 	VALUE_LVAL.
> 	* elfread.c (elf_gnu_ifunc_resolve_addr): Set VALUE_LVAL to
> 	lval_memory.
> 	(elf_gnu_ifunc_resolver_return_stop): Likewise.
> 	* value.c (value_fn_field): Likewise.
> 	(value_from_contents_and_address_unresolved): Likewise.
> 	(value_from_contents_and_address): Likewise.
> ---
>  gdb/ada-lang.c | 2 +-
>  gdb/elfread.c  | 2 ++
>  gdb/value.c    | 5 +++--
>  3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 0647a9b..33591af 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -4473,8 +4473,8 @@ ensure_lval (struct value *val)
>        const CORE_ADDR addr =
>          value_as_long (value_allocate_space_in_inferior (len));
>
> -      set_value_address (val, addr);
>        VALUE_LVAL (val) = lval_memory;
> +      set_value_address (val, addr);
>        write_memory (addr, value_contents (val), len);
>      }
>
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index e49af6d..c6d0fdb 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -879,6 +879,7 @@ elf_gnu_ifunc_resolve_addr (struct gdbarch *gdbarch, CORE_ADDR pc)
>      name_at_pc = NULL;
>
>    function = allocate_value (func_func_type);
> +  VALUE_LVAL (function) = lval_memory;
>    set_value_address (function, pc);
>
>    /* STT_GNU_IFUNC resolver functions usually receive the HWCAP vector as
> @@ -992,6 +993,7 @@ elf_gnu_ifunc_resolver_return_stop (struct breakpoint *b)
>    gdb_assert (b->loc->next == NULL);
>
>    func_func = allocate_value (func_func_type);
> +  VALUE_LVAL (func_func) = lval_memory;
>    set_value_address (func_func, b->loc->related_address);
>
>    value = allocate_value (value_type);
> diff --git a/gdb/value.c b/gdb/value.c
> index a8ab5db..a093a9a 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -3280,6 +3280,7 @@ value_fn_field (struct value **arg1p, struct fn_field *f,
>      }
>
>    v = allocate_value (ftype);
> +  VALUE_LVAL (v) = lval_memory;
>    if (sym)
>      {
>        set_value_address (v, BLOCK_START (SYMBOL_BLOCK_VALUE (sym)));
> @@ -3666,8 +3667,8 @@ value_from_contents_and_address_unresolved (struct type *type,
>      v = allocate_value_lazy (type);
>    else
>      v = value_from_contents (type, valaddr);
> -  set_value_address (v, address);
>    VALUE_LVAL (v) = lval_memory;
> +  set_value_address (v, address);
>    return v;
>  }
>
> @@ -3692,8 +3693,8 @@ value_from_contents_and_address (struct type *type,
>    if (TYPE_DATA_LOCATION (resolved_type_no_typedef) != NULL
>        && TYPE_DATA_LOCATION_KIND (resolved_type_no_typedef) == PROP_CONST)
>      address = TYPE_DATA_LOCATION_ADDR (resolved_type_no_typedef);
> -  set_value_address (v, address);
>    VALUE_LVAL (v) = lval_memory;
> +  set_value_address (v, address);
>    return v;
>  }
>
>

It sounds like if we go the route of having value_has_address only 
return true for lval_memory, we could get rid of these explicit 
assignments of VALUE_LVAL and make set_value_address set 
VALUE->location.address.

I agree with your initial assessment that only lval_memory should have 
an address. But maybe GDB is using lval_register with other meanings?

Re: [PATCH 1/3] New function value_has_address

[Pedro Alves]

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.

Thanks,
Pedro Alves

Re: [PATCH 2/3] Set VALUE_VAL before set_value_address

[Pedro Alves]

On 11/22/2016 03:48 PM, Yao Qi wrote:
> Since we have a check on VALUE_VAL in set_value_address, we need to
> set VALUE_VAL properly before set_value_address.

LGTM.

I wonder whether it'd be hard to add a new function that
takes care of the ordering:

  struct value *allocate_memory_value (struct type *type, CORE_ADDR address);

that would allocate a lazy value with lval == lval_memory and
address filled in.  We use that instead throughout.

This would translate more directly to converting struct value to a
class hierarchy down the road, with allocate_memory_value mapping
to a struct memory_value constructor.

Thanks,
Pedro Alves

Re: [PATCH 3/3] Restrict value_has_address

[Pedro Alves]

On 11/22/2016 03:48 PM, Yao Qi wrote:
> gdb:
> 
> 2016-11-21  Yao Qi  <yao.qi@linaro.org>
> 
> 	* value.c (value_has_address): Return true if lval is
> 	either lval_memory or lval_register.
> ---
>  gdb/value.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/value.c b/gdb/value.c
> index a093a9a..5ac9123 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -1540,14 +1540,14 @@ value_lval_const (const struct value *value)
>    return value->lval;
>  }
>  
> -/* Return true if VALUE has address, otherwise return false.  */
> +/* Return true if VALUE->location.address is valid, otherwise return
> +   false.  */

I think it'd be clearer to say:

/* Return true if the VALUE's lval uses the value.location.address
   union member as location.  False otherwise.  */

>  
>  static int

BTW, this could be bool (in patch #1).

Thanks,
Pedro Alves

Re: [PATCH 1/3] New function value_has_address

[Ulrich Weigand]

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

Re: [PATCH 1/3] New function value_has_address

[Pedro Alves]

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

Re: [PATCH 1/3] New function value_has_address

[Yao Qi]

On Tue, Nov 22, 2016 at 07:16:15PM +0100, Ulrich Weigand wrote:
> > 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.

That is what I am saying in the last paragraph of cover letter.

> 
> 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 is what I am trying to do in next step.  Let me finish it and
include it in this series as well.

-- 
Yao (齐尧)

Re: [PATCH 1/3] New function value_has_address

[Ulrich Weigand]

Yao Qi wrote:
> On Tue, Nov 22, 2016 at 07:16:15PM +0100, Ulrich Weigand wrote:
> > > 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.
> 
> That is what I am saying in the last paragraph of cover letter.

I see, I missed that.  I think given that we really only ever should
use value.location.address on lval_memory values, maybe the clearest way
would be to guard accesses with an explicit VALUE_LVAL == lval_memory
check instead of moving that check to a function.  Then we don't have
to debate how that function should be called either :-)

> > 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 is what I am trying to do in next step.  Let me finish it and
> include it in this series as well.

Very good, thanks!

[ B.t.w. I noticed one more case of fields that probably ought to
move into the location union: value.parent, .bitsize, and .bitpos
are only ever used for bitfields.   Maybe we should create a new
lval_component (possibly renamed from lval_internalval_component)
that uses those three fields as location; where all further data
is retrieved from the parent value.  But that's certainly another
independent change ... ]

Bye,
Ulrich

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

[PATCH 3/3] Restrict checking value.lval on using address

[Yao Qi]

With the previous change, value.location.address is only valid for
lval_memory.  This patch restrict some checking on value.lval on
using address.  Since we have a check on VALUE_VAL in
set_value_address, we need to set VALUE_VAL properly before
set_value_address too.

gdb:

2016-11-25  Yao Qi  <yao.qi@linaro.org>

	* ada-lang.c (ensure_lval): Call set_value_address after setting
	VALUE_LVAL.
	* elfread.c (elf_gnu_ifunc_resolve_addr): Set VALUE_LVAL to
	lval_memory.
	(elf_gnu_ifunc_resolver_return_stop): Likewise.
	* value.c (value_fn_field): Likewise.
	(value_from_contents_and_address_unresolved): Likewise.
	(value_from_contents_and_address): Likewise.
	(value_address): Check value->lval isn't
	lval_memory.
	(value_raw_address): Likewise.
	(set_value_address): Assert value->lval is lval_memory.
---
 gdb/ada-lang.c |  2 +-
 gdb/elfread.c  |  2 ++
 gdb/value.c    | 17 ++++++-----------
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 0647a9b..33591af 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -4473,8 +4473,8 @@ ensure_lval (struct value *val)
       const CORE_ADDR addr =
         value_as_long (value_allocate_space_in_inferior (len));
 
-      set_value_address (val, addr);
       VALUE_LVAL (val) = lval_memory;
+      set_value_address (val, addr);
       write_memory (addr, value_contents (val), len);
     }
 
diff --git a/gdb/elfread.c b/gdb/elfread.c
index e49af6d..c6d0fdb 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -879,6 +879,7 @@ elf_gnu_ifunc_resolve_addr (struct gdbarch *gdbarch, CORE_ADDR pc)
     name_at_pc = NULL;
 
   function = allocate_value (func_func_type);
+  VALUE_LVAL (function) = lval_memory;
   set_value_address (function, pc);
 
   /* STT_GNU_IFUNC resolver functions usually receive the HWCAP vector as
@@ -992,6 +993,7 @@ elf_gnu_ifunc_resolver_return_stop (struct breakpoint *b)
   gdb_assert (b->loc->next == NULL);
 
   func_func = allocate_value (func_func_type);
+  VALUE_LVAL (func_func) = lval_memory;
   set_value_address (func_func, b->loc->related_address);
 
   value = allocate_value (value_type);
diff --git a/gdb/value.c b/gdb/value.c
index eff5462..9fb5fe1 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1539,9 +1539,7 @@ value_lval_const (const struct value *value)
 CORE_ADDR
 value_address (const struct value *value)
 {
-  if (value->lval == lval_internalvar
-      || value->lval == lval_internalvar_component
-      || value->lval == lval_xcallable)
+  if (value->lval != lval_memory)
     return 0;
   if (value->parent != NULL)
     return value_address (value->parent) + value->offset;
@@ -1557,9 +1555,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->lval != lval_memory)
     return 0;
   return value->location.address;
 }
@@ -1567,9 +1563,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->lval == lval_memory);
   value->location.address = addr;
 }
 
@@ -3268,6 +3262,7 @@ value_fn_field (struct value **arg1p, struct fn_field *f,
     }
 
   v = allocate_value (ftype);
+  VALUE_LVAL (v) = lval_memory;
   if (sym)
     {
       set_value_address (v, BLOCK_START (SYMBOL_BLOCK_VALUE (sym)));
@@ -3654,8 +3649,8 @@ value_from_contents_and_address_unresolved (struct type *type,
     v = allocate_value_lazy (type);
   else
     v = value_from_contents (type, valaddr);
-  set_value_address (v, address);
   VALUE_LVAL (v) = lval_memory;
+  set_value_address (v, address);
   return v;
 }
 
@@ -3680,8 +3675,8 @@ value_from_contents_and_address (struct type *type,
   if (TYPE_DATA_LOCATION (resolved_type_no_typedef) != NULL
       && TYPE_DATA_LOCATION_KIND (resolved_type_no_typedef) == PROP_CONST)
     address = TYPE_DATA_LOCATION_ADDR (resolved_type_no_typedef);
-  set_value_address (v, address);
   VALUE_LVAL (v) = lval_memory;
+  set_value_address (v, address);
   return v;
 }
 
-- 
1.9.1

[PATCH 0/3] regnum and next_frame_id are only used for lval_register

[Yao Qi]

This patch series changes 'struct value' fields regnum and next_frame_id
are only used for lval_register.  This patch series is from the discussion
https://sourceware.org/ml/gdb-patches/2016-11/msg00630.html

Nowadays, computed value still uses next_frame_id, which is a misuse or abuse
to me.  Patch #1 adds a frame id in piece_closure, so computed value can get
frame id there.  Patch #2 moves fields regnum and next_frame_id to
value.location.  Then, value.location.address is only used by lval_memory,
patch #3 restrict the checking on using address.

Regression tested on x86_64-linux.

*** BLURB HERE ***

Yao Qi (3):
  Move computed value's frame id to piece_closure
  Adjust Value.location for lval_register
  Restrict checking value.lval on using address

 gdb/ada-lang.c  |  2 +-
 gdb/dwarf2loc.c | 25 ++++++++++++-------------
 gdb/elfread.c   |  2 ++
 gdb/valops.c    |  1 -
 gdb/value.c     | 57 +++++++++++++++++++++------------------------------------
 5 files changed, 36 insertions(+), 51 deletions(-)

-- 
1.9.1

[PATCH 1/3] Move computed value's frame id to piece_closure

[Yao Qi]

Nowadays, we set computed value's frame id, which is a misuse to me.
The computed value itself doesn't care about frame id, but function
value_computed_funcs (val)->read (or read_pieced_value) cares about
which frame the register is relative to, so 'struct piece_closure' is
a better place to fit frame id.

This patch adds a frame id in 'struct piece_closure', and use it
instead of using computed value's frame id.

gdb:

2016-11-24  Yao Qi  <yao.qi@linaro.org>

	* dwarf2loc.c (struct piece_closure) <frame_id>: New field.
	(allocate_piece_closure): Add new parameter 'frame' and set
	closure's frame_id field accordingly.
	(read_pieced_value): Get frame from closure instead of value.
	(dwarf2_evaluate_loc_desc_full): Remove code getting frame id.
	Don't set value's frame id.
---
 gdb/dwarf2loc.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 44dceda..872d033 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1464,6 +1464,10 @@ struct piece_closure
 
   /* The pieces themselves.  */
   struct dwarf_expr_piece *pieces;
+
+  /* Frame ID of frame to which a register value is relative, used
+     only by DWARF_VALUE_REGISTER.  */
+  struct frame_id frame_id;
 };
 
 /* Allocate a closure for a value formed from separately-described
@@ -1472,7 +1476,7 @@ struct piece_closure
 static struct piece_closure *
 allocate_piece_closure (struct dwarf2_per_cu_data *per_cu,
 			int n_pieces, struct dwarf_expr_piece *pieces,
-			int addr_size)
+			int addr_size, struct frame_info *frame)
 {
   struct piece_closure *c = XCNEW (struct piece_closure);
   int i;
@@ -1482,6 +1486,10 @@ allocate_piece_closure (struct dwarf2_per_cu_data *per_cu,
   c->n_pieces = n_pieces;
   c->addr_size = addr_size;
   c->pieces = XCNEWVEC (struct dwarf_expr_piece, n_pieces);
+  if (frame == NULL)
+    c->frame_id = null_frame_id;
+  else
+    c->frame_id = get_frame_id (get_next_frame_sentinel_okay (frame));
 
   memcpy (c->pieces, pieces, n_pieces * sizeof (struct dwarf_expr_piece));
   for (i = 0; i < n_pieces; ++i)
@@ -1672,18 +1680,12 @@ read_pieced_value (struct value *v)
   gdb_byte *contents;
   struct piece_closure *c
     = (struct piece_closure *) value_computed_closure (v);
-  struct frame_info *frame;
   size_t type_len;
   size_t buffer_size = 0;
   std::vector<gdb_byte> buffer;
   int bits_big_endian
     = gdbarch_bits_big_endian (get_type_arch (value_type (v)));
 
-  /* VALUE_FRAME_ID is used instead of VALUE_NEXT_FRAME_ID here
-     because FRAME is passed to get_frame_register_bytes(), which
-     does its own "->next" operation.  */
-  frame = frame_find_by_id (VALUE_FRAME_ID (v));
-
   if (value_type (v) != value_enclosing_type (v))
     internal_error (__FILE__, __LINE__,
 		    _("Should not be able to create a lazy value with "
@@ -1743,6 +1745,8 @@ read_pieced_value (struct value *v)
 	{
 	case DWARF_VALUE_REGISTER:
 	  {
+	    struct frame_info *frame
+	      = frame_find_by_id (get_prev_frame_id_by_id (c->frame_id));
 	    struct gdbarch *arch = get_frame_arch (frame);
 	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno);
 	    int optim, unavail;
@@ -2311,10 +2315,6 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
   if (ctx.num_pieces > 0)
     {
       struct piece_closure *c;
-      struct frame_id frame_id
-        = frame == NULL 
-	  ? null_frame_id
-	  : get_frame_id (get_next_frame_sentinel_okay (frame));
       ULONGEST bit_size = 0;
       int i;
 
@@ -2324,12 +2324,11 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
 	invalid_synthetic_pointer ();
 
       c = allocate_piece_closure (per_cu, ctx.num_pieces, ctx.pieces,
-				  ctx.addr_size);
+				  ctx.addr_size, frame);
       /* We must clean up the value chain after creating the piece
 	 closure but before allocating the result.  */
       do_cleanups (value_chain);
       retval = allocate_computed_value (type, &pieced_value_funcs, c);
-      VALUE_NEXT_FRAME_ID (retval) = frame_id;
       set_value_offset (retval, byte_offset);
     }
   else
-- 
1.9.1

[PATCH 2/3] Adjust Value.location for lval_register

[Yao Qi]

value.regnum and value.next_frame_id are only used for lval_register,
so this patch moves them to union value.location.  As a result, when
we copy value, only copy location, don't need to copy regnum and
next_frame_id.

gdb:

2016-11-23  Yao Qi  <yao.qi@linaro.org>

	* valops.c (value_slice): Don't set frame id of slice.
	* value.c (struct value) <regnum, next_frame_id>: Move them to...
	(struct value) <location>: ... here.  Update comments.
	(allocate_value_lazy): Don't set frame id and regnum.
	(deprecated_value_next_frame_id_hack): Adjust.
	(deprecated_value_regnum_hack): Adjust.
	(value_copy): Don't copy frame id and regnu.
	(value_primitive_field): Likewise.
	(value_from_component): Likewise.
---
 gdb/valops.c |  1 -
 gdb/value.c  | 40 +++++++++++++++-------------------------
 2 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/gdb/valops.c b/gdb/valops.c
index 8a45641..3a7550d 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -3827,7 +3827,6 @@ value_slice (struct value *array, int lowbound, int length)
       }
 
     set_value_component_location (slice, array);
-    VALUE_NEXT_FRAME_ID (slice) = VALUE_NEXT_FRAME_ID (array);
     set_value_offset (slice, value_offset (array) + offset);
   }
 
diff --git a/gdb/value.c b/gdb/value.c
index 8d33501..eff5462 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -205,17 +205,23 @@ struct value
   /* If the value has been released.  */
   unsigned int released : 1;
 
-  /* Register number if the value is from a register.  */
-  short regnum;
-
   /* 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.  */
+    /* If lval == lval_memory, this is the address in the inferior  */
     CORE_ADDR address;
 
+    /*If lval == lval_register, the value is from a register.  */
+    struct
+    {
+      /* Register number.  */
+      short regnum;
+      /* Frame ID of "next" frame to which a register value is relative.
+	 If the register value is found relative to frame F, then the
+	 frame id of F->next will be stored in next_frame_id.  */
+      struct frame_id next_frame_id;
+    } reg;
+
     /* Pointer to internal variable.  */
     struct internalvar *internalvar;
 
@@ -237,9 +243,7 @@ struct value
 
   /* Describes offset of a value within lval of a structure in target
      addressable memory units.  If lval == lval_memory, this is an offset to
-     the address.  If lval == lval_register, this is a further offset from
-     location.address within the registers structure.  Note also the member
-     embedded_offset below.  */
+     the address.  Note also the member embedded_offset below.  */
   LONGEST offset;
 
   /* Only used for bitfields; number of bits contained in them.  */
@@ -262,12 +266,6 @@ struct value
      bitfields.  */
   struct value *parent;
 
-  /* Frame ID of "next" frame to which a register value is relative.  A
-     register value is indicated when the lval enum (above) is set to
-     lval_register.  So, if the register value is found relative to frame F,
-     then the frame id of F->next will be stored in next_frame_id.  */
-  struct frame_id next_frame_id;
-
   /* Type of the value.  */
   struct type *type;
 
@@ -945,11 +943,9 @@ allocate_value_lazy (struct type *type)
   val->enclosing_type = type;
   VALUE_LVAL (val) = not_lval;
   val->location.address = 0;
-  VALUE_NEXT_FRAME_ID (val) = null_frame_id;
   val->offset = 0;
   val->bitpos = 0;
   val->bitsize = 0;
-  VALUE_REGNUM (val) = -1;
   val->lazy = 1;
   val->embedded_offset = 0;
   val->pointed_to_offset = 0;
@@ -1586,13 +1582,13 @@ deprecated_value_internalvar_hack (struct value *value)
 struct frame_id *
 deprecated_value_next_frame_id_hack (struct value *value)
 {
-  return &value->next_frame_id;
+  return &value->location.reg.next_frame_id;
 }
 
 short *
 deprecated_value_regnum_hack (struct value *value)
 {
-  return &value->regnum;
+  return &value->location.reg.regnum;
 }
 
 int
@@ -1788,8 +1784,6 @@ value_copy (struct value *arg)
   val->offset = arg->offset;
   val->bitpos = arg->bitpos;
   val->bitsize = arg->bitsize;
-  VALUE_NEXT_FRAME_ID (val) = VALUE_NEXT_FRAME_ID (arg);
-  VALUE_REGNUM (val) = VALUE_REGNUM (arg);
   val->lazy = arg->lazy;
   val->embedded_offset = value_embedded_offset (arg);
   val->pointed_to_offset = arg->pointed_to_offset;
@@ -3229,8 +3223,6 @@ value_primitive_field (struct value *arg1, LONGEST offset,
 		   + value_embedded_offset (arg1));
     }
   set_value_component_location (v, arg1);
-  VALUE_REGNUM (v) = VALUE_REGNUM (arg1);
-  VALUE_NEXT_FRAME_ID (v) = VALUE_NEXT_FRAME_ID (arg1);
   return v;
 }
 
@@ -3814,8 +3806,6 @@ value_from_component (struct value *whole, struct type *type, LONGEST offset)
     }
   v->offset = value_offset (whole) + offset + value_embedded_offset (whole);
   set_value_component_location (v, whole);
-  VALUE_REGNUM (v) = VALUE_REGNUM (whole);
-  VALUE_NEXT_FRAME_ID (v) = VALUE_NEXT_FRAME_ID (whole);
 
   return v;
 }
-- 
1.9.1

Re: [PATCH 1/3] Move computed value's frame id to piece_closure

[Ulrich Weigand]

Yao Qi wrote:

> +  if (frame == NULL)
> +    c->frame_id = null_frame_id;
> +  else
> +    c->frame_id = get_frame_id (get_next_frame_sentinel_okay (frame));
[...]
> +	    struct frame_info *frame
> +	      = frame_find_by_id (get_prev_frame_id_by_id (c->frame_id));

I guess now that this is a separate field, there's no longer a reason
to go via next-frame / prev-frame here.  It looks like this was introduced
when VALUE_FRAME_ID was changed to VALUE_NEXT_FRAME_ID, but this isn't
really necessary for the piece logic here.  I think this can simply be:

    c->frame_id = get_frame_id (frame);
[...]
	    struct frame_info *frame = frame_find_by_id (c->frame_id);

instead.

Otherwise, this looks good to me.

Bye,
Ulrich

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

Re: [PATCH 2/3] Adjust Value.location for lval_register

[Ulrich Weigand]

Yao Qi wrote:

> +      /* Register number.  */
> +      short regnum;

Now this is no longer space-constrained (the location union is two pointers
in size anyway), so this can just be an "int" like regnums are elsewhere.

> +      /* Frame ID of "next" frame to which a register value is relative.
> +	 If the register value is found relative to frame F, then the
> +	 frame id of F->next will be stored in next_frame_id.  */
> +      struct frame_id next_frame_id;
> +    } reg;


>    /* Describes offset of a value within lval of a structure in target
>       addressable memory units.  If lval == lval_memory, this is an offset to
> -     the address.  If lval == lval_register, this is a further offset from
> -     location.address within the registers structure.  Note also the member
> -     embedded_offset below.  */
> +     the address.  Note also the member embedded_offset below.  */
>    LONGEST offset;

Hmm, I think we recently had the discussion that *any* values should allow
using an offset.  The comment should probably reflect this.


Otherwise, this looks good to me.

Bye,
Ulrich

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

Re: [PATCH 3/3] Restrict checking value.lval on using address

[Ulrich Weigand]

Yao Qi wrote:

> 	* ada-lang.c (ensure_lval): Call set_value_address after setting
> 	VALUE_LVAL.
> 	* elfread.c (elf_gnu_ifunc_resolve_addr): Set VALUE_LVAL to
> 	lval_memory.
> 	(elf_gnu_ifunc_resolver_return_stop): Likewise.
> 	* value.c (value_fn_field): Likewise.
> 	(value_from_contents_and_address_unresolved): Likewise.
> 	(value_from_contents_and_address): Likewise.
> 	(value_address): Check value->lval isn't
> 	lval_memory.
> 	(value_raw_address): Likewise.
> 	(set_value_address): Assert value->lval is lval_memory.

Looks good to me.

Thanks,
Ulrich

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

Re: [PATCH 2/3] Adjust Value.location for lval_register

[Yao Qi]

On Fri, Nov 25, 2016 at 12:51:30PM +0100, Ulrich Weigand wrote:
> 
> 
> >    /* Describes offset of a value within lval of a structure in target
> >       addressable memory units.  If lval == lval_memory, this is an offset to
> > -     the address.  If lval == lval_register, this is a further offset from
> > -     location.address within the registers structure.  Note also the member
> > -     embedded_offset below.  */
> > +     the address.  Note also the member embedded_offset below.  */
> >    LONGEST offset;
> 
> Hmm, I think we recently had the discussion that *any* values should allow
> using an offset.  The comment should probably reflect this.
> 

How about "Describes offset of a value within lval of a structure.  Note 
also the member embedded_offset below."?

-- 
Yao (齐尧)

Re: [PATCH 2/3] Adjust Value.location for lval_register

[Ulrich Weigand]

> On Fri, Nov 25, 2016 at 12:51:30PM +0100, Ulrich Weigand wrote:
> > 
> > 
> > >    /* Describes offset of a value within lval of a structure in target
> > >       addressable memory units.  If lval == lval_memory, this is an offset to
> > > -     the address.  If lval == lval_register, this is a further offset from
> > > -     location.address within the registers structure.  Note also the member
> > > -     embedded_offset below.  */
> > > +     the address.  Note also the member embedded_offset below.  */
> > >    LONGEST offset;
> > 
> > Hmm, I think we recently had the discussion that *any* values should allow
> > using an offset.  The comment should probably reflect this.
> > 
> 
> How about "Describes offset of a value within lval of a structure.  Note 
> also the member embedded_offset below."?

I think we should leave in the "in target addressable memory units" to clarify
that this is a *byte* offset, not a bit offset.

Otherwise, looks good to me.

Bye,
Ulrich

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

Re: [PATCH 1/3] Move computed value's frame id to piece_closure

[Yao Qi]

On Fri, Nov 25, 2016 at 12:48:36PM +0100, Ulrich Weigand wrote:
> Yao Qi wrote:
> 
> > +  if (frame == NULL)
> > +    c->frame_id = null_frame_id;
> > +  else
> > +    c->frame_id = get_frame_id (get_next_frame_sentinel_okay (frame));
> [...]
> > +	    struct frame_info *frame
> > +	      = frame_find_by_id (get_prev_frame_id_by_id (c->frame_id));
> 
> I guess now that this is a separate field, there's no longer a reason
> to go via next-frame / prev-frame here.  It looks like this was introduced
> when VALUE_FRAME_ID was changed to VALUE_NEXT_FRAME_ID, but this isn't
> really necessary for the piece logic here.  I think this can simply be:
> 
>     c->frame_id = get_frame_id (frame);
> [...]
> 	    struct frame_info *frame = frame_find_by_id (c->frame_id);
> 
> instead.
> 
> Otherwise, this looks good to me.
> 

Patch below is pushed in.

-- 
Yao (齐尧)

From ee40d8d45213caf0cfb63e603f0fd5a58532e751 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Mon, 28 Nov 2016 17:09:26 +0000
Subject: [PATCH] Move computed value's frame id to piece_closure

Nowadays, we set computed value's frame id, which is a misuse to me.
The computed value itself doesn't care about frame id, but function
value_computed_funcs (val)->read (or read_pieced_value) cares about
which frame the register is relative to, so 'struct piece_closure' is
a better place to fit frame id.

This patch adds a frame id in 'struct piece_closure', and use it
instead of using computed value's frame id.

gdb:

2016-11-28  Yao Qi  <yao.qi@linaro.org>

	* dwarf2loc.c (struct piece_closure) <frame_id>: New field.
	(allocate_piece_closure): Add new parameter 'frame' and set
	closure's frame_id field accordingly.
	(read_pieced_value): Get frame from closure instead of value.
	(dwarf2_evaluate_loc_desc_full): Remove code getting frame id.
	Don't set value's frame id.

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 128f654..aaf88e4 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1465,6 +1465,10 @@ struct piece_closure
 
   /* The pieces themselves.  */
   struct dwarf_expr_piece *pieces;
+
+  /* Frame ID of frame to which a register value is relative, used
+     only by DWARF_VALUE_REGISTER.  */
+  struct frame_id frame_id;
 };
 
 /* Allocate a closure for a value formed from separately-described
@@ -1473,7 +1477,7 @@ struct piece_closure
 static struct piece_closure *
 allocate_piece_closure (struct dwarf2_per_cu_data *per_cu,
 			int n_pieces, struct dwarf_expr_piece *pieces,
-			int addr_size)
+			int addr_size, struct frame_info *frame)
 {
   struct piece_closure *c = XCNEW (struct piece_closure);
   int i;
@@ -1483,6 +1487,10 @@ allocate_piece_closure (struct dwarf2_per_cu_data *per_cu,
   c->n_pieces = n_pieces;
   c->addr_size = addr_size;
   c->pieces = XCNEWVEC (struct dwarf_expr_piece, n_pieces);
+  if (frame == NULL)
+    c->frame_id = null_frame_id;
+  else
+    c->frame_id = get_frame_id (frame);
 
   memcpy (c->pieces, pieces, n_pieces * sizeof (struct dwarf_expr_piece));
   for (i = 0; i < n_pieces; ++i)
@@ -1731,18 +1739,12 @@ read_pieced_value (struct value *v)
   gdb_byte *contents;
   struct piece_closure *c
     = (struct piece_closure *) value_computed_closure (v);
-  struct frame_info *frame;
   size_t type_len;
   size_t buffer_size = 0;
   std::vector<gdb_byte> buffer;
   int bits_big_endian
     = gdbarch_bits_big_endian (get_type_arch (value_type (v)));
 
-  /* VALUE_FRAME_ID is used instead of VALUE_NEXT_FRAME_ID here
-     because FRAME is passed to get_frame_register_bytes(), which
-     does its own "->next" operation.  */
-  frame = frame_find_by_id (VALUE_FRAME_ID (v));
-
   if (value_type (v) != value_enclosing_type (v))
     internal_error (__FILE__, __LINE__,
 		    _("Should not be able to create a lazy value with "
@@ -1802,6 +1804,7 @@ read_pieced_value (struct value *v)
 	{
 	case DWARF_VALUE_REGISTER:
 	  {
+	    struct frame_info *frame = frame_find_by_id (c->frame_id);
 	    struct gdbarch *arch = get_frame_arch (frame);
 	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno);
 	    int optim, unavail;
@@ -2370,10 +2373,6 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
   if (ctx.num_pieces > 0)
     {
       struct piece_closure *c;
-      struct frame_id frame_id
-        = frame == NULL 
-	  ? null_frame_id
-	  : get_frame_id (get_next_frame_sentinel_okay (frame));
       ULONGEST bit_size = 0;
       int i;
 
@@ -2383,12 +2382,11 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
 	invalid_synthetic_pointer ();
 
       c = allocate_piece_closure (per_cu, ctx.num_pieces, ctx.pieces,
-				  ctx.addr_size);
+				  ctx.addr_size, frame);
       /* We must clean up the value chain after creating the piece
 	 closure but before allocating the result.  */
       do_cleanups (value_chain);
       retval = allocate_computed_value (type, &pieced_value_funcs, c);
-      VALUE_NEXT_FRAME_ID (retval) = frame_id;
       set_value_offset (retval, byte_offset);
     }
   else

Re: [PATCH 3/3] Restrict checking value.lval on using address

[Yao Qi]

On Fri, Nov 25, 2016 at 12:52:10PM +0100, Ulrich Weigand wrote:
> Yao Qi wrote:
> 
> > 	* ada-lang.c (ensure_lval): Call set_value_address after setting
> > 	VALUE_LVAL.
> > 	* elfread.c (elf_gnu_ifunc_resolve_addr): Set VALUE_LVAL to
> > 	lval_memory.
> > 	(elf_gnu_ifunc_resolver_return_stop): Likewise.
> > 	* value.c (value_fn_field): Likewise.
> > 	(value_from_contents_and_address_unresolved): Likewise.
> > 	(value_from_contents_and_address): Likewise.
> > 	(value_address): Check value->lval isn't
> > 	lval_memory.
> > 	(value_raw_address): Likewise.
> > 	(set_value_address): Assert value->lval is lval_memory.
> 
> Looks good to me.
> 

Patch is pushed in.

-- 
Yao (齐尧)

Re: [PATCH 2/3] Adjust Value.location for lval_register

[Yao Qi]

On Fri, Nov 25, 2016 at 01:10:02PM +0100, Ulrich Weigand wrote:
> > On Fri, Nov 25, 2016 at 12:51:30PM +0100, Ulrich Weigand wrote:
> > > 
> > > 
> > > >    /* Describes offset of a value within lval of a structure in target
> > > >       addressable memory units.  If lval == lval_memory, this is an offset to
> > > > -     the address.  If lval == lval_register, this is a further offset from
> > > > -     location.address within the registers structure.  Note also the member
> > > > -     embedded_offset below.  */
> > > > +     the address.  Note also the member embedded_offset below.  */
> > > >    LONGEST offset;
> > > 
> > > Hmm, I think we recently had the discussion that *any* values should allow
> > > using an offset.  The comment should probably reflect this.
> > > 
> > 
> > How about "Describes offset of a value within lval of a structure.  Note 
> > also the member embedded_offset below."?
> 
> I think we should leave in the "in target addressable memory units" to clarify
> that this is a *byte* offset, not a bit offset.
> 
> Otherwise, looks good to me.
> 

OK, patch below is pushed in.

-- 
Yao (齐尧)

From 7dc54575d91a2b41f6c3e838eec44a7017a24436 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Mon, 28 Nov 2016 17:09:26 +0000
Subject: [PATCH] Adjust Value.location for lval_register

value.regnum and value.next_frame_id are only used for lval_register,
so this patch moves them to union value.location.  As a result, when
we copy value, only copy location, don't need to copy regnum and
next_frame_id.

This patch also changes regnum's type to int as there is no space
constraint, so update deprecated_value_regnum_hack return type too.

gdb:

2016-11-28  Yao Qi  <yao.qi@linaro.org>

	* valops.c (value_slice): Don't set frame id of slice.
	* value.c (struct value) <regnum, next_frame_id>: Move them to...
	(struct value) <location>: ... here.  Update comments.
	(allocate_value_lazy): Don't set frame id and regnum.
	(deprecated_value_next_frame_id_hack): Adjust.
	(deprecated_value_regnum_hack): Adjust.
	(value_copy): Don't copy frame id and regnu.
	(value_primitive_field): Likewise.
	(value_from_component): Likewise.
	(deprecated_value_regnum_hack): Return int *.
	* value.h (deprecated_value_regnum_hack): Update declaration.

diff --git a/gdb/valops.c b/gdb/valops.c
index 8a45641..3a7550d 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -3827,7 +3827,6 @@ value_slice (struct value *array, int lowbound, int length)
       }
 
     set_value_component_location (slice, array);
-    VALUE_NEXT_FRAME_ID (slice) = VALUE_NEXT_FRAME_ID (array);
     set_value_offset (slice, value_offset (array) + offset);
   }
 
diff --git a/gdb/value.c b/gdb/value.c
index 8d33501..13a0bb9 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -205,17 +205,23 @@ struct value
   /* If the value has been released.  */
   unsigned int released : 1;
 
-  /* Register number if the value is from a register.  */
-  short regnum;
-
   /* 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.  */
+    /* If lval == lval_memory, this is the address in the inferior  */
     CORE_ADDR address;
 
+    /*If lval == lval_register, the value is from a register.  */
+    struct
+    {
+      /* Register number.  */
+      int regnum;
+      /* Frame ID of "next" frame to which a register value is relative.
+	 If the register value is found relative to frame F, then the
+	 frame id of F->next will be stored in next_frame_id.  */
+      struct frame_id next_frame_id;
+    } reg;
+
     /* Pointer to internal variable.  */
     struct internalvar *internalvar;
 
@@ -236,10 +242,8 @@ struct value
   } location;
 
   /* Describes offset of a value within lval of a structure in target
-     addressable memory units.  If lval == lval_memory, this is an offset to
-     the address.  If lval == lval_register, this is a further offset from
-     location.address within the registers structure.  Note also the member
-     embedded_offset below.  */
+     addressable memory units.  Note also the member embedded_offset
+     below.  */
   LONGEST offset;
 
   /* Only used for bitfields; number of bits contained in them.  */
@@ -262,12 +266,6 @@ struct value
      bitfields.  */
   struct value *parent;
 
-  /* Frame ID of "next" frame to which a register value is relative.  A
-     register value is indicated when the lval enum (above) is set to
-     lval_register.  So, if the register value is found relative to frame F,
-     then the frame id of F->next will be stored in next_frame_id.  */
-  struct frame_id next_frame_id;
-
   /* Type of the value.  */
   struct type *type;
 
@@ -945,11 +943,9 @@ allocate_value_lazy (struct type *type)
   val->enclosing_type = type;
   VALUE_LVAL (val) = not_lval;
   val->location.address = 0;
-  VALUE_NEXT_FRAME_ID (val) = null_frame_id;
   val->offset = 0;
   val->bitpos = 0;
   val->bitsize = 0;
-  VALUE_REGNUM (val) = -1;
   val->lazy = 1;
   val->embedded_offset = 0;
   val->pointed_to_offset = 0;
@@ -1586,13 +1582,13 @@ deprecated_value_internalvar_hack (struct value *value)
 struct frame_id *
 deprecated_value_next_frame_id_hack (struct value *value)
 {
-  return &value->next_frame_id;
+  return &value->location.reg.next_frame_id;
 }
 
-short *
+int *
 deprecated_value_regnum_hack (struct value *value)
 {
-  return &value->regnum;
+  return &value->location.reg.regnum;
 }
 
 int
@@ -1788,8 +1784,6 @@ value_copy (struct value *arg)
   val->offset = arg->offset;
   val->bitpos = arg->bitpos;
   val->bitsize = arg->bitsize;
-  VALUE_NEXT_FRAME_ID (val) = VALUE_NEXT_FRAME_ID (arg);
-  VALUE_REGNUM (val) = VALUE_REGNUM (arg);
   val->lazy = arg->lazy;
   val->embedded_offset = value_embedded_offset (arg);
   val->pointed_to_offset = arg->pointed_to_offset;
@@ -3229,8 +3223,6 @@ value_primitive_field (struct value *arg1, LONGEST offset,
 		   + value_embedded_offset (arg1));
     }
   set_value_component_location (v, arg1);
-  VALUE_REGNUM (v) = VALUE_REGNUM (arg1);
-  VALUE_NEXT_FRAME_ID (v) = VALUE_NEXT_FRAME_ID (arg1);
   return v;
 }
 
@@ -3814,8 +3806,6 @@ value_from_component (struct value *whole, struct type *type, LONGEST offset)
     }
   v->offset = value_offset (whole) + offset + value_embedded_offset (whole);
   set_value_component_location (v, whole);
-  VALUE_REGNUM (v) = VALUE_REGNUM (whole);
-  VALUE_NEXT_FRAME_ID (v) = VALUE_NEXT_FRAME_ID (whole);
 
   return v;
 }
diff --git a/gdb/value.h b/gdb/value.h
index 281b5a8..f776323 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -448,7 +448,7 @@ extern struct frame_id *deprecated_value_next_frame_id_hack (struct value *);
 #define VALUE_FRAME_ID(val) (get_prev_frame_id_by_id (VALUE_NEXT_FRAME_ID (val)))
 
 /* Register number if the value is from a register.  */
-extern short *deprecated_value_regnum_hack (struct value *);
+extern int *deprecated_value_regnum_hack (struct value *);
 #define VALUE_REGNUM(val) (*deprecated_value_regnum_hack (val))
 
 /* Return value after lval_funcs->coerce_ref (after check_typedef).  Return