[PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Sergio Durigan Junior]

While regtesting 7.4 against 7.5 branch on ppc64/s390x RHEL 6.3, I
noticed this failure.  The patch which introduced this failure was
committed because of:

      http://sourceware.org/bugzilla/show_bug.cgi?id=12659

On x86*, the output of `info register pc fp' is:

    info register pc fp
    pc: 0x400520
    fp: 0x7fffffffc490
    (gdb) PASS: gdb.base/pc-fp.exp: info register pc fp

On ppc64/s390x, it is:

    info register pc fp
    pc             0x10000658       0x10000658 <main+20>
    fp: 0xfffffffd120
    (gdb) FAIL: gdb.base/pc-fp.exp: info register pc fp

Since this difference in the output does not seem to be an error itself,
the patch below just adjusts the testcase to match this kind of output
as well.  It does not fail on x86*.

OK?

-- 
Sergio

gdb/testsuite:
2012-07-31  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR gdb/12659:
	Adjust testcase for PPC64/s390x architectures.
	* gdb.base/pc-fp.exp: Match different output for `info register pc
	fp' on PPC64/s390x architectures.

Index: src/gdb/testsuite/gdb.base/pc-fp.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.base/pc-fp.exp
+++ src/gdb/testsuite/gdb.base/pc-fp.exp
@@ -66,4 +66,4 @@ gdb_test "info register \$fp" "${valueof
 # Regression test for
 # http://sourceware.org/bugzilla/show_bug.cgi?id=12659
 gdb_test "info register pc fp" \
-    "pc: ${valueof_pc}\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"
+    "pc(:)?.*${valueof_pc}(.*${hex} <.*>)?\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Andreas Schwab]

Sergio Durigan Junior <sergiodj@redhat.com> writes:

> While regtesting 7.4 against 7.5 branch on ppc64/s390x RHEL 6.3, I
> noticed this failure.  The patch which introduced this failure was
> committed because of:
>
>       http://sourceware.org/bugzilla/show_bug.cgi?id=12659
>
> On x86*, the output of `info register pc fp' is:
>
>     info register pc fp
>     pc: 0x400520
>     fp: 0x7fffffffc490
>     (gdb) PASS: gdb.base/pc-fp.exp: info register pc fp
>
> On ppc64/s390x, it is:
>
>     info register pc fp
>     pc             0x10000658       0x10000658 <main+20>
>     fp: 0xfffffffd120
>     (gdb) FAIL: gdb.base/pc-fp.exp: info register pc fp

I'm getting this:

info register pc fp
pc             0x8000042c       0x8000042c <main+4>
fp             0xefae83e4       0xefae83e4
(gdb) FAIL: gdb.base/pc-fp.exp: info register pc fp

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Sergio Durigan Junior]

On Tuesday, July 31 2012, Andreas Schwab wrote:

> Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
>> While regtesting 7.4 against 7.5 branch on ppc64/s390x RHEL 6.3, I
>> noticed this failure.  The patch which introduced this failure was
>> committed because of:
>>
>>       http://sourceware.org/bugzilla/show_bug.cgi?id=12659>
>> On x86*, the output of `info register pc fp' is:
>>
>>     info register pc fp
>>     pc: 0x400520
>>     fp: 0x7fffffffc490
>>     (gdb) PASS: gdb.base/pc-fp.exp: info register pc fp
>>
>> On ppc64/s390x, it is:
>>
>>     info register pc fp
>>     pc             0x10000658       0x10000658 <main+20>
>>     fp: 0xfffffffd120
>>     (gdb) FAIL: gdb.base/pc-fp.exp: info register pc fp
>
> I'm getting this:
>
> info register pc fp
> pc             0x8000042c       0x8000042c <main+4>
> fp             0xefae83e4       0xefae83e4
> (gdb) FAIL: gdb.base/pc-fp.exp: info register pc fp

This should also fail using GDB 7.5.  Could you please test the
following patch?

Thanks,

-- 
Sergio

Index: src/gdb/testsuite/gdb.base/pc-fp.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.base/pc-fp.exp
+++ src/gdb/testsuite/gdb.base/pc-fp.exp
@@ -66,4 +66,4 @@ gdb_test "info register \$fp" "${valueof
 # Regression test for
 # http://sourceware.org/bugzilla/show_bug.cgi?id=12659
 gdb_test "info register pc fp" \
-    "pc: ${valueof_pc}\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"
+    "pc(:)?( |\t)+${valueof_pc}(( |\t)+${valueof_pc} <.*>)?\[\r\n\]+fp(:)?( |\t)+${valueof_fp}(( |\t)+${valueof_fp})?\[\r\n\]+"

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Andreas Schwab]

Sergio Durigan Junior <sergiodj@redhat.com> writes:

> This should also fail using GDB 7.5.  Could you please test the
> following patch?

Works for me.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Pedro Alves]

