[RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Doug Evans]

Hi.

I couldn't find an explanation of *why* ui_out_field_core_addr pads
the address with leading zeroes.

Does anyone know?

2010-03-18  Doug Evans  <dje@google.com>

	* ui-out.c (ui_out_field_core_addr): Don't pad address with leading
	zeroes.

Index: ui-out.c
===================================================================
RCS file: /cvs/src/src/gdb/ui-out.c,v
retrieving revision 1.45
diff -u -p -r1.45 ui-out.c
--- ui-out.c	12 Jan 2010 21:40:24 -0000	1.45
+++ ui-out.c	18 Mar 2010 17:19:07 -0000
@@ -498,12 +498,8 @@ ui_out_field_core_addr (struct ui_out *u
     address &= ((CORE_ADDR) 1 << addr_bit) - 1;
 
   /* FIXME: cagney/2002-05-03: Need local_address_string() function
-     that returns the language localized string formatted to a width
-     based on gdbarch_addr_bit.  */
-  if (addr_bit <= 32)
-    strcpy (addstr, hex_string_custom (address, 8));
-  else
-    strcpy (addstr, hex_string_custom (address, 16));
+     that returns the language localized string.  */
+  strcpy (addstr, hex_string (address));
 
   ui_out_field_string (uiout, fldname, addstr);
 }

E.g.

Before:

(gdb) b main
Breakpoint 1 at 0x40049c: file hello.c, line 6.
(gdb) i b
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   0x000000000040049c in main at hello.c:6
(gdb) 

After:

(gdb) b main
Breakpoint 1 at 0x40049c: file hello.c, line 6.
(gdb) i b
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   0x40049c           in main at hello.c:6
(gdb) 

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Pedro Alves]

On Thursday 18 March 2010 17:30:15, Doug Evans wrote:
> I couldn't find an explanation of why ui_out_field_core_addr pads
> the address with leading zeroes.
> 
> Does anyone know?

Personally, I find that zeros on the left serve as
a hint at the largest possible address width in
the architecture.  That'd be my educated guess.

It also aids visual alignment I suppose.  Try also adding
a breakpoint in a shared library in your example,
and redoing `info break'. The column is presently
aligned left.  I suspect it won't look as neat after
your patch.

-- 
Pedro Alves

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Joel Brobecker]

> I couldn't find an explanation of *why* ui_out_field_core_addr pads
> the address with leading zeroes.
> 
> Does anyone know?

I don't (it was probably even before my time). But this raised an
interesting question: Initially, I thought that the current behavior
is much nicer. But then I looked at both outputs again, and asked
myself: which one is easier/faster to read? FWIW, I'm leaning towards
your suggestion...

-- 
Joel

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Doug Evans]

On Thu, Mar 18, 2010 at 10:47 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Thursday 18 March 2010 17:30:15, Doug Evans wrote:
>> I couldn't find an explanation of why ui_out_field_core_addr pads
>> the address with leading zeroes.
>>
>> Does anyone know?
>
> Personally, I find that zeros on the left serve as
> a hint at the largest possible address width in
> the architecture.  That'd be my educated guess.

There are other ways to find that without sacrificing readability.

> It also aids visual alignment I suppose.  Try also adding
> a breakpoint in a shared library in your example,
> and redoing `info break'. The column is presently
> aligned left.  I suspect it won't look as neat after
> your patch.

I realize that *could* happen, but since we're doing this fancy ui-out
thing, I would expect it's the job of the front end to do any desired
alignment.

For reference sake, this is what I get with my patch:

(gdb) b main
Breakpoint 2 at 0x40049c: file hello.c, line 6.
(gdb) i b
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   0x40049c           in main at hello.c:6
(gdb) b printf
Breakpoint 3 at 0x7ffff7acbe40
(gdb) i b
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   0x40049c           in main at hello.c:6
3       breakpoint     keep y   0x7ffff7acbe40     <printf>
(gdb)

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Mark Kettenis]

> Date: Thu, 18 Mar 2010 10:30:15 -0700 (PDT)
> From: dje@google.com (Doug Evans)
> 
> Hi.
> 
> I couldn't find an explanation of *why* ui_out_field_core_addr pads
> the address with leading zeroes.
> 
> Does anyone know?

