[rfc] More intelligent indenting of multi-line table entries (Re: [RFA] Implement support for PowerPC BookE ranged breakpoints)

Ulrich Weigand
2011-02-24T20:41:26Z

Thiago Jung Bauermann wrote:

I'll get back to you with a full review of the latest patch, I just wanted
to quickly address one specific issue:

> > > +/* Implement the "print_one" breakpoint_ops method for
> > > + ranged breakpoints. */
> > > +
> > > +static void
> > > +print_one_ranged_breakpoint (struct breakpoint *b,
> > > + struct bp_location **last_loc,
> > > + char *wrap_indent, struct ui_stream *stb)
> > > +{
> > > + struct value_print_options opts;
> > > +
> > > + /* We're prepared to deal with only one location. */
> > > + gdb_assert (b->loc && !b->loc->next);
> > > +
> > > + get_user_print_options (&opts);
> > > +
> > > + if (opts.addressprint)
> > > + ui_out_field_skip (uiout, "addr");
> > > + annotate_field (5);
> > > + if (b->loc->enabled)
> > > + print_breakpoint_location (b, b->loc, wrap_indent, stb);
> > > + if (b->loc)
> > > + *last_loc = b->loc;
> > > +}
> >
> > I don't like the API change just to pass through those variables,
> > which don't hold any state information and could just as well be
> > recreated by the print_breakpoint_location routine itself ...
>
> For stb it seems to be true (I'm not very familiar with GDB's output
> mechanisms). Unfortunately wrap_indent can't be recreated.
> print_one_breakpoint_location sets it based on its print_address_bits
> argument. breakpoint_1 calculates print_address_bits taking into account
> all breakpoints which pass the filter it is given, or just one
> breakpoint. print_one_ranged_breakpoint and print_breakpoint_location
> don't have enough information to make the same calculation that
> breakpoint_1 did.
>
> This version of the patch drops the stb argument from the print_one
> method, but keeps wrap_indent (I could have exchanged wrap_indent for
> print_address_bits too). What do you think?

So the whole point of the wrap_indent is to make sure that if the
description of a breakpoint location is too long to fit into one
line, it is wrapped at the correct point. That point happens to
be the column named "what" in the breakpoint UI table.

Now the thing is, the UI table layer already knows exactly where
that column starts, so it is kind of pointless to attempt to
re-compute that offset. In particular, since the code currently
gets it completely wrong anyway: at some point, another column of
variable size was added --see print_type_col_width in breakpoint_1--
but the wrap_indent logic was never updated.

My thought was to remove that redundancy completely, and simply
ask the UI table layer for the correct position. It turns out
that this needs some new functionality exported from that layer,
but a simply query for the field information you originally
passed to that layer suffices.

The following patch implements this suggestion; it fixes the currently
broken indentation and gets rid of the silly arguments to the
print_breakpoint_location function.

Tested on i386-linux and by manual inspection of line wrapping.

Any comments?

Bye,
Ulrich

ChangeLog:

* breakpoint.c (wrap_indent_at_field): New function.
(print_breakpoint_location): Use it instead of WRAP_INDENT argument.
Allocate ui_stream locally instead of using STB argument.
(print_one_breakpoint_location): Update call.
* ui-out.c (ui_out_query_field): New function.
* ui-out.h (ui_out_query_field): Add prototype.

Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.538
diff -u -p -r1.538 breakpoint.c
--- gdb/breakpoint.c 21 Feb 2011 14:59:34 -0000 1.538
+++ gdb/breakpoint.c 24 Feb 2011 20:16:23 -0000
@@ -4719,12 +4719,40 @@ bpstat_causes_stop (bpstat bs)

\f

+/* Compute a string of spaces suitable to indent the next line
+ so it starts at the position corresponding to the table column
+ named COL_NAME in the currently active table of UIOUT. */
+
+static char *
+wrap_indent_at_field (struct ui_out *uiout, const char *col_name)
+{
+ static char wrap_indent[80];
+ int i, total_width, width, align;
+ char *text;
+
+ total_width = 0;
+ for (i = 1; ui_out_query_field (uiout, i, &width, &align, &text); i++)
+ {
+ if (strcmp (text, col_name) == 0)
+ {
+ gdb_assert (total_width < sizeof wrap_indent);
+ memset (wrap_indent, ' ', total_width);
+ wrap_indent[total_width] = 0;
+
+ return wrap_indent;
+ }
+
+ total_width += width + 1;
+ }
+
+ return NULL;
+}
+
/* Print the LOC location out of the list of B->LOC locations. */