On 07/31/2012 10:25 PM, Sergio Durigan Junior wrote:
> While regtesting 7.4 against 7.5 branch on ppc64/s390x RHEL 6.3, I
> noticed this failure.  The patch which introduced this failure was
> committed because of:
> 
>       http://sourceware.org/bugzilla/show_bug.cgi?id=12659
> 
> On x86*, the output of `info register pc fp' is:
> 
>     info register pc fp
>     pc: 0x400520
>     fp: 0x7fffffffc490
>     (gdb) PASS: gdb.base/pc-fp.exp: info register pc fp
> 
> On ppc64/s390x, it is:
> 
>     info register pc fp
>     pc             0x10000658       0x10000658 <main+20>
>     fp: 0xfffffffd120
>     (gdb) FAIL: gdb.base/pc-fp.exp: info register pc fp
> 
> Since this difference in the output does not seem to be an error itself,
> the patch below just adjusts the testcase to match this kind of output
> as well.  It does not fail on x86*.

Why is the output format different?  It looks like consistency here would be good.

On 07/31/2012 10:25 PM, Sergio Durigan Junior wrote:> --- src.orig/gdb/testsuite/gdb.base/pc-fp.exp
> +++ src/gdb/testsuite/gdb.base/pc-fp.exp
> @@ -66,4 +66,4 @@ gdb_test "info register \$fp" "${valueof
>  # Regression test for
>  # http://sourceware.org/bugzilla/show_bug.cgi?id=12659
>  gdb_test "info register pc fp" \
> -    "pc: ${valueof_pc}\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"
> +    "pc(:)?.*${valueof_pc}(.*${hex} <.*>)?\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"

Relaxing the output like that means that inadvertent changes to x86's
or ppc/s390x output might go unnoticed.  It's best to have

  if [istarget xxx]
     one way
  elseif [istarget yyy]
     another way

etc. checks in these cases.

-- 
Pedro Alves

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Ulrich Weigand]

Pedro Alves wrote:
> On 07/31/2012 10:25 PM, Sergio Durigan Junior wrote:
> > While regtesting 7.4 against 7.5 branch on ppc64/s390x RHEL 6.3, I
> > noticed this failure.  The patch which introduced this failure was
> > committed because of:
> > 
> >       http://sourceware.org/bugzilla/show_bug.cgi?id=12659
> > 
> > On x86*, the output of `info register pc fp' is:
> > 
> >     info register pc fp
> >     pc: 0x400520
> >     fp: 0x7fffffffc490
> >     (gdb) PASS: gdb.base/pc-fp.exp: info register pc fp
> > 
> > On ppc64/s390x, it is:
> > 
> >     info register pc fp
> >     pc             0x10000658       0x10000658 <main+20>
> >     fp: 0xfffffffd120
> >     (gdb) FAIL: gdb.base/pc-fp.exp: info register pc fp
> > 
> > Since this difference in the output does not seem to be an error itself,
> > the patch below just adjusts the testcase to match this kind of output
> > as well.  It does not fail on x86*.
> 
> Why is the output format different?  It looks like consistency here would be good.

The problem is that "pc", "fp", etc can refer to different things under
the covers: either a register defined by the target code, or else a
"user register" defined by GDB common code.

On many targets (but not Intel), "pc" is the name of a register defined
by the target.  In this case, registers_info uses the standard
gdbarch_print_registers_info routine to output its content; this gives
a larger space between register name and value, and outputs the
contents both in hex and in the register's default type, usually a
function pointer type.

On targets where "pc" is *not* the name of a register defined by the
target, registers_info still recognizes the name as "user register",
and uses a separate code path to print its value.  This results in
a different (shorter) output ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Sergio Durigan Junior]

On Wednesday, August 01 2012, Ulrich Weigand wrote:

> Pedro Alves wrote:
>> On 07/31/2012 10:25 PM, Sergio Durigan Junior wrote:

>> > Since this difference in the output does not seem to be an error itself,
>> > the patch below just adjusts the testcase to match this kind of output
>> > as well.  It does not fail on x86*.
>> 
>> Why is the output format different?  It looks like consistency here would be good.
>
> The problem is that "pc", "fp", etc can refer to different things under
> the covers: either a register defined by the target code, or else a
> "user register" defined by GDB common code.
>
> On many targets (but not Intel), "pc" is the name of a register defined
> by the target.  In this case, registers_info uses the standard
> gdbarch_print_registers_info routine to output its content; this gives
> a larger space between register name and value, and outputs the
> contents both in hex and in the register's default type, usually a
> function pointer type.
>
> On targets where "pc" is *not* the name of a register defined by the
> target, registers_info still recognizes the name as "user register",
> and uses a separate code path to print its value.  This results in
> a different (shorter) output ...

Thanks for the explanation, I was debugging it here and came to this
same conclusion.

I will try to make the printing routine for user registers to behave
like the one for target registers, unless you have other opinion about it.

-- 
Sergio

info registers output

[Pedro Alves]

On 08/01/2012 08:40 PM, Ulrich Weigand wrote:
> Pedro Alves wrote:

>> Why is the output format different?  It looks like consistency here would be good.
> 
> The problem is that "pc", "fp", etc can refer to different things under
> the covers: either a register defined by the target code, or else a
> "user register" defined by GDB common code.
> 
> On many targets (but not Intel), "pc" is the name of a register defined
> by the target.  In this case, registers_info uses the standard
> gdbarch_print_registers_info routine to output its content; this gives
> a larger space between register name and value, and outputs the
> contents both in hex and in the register's default type, usually a
> function pointer type.
> 
> On targets where "pc" is *not* the name of a register defined by the
> target, registers_info still recognizes the name as "user register",
> and uses a separate code path to print its value.  This results in
> a different (shorter) output ...

Ah.  I wonder if that's been made on purpose.  You get this on amd64:

(gdb) info registers rip pc
rip            0x390f407e68     0x390f407e68 <start_thread+552>
pc: 0x390f407e68

GDB knows the type of "pc", and so should be able to print "pc" like "rip".

Would that be a good idea?

-- 
Pedro Alves

Re: info registers output

[Ulrich Weigand]

Pedro Alves wrote:
> Ah.  I wonder if that's been made on purpose.  You get this on amd64:
> 
> (gdb) info registers rip pc
> rip            0x390f407e68     0x390f407e68 <start_thread+552>
> pc: 0x390f407e68
> 
> GDB knows the type of "pc", and so should be able to print "pc" like "rip".
> 
> Would that be a good idea?