To make sure the addresses line up properly?

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Mark Kettenis]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

> Date: Thu, 18 Mar 2010 11:23:38 -0700
> From: Doug Evans <dje@google.com>
> 
> > It also aids visual alignment I suppose.  Try also adding
> > a breakpoint in a shared library in your example,
> > and redoing `info break'. The column is presently
> > aligned left.  I suspect it won't look as neat after
> > your patch.
> 
> I realize that *could* happen, but since we're doing this fancy ui-out
> thing, I would expect it's the job of the front end to do any desired
> alignment.
> 
> For reference sake, this is what I get with my patch:
> 
> (gdb) b main
> Breakpoint 2 at 0x40049c: file hello.c, line 6.
> (gdb) i b
> Num     Type           Disp Enb Address            What
> 2       breakpoint     keep y   0x40049c           in main at hello.c:6
> (gdb) b printf
> Breakpoint 3 at 0x7ffff7acbe40
> (gdb) i b
> Num     Type           Disp Enb Address            What
> 2       breakpoint     keep y   0x40049c           in main at hello.c:6
> 3       breakpoint     keep y   0x7ffff7acbe40     <printf>
> (gdb)

So since the CLI obviously doesn't align the unpadded addresses
properly, I'd say your diff shouldn't go in until it does.

Cheers,

Mark

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Doug Evans]

On Thu, Mar 18, 2010 at 11:55 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> Date: Thu, 18 Mar 2010 11:23:38 -0700
>> From: Doug Evans <dje@google.com>
>>
>> > It also aids visual alignment I suppose.  Try also adding
>> > a breakpoint in a shared library in your example,
>> > and redoing `info break'. The column is presently
>> > aligned left.  I suspect it won't look as neat after
>> > your patch.
>>
>> I realize that *could* happen, but since we're doing this fancy ui-out
>> thing, I would expect it's the job of the front end to do any desired
>> alignment.
>>
>> For reference sake, this is what I get with my patch:
>>
>> (gdb) b main
>> Breakpoint 2 at 0x40049c: file hello.c, line 6.
>> (gdb) i b
>> Num     Type           Disp Enb Address            What
>> 2       breakpoint     keep y   0x40049c           in main at hello.c:6
>> (gdb) b printf
>> Breakpoint 3 at 0x7ffff7acbe40
>> (gdb) i b
>> Num     Type           Disp Enb Address            What
>> 2       breakpoint     keep y   0x40049c           in main at hello.c:6
>> 3       breakpoint     keep y   0x7ffff7acbe40     <printf>
>> (gdb)
>
> So since the CLI obviously doesn't align the unpadded addresses
> properly, I'd say your diff shouldn't go in until it does.

I'm not sure I understand.  Clearly there is some alignment going on
otherwise the "What" column would be all messed up, but it is not.
I'm going to assume you're saying that the proper alignment would be
to right-justify the addresses.  Correct me if this assumption is
wrong.

Having stared at both alignments, I don't see aligning the address to
the right as being easier to read than aligning them to the left, and
even less so.

Right Aligned

(gdb) i b
Num     Type           Disp Enb            Address What
2       breakpoint     keep y             0x40049c in main at hello.c:6
3       breakpoint     keep y       0x7ffff7acbe40 <printf>
(gdb)

Left Aligned

(gdb) i b
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   0x40049c           in main at hello.c:6
3       breakpoint     keep y   0x7ffff7acbe40     <printf>
(gdb)

Did you have something else in mind?

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Jan Kratochvil]

On Thu, 18 Mar 2010 20:19:11 +0100, Doug Evans wrote:
> Having stared at both alignments, I don't see aligning the address to
> the right as being easier to read than aligning them to the left, and
> even less so.
> 
> Right Aligned
> 
> (gdb) i b
> Num     Type           Disp Enb            Address What
> 2       breakpoint     keep y             0x40049c in main at hello.c:6
> 3       breakpoint     keep y       0x7ffff7acbe40 <printf>
> (gdb)
> 
> Left Aligned
> 
> (gdb) i b
> Num     Type           Disp Enb Address            What
> 2       breakpoint     keep y   0x40049c           in main at hello.c:6
> 3       breakpoint     keep y   0x7ffff7acbe40     <printf>
> (gdb)

