RFA: fix pretty-printing in "bt full"

RFA: fix pretty-printing in "bt full"

[Tom Tromey]

Consider this program:

    struct s {
      int x;
      int y;
    };

    int func2 (struct s val) { return val.y + 5; }

    int main (int argc, char **argv)
    {
      struct s a;
      a.x = 5;
      a.y = 7;

      return func2 (a) != 12;
    }

If I set a breakpoint on func2, run, and then invoke "bt full", I see:

(gdb) bt full
#0  func2 (val={x = 5, y = 7}) at b.c:6
No locals.
#1  0x080483a9 in main (argc=1, argv=0xbffff7b4) at b.c:14
	a = {
  x = 5, 
  y = 7
}


Note the odd indentation of "a" (there is a tab before "a = ", so you
may need to take your mail program's tab settings into account.  On my
terminal this is visually like 8 spaces).

This patch fixes the problem by changing print_variable_value to
accept an indentation argument, like the various val_print functions
do.

After the patch the output looks like this:

(gdb) bt full
#0  func2 (val={x = 5, y = 7}) at b.c:6
No locals.
#1  0x080483a9 in main (argc=1, argv=0xbffff7a4) at b.c:14
        a = {
          x = 5, 
          y = 7
        }

I think this looks nicer, because the contents of "a" are indented
underneath "a".

Built and regtested on x86-64 (compile farm).

Please review.

thanks,
Tom

2008-12-15  Tom Tromey  <tromey@redhat.com>

	* stack.c (print_block_frame_locals): Print spaces, not tabs.
	Pass indentation level to print_variable_value.
	(print_frame_arg_vars): Update.
	* value.h (print_variable_value): Update.
	* printcmd.c (print_variable_value): Add 'indent' argument.  Use
	common_val_print.
	* f-valprint.c (info_common_command): Update.

diff --git a/gdb/f-valprint.c b/gdb/f-valprint.c
index f893b49..26fe220 100644
--- a/gdb/f-valprint.c
+++ b/gdb/f-valprint.c
@@ -567,7 +567,7 @@ info_common_command (char *comname, int from_tty)
       while (entry != NULL)
 	{
 	  printf_filtered ("%s = ", SYMBOL_PRINT_NAME (entry->symbol));
-	  print_variable_value (entry->symbol, fi, gdb_stdout);
+	  print_variable_value (entry->symbol, fi, gdb_stdout, 0);
 	  printf_filtered ("\n");
 	  entry = entry->next;
 	}
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 6d6b915..47dbc98 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1731,17 +1731,20 @@ disable_display_command (char *args, int from_tty)
 \f
 
 /* Print the value in stack frame FRAME of a variable specified by a
-   struct symbol.  */
+   struct symbol.  STREAM is the ui_file on which to print the value.
+   INDENT is used to control indentation of sub-parts of the value; it
+   should be set to the number of indentation levels already emitted
+   at the point at which this function is called.  */
 
 void
 print_variable_value (struct symbol *var, struct frame_info *frame,
-		      struct ui_file *stream)
+		      struct ui_file *stream, int indent)
 {
   struct value *val = read_var_value (var, frame);
   struct value_print_options opts;
 
   get_user_print_options (&opts);
-  value_print (val, stream, &opts);
+  common_val_print (val, stream, indent, &opts, current_language);
 }
 
 static void
diff --git a/gdb/stack.c b/gdb/stack.c
index 3c1019b..dab332a 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1375,10 +1375,10 @@ print_block_frame_locals (struct block *b, struct frame_info *frame,
 	    break;
 	  values_printed = 1;
 	  for (j = 0; j < num_tabs; j++)
-	    fputs_filtered ("\t", stream);
+	    fputs_filtered ("        ", stream);
 	  fputs_filtered (SYMBOL_PRINT_NAME (sym), stream);
 	  fputs_filtered (" = ", stream);
-	  print_variable_value (sym, frame, stream);
+	  print_variable_value (sym, frame, stream, 4 * num_tabs);
 	  fprintf_filtered (stream, "\n");
 	  break;
 
@@ -1591,7 +1591,7 @@ print_frame_arg_vars (struct frame_info *frame, struct ui_file *stream)
 
 	  sym2 = lookup_symbol (SYMBOL_LINKAGE_NAME (sym),
 				b, VAR_DOMAIN, NULL);
-	  print_variable_value (sym2, frame, stream);
+	  print_variable_value (sym2, frame, stream, 0);
 	  fprintf_filtered (stream, "\n");
 	}
     }