Would make sense to me.  (In fact, there probably ought to be a single
routine to print a register, called by both code paths, to avoid having
the code diverge again in the future ...)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: info registers output

[Sergio Durigan Junior]

On Wednesday, August 01 2012, Ulrich Weigand wrote:

> Pedro Alves wrote:
>> Ah.  I wonder if that's been made on purpose.  You get this on amd64:
>> 
>> (gdb) info registers rip pc
>> rip            0x390f407e68     0x390f407e68 <start_thread+552>
>> pc: 0x390f407e68
>> 
>> GDB knows the type of "pc", and so should be able to print "pc" like "rip".
>> 
>> Would that be a good idea?
>
> Would make sense to me.  (In fact, there probably ought to be a single
> routine to print a register, called by both code paths, to avoid having
> the code diverge again in the future ...)

FWIW I created sourceware.org/bugzilla/show_bug.cgi?id=14428 for this.

Thanks,

-- 
Sergio

Re: info registers output

[Pedro Alves]

On 08/01/2012 09:49 PM, Ulrich Weigand wrote:
> Pedro Alves wrote:
>> Ah.  I wonder if that's been made on purpose.  You get this on amd64:
>>
>> (gdb) info registers rip pc
>> rip            0x390f407e68     0x390f407e68 <start_thread+552>
>> pc: 0x390f407e68
>>
>> GDB knows the type of "pc", and so should be able to print "pc" like "rip".
>>
>> Would that be a good idea?
> 
> Would make sense to me.  (In fact, there probably ought to be a single
> routine to print a register, called by both code paths, to avoid having
> the code diverge again in the future ...)

Here's a patch that does that.  If some arch wants to print user regs
differently, we'll either have to add a new gdbarch, or make
gdbarch_print_registers_info handle user regs.

We now get, for amd64:

(gdb) info registers pc rip sp rsp fp rbp
pc             0x45762b 0x45762b <main+15>
rip            0x45762b 0x45762b <main+15>
sp             0x7fffffffdbe0   0x7fffffffdbe0
rsp            0x7fffffffdbe0   0x7fffffffdbe0
fp             0x7fffffffdc10   0x7fffffffdc10
rbp            0x7fffffffdc10   0x7fffffffdc10

Before we'd get:

(gdb) info registers pc rip sp rsp fp rbp
pc: 0x45762b
rip            0x45762b 0x45762b <main+15>
sp: 0x7fffffffdbe0
rsp            0x7fffffffdbe0   0x7fffffffdbe0
fp: 0x7fffffffdc10
rbp            0x7fffffffdc10   0x7fffffffdc10

How does it look?

Tested on AMD64 Fedora 17.

2012-08-27  Pedro Alves  <palves@redhat.com>

	PR gdb/14428

	* infcmd.c (default_print_one_register_info): New, factored out
	from default_print_registers_info.
	(default_print_registers_info): Use it.  Mark value unavailable if
	necessary.
	(registers_info): Print user registers with
	default_print_one_register_info.
---

 gdb/infcmd.c                     |  167 +++++++++++++++++++++-----------------
 gdb/testsuite/gdb.base/pc-fp.exp |    2 
 2 files changed, 93 insertions(+), 76 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 9d43193..2067948 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2015,6 +2015,84 @@ path_command (char *dirname, int from_tty)
 }
 \f
 
+/* Print out the register NAME, of GDBARCH, with value VAL, to FILE,
+   in the default fashion.  */
+
+static void
+default_print_one_register_info (struct gdbarch *gdbarch,
+				 struct ui_file *file,
+				 const char *name,
+				 struct value *val)
+{
+  struct type *regtype = value_type (val);
+
+  fputs_filtered (name, file);
+  print_spaces_filtered (15 - strlen (name), file);
+
+  if (!value_entirely_available (val))
+    {
+      fprintf_filtered (file, "*value not available*\n");
+      return;
+    }
+
+  /* If virtual format is floating, print it that way, and in raw
+     hex.  */
+  if (TYPE_CODE (regtype) == TYPE_CODE_FLT
+      || TYPE_CODE (regtype) == TYPE_CODE_DECFLOAT)
+    {
+      int j;
+      struct value_print_options opts;
+      const gdb_byte *valaddr = value_contents_for_printing (val);
+
+      get_user_print_options (&opts);
+      opts.deref_ref = 1;
+
+      val_print (regtype,
+		 value_contents_for_printing (val),
+		 value_embedded_offset (val), 0,
+		 file, 0, val, &opts, current_language);
+
+      fprintf_filtered (file, "\t(raw 0x");
+      for (j = 0; j < TYPE_LENGTH (regtype); j++)
+	{
+	  int idx;
+
+	  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
+	    idx = j;
+	  else
+	    idx = TYPE_LENGTH (regtype) - 1 - j;
+	  fprintf_filtered (file, "%02x", (unsigned char) valaddr[idx]);
+	}
+      fprintf_filtered (file, ")");
+    }
+  else
+    {
+      struct value_print_options opts;
+
+      /* Print the register in hex.  */
+      get_formatted_print_options (&opts, 'x');
+      opts.deref_ref = 1;
+      val_print (regtype,
+		 value_contents_for_printing (val),
+		 value_embedded_offset (val), 0,
+		 file, 0, val, &opts, current_language);
+      /* If not a vector register, print it also according to its
+	 natural format.  */
+      if (TYPE_VECTOR (regtype) == 0)
+	{
+	  get_user_print_options (&opts);
+	  opts.deref_ref = 1;
+	  fprintf_filtered (file, "\t");
+	  val_print (regtype,
+		     value_contents_for_printing (val),
+		     value_embedded_offset (val), 0,
+		     file, 0, val, &opts, current_language);
+	}
+    }
+
+  fprintf_filtered (file, "\n");
+}
+
 /* Print out the machine register regnum.  If regnum is -1, print all
    registers (print_all == 1) or all non-float and non-vector
    registers (print_all == 0).
@@ -2068,76 +2146,16 @@ default_print_registers_info (struct gdbarch *gdbarch,
 	  || *(gdbarch_register_name (gdbarch, i)) == '\0')
 	continue;
 
-      fputs_filtered (gdbarch_register_name (gdbarch, i), file);
-      print_spaces_filtered (15 - strlen (gdbarch_register_name
-					  (gdbarch, i)), file);
-
       regtype = register_type (gdbarch, i);
       val = allocate_value (regtype);
 
       /* Get the data in raw format.  */
       if (! frame_register_read (frame, i, value_contents_raw (val)))