I am definitely for the "Right Aligned" one and against the "Left Aligned"
one.  While analyzing various crash dumps one looks for the same last three
digits (ignoring PIC/PIE different PAGE_SIZE-aligned placement).  Also
sometimes sorting addresses from various sources by hand to find out the
memory layout and having to additionally re-align even the GDB dump itself is
not helpful.


Thanks,
Jan

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Doug Evans]

On Thu, Mar 18, 2010 at 12:25 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Thu, 18 Mar 2010 20:19:11 +0100, Doug Evans wrote:
>> Having stared at both alignments, I don't see aligning the address to
>> the right as being easier to read than aligning them to the left, and
>> even less so.
>>
>> Right Aligned
>>
>> (gdb) i b
>> Num     Type           Disp Enb            Address What
>> 2       breakpoint     keep y             0x40049c in main at hello.c:6
>> 3       breakpoint     keep y       0x7ffff7acbe40 <printf>
>> (gdb)
>>
>> Left Aligned
>>
>> (gdb) i b
>> Num     Type           Disp Enb Address            What
>> 2       breakpoint     keep y   0x40049c           in main at hello.c:6
>> 3       breakpoint     keep y   0x7ffff7acbe40     <printf>
>> (gdb)
>
> I am definitely for the "Right Aligned" one and against the "Left Aligned"
> one.  While analyzing various crash dumps one looks for the same last three
> digits (ignoring PIC/PIE different PAGE_SIZE-aligned placement).  Also
> sometimes sorting addresses from various sources by hand to find out the
> memory layout and having to additionally re-align even the GDB dump itself is
> not helpful.

[It seemed like ui-out should make this easy, and voila.]

2010-03-18  Doug Evans  <dje@google.com>

        * ui-out.c (ui_out_field_core_addr): Don't pad address with leading
        zeroes.
        * breakpoint.c (breakpoint_1): Right align breakpoint addresses.

Index: ui-out.c
===================================================================
RCS file: /cvs/src/src/gdb/ui-out.c,v
retrieving revision 1.45
diff -u -p -r1.45 ui-out.c
--- ui-out.c    12 Jan 2010 21:40:24 -0000      1.45
+++ ui-out.c    18 Mar 2010 17:19:07 -0000
@@ -498,12 +498,8 @@ ui_out_field_core_addr (struct ui_out *u
     address &= ((CORE_ADDR) 1 << addr_bit) - 1;

   /* FIXME: cagney/2002-05-03: Need local_address_string() function
-     that returns the language localized string formatted to a width
-     based on gdbarch_addr_bit.  */
-  if (addr_bit <= 32)
-    strcpy (addstr, hex_string_custom (address, 8));
-  else
-    strcpy (addstr, hex_string_custom (address, 16));
+     that returns the language localized string.  */
+  strcpy (addstr, hex_string (address));

   ui_out_field_string (uiout, fldname, addstr);
 }
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.462
diff -u -p -r1.462 breakpoint.c
--- breakpoint.c        16 Mar 2010 08:42:20 -0000      1.462
+++ breakpoint.c        18 Mar 2010 19:35:08 -0000
@@ -4743,9 +4743,9 @@ breakpoint_1 (int bnum, int allflag)
          if (nr_printable_breakpoints > 0)
            annotate_field (4);
          if (print_address_bits <= 32)
-           ui_out_table_header (uiout, 10, ui_left, "addr", "Address");/* 5 */
+           ui_out_table_header (uiout, 10, ui_right, "addr", "Address");/* 5 */
          else
-           ui_out_table_header (uiout, 18, ui_left, "addr", "Address");/* 5 */
+           ui_out_table_header (uiout, 18, ui_right, "addr", "Address");/* 5 */
        }
   if (nr_printable_breakpoints > 0)
     annotate_field (5);

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Pedro Alves]