diff --git a/gdb/value.h b/gdb/value.h
index a882004..6c4499b 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -559,7 +559,8 @@ extern int val_print_string (CORE_ADDR addr, int len, int width,
 
 extern void print_variable_value (struct symbol *var,
 				  struct frame_info *frame,
-				  struct ui_file *stream);
+				  struct ui_file *stream,
+				  int indent);
 
 extern int check_field (struct type *, const char *);
 

Re: RFA: fix pretty-printing in "bt full"

[Joel Brobecker]

> I think this looks nicer, because the contents of "a" are indented
> underneath "a".

I agree.

> 2008-12-15  Tom Tromey  <tromey@redhat.com>
> 
> 	* stack.c (print_block_frame_locals): Print spaces, not tabs.
> 	Pass indentation level to print_variable_value.
> 	(print_frame_arg_vars): Update.
> 	* value.h (print_variable_value): Update.
> 	* printcmd.c (print_variable_value): Add 'indent' argument.  Use
> 	common_val_print.
> 	* f-valprint.c (info_common_command): Update.

Just one comment: How about letting print_variable_value do all
the indenting instead of having the caller indent the first line?
I think that would simplify the usage as well as the description.
What do you think?

The rest looks OK to me.

Thanks,
-- 
Joel

Re: RFA: fix pretty-printing in "bt full"

[Tom Tromey]

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

Joel> Just one comment: How about letting print_variable_value do all
Joel> the indenting instead of having the caller indent the first line?
Joel> I think that would simplify the usage as well as the description.
Joel> What do you think?

Ok, here's another version.  This one renames the function, since
after its purpose changed the old name seemed incorrect.

Also, this may introduce a behavior change in print_frame_arg_vars.  I
haven't tried to test this, but the old code printed the variable name
before looking up the second symbol; it seems to me that the new code
could fail while looking up the second symbol and then not print
anything.

Built and regtested on x86-64 (compile farm).

Tom

2008-12-22  Tom Tromey  <tromey@redhat.com>

	* stack.c (print_block_frame_locals): Print spaces, not tabs.
	Update for call to print_variable_and_value.
	(print_frame_arg_vars): Update.
	* value.h (print_variable_and_value): Rename from
	print_variable_value.  Add 'name' and 'indent' parameters.
	* printcmd.c (print_variable_and_value): Rename from
	print_variable_value.  Add 'name' and 'indent' parameters.  Use
	common_val_print.
	* f-valprint.c (info_common_command): Update.

diff --git a/gdb/f-valprint.c b/gdb/f-valprint.c
index f893b49..a2d2a20 100644
--- a/gdb/f-valprint.c
+++ b/gdb/f-valprint.c
@@ -566,9 +566,7 @@ info_common_command (char *comname, int from_tty)
 
       while (entry != NULL)
 	{
-	  printf_filtered ("%s = ", SYMBOL_PRINT_NAME (entry->symbol));
-	  print_variable_value (entry->symbol, fi, gdb_stdout);
-	  printf_filtered ("\n");
+	  print_variable_and_value (NULL, entry->symbol, fi, gdb_stdout, 0);
 	  entry = entry->next;
 	}
     }
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 6d6b915..c4262d5 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1731,17 +1731,28 @@ disable_display_command (char *args, int from_tty)
 \f
 
 /* Print the value in stack frame FRAME of a variable specified by a
-   struct symbol.  */
+   struct symbol.  NAME is the name to print; if NULL then VAR's print
+   name will be used.  STREAM is the ui_file on which to print the
+   value.  INDENT specifies the number of indent levels to print
+   before printing the variable name.  */
 
 void
-print_variable_value (struct symbol *var, struct frame_info *frame,
-		      struct ui_file *stream)
+print_variable_and_value (const char *name, struct symbol *var,
+			  struct frame_info *frame,
+			  struct ui_file *stream, int indent)
 {
-  struct value *val = read_var_value (var, frame);
+  struct value *val;
   struct value_print_options opts;
 
+  if (!name)
+    name = SYMBOL_PRINT_NAME (var);
+
+  fprintf_filtered (stream, "%s%s = ", n_spaces (2 * indent), name);
+
+  val = read_var_value (var, frame);
   get_user_print_options (&opts);
-  value_print (val, stream, &opts);
+  common_val_print (val, stream, indent, &opts, current_language);
+  fprintf_filtered (stream, "\n");
 }
 
 static void