-	{
-	  fprintf_filtered (file, "*value not available*\n");
-	  continue;
-	}
-
-      /* If virtual format is floating, print it that way, and in raw
-         hex.  */
-      if (TYPE_CODE (regtype) == TYPE_CODE_FLT
-	  || TYPE_CODE (regtype) == TYPE_CODE_DECFLOAT)
-	{
-	  int j;
-	  struct value_print_options opts;
-	  const gdb_byte *valaddr = value_contents_for_printing (val);
-
-	  get_user_print_options (&opts);
-	  opts.deref_ref = 1;
-
-	  val_print (regtype,
-		     value_contents_for_printing (val),
-		     value_embedded_offset (val), 0,
-		     file, 0, val, &opts, current_language);
-
-	  fprintf_filtered (file, "\t(raw 0x");
-	  for (j = 0; j < register_size (gdbarch, i); j++)
-	    {
-	      int idx;
-
-	      if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
-		idx = j;
-	      else
-		idx = register_size (gdbarch, i) - 1 - j;
-	      fprintf_filtered (file, "%02x", (unsigned char) valaddr[idx]);
-	    }
-	  fprintf_filtered (file, ")");
-	}
-      else
-	{
-	  struct value_print_options opts;
-
-	  /* Print the register in hex.  */
-	  get_formatted_print_options (&opts, 'x');
-	  opts.deref_ref = 1;
-	  val_print (regtype,
-		     value_contents_for_printing (val),
-		     value_embedded_offset (val), 0,
-		     file, 0, val, &opts, current_language);
-          /* If not a vector register, print it also according to its
-             natural format.  */
-	  if (TYPE_VECTOR (regtype) == 0)
-	    {
-	      get_user_print_options (&opts);
-	      opts.deref_ref = 1;
-	      fprintf_filtered (file, "\t");
-	      val_print (regtype,
-			 value_contents_for_printing (val),
-			 value_embedded_offset (val), 0,
-			 file, 0, val, &opts, current_language);
-	    }
-	}
+	mark_value_bytes_unavailable (val, 0, TYPE_LENGTH (value_type (val)));
 
-      fprintf_filtered (file, "\n");
+      default_print_one_register_info (gdbarch, file,
+				       gdbarch_register_name (gdbarch, i),
+				       val);
     }
 }
 
