[PATCH] Print <unavailable> for unavailable registers

[PATCH] Print <unavailable> for unavailable registers

[Andrew Burgess]

Following on from (but unrelated to) this mail:
  http://sourceware.org/ml/gdb-patches/2013-08/msg00170.html

Printing "*value not available*" for unavailable values within
"info registers" seems inconsistent to me, if we just print an
unavailable register we'll get "<unavailable>".

The patch below makes "info registers" print "<unavailable>".

Taking a look around we also had code in the sh64-tdep.c file that
printed "*value not available*", I've changed this to also print
"<unavailable>", however, (1) this is being printed for unavailable,
and optimized out registers, but it was kind-of broken in this regard
already, (2) I don't use this target so don't know if this is really
the best change, but it felt like it probably was, and (3) I've also
made the sh64 code return early after printing "<unavailable>", it
previously fell through and tried to print the value anyway, I'm
guessing this was a bug, but the code has been that way for a long
time now.... I'd be happy to drop the sh64 changes if that was
preferred.

OK to apply?

Thanks,
Andrew

gdb/ChangeLog

2013-08-06  Andrew Burgess  <aburgess@broadcom.com>

	* infcmd.c (default_print_one_register_info): use
	val_print_unavailable for unavailable values.
	* sh64-tdep.c (sh64_do_register): Use val_print_unavailable for
	unreadable registers.  Return early for unreadable registers.

gdb/testsuite/ChangeLog

2013-08-06  Andrew Burgess  <aburgess@broadcom.com>

	* gdb.trace/unavailable.exp (gdb_unavailable_registers_test):
	Change expected pattern for unavailable registers to
	<unavailable>.


diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index f6a5290..297dad0 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2030,7 +2030,8 @@ default_print_one_register_info (struct ui_file *file,
 
   if (!value_entirely_available (val))
     {
-      fprintf_filtered (file, "*value not available*\n");
+      val_print_unavailable (file);
+      fprintf_filtered (file, "\n");
       return;
     }
   else if (value_optimized_out (val))
diff --git a/gdb/sh64-tdep.c b/gdb/sh64-tdep.c
index b640b1d..37fb72f 100644
--- a/gdb/sh64-tdep.c
+++ b/gdb/sh64-tdep.c
@@ -2045,7 +2045,11 @@ sh64_do_register (struct gdbarch *gdbarch, struct ui_file *file,
 
   /* Get the data in raw format.  */
   if (!deprecated_frame_register_read (frame, regnum, raw_buffer))
-    fprintf_filtered (file, "*value not available*\n");
+    {
+      val_print_unavailable (file);
+      fprintf_filtered (file, "\n");
+      return;
+    }
 
   get_formatted_print_options (&opts, 'x');
   opts.deref_ref = 1;
diff --git a/gdb/testsuite/gdb.trace/unavailable.exp b/gdb/testsuite/gdb.trace/unavailable.exp
index 8e2e105..ed14798 100644
--- a/gdb/testsuite/gdb.trace/unavailable.exp
+++ b/gdb/testsuite/gdb.trace/unavailable.exp
@@ -297,11 +297,11 @@ proc gdb_unavailable_registers_test { } {
 	test_register "\$pc"
 
 	gdb_test "info registers" \
-	    "\\*value not available\\*.*\\*value not available\\*" \
+	    "<unavailable>.*<unavailable>" \
 	    "info registers, multiple registers not available"
 
 	gdb_test "info registers \$$spreg" \
-	    "\\*value not available\\*" \
+	    "<unavailable>" \
 	    "info registers \$$spreg reports not available"
 
 	gdb_test "tfind none" "#0  end .*" "cease trace debugging"

Re: PING: [PATCH] Print <unavailable> for unavailable registers

[Andrew Burgess]

On 06/08/2013 2:09 PM, Andrew Burgess wrote:
> Following on from (but unrelated to) this mail:
>   http://sourceware.org/ml/gdb-patches/2013-08/msg00170.html
> 
> Printing "*value not available*" for unavailable values within
> "info registers" seems inconsistent to me, if we just print an
> unavailable register we'll get "<unavailable>".
> 
> The patch below makes "info registers" print "<unavailable>".

The original patch was supposed to be applied after [1],
however I never managed to get [1] accepted.  I've flipped
the ordering, so that this patch can be applied first, if
this gets merged I'll post a revised version of [1] later,
just for the record.

To my reading, Mark seems to imply in this mail:
  http://sourceware.org/ml/gdb-patches/2013-08/msg00322.html
that he does not disagree with displaying registers as
"<unavailable>", so I feel it's acceptable to push this
patch forward.

After this patch "info registers" will display unavailable
(for example, not collected by tracing) as <unavailable>,
while optimized out register values will still be displayed
using the old "*value not available*" string.

The previous version of this patch included some sh64 changes,
I've removed them from this patch to try and increase the chance
of acceptance.

[1] http://sourceware.org/ml/gdb-patches/2013-08/msg00170.html

OK?
Andrew


gdb/ChangeLog

2013-08-19  Andrew Burgess  <aburgess@broadcom.com>

	* infcmd.c (default_print_one_register_info): use
	val_print_unavailable for unavailable register values, continue as
	before for optimized out register values.
	(default_print_registers_info): Switch to using
	get_frame_register_value.

gdb/testsuite/ChangeLog

2013-08-19  Andrew Burgess  <aburgess@broadcom.com>

	* gdb.trace/unavailable.exp (gdb_unavailable_registers_test):
	Change expected pattern for unavailable registers to
	<unavailable>.

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 154cde2..83e0323 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2030,6 +2030,12 @@ default_print_one_register_info (struct ui_file *file,
 
   if (!value_entirely_available (val))
     {
+      val_print_unavailable (file);
+      fprintf_filtered (file, "\n");
+      return;
+    }
+  else if (value_optimized_out (val))
+    {
       fprintf_filtered (file, "*value not available*\n");
       return;
     }
@@ -2107,9 +2113,6 @@ default_print_registers_info (struct gdbarch *gdbarch,
 
   for (i = 0; i < numregs; i++)
     {
-      struct type *regtype;
-      struct value *val;
-
       /* Decide between printing all regs, non-float / vector regs, or
          specific reg.  */
       if (regnum == -1)
@@ -2137,16 +2140,9 @@ default_print_registers_info (struct gdbarch *gdbarch,
 	  || *(gdbarch_register_name (gdbarch, i)) == '\0')
 	continue;
 
-      regtype = register_type (gdbarch, i);
-      val = allocate_value (regtype);
-
-      /* Get the data in raw format.  */
-      if (! deprecated_frame_register_read (frame, i, value_contents_raw (val)))
-	mark_value_bytes_unavailable (val, 0, TYPE_LENGTH (value_type (val)));
-
       default_print_one_register_info (file,
 				       gdbarch_register_name (gdbarch, i),
-				       val);
+				       get_frame_register_value (frame, i));
     }
 }
 
diff --git a/gdb/testsuite/gdb.trace/unavailable.exp b/gdb/testsuite/gdb.trace/unavailable.exp
index 8e2e105..ed14798 100644
--- a/gdb/testsuite/gdb.trace/unavailable.exp
+++ b/gdb/testsuite/gdb.trace/unavailable.exp
@@ -297,11 +297,11 @@ proc gdb_unavailable_registers_test { } {
 	test_register "\$pc"
 
 	gdb_test "info registers" \
-	    "\\*value not available\\*.*\\*value not available\\*" \
+	    "<unavailable>.*<unavailable>" \
 	    "info registers, multiple registers not available"
 
 	gdb_test "info registers \$$spreg" \
-	    "\\*value not available\\*" \
+	    "<unavailable>" \
 	    "info registers \$$spreg reports not available"
 
 	gdb_test "tfind none" "#0  end .*" "cease trace debugging"

PING: Re: [PATCH] Print <unavailable> for unavailable registers

[Andrew Burgess]

On 06/08/2013 2:09 PM, Andrew Burgess wrote:
> Following on from (but unrelated to) this mail:
>   http://sourceware.org/ml/gdb-patches/2013-08/msg00170.html
> 
> Printing "*value not available*" for unavailable values within
> "info registers" seems inconsistent to me, if we just print an
> unavailable register we'll get "<unavailable>".
> 
> The patch below makes "info registers" print "<unavailable>".

I've re-written this patch, here's the latest version.

OK to apply?

Thanks,
Andrew

gdb/ChangeLog

2013-09-18  Andrew Burgess  <aburgess@broadcom.com>

	* infcmd.c (default_print_one_register_info): Print unavailable
	registers using val_print_uavailable.

gdb/testsuite/ChangeLog

2013-09-18  Andrew Burgess  <aburgess@broadcom.com>

	* gdb.trace/unavailable.exp (gdb_unavailable_registers_test):
	Change pattern to expect <unavailable> string.
        
Index: ./gdb/infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.335
diff -u -p -r1.335 infcmd.c
--- ./gdb/infcmd.c	18 Sep 2013 14:02:31 -0000	1.335
+++ ./gdb/infcmd.c	18 Sep 2013 14:43:13 -0000
@@ -2030,7 +2030,8 @@ default_print_one_register_info (struct 
 
   if (!value_entirely_available (val))
     {
-      fprintf_filtered (file, "*value not available*\n");
+      val_print_unavailable (file);
+      fprintf_filtered (file, "\n");
       return;
     }
   else if (value_optimized_out (val))
Index: ./gdb/testsuite/gdb.trace/unavailable.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.trace/unavailable.exp,v
retrieving revision 1.23
diff -u -p -r1.23 unavailable.exp
--- ./gdb/testsuite/gdb.trace/unavailable.exp	15 Mar 2013 01:41:29 -0000	1.23
+++ ./gdb/testsuite/gdb.trace/unavailable.exp	18 Sep 2013 14:43:13 -0000
@@ -297,11 +297,11 @@ proc gdb_unavailable_registers_test { } 
 	test_register "\$pc"
 
 	gdb_test "info registers" \
-	    "\\*value not available\\*.*\\*value not available\\*" \
+	    "<unavailable>.*<unavailable>" \
 	    "info registers, multiple registers not available"
 
 	gdb_test "info registers \$$spreg" \
-	    "\\*value not available\\*" \
+	    "<unavailable>" \
 	    "info registers \$$spreg reports not available"
 
 	gdb_test "tfind none" "#0  end .*" "cease trace debugging"




> 
> Taking a look around we also had code in the sh64-tdep.c file that
> printed "*value not available*", I've changed this to also print
> "<unavailable>", however, (1) this is being printed for unavailable,
> and optimized out registers, but it was kind-of broken in this regard
> already, (2) I don't use this target so don't know if this is really
> the best change, but it felt like it probably was, and (3) I've also
> made the sh64 code return early after printing "<unavailable>", it
> previously fell through and tried to print the value anyway, I'm
> guessing this was a bug, but the code has been that way for a long
> time now.... I'd be happy to drop the sh64 changes if that was
> preferred.
> 
> OK to apply?
> 
> Thanks,
> Andrew
> 
> gdb/ChangeLog
> 
> 2013-08-06  Andrew Burgess  <aburgess@broadcom.com>
> 
> 	* infcmd.c (default_print_one_register_info): use
> 	val_print_unavailable for unavailable values.
> 	* sh64-tdep.c (sh64_do_register): Use val_print_unavailable for
> 	unreadable registers.  Return early for unreadable registers.
> 
> gdb/testsuite/ChangeLog
> 
> 2013-08-06  Andrew Burgess  <aburgess@broadcom.com>
> 
> 	* gdb.trace/unavailable.exp (gdb_unavailable_registers_test):
> 	Change expected pattern for unavailable registers to
> 	<unavailable>.
> 
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index f6a5290..297dad0 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -2030,7 +2030,8 @@ default_print_one_register_info (struct ui_file *file,
>  
>    if (!value_entirely_available (val))
>      {
> -      fprintf_filtered (file, "*value not available*\n");
> +      val_print_unavailable (file);
> +      fprintf_filtered (file, "\n");
>        return;
>      }
>    else if (value_optimized_out (val))
> diff --git a/gdb/sh64-tdep.c b/gdb/sh64-tdep.c
> index b640b1d..37fb72f 100644
> --- a/gdb/sh64-tdep.c
> +++ b/gdb/sh64-tdep.c
> @@ -2045,7 +2045,11 @@ sh64_do_register (struct gdbarch *gdbarch, struct ui_file *file,
>  
>    /* Get the data in raw format.  */
>    if (!deprecated_frame_register_read (frame, regnum, raw_buffer))
> -    fprintf_filtered (file, "*value not available*\n");
> +    {
> +      val_print_unavailable (file);
> +      fprintf_filtered (file, "\n");
> +      return;
> +    }
>  
>    get_formatted_print_options (&opts, 'x');
>    opts.deref_ref = 1;
> diff --git a/gdb/testsuite/gdb.trace/unavailable.exp b/gdb/testsuite/gdb.trace/unavailable.exp
> index 8e2e105..ed14798 100644
> --- a/gdb/testsuite/gdb.trace/unavailable.exp
> +++ b/gdb/testsuite/gdb.trace/unavailable.exp
> @@ -297,11 +297,11 @@ proc gdb_unavailable_registers_test { } {
>  	test_register "\$pc"
>  
>  	gdb_test "info registers" \
> -	    "\\*value not available\\*.*\\*value not available\\*" \
> +	    "<unavailable>.*<unavailable>" \
>  	    "info registers, multiple registers not available"
>  
>  	gdb_test "info registers \$$spreg" \
> -	    "\\*value not available\\*" \
> +	    "<unavailable>" \
>  	    "info registers \$$spreg reports not available"
>  
>  	gdb_test "tfind none" "#0  end .*" "cease trace debugging"
> 
> 
> 
> 

Re: PING: Re: [PATCH] Print <unavailable> for unavailable registers

[Pedro Alves]

On 09/18/2013 03:48 PM, Andrew Burgess wrote:
> On 06/08/2013 2:09 PM, Andrew Burgess wrote:
>> Following on from (but unrelated to) this mail:
>>   http://sourceware.org/ml/gdb-patches/2013-08/msg00170.html
>>
>> Printing "*value not available*" for unavailable values within
>> "info registers" seems inconsistent to me, if we just print an
>> unavailable register we'll get "<unavailable>".
>>
>> The patch below makes "info registers" print "<unavailable>".
> 
> I've re-written this patch, here's the latest version.
> 
> OK to apply?

I was waiting for the "<not saved>" discussion patch to
finalize before looking at this one.  Looks like only a docs
review is missing so that should be soon.

> Index: ./gdb/infcmd.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infcmd.c,v
> retrieving revision 1.335
> diff -u -p -r1.335 infcmd.c
> --- ./gdb/infcmd.c	18 Sep 2013 14:02:31 -0000	1.335
> +++ ./gdb/infcmd.c	18 Sep 2013 14:43:13 -0000
> @@ -2030,7 +2030,8 @@ default_print_one_register_info (struct 
>  
>    if (!value_entirely_available (val))
>      {
> -      fprintf_filtered (file, "*value not available*\n");
> +      val_print_unavailable (file);
> +      fprintf_filtered (file, "\n");
>        return;
>      }
>    else if (value_optimized_out (val))

Why do we do this instead of just deferring to val_print though?
val_print would be able to print partially available registers,
for instance.

(We'd need to do something about the "raw" printing bits below
though.)

-- 
Pedro Alves

Re: PING: Re: [PATCH] Print <unavailable> for unavailable registers

[Andrew Burgess]

On 18/09/2013 5:06 PM, Pedro Alves wrote:

>> Index: ./gdb/infcmd.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/infcmd.c,v
>> retrieving revision 1.335
>> diff -u -p -r1.335 infcmd.c
>> --- ./gdb/infcmd.c	18 Sep 2013 14:02:31 -0000	1.335
>> +++ ./gdb/infcmd.c	18 Sep 2013 14:43:13 -0000
>> @@ -2030,7 +2030,8 @@ default_print_one_register_info (struct 
>>  
>>    if (!value_entirely_available (val))
>>      {
>> -      fprintf_filtered (file, "*value not available*\n");
>> +      val_print_unavailable (file);
>> +      fprintf_filtered (file, "\n");
>>        return;
>>      }
>>    else if (value_optimized_out (val))
> 
> Why do we do this instead of just deferring to val_print though?
> val_print would be able to print partially available registers,
> for instance.
> 
> (We'd need to do something about the "raw" printing bits below
> though.)

OK that's a reasonable point.  I've rewritten this patch to make use
of the val_print call to print the <not-saved> / <unavailable>.

For now I unconditionally skip the second attempt to print the value,
the "raw" form, for optimized-out and unavailable values, we might
be able to do better in the future but I was keen to avoid output
like this: "rax:  <unavailable> <unavailable>" with the
<unavailable> being repeated.  If the value of a register is
partially unavailable, and so we did manage to print something then
it might make sense to try and print the raw form... 

Is this OK to apply?

Thanks,
Andrew


gdb/ChangeLog
    
2013-10-16  Andrew Burgess  <aburgess@broadcom.com>

	* infcmd.c (default_print_one_register_info): Use val_print to
	print all values even optimized out or unavailable ones.  Don't
	try to print a raw form of optimized out or unavailable values.

gdb/testsuite/ChangeLog
    
2013-10-16  Andrew Burgess  <aburgess@broadcom.com>

	* gdb.trace/unavailable.exp (gdb_unavailable_registers_test):
	Expect <unavailable> pattern.

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 20f8857..3b41837 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2024,21 +2024,13 @@ default_print_one_register_info (struct ui_file *file,
 				 struct value *val)
 {
   struct type *regtype = value_type (val);
+  int print_raw_format;
 
   fputs_filtered (name, file);
   print_spaces_filtered (15 - strlen (name), file);
 
-  if (!value_entirely_available (val))
-    {
-      fprintf_filtered (file, "*value not available*\n");
-      return;
-    }
-  else if (value_optimized_out (val))
-    {
-      val_print_optimized_out (val, file);
-      fprintf_filtered (file, "\n");
-      return;
-    }
+  print_raw_format = (value_entirely_available (val)
+		      && !value_optimized_out (val));
 
   /* If virtual format is floating, print it that way, and in raw
      hex.  */
@@ -2058,9 +2050,12 @@ default_print_one_register_info (struct ui_file *file,
 		 value_embedded_offset (val), 0,
 		 file, 0, val, &opts, current_language);
 
-      fprintf_filtered (file, "\t(raw ");
-      print_hex_chars (file, valaddr, TYPE_LENGTH (regtype), byte_order);
-      fprintf_filtered (file, ")");
+      if (print_raw_format)
+	{
+	  fprintf_filtered (file, "\t(raw ");
+	  print_hex_chars (file, valaddr, TYPE_LENGTH (regtype), byte_order);
+	  fprintf_filtered (file, ")");
+	}
     }
   else
     {
@@ -2075,7 +2070,7 @@ default_print_one_register_info (struct ui_file *file,
 		 file, 0, val, &opts, current_language);
       /* If not a vector register, print it also according to its
 	 natural format.  */
-      if (TYPE_VECTOR (regtype) == 0)
+      if (print_raw_format && TYPE_VECTOR (regtype) == 0)
 	{
 	  get_user_print_options (&opts);
 	  opts.deref_ref = 1;
diff --git a/gdb/testsuite/gdb.trace/unavailable.exp b/gdb/testsuite/gdb.trace/unavailable.exp
index 8e2e105..ed14798 100644
--- a/gdb/testsuite/gdb.trace/unavailable.exp
+++ b/gdb/testsuite/gdb.trace/unavailable.exp
@@ -297,11 +297,11 @@ proc gdb_unavailable_registers_test { } {
 	test_register "\$pc"
 
 	gdb_test "info registers" \
-	    "\\*value not available\\*.*\\*value not available\\*" \
+	    "<unavailable>.*<unavailable>" \
 	    "info registers, multiple registers not available"
 
 	gdb_test "info registers \$$spreg" \
-	    "\\*value not available\\*" \
+	    "<unavailable>" \
 	    "info registers \$$spreg reports not available"
 
 	gdb_test "tfind none" "#0  end .*" "cease trace debugging"

Re: PING: Re: [PATCH] Print <unavailable> for unavailable registers

[Pedro Alves]

On 10/16/2013 03:26 PM, Andrew Burgess wrote:
> On 18/09/2013 5:06 PM, Pedro Alves wrote:
> 
>>> Index: ./gdb/infcmd.c
>>> ===================================================================
>>> RCS file: /cvs/src/src/gdb/infcmd.c,v
>>> retrieving revision 1.335
>>> diff -u -p -r1.335 infcmd.c
>>> --- ./gdb/infcmd.c	18 Sep 2013 14:02:31 -0000	1.335
>>> +++ ./gdb/infcmd.c	18 Sep 2013 14:43:13 -0000
>>> @@ -2030,7 +2030,8 @@ default_print_one_register_info (struct 
>>>  
>>>    if (!value_entirely_available (val))
>>>      {
>>> -      fprintf_filtered (file, "*value not available*\n");
>>> +      val_print_unavailable (file);
>>> +      fprintf_filtered (file, "\n");
>>>        return;
>>>      }
>>>    else if (value_optimized_out (val))
>>
>> Why do we do this instead of just deferring to val_print though?
>> val_print would be able to print partially available registers,
>> for instance.
>>
>> (We'd need to do something about the "raw" printing bits below
>> though.)
> 
> OK that's a reasonable point.  I've rewritten this patch to make use
> of the val_print call to print the <not-saved> / <unavailable>.
> 
> For now I unconditionally skip the second attempt to print the value,
> the "raw" form, for optimized-out and unavailable values, we might
> be able to do better in the future but I was keen to avoid output
> like this: "rax:  <unavailable> <unavailable>" with the
> <unavailable> being repeated.  If the value of a register is
> partially unavailable, and so we did manage to print something then
> it might make sense to try and print the raw form... 

Yeah, we could print some character like UU or XX or $$ or some such
for the unavailable bytes, for example, like, say, printing
a 32-bit var:

  <unavailable>   0011UUUU

> 
> Is this OK to apply?

OK.

Thanks,
-- 
Pedro Alves

Re: PING: Re: [PATCH] Print <unavailable> for unavailable registers

[Andrew Burgess]

On 24/10/2013 8:44 PM, Pedro Alves wrote:
> On 10/16/2013 03:26 PM, Andrew Burgess wrote:

>> Is this OK to apply?
> 
> OK.
> 

I've pushed this patch now, sorry for the delay.

First commit to the new git repo so I hope I got it all right.

Thanks,
Andrew