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

[Joel Brobecker <brobecker@adacore.com>]

> 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