@@ -2198,17 +2216,16 @@ registers_info (char *addr_exp, int fpregs)
 	    if (regnum >= gdbarch_num_regs (gdbarch)
 			  + gdbarch_num_pseudo_regs (gdbarch))
 	      {
-		struct value_print_options opts;
-		struct value *val = value_of_user_reg (regnum, frame);
-
-		printf_filtered ("%.*s: ", (int) (end - start), start);
-		get_formatted_print_options (&opts, 'x');
-		val_print_scalar_formatted (check_typedef (value_type (val)),
-					    value_contents_for_printing (val),
-					    value_embedded_offset (val),
-					    val,
-					    &opts, 0, gdb_stdout);
-		printf_filtered ("\n");
+		struct value *regval = value_of_user_reg (regnum, frame);
+		const char *regname = user_reg_map_regnum_to_name (gdbarch,
+								   regnum);
+
+		/* Print in the same fashion
+		   gdbarch_print_registers_info's default
+		   implementation prints.  */
+		default_print_one_register_info (gdbarch, gdb_stdout,
+						 regname,
+						 regval);
 	      }
 	    else
 	      gdbarch_print_registers_info (gdbarch, gdb_stdout,
diff --git a/gdb/testsuite/gdb.base/pc-fp.exp b/gdb/testsuite/gdb.base/pc-fp.exp
index beb5087..685e2d9 100644
--- a/gdb/testsuite/gdb.base/pc-fp.exp
+++ b/gdb/testsuite/gdb.base/pc-fp.exp
@@ -66,4 +66,4 @@ gdb_test "info register \$fp" "${valueof_fp}.*"
 # Regression test for
 # http://sourceware.org/bugzilla/show_bug.cgi?id=12659
 gdb_test "info register pc fp" \
-    "pc(:)?( |\t)+${valueof_pc}(( |\t)+${valueof_pc} <.*>)?\[\r\n\]+fp(:)?( |\t)+${valueof_fp}(( |\t)+${valueof_fp})?\[\r\n\]+"
+    "pc +${valueof_pc}\t${valueof_pc} <.*>\[\r\n\]+fp +${valueof_fp}\t${valueof_fp}\[\r\n\]+"

Re: info registers output

[Ulrich Weigand]

Pedro Alves wrote:
> On 08/01/2012 09:49 PM, Ulrich Weigand wrote:
> > Would make sense to me.  (In fact, there probably ought to be a single
> > routine to print a register, called by both code paths, to avoid having
> > the code diverge again in the future ...)
> 
> Here's a patch that does that.  If some arch wants to print user regs
> differently, we'll either have to add a new gdbarch, or make
> gdbarch_print_registers_info handle user regs.
> 
> We now get, for amd64:
> 
> (gdb) info registers pc rip sp rsp fp rbp
> pc             0x45762b 0x45762b <main+15>
> rip            0x45762b 0x45762b <main+15>
> sp             0x7fffffffdbe0   0x7fffffffdbe0
> rsp            0x7fffffffdbe0   0x7fffffffdbe0
> fp             0x7fffffffdc10   0x7fffffffdc10
> rbp            0x7fffffffdc10   0x7fffffffdc10
> 
> Before we'd get:
> 
> (gdb) info registers pc rip sp rsp fp rbp
> pc: 0x45762b
> rip            0x45762b 0x45762b <main+15>
> sp: 0x7fffffffdbe0
> rsp            0x7fffffffdbe0   0x7fffffffdbe0
> fp: 0x7fffffffdc10
> rbp            0x7fffffffdc10   0x7fffffffdc10
> 
> How does it look?

Looks good to me.   The only minor nit would be:

> +/* Print out the register NAME, of GDBARCH, with value VAL, to FILE,
> +   in the default fashion.  */
> +
> +static void
> +default_print_one_register_info (struct gdbarch *gdbarch,
> +				 struct ui_file *file,
> +				 const char *name,
> +				 struct value *val)

Does this really need a GDBARCH argument?  It's only used here:

> +      for (j = 0; j < TYPE_LENGTH (regtype); j++)
> +	{
> +	  int idx;
> +
> +	  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> +	    idx = j;
> +	  else
> +	    idx = TYPE_LENGTH (regtype) - 1 - j;
> +	  fprintf_filtered (file, "%02x", (unsigned char) valaddr[idx]);
> +	}

where we really want the type's byte order anyway ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: info registers output

[Pedro Alves]

On 08/28/2012 01:41 AM, Ulrich Weigand wrote:

> Looks good to me.   The only minor nit would be:
> 
>> +/* Print out the register NAME, of GDBARCH, with value VAL, to FILE,
>> +   in the default fashion.  */
>> +
>> +static void
>> +default_print_one_register_info (struct gdbarch *gdbarch,
>> +				 struct ui_file *file,
>> +				 const char *name,
>> +				 struct value *val)
> 
> Does this really need a GDBARCH argument?  It's only used here:
> 
>> +      for (j = 0; j < TYPE_LENGTH (regtype); j++)
>> +	{
>> +	  int idx;
>> +
>> +	  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
>> +	    idx = j;
>> +	  else
>> +	    idx = TYPE_LENGTH (regtype) - 1 - j;
>> +	  fprintf_filtered (file, "%02x", (unsigned char) valaddr[idx]);
>> +	}
> 
> where we really want the type's byte order anyway ...

Indeed, thanks.  I've applied with that change, as below.

2012-08-28  Pedro Alves  <palves@redhat.com>

	PR gdb/14428

	gdb/
	* infcmd.c (default_print_one_register_info): New, factored out
	from default_print_registers_info.
	(default_print_registers_info): Use it.  Mark value unavailable if
	necessary.
	(registers_info): Print user registers with
	default_print_one_register_info.

	gdb/testsuite/
	* gdb.base/pc-fp.exp: Adjust expected output of 'info registers pc fp'.
---
 gdb/infcmd.c                     |  167 +++++++++++++++++++++-----------------
 gdb/testsuite/gdb.base/pc-fp.exp |    2 
 2 files changed, 93 insertions(+), 76 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 9d43193..8e2f74e 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2015,6 +2015,84 @@ path_command (char *dirname, int from_tty)
 }
 \f
 
+/* Print out the register NAME with value VAL, to FILE, in the default
+   fashion.  */
+
+static void
+default_print_one_register_info (struct ui_file *file,
+				 const char *name,
+				 struct value *val)
+{
+  struct type *regtype = value_type (val);
+
+  fputs_filtered (name, file);
+  print_spaces_filtered (15 - strlen (name), file);
+
+  if (!value_entirely_available (val))
+    {
+      fprintf_filtered (file, "*value not available*\n");
+      return;
+    }
+
+  /* If virtual format is floating, print it that way, and in raw
+     hex.  */
+  if (TYPE_CODE (regtype) == TYPE_CODE_FLT
+      || TYPE_CODE (regtype) == TYPE_CODE_DECFLOAT)
+    {
+      int j;
+      struct value_print_options opts;
+      const gdb_byte *valaddr = value_contents_for_printing (val);
+      enum bfd_endian byte_order = gdbarch_byte_order (get_type_arch (regtype));
+
+      get_user_print_options (&opts);
+      opts.deref_ref = 1;
+
+      val_print (regtype,
+		 value_contents_for_printing (val),
+		 value_embedded_offset (val), 0,
+		 file, 0, val, &opts, current_language);
+
+      fprintf_filtered (file, "\t(raw 0x");
+      for (j = 0; j < TYPE_LENGTH (regtype); j++)
+	{
+	  int idx;
+
+	  if (byte_order == BFD_ENDIAN_BIG)
+	    idx = j;
+	  else
+	    idx = TYPE_LENGTH (regtype) - 1 - j;
+	  fprintf_filtered (file, "%02x", (unsigned char) valaddr[idx]);
+	}
+      fprintf_filtered (file, ")");
+    }
+  else
+    {
+      struct value_print_options opts;
+
+      /* Print the register in hex.  */
+      get_formatted_print_options (&opts, 'x');
+      opts.deref_ref = 1;
+      val_print (regtype,
+		 value_contents_for_printing (val),
+		 value_embedded_offset (val), 0,
+		 file, 0, val, &opts, current_language);
+      /* If not a vector register, print it also according to its
+	 natural format.  */
+      if (TYPE_VECTOR (regtype) == 0)
+	{
+	  get_user_print_options (&opts);
+	  opts.deref_ref = 1;
+	  fprintf_filtered (file, "\t");
+	  val_print (regtype,
+		     value_contents_for_printing (val),
+		     value_embedded_offset (val), 0,
+		     file, 0, val, &opts, current_language);
+	}
+    }
+
+  fprintf_filtered (file, "\n");
+}
+
 /* Print out the machine register regnum.  If regnum is -1, print all
    registers (print_all == 1) or all non-float and non-vector
    registers (print_all == 0).
@@ -2068,76 +2146,16 @@ default_print_registers_info (struct gdbarch *gdbarch,
 	  || *(gdbarch_register_name (gdbarch, i)) == '\0')
 	continue;
 
-      fputs_filtered (gdbarch_register_name (gdbarch, i), file);
-      print_spaces_filtered (15 - strlen (gdbarch_register_name
-					  (gdbarch, i)), file);
-
       regtype = register_type (gdbarch, i);
       val = allocate_value (regtype);
 
       /* Get the data in raw format.  */
       if (! frame_register_read (frame, i, value_contents_raw (val)))