Did you try grepping for other uses of ui_out_field_core_addr?
Would other places need adjustment as well?
And what about posting the before-with-no-patch-what-soever
vs after-patch-that-aligns-right examples of "info break",
instead of just leftalign-after-patch vs leftalign-after-patch?

-- 
Pedro Alves

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Daniel Jacobowitz]

On Thu, Mar 18, 2010 at 12:38:09PM -0700, Doug Evans wrote:
> 2010-03-18  Doug Evans  <dje@google.com>
> 
>         * ui-out.c (ui_out_field_core_addr): Don't pad address with leading
>         zeroes.
>         * breakpoint.c (breakpoint_1): Right align breakpoint addresses.

Does this right align the header, or just the contents?

I may be alone, and at risk of bikeshedding, but I think this looks
weird:

Num         Address
1            0x4414
2      0x1231230000

I'm more a fan of:

Num  Address
1               0x4414
2         0x1231230000

[They both look a bit weird to me though.  My ideal UI for this stuff
resizes based on contents...]

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Pedro Alves]

On Thursday 18 March 2010 19:51:05, Daniel Jacobowitz wrote:
> I may be alone, and at risk of bikeshedding, but I think this looks
> weird:

You're not.  That was the reason I asked for before-any-patch,
vs currently proposed change.

> [They both look a bit weird to me though.  My ideal UI for this stuff
> resizes based on contents...]

We do this currently, but the code bases on address
bits, assuming we'll print the leading zeros.

-- 
Pedro Alves

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Mark Kettenis]

> Date: Thu, 18 Mar 2010 15:51:05 -0400
> From: Daniel Jacobowitz <dan@codesourcery.com>
>
> I may be alone, and at risk of bikeshedding, but I think this looks
> weird:
> 
> Num         Address
> 1            0x4414
> 2      0x1231230000
> 
> I'm more a fan of:
> 
> Num  Address
> 1               0x4414
> 2         0x1231230000
> 
> [They both look a bit weird to me though.  My ideal UI for this stuff
> resizes based on contents...]

Yup; I actually think the way this is currently printed isn't so bad
after all.

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Doug Evans]

On Thu, Mar 18, 2010 at 12:47 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> Did you try grepping for other uses of ui_out_field_core_addr?
> Would other places need adjustment as well?

Before submitting the patch RFA I would indeed check for other uses.
I wasn't ready to go that far yet.

> And what about posting the before-with-no-patch-what-soever
> vs after-patch-that-aligns-right examples of "info break",
> instead of just leftalign-after-patch vs leftalign-after-patch?

As you wish.

Before, without patch:

(gdb) i b
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   0x000000000040049c in main at hello.c:6
3       breakpoint     keep y   0x00007ffff7acbe40 <printf>

After patch:

(gdb) i b
Num     Type           Disp Enb            Address What
2       breakpoint     keep y             0x40049c in main at hello.c:6
3       breakpoint     keep y       0x7ffff7acbe40 <printf>

Re: [RFC] Why does ui_out_field_core_addr pad with leading zeroes?

[Doug Evans]

On Thu, Mar 18, 2010 at 12:51 PM, Daniel Jacobowitz
<dan@codesourcery.com> wrote:
> On Thu, Mar 18, 2010 at 12:38:09PM -0700, Doug Evans wrote:
>> 2010-03-18  Doug Evans  <dje@google.com>
>>
>>         * ui-out.c (ui_out_field_core_addr): Don't pad address with leading
>>         zeroes.
>>         * breakpoint.c (breakpoint_1): Right align breakpoint addresses.
>
> Does this right align the header, or just the contents?
>
> I may be alone, and at risk of bikeshedding, but I think this looks
> weird:
>
> Num         Address
> 1            0x4414
> 2      0x1231230000
>
> I'm more a fan of:
>
> Num  Address
> 1               0x4414
> 2         0x1231230000

Would you left justify all headers with right-justified data?
Or should ui-out support differentiating header justification with the data's?
Feels like a stretch to me.

> [They both look a bit weird to me though.  My ideal UI for this stuff
> resizes based on contents...]

Yeah.