[PATCH] Assert on lval_register

[PATCH] Assert on lval_register

[Yao Qi]

This patch adds asserts where the value's lval must be lval_register.
This triggers an error in frame_register_unwind because VALUE_REGNUM
is used but value's lval is not lval_register.

This also reveals a design issue in frame_register_unwind, that is
arguments addrp and realnump are mutually exclusive, we either use
addrp (for lval_memory), or use realnump (for lval_register).  This
can be done in a separate patch.

Regression tested on x86_64-linux.

gdb:

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

	* frame.c (frame_register_unwind): Set *realnump if *lvalp is
	lval_register.
	* value.c (deprecated_value_next_frame_id_hack): Assert
	value->lval is lval_register.
	(deprecated_value_regnum_hack): Likewise.
---
 gdb/frame.c | 3 ++-
 gdb/value.c | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 5414cb3..1c388d1 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1107,7 +1107,8 @@ frame_register_unwind (struct frame_info *frame, int regnum,
   *unavailablep = !value_entirely_available (value);
   *lvalp = VALUE_LVAL (value);
   *addrp = value_address (value);
-  *realnump = VALUE_REGNUM (value);
+  if (*lvalp == lval_register)
+    *realnump = VALUE_REGNUM (value);
 
   if (bufferp)
     {
diff --git a/gdb/value.c b/gdb/value.c
index cc291cf..022900f 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1576,12 +1576,14 @@ deprecated_value_internalvar_hack (struct value *value)
 struct frame_id *
 deprecated_value_next_frame_id_hack (struct value *value)
 {
+  gdb_assert (value->lval == lval_register);
   return &value->location.reg.next_frame_id;
 }
 
 int *
 deprecated_value_regnum_hack (struct value *value)
 {
+  gdb_assert (value->lval == lval_register);
   return &value->location.reg.regnum;
 }
 
-- 
1.9.1

Re: [PATCH] Assert on lval_register

[Ulrich Weigand]

Yao Qi wrote:

> This patch adds asserts where the value's lval must be lval_register.
> This triggers an error in frame_register_unwind because VALUE_REGNUM
> is used but value's lval is not lval_register.

Makes sense to me.
 
> This also reveals a design issue in frame_register_unwind, that is
> arguments addrp and realnump are mutually exclusive, we either use
> addrp (for lval_memory), or use realnump (for lval_register).  This
> can be done in a separate patch.

I think we should simply get rid of frame_register_unwind.  Callers
should be changed to use frame_unwind_register_value directly, and
just operate on the value.

>    *lvalp = VALUE_LVAL (value);
>    *addrp = value_address (value);
> -  *realnump = VALUE_REGNUM (value);
> +  if (*lvalp == lval_register)
> +    *realnump = VALUE_REGNUM (value);

But as long as the above change is not done yet, maybe we ought to
at least provide a defined value (e.g. -1), to avoid callers maybe
making use of uninitialized variables?

Bye,
Ulrich

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

Re: [PATCH] Assert on lval_register

[Yao Qi]

On 16-11-30 22:16:37, Ulrich Weigand wrote:
 
> > This also reveals a design issue in frame_register_unwind, that is
> > arguments addrp and realnump are mutually exclusive, we either use
> > addrp (for lval_memory), or use realnump (for lval_register).  This
> > can be done in a separate patch.
> 
> I think we should simply get rid of frame_register_unwind.  Callers
> should be changed to use frame_unwind_register_value directly, and
> just operate on the value.
> 

Yeah, that is what I was trying to do, but we should be careful on
the value management in the end of frame_register_unwind,

  /* Dispose of the new value.  This prevents watchpoints from
     trying to watch the saved frame pointer.  */
  release_value (value);
  value_free (value);

> >    *lvalp = VALUE_LVAL (value);
> >    *addrp = value_address (value);
> > -  *realnump = VALUE_REGNUM (value);
> > +  if (*lvalp == lval_register)
> > +    *realnump = VALUE_REGNUM (value);
> 
> But as long as the above change is not done yet, maybe we ought to
> at least provide a defined value (e.g. -1), to avoid callers maybe
> making use of uninitialized variables?
> 

OK, how about the patch below?

-- 
Yao (齐尧)

From 581e1b80ea631d5bcfb5fc64b07a36fd71f5d55e Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Thu, 24 Nov 2016 09:08:00 +0000
Subject: [PATCH] Assert on lval_register

This patch adds asserts where the value's lval must be lval_register.
This triggers an error in frame_register_unwind because VALUE_REGNUM
is used but value's lval is not lval_register.

This also reveals a design issue in frame_register_unwind, that is
arguments addrp and realnump are mutually exclusive, we either use
addrp (for lval_memory), or use realnump (for lval_register).  This
can be done in a separate patch.

gdb:

2016-12-06  Yao Qi  <yao.qi@linaro.org>

	* frame.c (frame_register_unwind): Set *realnump if *lvalp is
	lval_register.
	* value.c (deprecated_value_next_frame_id_hack): Assert
	value->lval is lval_register.
	(deprecated_value_regnum_hack): Likewise.

diff --git a/gdb/frame.c b/gdb/frame.c
index 5414cb3..00001bc 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1107,7 +1107,10 @@ frame_register_unwind (struct frame_info *frame, int regnum,
   *unavailablep = !value_entirely_available (value);
   *lvalp = VALUE_LVAL (value);
   *addrp = value_address (value);
-  *realnump = VALUE_REGNUM (value);
+  if (*lvalp == lval_register)
+    *realnump = VALUE_REGNUM (value);
+  else
+    *realnump = -1;
 
   if (bufferp)
     {
diff --git a/gdb/value.c b/gdb/value.c
index cc291cf..022900f 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1576,12 +1576,14 @@ deprecated_value_internalvar_hack (struct value *value)
 struct frame_id *
 deprecated_value_next_frame_id_hack (struct value *value)
 {
+  gdb_assert (value->lval == lval_register);
   return &value->location.reg.next_frame_id;
 }
 
 int *
 deprecated_value_regnum_hack (struct value *value)
 {
+  gdb_assert (value->lval == lval_register);
   return &value->location.reg.regnum;
 }
 

Re: [PATCH] Assert on lval_register

[Ulrich Weigand]

Yao Qi wrote:

> On 16-11-30 22:16:37, Ulrich Weigand wrote:
> > I think we should simply get rid of frame_register_unwind.  Callers
> > should be changed to use frame_unwind_register_value directly, and
> > just operate on the value.
> 
> Yeah, that is what I was trying to do, but we should be careful on
> the value management in the end of frame_register_unwind,
> 
>   /* Dispose of the new value.  This prevents watchpoints from
>      trying to watch the saved frame pointer.  */
>   release_value (value);
>   value_free (value);

Right.  Those callers that care will need to handle this themselves.

> OK, how about the patch below?

Looks good to me.

Thanks,
Ulrich

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

Re: [PATCH] Assert on lval_register

[Yao Qi]

On 16-12-06 14:13:28, Ulrich Weigand wrote:
> 
> > OK, how about the patch below?
> 
> Looks good to me.
> 

Patch is pushed in. 

-- 
Yao (齐尧)