-	{
-	  fprintf_filtered (file, "*value not available*\n");
-	  continue;
-	}
-
-      /* If virtual format is floating, print it that way, and in raw
-         hex.  */
-      if (TYPE_CODE (regtype) == TYPE_CODE_FLT
-	  || TYPE_CODE (regtype) == TYPE_CODE_DECFLOAT)
-	{
-	  int j;
-	  struct value_print_options opts;
-	  const gdb_byte *valaddr = value_contents_for_printing (val);
-
-	  get_user_print_options (&opts);
-	  opts.deref_ref = 1;
-
-	  val_print (regtype,
-		     value_contents_for_printing (val),
-		     value_embedded_offset (val), 0,
-		     file, 0, val, &opts, current_language);
-
-	  fprintf_filtered (file, "\t(raw 0x");
-	  for (j = 0; j < register_size (gdbarch, i); j++)
-	    {
-	      int idx;
-
-	      if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
-		idx = j;
-	      else
-		idx = register_size (gdbarch, i) - 1 - j;
-	      fprintf_filtered (file, "%02x", (unsigned char) valaddr[idx]);
-	    }
-	  fprintf_filtered (file, ")");
-	}
-      else
-	{
-	  struct value_print_options opts;
-
-	  /* Print the register in hex.  */
-	  get_formatted_print_options (&opts, 'x');
-	  opts.deref_ref = 1;
-	  val_print (regtype,
-		     value_contents_for_printing (val),
-		     value_embedded_offset (val), 0,
-		     file, 0, val, &opts, current_language);
-          /* If not a vector register, print it also according to its
-             natural format.  */
-	  if (TYPE_VECTOR (regtype) == 0)
-	    {
-	      get_user_print_options (&opts);
-	      opts.deref_ref = 1;
-	      fprintf_filtered (file, "\t");
-	      val_print (regtype,
-			 value_contents_for_printing (val),
-			 value_embedded_offset (val), 0,
-			 file, 0, val, &opts, current_language);
-	    }
-	}
+	mark_value_bytes_unavailable (val, 0, TYPE_LENGTH (value_type (val)));
 
-      fprintf_filtered (file, "\n");
+      default_print_one_register_info (file,
+				       gdbarch_register_name (gdbarch, i),
+				       val);
     }
 }
 
@@ -2198,17 +2216,16 @@ registers_info (char *addr_exp, int fpregs)
 	    if (regnum >= gdbarch_num_regs (gdbarch)
 			  + gdbarch_num_pseudo_regs (gdbarch))
 	      {
-		struct value_print_options opts;
-		struct value *val = value_of_user_reg (regnum, frame);
-
-		printf_filtered ("%.*s: ", (int) (end - start), start);
-		get_formatted_print_options (&opts, 'x');
-		val_print_scalar_formatted (check_typedef (value_type (val)),
-					    value_contents_for_printing (val),
-					    value_embedded_offset (val),
-					    val,
-					    &opts, 0, gdb_stdout);
-		printf_filtered ("\n");
+		struct value *regval = value_of_user_reg (regnum, frame);
+		const char *regname = user_reg_map_regnum_to_name (gdbarch,
+								   regnum);
+
+		/* Print in the same fashion
+		   gdbarch_print_registers_info's default
+		   implementation prints.  */
+		default_print_one_register_info (gdb_stdout,
+						 regname,
+						 regval);
 	      }
 	    else
 	      gdbarch_print_registers_info (gdbarch, gdb_stdout,
diff --git a/gdb/testsuite/gdb.base/pc-fp.exp b/gdb/testsuite/gdb.base/pc-fp.exp
index beb5087..685e2d9 100644
--- a/gdb/testsuite/gdb.base/pc-fp.exp
+++ b/gdb/testsuite/gdb.base/pc-fp.exp
@@ -66,4 +66,4 @@ gdb_test "info register \$fp" "${valueof_fp}.*"
 # Regression test for
 # http://sourceware.org/bugzilla/show_bug.cgi?id=12659
 gdb_test "info register pc fp" \
-    "pc(:)?( |\t)+${valueof_pc}(( |\t)+${valueof_pc} <.*>)?\[\r\n\]+fp(:)?( |\t)+${valueof_fp}(( |\t)+${valueof_fp})?\[\r\n\]+"
+    "pc +${valueof_pc}\t${valueof_pc} <.*>\[\r\n\]+fp +${valueof_fp}\t${valueof_fp}\[\r\n\]+"

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> # Regression test for
>> # http://sourceware.org/bugzilla/show_bug.cgi?id=12659
>> gdb_test "info register pc fp" \
>> -    "pc: ${valueof_pc}\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"
>> + "pc(:)?.*${valueof_pc}(.*${hex} <.*>)?\[\r\n\]+fp:
>> ${valueof_fp}\[\r\n\]+"

Pedro> Relaxing the output like that means that inadvertent changes to x86's
Pedro> or ppc/s390x output might go unnoticed.  It's best to have

In this particular case, the check is really just to verify that the
named register, and nothing else, appears at the start of the line.

Before 12659 was fixed, "info register pc fp" printed:

sp fp: blah blah
fp: blah blah

The "fp" on the first line was the bogus bit.

I think the test would remain correct, with regards to what it was
intended to check, if it even went as far as "pc: .*\[\r\n\]+fp: .*";
checking the values is additional here.

Tom

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Pedro Alves]