-static void print_breakpoint_location (struct breakpoint *b,
- struct bp_location *loc,
- char *wrap_indent,
- struct ui_stream *stb)
+static void
+print_breakpoint_location (struct breakpoint *b,
+ struct bp_location *loc)
{
struct cleanup *old_chain = save_current_program_space ();

@@ -4743,8 +4771,9 @@ static void print_breakpoint_location (s
ui_out_text (uiout, "in ");
ui_out_field_string (uiout, "func",
SYMBOL_PRINT_NAME (sym));
- ui_out_wrap_hint (uiout, wrap_indent);
- ui_out_text (uiout, " at ");
+ ui_out_text (uiout, " ");
+ ui_out_wrap_hint (uiout, wrap_indent_at_field (uiout, "what"));
+ ui_out_text (uiout, "at ");
}
ui_out_field_string (uiout, "file", b->source_file);
ui_out_text (uiout, ":");
@@ -4762,9 +4791,14 @@ static void print_breakpoint_location (s
}
else if (loc)
{
+ struct ui_stream *stb = ui_out_stream_new (uiout);
+ struct cleanup *stb_chain = make_cleanup_ui_out_stream_delete (stb);
+
print_address_symbolic (loc->gdbarch, loc->address, stb->stream,
demangle, "");
ui_out_field_stream (uiout, "at", stb);
+
+ do_cleanups (stb_chain);
}
else
ui_out_field_string (uiout, "pending", b->addr_string);
@@ -4833,9 +4867,6 @@ print_one_breakpoint_location (struct br
{
struct command_line *l;
static char bpenables[] = "nynny";
- char wrap_indent[80];
- struct ui_stream *stb = ui_out_stream_new (uiout);
- struct cleanup *old_chain = make_cleanup_ui_out_stream_delete (stb);
struct cleanup *bkpt_chain;

int header_of_multiple = 0;
@@ -4897,15 +4928,6 @@ print_one_breakpoint_location (struct br

/* 5 and 6 */
- strcpy (wrap_indent, " ");
- if (opts.addressprint)
- {
- if (print_address_bits <= 32)
- strcat (wrap_indent, " ");
- else
- strcat (wrap_indent, " ");
- }
-
if (b->ops != NULL && b->ops->print_one != NULL)
{
/* Although the print_one can possibly print all locations,
@@ -4970,7 +4992,7 @@ print_one_breakpoint_location (struct br
}
annotate_field (5);
if (!header_of_multiple)
- print_breakpoint_location (b, loc, wrap_indent, stb);
+ print_breakpoint_location (b, loc);
if (b->loc)
*last_loc = b->loc;
break;
@@ -5126,7 +5148,6 @@ print_one_breakpoint_location (struct br
}

do_cleanups (bkpt_chain);
- do_cleanups (old_chain);
}

static void
Index: gdb/ui-out.c
===================================================================
RCS file: /cvs/src/src/gdb/ui-out.c,v
retrieving revision 1.51
diff -u -p -r1.51 ui-out.c
--- gdb/ui-out.c 12 Jan 2011 01:23:28 -0000 1.51
+++ gdb/ui-out.c 24 Feb 2011 20:16:23 -0000
@@ -1141,6 +1141,28 @@ ui_out_data (struct ui_out *uiout)
return uiout->data;
}

+/* Access table field parameters. */
+int
+ui_out_query_field (struct ui_out *uiout, int colno,
+ int *width, int *alignment, char **col_name)
+{
+ struct ui_out_hdr *hdr;
+
+ if (!uiout->table.flag)
+ return 0;
+
+ for (hdr = uiout->table.header_first; hdr; hdr = hdr->next)
+ if (hdr->colno == colno)
+ {
+ *width = hdr->width;
+ *alignment = hdr->alignment;
+ *col_name = hdr->col_name;
+ return 1;
+ }
+
+ return 0;
+}
+
/* Initalize private members at startup. */

struct ui_out *
Index: gdb/ui-out.h
===================================================================
RCS file: /cvs/src/src/gdb/ui-out.h,v
retrieving revision 1.32
diff -u -p -r1.32 ui-out.h
--- gdb/ui-out.h 12 Jan 2011 01:23:28 -0000 1.32
+++ gdb/ui-out.h 24 Feb 2011 20:16:23 -0000
@@ -156,6 +156,9 @@ extern int ui_out_get_verblvl (struct ui

extern int ui_out_test_flags (struct ui_out *uiout, int mask);

+extern int ui_out_query_field (struct ui_out *uiout, int colno,
+ int *width, int *alignment, char **col_name);
+
#if 0
extern void ui_out_result_begin (struct ui_out *uiout, char *class);

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