Re: [PING][PATCH] Align natural-format register values to the same column

[Ruslan Kabatsayev <b7 dot 10110111 at gmail dot com>]

Hi Simon,

On 23/07/17 22:46, Simon Marchi wrote:
> Hi Ruslan,
> 
> Thanks for the patch, it does indeed look pretty bad right now.
> 
> If we are going to do the alignment by hand using spaces (which seems like a good idea, as it gives more control), I'd rather not keep the tab as an historic artifact.  How many tests would have to be fixed, and how difficult would it be?
> 
I'm not sure how many, because I can't possibly run tests for all the architectures. Also, there are many tests with intermittent failures (racy), which make counting harder even for a single arch.

At first I did try to fix things for i386, but after fixing several dozens of lines of expected output I realized that it'll not fix anything for ARM etc., so I instead went with keeping the tab.

>>> Currently, commands such as "info reg", "info all-reg", as well as register
>>> window in the TUI print badly aligned columns, like here:
>>>
>>> eax            0x1      1
>>> ecx            0xffffd3e0       -11296
>>> edx            0xffffd404       -11260
>>> ebx            0xf7fa5ff4       -134586380
>>> esp            0xffffd390       0xffffd390
>>> ebp            0xffffd3c8       0xffffd3c8
>>> esi            0x0      0
>>> edi            0x0      0
>>> eip            0x8048b60        0x8048b60 <main+16>
>>> eflags         0x286    [ PF SF IF ]
>>> cs             0x23     35
>>> ss             0x2b     43
>>> ds             0x2b     43
>>> es             0x2b     43
>>> fs             0x0      0
>>> gs             0x63     99
>>>
>>> After this patch, these commands print the third column values consistently
>>> aligned one under another, provided the second column is not too long.
>>> Originally, the third column was (attempted to be) aligned using a simple tab
>>> character. Lots of tests in the test suite check for it, so this patch retains
>>> the tab in the output after the second column. This allows these tests to
>>> continue working unchanged. What is different is that now the tab may be
>>> followed by several spaces, which complete the task of aligning the third
>>> column when the sole tab doesn't work well.
>>>
>>> gdb/ChangeLog:
>>>
>>>     * infcmd.c (default_print_one_register_info): Align natural-
>>>     format column value consistently one after another.
>>> ---
>>>  gdb/infcmd.c |   27 ++++++++++++++++++---------
>>>  1 file changed, 18 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>>> index defa7b0..5de4e68 100644
>>> --- a/gdb/infcmd.c
>>> +++ b/gdb/infcmd.c
>>> @@ -2280,9 +2280,10 @@ default_print_one_register_info (struct ui_file *file,
>>>  {
>>>    struct type *regtype = value_type (val);
>>>    int print_raw_format;
>>> +  string_file format_stream;
>>>
>>> -  fputs_filtered (name, file);
>>> -  print_spaces_filtered (15 - strlen (name), file);
>>> +  format_stream.puts (name);
>>> +  format_stream.puts (n_spaces (15 - strlen (name)));
>>>
>>>    print_raw_format = (value_entirely_available (val)
>>>                && !value_optimized_out (val));
>>> @@ -2301,14 +2302,18 @@ default_print_one_register_info (struct ui_file *file,
>>>
>>>        val_print (regtype,
>>>           value_embedded_offset (val), 0,
>>> -         file, 0, val, &opts, current_language);
>>> +         &format_stream, 0, val, &opts, current_language);
>>>
>>>        if (print_raw_format)
>>>      {
>>> -      fprintf_filtered (file, "\t(raw ");
>>> -      print_hex_chars (file, valaddr, TYPE_LENGTH (regtype), byte_order,
>>> +      const int size_with_tab = format_stream.size () / 8 * 8 + 8;
>>> +      format_stream.putc ('\t');
>>> +      if (size_with_tab < 32)
>>> +          format_stream.puts (n_spaces (32 - size_with_tab));
> 
> Could you extract that constant (32) to a const variable with some meaningful name?  If we ever change the width, I predict that we'll forget to update the instances of the same 32 down there.
Yeah, I can do this. But should the magic 15 above be named as well (as 32 is the second "tab stop" while 15 is the first one)?
> 
> The padding could also be made into a small function to avoid repeating the code.
> 
>>> +      format_stream.puts ("(raw ");
>>> +      print_hex_chars (&format_stream, valaddr, TYPE_LENGTH (regtype), byte_order,
>>>                 true);
>>> -      fprintf_filtered (file, ")");
>>> +      format_stream.puts (")");
>>>      }
>>>      }
>>>    else
>>> @@ -2320,20 +2325,24 @@ default_print_one_register_info (struct ui_file *file,
>>>        opts.deref_ref = 1;
>>>        val_print (regtype,
>>>           value_embedded_offset (val), 0,
>>> -         file, 0, val, &opts, current_language);
>>> +         &format_stream, 0, val, &opts, current_language);
>>>        /* If not a vector register, print it also according to its
>>>       natural format.  */
>>>        if (print_raw_format && TYPE_VECTOR (regtype) == 0)
>>>      {
>>> +      const int size_with_tab = format_stream.size () / 8 * 8 + 8;
>>> +      format_stream.putc ('\t');
>>> +      if (size_with_tab < 32)
>>> +          format_stream.puts (n_spaces (32 - size_with_tab));
>>>        get_user_print_options (&opts);
>>>        opts.deref_ref = 1;
>>> -      fprintf_filtered (file, "\t");
>>>        val_print (regtype,
>>>               value_embedded_offset (val), 0,
>>> -             file, 0, val, &opts, current_language);
>>> +             &format_stream, 0, val, &opts, current_language);
>>>      }
>>>      }
>>>
>>> +  fputs_filtered (format_stream.c_str (), file);
>>>    fprintf_filtered (file, "\n");
>>>  }
>>>
>>>
> 
> Thanks!
> 
> Simon

Regards,
Ruslan