On 08/01/2012 08:52 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
>>> # Regression test for
>>> # http://sourceware.org/bugzilla/show_bug.cgi?id=12659
>>> gdb_test "info register pc fp" \
>>> -    "pc: ${valueof_pc}\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"
>>> + "pc(:)?.*${valueof_pc}(.*${hex} <.*>)?\[\r\n\]+fp:
>>> ${valueof_fp}\[\r\n\]+"
> 
> Pedro> Relaxing the output like that means that inadvertent changes to x86's
> Pedro> or ppc/s390x output might go unnoticed.  It's best to have
> 
> In this particular case, the check is really just to verify that the
> named register, and nothing else, appears at the start of the line.
> 
> Before 12659 was fixed, "info register pc fp" printed:
> 
> sp fp: blah blah
> fp: blah blah
> 
> The "fp" on the first line was the bogus bit.
> 
> I think the test would remain correct, with regards to what it was
> intended to check, if it even went as far as "pc: .*\[\r\n\]+fp: .*";
> checking the values is additional here.

Ah, in that case, I agree.

-- 
Pedro Alves

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Sergio Durigan Junior]

On Wednesday, August 01 2012, Pedro Alves wrote:

> On 08/01/2012 08:52 PM, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>> 
>>>> # Regression test for
>>>> # http://sourceware.org/bugzilla/show_bug.cgi?id=12659>>> gdb_test "info register pc fp" \
>>>> -    "pc: ${valueof_pc}\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"
>>>> + "pc(:)?.*${valueof_pc}(.*${hex} <.*>)?\[\r\n\]+fp:
>>>> ${valueof_fp}\[\r\n\]+"
>> 
>> Pedro> Relaxing the output like that means that inadvertent changes to x86's
>> Pedro> or ppc/s390x output might go unnoticed.  It's best to have
>> 
>> In this particular case, the check is really just to verify that the
>> named register, and nothing else, appears at the start of the line.
>> 
>> Before 12659 was fixed, "info register pc fp" printed:
>> 
>> sp fp: blah blah
>> fp: blah blah
>> 
>> The "fp" on the first line was the bogus bit.
>> 
>> I think the test would remain correct, with regards to what it was
>> intended to check, if it even went as far as "pc: .*\[\r\n\]+fp: .*";
>> checking the values is additional here.
>
> Ah, in that case, I agree.

In this case the testcase would have to omit the `:' after the name of
the register, since as we have seen the colon is not always present.

I will open a bugzilla to register that we want to make the output of
`info register' uniform accross all platforms.

Is this patch OK then?

-- 
Sergio

2012-08-01  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.base/pc-fp.exp: Adjust testcase to match a wider range of
	outputs.

Index: src/gdb/testsuite/gdb.base/pc-fp.exp

===================================================================
--- src.orig/gdb/testsuite/gdb.base/pc-fp.exp
+++ src/gdb/testsuite/gdb.base/pc-fp.exp
@@ -66,4 +66,4 @@ gdb_test "info register \$fp" "${valueof
 # Regression test for
 # http://sourceware.org/bugzilla/show_bug.cgi?id=12659
 gdb_test "info register pc fp" \
-    "pc: ${valueof_pc}\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"
+    "pc.*\[\r\n\]+fp.*\[\r\n\]+"

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Pedro Alves]

On 08/01/2012 09:49 PM, Sergio Durigan Junior wrote:
> On Wednesday, August 01 2012, Pedro Alves wrote:

