[RFC/TileGX 5/6]show registers in columns

[RFC/TileGX 5/6]show registers in columns

[Jiong Wang]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

tilegx has 64 registers, to show them in row will be not very convinent 
to explore,
so we improve this by showing them in columns.

(gdb) info registers
r0   0x0000000000000001    r19  0x0000000000000000    r38 0x0000000000000040
r1   0x000001ffffc7f458         r20  0x000001ffffc7f278         r39 
0x0000000000000000
r2   0x000001ffffc7f468         r21  0x000001ffffc7f298         r40 
0x000000000015b3e8
r3   0x0000000000010720    r22  0x000001ffffc7f280         r41 
0x000000000015882c
...
...

gdb/ChangeLog:

        * tilegx-tdep.c (tilegx_print_registers_info): New function.
        show registers in columns.

please review.

---
Regards,
Jiong
Tilera Corporation.


[-- Attachment #2: 0006-show-registers-in-columns.patch --]
[-- Type: text/x-patch, Size: 2490 bytes --]

---
 gdb/tilegx-tdep.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 63 insertions(+)

diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index 7f36bed..c19bdf9 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -940,6 +940,66 @@ tilegx_cannot_reference_register (struct gdbarch *gdbarch, int regno)
     return 1;
 }
 
+
+/* Tilera private register printer.  */
+#define MAX_COLUMNS 3
+static void
+tilegx_print_registers_info (struct gdbarch *gdbarch, struct ui_file *file,
+                             struct frame_info *frame,
+                             int regnum, int print_all)
+{
+  int i;
+  int j;
+  int k;
+  gdb_byte buffer[MAX_REGISTER_SIZE];
+
+  if (regnum != -1)
+    {
+      default_print_registers_info (gdbarch, file, frame, regnum, print_all);
+      return;
+    }
+
+  for (i = 0; i < TILEGX_NUM_EASY_REGS / MAX_COLUMNS + 1; ++i)
+    {
+      for (j = i;
+           j < TILEGX_NUM_EASY_REGS + 1;
+           j += TILEGX_NUM_EASY_REGS / MAX_COLUMNS + 1)
+        {
+	  /* Some registers are never available. Skip them and print PC.  */
+	  if (j > TILEGX_LR_REGNUM)
+	    j = TILEGX_PC_REGNUM;
+
+	  if (j > i)
+	    fprintf_filtered (file, "    ");
+
+	  fputs_filtered (gdbarch_register_name (gdbarch, j), file);
+	  print_spaces_filtered (5 - strlen (gdbarch_register_name
+					     (gdbarch, j)), file);
+
+	  /* Get the data in raw format.  */
+	  if (! deprecated_frame_register_read (frame, j, buffer))
+	    {
+	      fprintf_filtered (file, " *** no value *** ");
+	    }
+	  else
+	    {
+	      fprintf_filtered (file, "0x");
+	      for (k = 0; k < tilegx_reg_size; k++)
+		{
+		  int idx;
+		  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
+		    idx = k;
+		  else
+		    idx = tilegx_reg_size - 1 - k;
+		  fprintf_filtered (file, "%02x", (unsigned char) buffer[idx]);
+		}
+	    }
+        }
+
+      fprintf_filtered (file, "\n");
+    }
+}
+
 static struct gdbarch *
 tilegx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 {
@@ -1000,6 +1060,9 @@ tilegx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* Stack grows down.  */
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
 
+  /* Tilera private register printer */
+  set_gdbarch_print_registers_info (gdbarch, tilegx_print_registers_info);
+
   /* Frame Info.  */
   set_gdbarch_unwind_sp (gdbarch, tilegx_unwind_sp);
   set_gdbarch_unwind_pc (gdbarch, tilegx_unwind_pc);
-- 
1.7.10.3

Re: [RFC/TileGX 5/6]show registers in columns

[Joel Brobecker]

> tilegx has 64 registers, to show them in row will be not very
> convinent to explore,
> so we improve this by showing them in columns.
> 
> (gdb) info registers
> r0   0x0000000000000001    r19  0x0000000000000000    r38 0x0000000000000040
> r1   0x000001ffffc7f458         r20  0x000001ffffc7f278         r39
> 0x0000000000000000
> r2   0x000001ffffc7f468         r21  0x000001ffffc7f298         r40
> 0x000000000015b3e8
> r3   0x0000000000010720    r22  0x000001ffffc7f280         r41
> 0x000000000015882c

Forgive me, but the above doesn't look easier to read... Was it
because of wrap-around by your mailer or your MTA?

> gdb/ChangeLog:
> 
>        * tilegx-tdep.c (tilegx_print_registers_info): New function.
>        show registers in columns.

I am not a big specialist of formatting this type of information.
To me the first 2 obvious remarks is that:

  - The number of columns should be dynamic, depending on the width
    of the terminal; and if the width is unset, then default to
    80 characters. We may also have to make a decision if the width
    is unlimited.

  - This will need confirmation from the other Global Maintainers
    but I think it would be better if you used the ui-out routines
    to print the registers.

One additional remark is that, with 64 registers, it might be good
enough to print them in two columns always, and not deal with
complexity of the terminal width. It even looks like you might be
able to fit in 3 columns within 80 chars, if you prefer...


> 
> please review.
> 
> ---
> Regards,
> Jiong
> Tilera Corporation.
> 

> ---
>  gdb/tilegx-tdep.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 63 insertions(+)
> 
> diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
> index 7f36bed..c19bdf9 100644
> --- a/gdb/tilegx-tdep.c
> +++ b/gdb/tilegx-tdep.c
> @@ -940,6 +940,66 @@ tilegx_cannot_reference_register (struct gdbarch *gdbarch, int regno)
>      return 1;
>  }
>  
> +
> +/* Tilera private register printer.  */
> +#define MAX_COLUMNS 3
> +static void
> +tilegx_print_registers_info (struct gdbarch *gdbarch, struct ui_file *file,
> +                             struct frame_info *frame,
> +                             int regnum, int print_all)
> +{
> +  int i;
> +  int j;
> +  int k;
> +  gdb_byte buffer[MAX_REGISTER_SIZE];
> +
> +  if (regnum != -1)
> +    {
> +      default_print_registers_info (gdbarch, file, frame, regnum, print_all);
> +      return;
> +    }
> +
> +  for (i = 0; i < TILEGX_NUM_EASY_REGS / MAX_COLUMNS + 1; ++i)
> +    {
> +      for (j = i;
> +           j < TILEGX_NUM_EASY_REGS + 1;
> +           j += TILEGX_NUM_EASY_REGS / MAX_COLUMNS + 1)
> +        {
> +	  /* Some registers are never available. Skip them and print PC.  */
> +	  if (j > TILEGX_LR_REGNUM)
> +	    j = TILEGX_PC_REGNUM;
> +
> +	  if (j > i)
> +	    fprintf_filtered (file, "    ");
> +
> +	  fputs_filtered (gdbarch_register_name (gdbarch, j), file);
> +	  print_spaces_filtered (5 - strlen (gdbarch_register_name
> +					     (gdbarch, j)), file);
> +
> +	  /* Get the data in raw format.  */
> +	  if (! deprecated_frame_register_read (frame, j, buffer))
> +	    {
> +	      fprintf_filtered (file, " *** no value *** ");
> +	    }
> +	  else
> +	    {
> +	      fprintf_filtered (file, "0x");
> +	      for (k = 0; k < tilegx_reg_size; k++)
> +		{
> +		  int idx;
> +		  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> +		    idx = k;
> +		  else
> +		    idx = tilegx_reg_size - 1 - k;
> +		  fprintf_filtered (file, "%02x", (unsigned char) buffer[idx]);
> +		}
> +	    }
> +        }
> +
> +      fprintf_filtered (file, "\n");
> +    }
> +}
> +
>  static struct gdbarch *
>  tilegx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  {
> @@ -1000,6 +1060,9 @@ tilegx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    /* Stack grows down.  */
>    set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
>  
> +  /* Tilera private register printer */
> +  set_gdbarch_print_registers_info (gdbarch, tilegx_print_registers_info);
> +
>    /* Frame Info.  */
>    set_gdbarch_unwind_sp (gdbarch, tilegx_unwind_sp);
>    set_gdbarch_unwind_pc (gdbarch, tilegx_unwind_pc);
> -- 
> 1.7.10.3
> 
> 


-- 
Joel

Re: [RFC/TileGX 5/6]show registers in columns

[Jiong Wang]

[-- Attachment #1: Type: text/plain, Size: 4866 bytes --]

于 2013/1/18 21:50, Joel Brobecker 写道:
>> tilegx has 64 registers, to show them in row will be not very
>> convinent to explore,
>> so we improve this by showing them in columns.
>>
>> (gdb) info registers
>> r0   0x0000000000000001    r19  0x0000000000000000    r38 0x0000000000000040
>> r1   0x000001ffffc7f458         r20  0x000001ffffc7f278         r39
>> 0x0000000000000000
>> r2   0x000001ffffc7f468         r21  0x000001ffffc7f298         r40
>> 0x000000000015b3e8
>> r3   0x0000000000010720    r22  0x000001ffffc7f280         r41
>> 0x000000000015882c
> Forgive me, but the above doesn't look easier to read... Was it
> because of wrap-around by your mailer or your MTA?

yes, I think it's caused by my mailer. please see attachment for the 
comparision

>
>> gdb/ChangeLog:
>>
>>         * tilegx-tdep.c (tilegx_print_registers_info): New function.
>>         show registers in columns.
> I am not a big specialist of formatting this type of information.
> To me the first 2 obvious remarks is that:
>
>    - The number of columns should be dynamic, depending on the width
>      of the terminal; and if the width is unset, then default to
>      80 characters. We may also have to make a decision if the width
>      is unlimited.
>
>    - This will need confirmation from the other Global Maintainers
>      but I think it would be better if you used the ui-out routines
>      to print the registers.
>
> One additional remark is that, with 64 registers, it might be good
> enough to print them in two columns always, and not deal with
> complexity of the terminal width. It even looks like you might be
> able to fit in 3 columns within 80 chars, if you prefer...

thanks for your good suggestion.
yes, 3 columns still be within 80 chars. and by display in 3 columns, it 
is with larger probability that the user do not need to scroll down and 
up his mouse to view the results. by 2 columns, the list is still a bit 
long, and the information is not that centered.

So, is this patch OK?
>
>
>> please review.
>>
>> ---
>> Regards,
>> Jiong
>> Tilera Corporation.
>>
>> ---
>>   gdb/tilegx-tdep.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 63 insertions(+)
>>
>> diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
>> index 7f36bed..c19bdf9 100644
>> --- a/gdb/tilegx-tdep.c
>> +++ b/gdb/tilegx-tdep.c
>> @@ -940,6 +940,66 @@ tilegx_cannot_reference_register (struct gdbarch *gdbarch, int regno)
>>       return 1;
>>   }
>>   
>> +
>> +/* Tilera private register printer.  */
>> +#define MAX_COLUMNS 3
>> +static void
>> +tilegx_print_registers_info (struct gdbarch *gdbarch, struct ui_file *file,
>> +                             struct frame_info *frame,
>> +                             int regnum, int print_all)
>> +{
>> +  int i;
>> +  int j;
>> +  int k;
>> +  gdb_byte buffer[MAX_REGISTER_SIZE];
>> +
>> +  if (regnum != -1)
>> +    {
>> +      default_print_registers_info (gdbarch, file, frame, regnum, print_all);
>> +      return;
>> +    }
>> +
>> +  for (i = 0; i < TILEGX_NUM_EASY_REGS / MAX_COLUMNS + 1; ++i)
>> +    {
>> +      for (j = i;
>> +           j < TILEGX_NUM_EASY_REGS + 1;
>> +           j += TILEGX_NUM_EASY_REGS / MAX_COLUMNS + 1)
>> +        {
>> +	  /* Some registers are never available. Skip them and print PC.  */
>> +	  if (j > TILEGX_LR_REGNUM)
>> +	    j = TILEGX_PC_REGNUM;
>> +
>> +	  if (j > i)
>> +	    fprintf_filtered (file, "    ");
>> +
>> +	  fputs_filtered (gdbarch_register_name (gdbarch, j), file);
>> +	  print_spaces_filtered (5 - strlen (gdbarch_register_name
>> +					     (gdbarch, j)), file);
>> +
>> +	  /* Get the data in raw format.  */
>> +	  if (! deprecated_frame_register_read (frame, j, buffer))
>> +	    {
>> +	      fprintf_filtered (file, " *** no value *** ");
>> +	    }
>> +	  else
>> +	    {
>> +	      fprintf_filtered (file, "0x");
>> +	      for (k = 0; k < tilegx_reg_size; k++)
>> +		{
>> +		  int idx;
>> +		  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
>> +		    idx = k;
>> +		  else
>> +		    idx = tilegx_reg_size - 1 - k;
>> +		  fprintf_filtered (file, "%02x", (unsigned char) buffer[idx]);
>> +		}
>> +	    }
>> +        }
>> +
>> +      fprintf_filtered (file, "\n");
>> +    }
>> +}
>> +
>>   static struct gdbarch *
>>   tilegx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>   {
>> @@ -1000,6 +1060,9 @@ tilegx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>     /* Stack grows down.  */
>>     set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
>>   
>> +  /* Tilera private register printer */
>> +  set_gdbarch_print_registers_info (gdbarch, tilegx_print_registers_info);
>> +
>>     /* Frame Info.  */
>>     set_gdbarch_unwind_sp (gdbarch, tilegx_unwind_sp);
>>     set_gdbarch_unwind_pc (gdbarch, tilegx_unwind_pc);
>> -- 
>> 1.7.10.3
>>
>>
>


[-- Attachment #2: display-compare.tar.bz2 --]
[-- Type: application/octet-stream, Size: 1260 bytes --]

Re: [RFC/TileGX 5/6]show registers in columns

[Joel Brobecker]

> yes, I think it's caused by my mailer. please see attachment for the
> comparision

One small request. For files as small as the ones you can, can you
avoid packing them. It requires extra work to read them...

> >To me the first 2 obvious remarks is that:
> >
> >   - The number of columns should be dynamic, depending on the width
> >     of the terminal; and if the width is unset, then default to
> >     80 characters. We may also have to make a decision if the width
> >     is unlimited.
> >
> >   - This will need confirmation from the other Global Maintainers
> >     but I think it would be better if you used the ui-out routines
> >     to print the registers.
> >
> >One additional remark is that, with 64 registers, it might be good
> >enough to print them in two columns always, and not deal with
> >complexity of the terminal width. It even looks like you might be
> >able to fit in 3 columns within 80 chars, if you prefer...
> 
> thanks for your good suggestion.
> yes, 3 columns still be within 80 chars. and by display in 3
> columns, it is with larger probability that the user do not need to
> scroll down and up his mouse to view the results. by 2 columns, the
> list is still a bit long, and the information is not that centered.
> 
> So, is this patch OK?

IMO, if would be better if you used the ui-out routines to display
the data, rather than fprintf_filtered. But I am not a big specialist
in this area. Maybe Tom knows better and could help review the
printing part.

> >>+/* Tilera private register printer.  */
> >>+#define MAX_COLUMNS 3

The name of the macro needs to be changed, if the number of columns
is going to be static. And The comment also needs to be more specific.


> >>+static void
> >>+tilegx_print_registers_info (struct gdbarch *gdbarch, struct ui_file *file,
> >>+                             struct frame_info *frame,
> >>+                             int regnum, int print_all)

The usual "/* Implement the .... gdbarch method. */" function description
is missing... It would also be nice to extend the function description
to explain why you are providing your own print function, rather than
relying on the default one.  If someone stumbles on this at a later
time, such a comment will help him understand what the motivation was,
without needing to reverse-engineer the code.

> >>+{
> >>+  int i;
> >>+  int j;
> >>+  int k;
> >>+  gdb_byte buffer[MAX_REGISTER_SIZE];
> >>+
> >>+  if (regnum != -1)
> >>+    {
> >>+      default_print_registers_info (gdbarch, file, frame, regnum, print_all);
> >>+      return;
> >>+    }
> >>+
> >>+  for (i = 0; i < TILEGX_NUM_EASY_REGS / MAX_COLUMNS + 1; ++i)
> >>+    {
> >>+      for (j = i;
> >>+           j < TILEGX_NUM_EASY_REGS + 1;
> >>+           j += TILEGX_NUM_EASY_REGS / MAX_COLUMNS + 1)
> >>+        {
> >>+	  /* Some registers are never available. Skip them and print PC.  */
> >>+	  if (j > TILEGX_LR_REGNUM)
> >>+	    j = TILEGX_PC_REGNUM;

This looks pretty bad to me. I'd much rather you explicitly excluded
the registers you do not want to print.

One nit: 2 spaces after a period (in your comment).

> >>+
> >>+	  if (j > i)
> >>+	    fprintf_filtered (file, "    ");
> >>+
> >>+	  fputs_filtered (gdbarch_register_name (gdbarch, j), file);
> >>+	  print_spaces_filtered (5 - strlen (gdbarch_register_name
> >>+					     (gdbarch, j)), file);
> >>+
> >>+	  /* Get the data in raw format.  */
> >>+	  if (! deprecated_frame_register_read (frame, j, buffer))

Please do not add a new usage of a deprecated function. Use
get_frame_register_value instead.

> >>+	    {
> >>+	      fprintf_filtered (file, " *** no value *** ");
> >>+	    }

Small style issue: With only one statement inside a block, we omit
the curly braces.

> >>+	  else
> >>+	    {
> >>+	      fprintf_filtered (file, "0x");
> >>+	      for (k = 0; k < tilegx_reg_size; k++)
> >>+		{
> >>+		  int idx;
> >>+		  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> >>+		    idx = k;
> >>+		  else
> >>+		    idx = tilegx_reg_size - 1 - k;
> >>+		  fprintf_filtered (file, "%02x", (unsigned char) buffer[idx]);

I do not think that this is the right way of printing the register
value, by doing all the work yourself. I am not completely sure
what the best solution would be, though. The other Global Maintainers
may have a more definitive answer, but perhaps plongest (value_as_long
(register_value)). My suggestion is a little iffy because it assumes
for instance that all register values are signed / or unsigned.

> >>+		}
> >>+	    }
> >>+        }
> >>+
> >>+      fprintf_filtered (file, "\n");
> >>+    }
> >>+}
> >>+
> >>  static struct gdbarch *
> >>  tilegx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> >>  {
> >>@@ -1000,6 +1060,9 @@ tilegx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> >>    /* Stack grows down.  */
> >>    set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
> >>+  /* Tilera private register printer */
> >>+  set_gdbarch_print_registers_info (gdbarch, tilegx_print_registers_info);
> >>+
> >>    /* Frame Info.  */
> >>    set_gdbarch_unwind_sp (gdbarch, tilegx_unwind_sp);
> >>    set_gdbarch_unwind_pc (gdbarch, tilegx_unwind_pc);

-- 
Joel

Re: [RFC/TileGX 5/6]show registers in columns

[Pedro Alves]

On 02/08/2013 07:09 PM, Joel Brobecker wrote:

>>>> +	  else
>>>> +	    {
>>>> +	      fprintf_filtered (file, "0x");
>>>> +	      for (k = 0; k < tilegx_reg_size; k++)
>>>> +		{
>>>> +		  int idx;
>>>> +		  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
>>>> +		    idx = k;
>>>> +		  else
>>>> +		    idx = tilegx_reg_size - 1 - k;
>>>> +		  fprintf_filtered (file, "%02x", (unsigned char) buffer[idx]);
> 
> 	I do not think that this is the right way of printing the register
> value, by doing all the work yourself. I am not completely sure
> what the best solution would be, though. The other Global Maintainers
> may have a more definitive answer, but perhaps plongest (value_as_long
> (register_value)). My suggestion is a little iffy because it assumes
> for instance that all register values are signed / or unsigned.

Usually you'd use val_print.  How about just exporting and
calling default_print_one_register_info?  You get consistency
with other archs for free, and unavailable values handled too.

-- 
Pedro Alves

Re: [RFC/TileGX 5/6]show registers in columns

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> IMO, if would be better if you used the ui-out routines to display
Joel> the data, rather than fprintf_filtered. But I am not a big specialist
Joel> in this area. Maybe Tom knows better and could help review the
Joel> printing part.

AFAICT, this is only ever called from the CLI and the TUI.
So plain printing is probably ok.
But I also thought Pedro's suggestion was good.

Tom

Re: [RFC/TileGX 5/6]show registers in columns

[Jiong Wang]

Hi all,

   thanks for all these comments, happy Chinese lunar new year!

>>> +		  fprintf_filtered (file, "%02x", (unsigned char) buffer[idx]);
>> 	I do not think that this is the right way of printing the register
>> value, by doing all the work yourself. I am not completely sure
>> what the best solution would be, though. The other Global Maintainers
>> may have a more definitive answer, but perhaps plongest (value_as_long
>> (register_value)). My suggestion is a little iffy because it assumes
>> for instance that all register values are signed / or unsigned.
> Usually you'd use val_print.

    the reason why we do not use val_print is because, for a zero value, 
for x86, the default output is:

rax            0x7ffff7bd5f60  140737349771104
rbx            0x0     0
rcx            0x0      0

while I think it's better that the output is padded with zero which is 
MIPS's approach, for example:

rax            0x00007FFFF7BD5F60
rbx            0x000000000000000
rcx            0x000000000000000

> How about just exporting and
> calling default_print_one_register_info?  You get consistency
> with other archs for free, and unavailable values handled too.

good, I find some code is factored out as 
"default_print_one_register_info" since Aug, 2012,
but it always print a '\n' for each register , so it can not used for 
multi column output.

and this function print register value in both hex format and natural 
format, it consumes extra
width that there is no enough space left for multi columns.

all these are quite subjective,  I think this patch do not affect the 
correctness of tilegx gdb, so I do not
insist on it.

thanks.

---
Regards,
Jiong