diff --git a/gdb/stack.c b/gdb/stack.c
index 3c1019b..51dd1bc 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1374,12 +1374,7 @@ print_block_frame_locals (struct block *b, struct frame_info *frame,
 	  if (SYMBOL_IS_ARGUMENT (sym))
 	    break;
 	  values_printed = 1;
-	  for (j = 0; j < num_tabs; j++)
-	    fputs_filtered ("\t", stream);
-	  fputs_filtered (SYMBOL_PRINT_NAME (sym), stream);
-	  fputs_filtered (" = ", stream);
-	  print_variable_value (sym, frame, stream);
-	  fprintf_filtered (stream, "\n");
+	  print_variable_and_value (NULL, sym, frame, stream, 4 * num_tabs);
 	  break;
 
 	default:
@@ -1575,8 +1570,6 @@ print_frame_arg_vars (struct frame_info *frame, struct ui_file *stream)
       if (SYMBOL_IS_ARGUMENT (sym))
 	{
 	  values_printed = 1;
-	  fputs_filtered (SYMBOL_PRINT_NAME (sym), stream);
-	  fputs_filtered (" = ", stream);
 
 	  /* We have to look up the symbol because arguments can have
 	     two entries (one a parameter, one a local) and the one we
@@ -1591,8 +1584,8 @@ print_frame_arg_vars (struct frame_info *frame, struct ui_file *stream)
 
 	  sym2 = lookup_symbol (SYMBOL_LINKAGE_NAME (sym),
 				b, VAR_DOMAIN, NULL);
-	  print_variable_value (sym2, frame, stream);
-	  fprintf_filtered (stream, "\n");
+	  print_variable_and_value (SYMBOL_PRINT_NAME (sym), sym2,
+				    frame, stream, 0);
 	}
     }
 
diff --git a/gdb/value.h b/gdb/value.h
index a882004..2961919 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -557,9 +557,11 @@ extern int val_print_string (CORE_ADDR addr, int len, int width,
 			     struct ui_file *stream,
 			     const struct value_print_options *options);
 
-extern void print_variable_value (struct symbol *var,
-				  struct frame_info *frame,
-				  struct ui_file *stream);
+extern void print_variable_and_value (const char *name,
+				      struct symbol *var,
+				      struct frame_info *frame,
+				      struct ui_file *stream,
+				      int indent);
 
 extern int check_field (struct type *, const char *);
 

Re: RFA: fix pretty-printing in "bt full"

[Joel Brobecker]

> Also, this may introduce a behavior change in print_frame_arg_vars.  I
> haven't tried to test this, but the old code printed the variable name
> before looking up the second symbol; it seems to me that the new code
> could fail while looking up the second symbol and then not print
> anything.

I don't think that this would be a problem in practice. I don't think
it should error out. See lookup_block_symbol:

      /* Note that parameter symbols do not always show up last in the
         list; this loop makes sure to take anything else other than
         parameter symbols first; it only uses parameter symbols as a
         last resort.  Note that this only takes up extra computation
         time on a match.  */

So, even if there is no local symbol for our parameter symbol,
lookup_symbol should still return the parameter one as a fallback.
But even if the call to lookup_symbol did error-out, not having
the name of the parameter under those unusual circumstance shouldn't
be too bad, given that the output will be broken by the error message.

> 2008-12-22  Tom Tromey  <tromey@redhat.com>
> 
> 	* stack.c (print_block_frame_locals): Print spaces, not tabs.
> 	Update for call to print_variable_and_value.
> 	(print_frame_arg_vars): Update.
> 	* value.h (print_variable_and_value): Rename from
> 	print_variable_value.  Add 'name' and 'indent' parameters.
> 	* printcmd.c (print_variable_and_value): Rename from
> 	print_variable_value.  Add 'name' and 'indent' parameters.  Use
> 	common_val_print.
> 	* f-valprint.c (info_common_command): Update.

OK.

-- 
Joel

Re: RFA: fix pretty-printing in "bt full"

[Tom Tromey]

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

Joel> I don't think that this would be a problem in practice. I don't
Joel> think it should error out.

Thanks.  I assumed that lookup_symbol could call error, but I didn't
trace through everything to try to see.

Joel> OK.

I checked it in.

Tom