> I will open a bugzilla to register that we want to make the output of
> `info register' uniform accross all platforms.

Thanks.

> Is this patch OK then?

Fine with me.

-- 
Pedro Alves

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Andreas Schwab]

Sergio Durigan Junior <sergiodj@redhat.com> writes:

> Index: src/gdb/testsuite/gdb.base/pc-fp.exp
>
> ===================================================================
> --- src.orig/gdb/testsuite/gdb.base/pc-fp.exp
> +++ src/gdb/testsuite/gdb.base/pc-fp.exp
> @@ -66,4 +66,4 @@ gdb_test "info register \$fp" "${valueof
>  # Regression test for
>  # http://sourceware.org/bugzilla/show_bug.cgi?id=12659
>  gdb_test "info register pc fp" \
> -    "pc: ${valueof_pc}\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"
> +    "pc.*\[\r\n\]+fp.*\[\r\n\]+"

That will of course match the erroneous output.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Sergio Durigan Junior]

On Wednesday, August 01 2012, Andreas Schwab wrote:

> Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
>> Index: src/gdb/testsuite/gdb.base/pc-fp.exp
>>
>> ===================================================================
>> --- src.orig/gdb/testsuite/gdb.base/pc-fp.exp
>> +++ src/gdb/testsuite/gdb.base/pc-fp.exp
>> @@ -66,4 +66,4 @@ gdb_test "info register \$fp" "${valueof
>>  # Regression test for
>>  # http://sourceware.org/bugzilla/show_bug.cgi?id=12659>  gdb_test "info register pc fp" \
>> -    "pc: ${valueof_pc}\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"
>> +    "pc.*\[\r\n\]+fp.*\[\r\n\]+"
>
> That will of course match the erroneous output.

Ok, sorry.

Pedro, the patch below is the second version I sent, to handle Andreas'
observations on m68k-linux.  It is more specialized than the previous
patch.  Is it still OK to apply?

Thanks,

-- 
Sergio

Index: src/gdb/testsuite/gdb.base/pc-fp.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.base/pc-fp.exp
+++ src/gdb/testsuite/gdb.base/pc-fp.exp
@@ -66,4 +66,4 @@ gdb_test "info register \$fp" "${valueof
 # Regression test for
 # http://sourceware.org/bugzilla/show_bug.cgi?id=12659
 gdb_test "info register pc fp" \
-    "pc: ${valueof_pc}\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"
+    "pc(:)?( |\t)+${valueof_pc}(( |\t)+${valueof_pc} <.*>)?\[\r\n\]+fp(:)?( |\t)+${valueof_fp}(( |\t)+${valueof_fp})?\[\r\n\]+"

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Pedro Alves]

On 08/02/2012 12:40 AM, Sergio Durigan Junior wrote:
> On Wednesday, August 01 2012, Andreas Schwab wrote:
> 
>> Sergio Durigan Junior <sergiodj@redhat.com> writes:
>>
>>> Index: src/gdb/testsuite/gdb.base/pc-fp.exp
>>>
>>> ===================================================================
>>> --- src.orig/gdb/testsuite/gdb.base/pc-fp.exp
>>> +++ src/gdb/testsuite/gdb.base/pc-fp.exp
>>> @@ -66,4 +66,4 @@ gdb_test "info register \$fp" "${valueof
>>>  # Regression test for
>>>  # http://sourceware.org/bugzilla/show_bug.cgi?id=12659>  gdb_test "info register pc fp" \
>>> -    "pc: ${valueof_pc}\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"
>>> +    "pc.*\[\r\n\]+fp.*\[\r\n\]+"
>>
>> That will of course match the erroneous output.
> 
> Ok, sorry.
> 
> Pedro, the patch below is the second version I sent, to handle Andreas'
> observations on m68k-linux.  It is more specialized than the previous
> patch.  Is it still OK to apply?

Sure, thanks.

-- 
Pedro Alves

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Sergio Durigan Junior]

On Thursday, August 02 2012, Pedro Alves wrote:

> On 08/02/2012 12:40 AM, Sergio Durigan Junior wrote:
>> On Wednesday, August 01 2012, Andreas Schwab wrote:
>> 
>>> Sergio Durigan Junior <sergiodj@redhat.com> writes:
>>>
>>>> Index: src/gdb/testsuite/gdb.base/pc-fp.exp
>>>>
>>>> ===================================================================
>>>> --- src.orig/gdb/testsuite/gdb.base/pc-fp.exp
>>>> +++ src/gdb/testsuite/gdb.base/pc-fp.exp
>>>> @@ -66,4 +66,4 @@ gdb_test "info register \$fp" "${valueof
>>>>  # Regression test for
>>>>  # http://sourceware.org/bugzilla/show_bug.cgi?id=12659>  gdb_test "info register pc fp" \
>>>> -    "pc: ${valueof_pc}\[\r\n\]+fp: ${valueof_fp}\[\r\n\]+"
>>>> +    "pc.*\[\r\n\]+fp.*\[\r\n\]+"
>>>
>>> That will of course match the erroneous output.
>> 
>> Ok, sorry.
>> 
>> Pedro, the patch below is the second version I sent, to handle Andreas'
>> observations on m68k-linux.  It is more specialized than the previous
>> patch.  Is it still OK to apply?
>
> Sure, thanks.

Committed to trunk and 7.5.

          http://sourceware.org/ml/gdb-cvs/2012-08/msg00030.html
          http://sourceware.org/ml/gdb-cvs/2012-08/msg00031.html

-- 
Sergio

Re: [PATCH] Adjust `pc-fp.exp' for ppc64/s390x (PR 12659)

[Mark Kettenis]

> Date: Wed, 01 Aug 2012 09:45:50 +0100
> From: Pedro Alves <palves@redhat.com>
> 
> On 07/31/2012 10:25 PM, Sergio Durigan Junior wrote:
> > While regtesting 7.4 against 7.5 branch on ppc64/s390x RHEL 6.3, I
> > noticed this failure.  The patch which introduced this failure was
> > committed because of:
> > 
> >       http://sourceware.org/bugzilla/show_bug.cgi?id=12659
> > 
> > On x86*, the output of `info register pc fp' is:
> > 
> >     info register pc fp
> >     pc: 0x400520
> >     fp: 0x7fffffffc490
> >     (gdb) PASS: gdb.base/pc-fp.exp: info register pc fp
> > 
> > On ppc64/s390x, it is:
> > 
> >     info register pc fp
> >     pc             0x10000658       0x10000658 <main+20>
> >     fp: 0xfffffffd120
> >     (gdb) FAIL: gdb.base/pc-fp.exp: info register pc fp
> > 
> > Since this difference in the output does not seem to be an error itself,
> > the patch below just adjusts the testcase to match this kind of output
> > as well.  It does not fail on x86*.
> 
> Why is the output format different?  It looks like consistency here
> would be good.

Indeed.  So I'd say patching up the testsuite for this difference
would be the wrong way to go.