[RFA 0/3] Have GDB better styled.

[RFA 0/3] Have GDB better styled.

[Philippe Waroquiers]

This patch series ensures that more GDB commands are outputting
file names, addresses and function names using style.

In particular, 'info functions', rbreak, info sources will now be
styled.

[RFA 2/3] Use address style to print addresses in breakpoint information.

[Philippe Waroquiers]

gdb/ChangeLog
2019-01-10  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* breakpoint.c (describe_other_breakpoints): Use address style
	to print addresses.
	(say_where): Likewise.
---
 gdb/breakpoint.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2ab8a76326..e08bcf002d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6701,7 +6701,7 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 			     : ((others == 1) ? " and" : ""));
 	  }
       printf_filtered (_("also set at pc "));
-      fputs_filtered (paddress (gdbarch, pc), gdb_stdout);
+      fputs_styled (paddress (gdbarch, pc), address_style.style (), gdb_stdout);
       printf_filtered (".\n");
     }
 }
@@ -12183,8 +12183,9 @@ say_where (struct breakpoint *b)
       if (opts.addressprint || b->loc->symtab == NULL)
 	{
 	  printf_filtered (" at ");
-	  fputs_filtered (paddress (b->loc->gdbarch, b->loc->address),
-			  gdb_stdout);
+	  fputs_styled (paddress (b->loc->gdbarch, b->loc->address),
+			address_style.style (),
+			gdb_stdout);
 	}
       if (b->loc->symtab != NULL)
 	{
-- 
2.20.1

Re: [RFA 2/3] Use address style to print addresses in breakpoint information.

[Tom Tromey]

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> gdb/ChangeLog
Philippe> 2019-01-10  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

Philippe> 	* breakpoint.c (describe_other_breakpoints): Use address style
Philippe> 	to print addresses.
Philippe> 	(say_where): Likewise.

This is ok.  Thanks.

Tom

[RFA 1/3] Use function_name_style to print Ada and C function names

[Philippe Waroquiers]

gdb/ChangeLog
2019-01-10  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* ada-typeprint.c (print_func_type): Print function name
	style to print function name.
	* c-typeprint.c (c_print_type_1): Likewise.
---
 gdb/ada-typeprint.c | 6 +++++-
 gdb/c-typeprint.c   | 8 ++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/gdb/ada-typeprint.c b/gdb/ada-typeprint.c
index 2b6cdaf264..8c42e8140d 100644
--- a/gdb/ada-typeprint.c
+++ b/gdb/ada-typeprint.c
@@ -30,6 +30,7 @@
 #include "language.h"
 #include "demangle.h"
 #include "c-lang.h"
+#include "cli/cli-style.h"
 #include "typeprint.h"
 #include "target-float.h"
 #include "ada-lang.h"
@@ -779,7 +780,10 @@ print_func_type (struct type *type, struct ui_file *stream, const char *name,
     fprintf_filtered (stream, "function");
 
   if (name != NULL && name[0] != '\0')
-    fprintf_filtered (stream, " %s", name);
+    {
+      fputs_filtered (" ", stream);
+      fputs_styled (name, function_name_style.style (), stream);
+    }
 
   if (len > 0)
     {
diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index 5e7e672e02..0d2d9b5106 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -28,6 +28,7 @@
 #include "language.h"
 #include "demangle.h"
 #include "c-lang.h"
+#include "cli/cli-style.h"
 #include "typeprint.h"
 #include "cp-abi.h"
 #include "cp-support.h"
@@ -115,6 +116,7 @@ c_print_type_1 (struct type *type,
     type = check_typedef (type);
 
   local_name = typedef_hash_table::find_typedef (flags, type);
+  code = TYPE_CODE (type);
   if (local_name != NULL)
     {
       fputs_filtered (local_name, stream);
@@ -124,7 +126,6 @@ c_print_type_1 (struct type *type,
   else
     {
       c_type_print_base_1 (type, stream, show, level, language, flags, podata);
-      code = TYPE_CODE (type);
       if ((varstring != NULL && *varstring != '\0')
 	  /* Need a space if going to print stars or brackets;
 	     but not if we will print just a type name.  */
@@ -144,7 +145,10 @@ c_print_type_1 (struct type *type,
 
   if (varstring != NULL)
     {
-      fputs_filtered (varstring, stream);
+      if (code == TYPE_CODE_FUNC || code == TYPE_CODE_METHOD)
+	fputs_styled (varstring, function_name_style.style (), stream );
+      else
+	fputs_filtered (varstring, stream);
 
       /* For demangled function names, we have the arglist as part of
          the name, so don't print an additional pair of ()'s.  */
-- 
2.20.1

Re: [RFA 1/3] Use function_name_style to print Ada and C function names

[Tom Tromey]

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> 	* ada-typeprint.c (print_func_type): Print function name
Philippe> 	style to print function name.
Philippe> 	* c-typeprint.c (c_print_type_1): Likewise.

These are printing type names, not function names -- so I wonder whether
we want to apply the function name styling here.

Any comments on this aspect?  I am not sure myself.

On the one hand, it isn't a function name; maybe we would like a type
style, but then maybe a type style would be too noisy somehow?

On the other hand, maybe calling out function type names looks nicer.

Philippe> +	fputs_styled (varstring, function_name_style.style (), stream );

Extra space before the close paren.

Tom

Re: [RFA 1/3] Use function_name_style to print Ada and C function names

[Philippe Waroquiers]

On Sat, 2019-01-12 at 09:22 -0700, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Philippe> 	* ada-typeprint.c (print_func_type): Print function name
> Philippe> 	style to print function name.
> Philippe> 	* c-typeprint.c (c_print_type_1): Likewise.
> 
> These are printing type names, not function names -- so I wonder whether
> we want to apply the function name styling here.
> 
> Any comments on this aspect?  I am not sure myself.
ada-typeprint.c:print_func_type is only called to print types that are
functions.  So, that effectively prints a function name.

c-typeprint.c (c_print_type_1) is effectively called to print whatever
type (so not only functions), but the following added condition
should ensure function name style is only used for function names:
-      fputs_filtered (varstring, stream);
+      if (code == TYPE_CODE_FUNC || code == TYPE_CODE_METHOD)
+       fputs_styled (varstring, function_name_style.style (), stream );
+      else
+       fputs_filtered (varstring, stream);

> 
> On the one hand, it isn't a function name; maybe we would like a type
> style, but then maybe a type style would be too noisy somehow?
> 
> On the other hand, maybe calling out function type names looks nicer.
> 
> Philippe> +	fputs_styled (varstring, function_name_style.style (), stream );
> 
> Extra space before the close paren.
I will fix this, once/if a go to push ...

Thanks

Philippe

Re: [RFA 1/3] Use function_name_style to print Ada and C function names

[Tom Tromey]

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> ada-typeprint.c:print_func_type is only called to print types that are
Philippe> functions.  So, that effectively prints a function name.

No opinion on that one, maybe Joel could say.

Philippe> c-typeprint.c (c_print_type_1) is effectively called to print whatever
Philippe> type (so not only functions), but the following added condition
Philippe> should ensure function name style is only used for function names:
Philippe> -      fputs_filtered (varstring, stream);
Philippe> +      if (code == TYPE_CODE_FUNC || code == TYPE_CODE_METHOD)
Philippe> +       fputs_styled (varstring, function_name_style.style (), stream );
Philippe> +      else
Philippe> +       fputs_filtered (varstring, stream);

Actually here I wonder if this can be hit.
Maybe
    typedef void Typename ();
... ?  But I'd sort of expect not.

Tom

Re: [RFA 1/3] Use function_name_style to print Ada and C function names

[Philippe Waroquiers]

On Sun, 2019-01-13 at 09:18 +0400, Joel Brobecker wrote:
> > > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> > 
> > Philippe> ada-typeprint.c:print_func_type is only called to print types that are
> > Philippe> functions.  So, that effectively prints a function name.
> > 
> > No opinion on that one, maybe Joel could say.
> 
> I tried to think about the general context, and the guideline in
> my mind is that there is a fine line where coloring is helpful and
> when it becomes too much, reducing the effectiveness of colorization.
> In this case, this patch seems to advocate in favor of colorization?
> 
> Perhaps we might indeed find it useful to colorize type names.
The patch does not color type names.  It colors function names
that happens to be passed along with the type to the type
printing code.
See below  longer explanation.

> But, for the sake of consistency, let's color all type names,
> rather than just function ones. And perhaps one thing we could do
> to avoid an explosion of colorization options, which themselves
> can lead to visual-confusion-by-color-mosaic effect, is to compromise
> a bit by saying type names should be styled similarly to their object
> counterparts.  Eg: same colour, but with a "dim" filter? For instance,
> if addresses are in blue, then types which are pointers/refs, etc,
> could be printed in dim blue?
> 
> One thing I might do, regardless of the decision, is define in the code
> a different style for this kind of entity, even if, internally, the style
> itself is just a reference (an "alias", if you will), of an existing
> style. That way, it is easier to keep track of the various kinds of
> entities and their style, thus allowing us the option of upgrading
> this kind of entities to their own style if we wanted to.  Otherwise,
> we'd have to audit all the existing calls to styling to see which
> ones we want to style separately.
As I understand (and the trial I did confirms), the
code is really coloring *function names*, and is not coloring
*function type names*.
(I have tried to clarify this in the commit msg of the RFAv2).

Here is an example when debugging a piece of valgrind code
with the patched GDB.

Valgrind defines a function type HT_Cmp_t:
typedef Word  (*HT_Cmp_t) ( const void* node1, const void* node2 );
(a comparison function type, to be used in hash tables).

(gdb) info func HT_Cmp_t
All functions matching regular expression "HT_Cmp_t":
# this does not show anything, as expected, as
# the type HT_Cmp_t is not a function.

(gdb) info types HT_Cmp_t
All types matching regular expression "HT_Cmp_t":

File ../include/pub_tool_hashtable.h:
75:	typedef Word (*)(const void *, const void *) HT_Cmp_t;
# this shows the function type. The only part colored in
# the above is the filename.  In particular, the type name HT_Cmp_t
# is not colored (as expected, because the patch aims at coloring
# function names).

(gdb) info func cmp_pool_str
All functions matching regular expression "cmp_pool_str":

File m_deduppoolalloc.c:
205:	static Word cmp_pool_str(const void *, const void *);
# this shows a specific function of the type HT_Cmp_t
# and cmp_pool_str is colored (as expected) in function name style.

The comments in ada-typeprint.c are pretty clear (but I however cannot
replicate easily completely the above in Ada, as there is
no real notion of function type in Ada, or at least I could not
find it in the Ada Reference Manual : as I understand, the concept
of 'Ada function type' is a GDB concept.

/* Print function or procedure type TYPE on STREAM.  Make it a header
   for function or procedure NAME if NAME is not null.  */

static void
print_func_type (struct type *type, struct ui_file *stream, const char *name,
		 const struct type_print_options *flags)

So, print_func_type effectively prints a function type, and optionally
prints a function name given via the NAME arg.
The patch only colors NAME using function name style.

The c code is less clear documented than the ada print_func_type :
it uses varstring as argname.
The comments I found around the c and Ada type printing that references
VARSTRING in upper case are:
ada-typeprint.c:817:   If VARSTRING is a non-empty string, print as an Ada variable/field
typeprint.c:396:   variable named VARSTRING.  (VARSTRING is demangled if necessary.)

IMO, these comments are somewhat misleading, because
at that stage, VARSTRING is not necessarily a variable or a field name,
it can also be a function name (and maybe other things?).
So, VARSTRING might better be a more general NAME (or ENTITY_NAME, ...).

So, in summary:

I think the patch ensures type printing code is only coloring
function names (given as a 'side information' to the type to print)
using function name style, it is not coloring the type name,
that is probably somewhere deep in the type data structure.

Whether or not we want to style type names is another question
(and another patch then).

IMO, the patch looks reasonable, and produces a much nicer/more
readable output :).

Thanks

Philippe

[RFA 3/3] Make symtab.c better styled.

[Philippe Waroquiers]

2019-01-10  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* symtab.c (output_source_filename): Use file name style
	to print file name.
	(print_symbol_info): Likewise.
	(print_msymbol_info): Use address style to print addresses.
---
 gdb/symtab.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index d5e18a64d0..9363bf555a 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -41,6 +41,7 @@
 #include "p-lang.h"
 #include "addrmap.h"
 #include "cli/cli-utils.h"
+#include "cli/cli-style.h"
 #include "fnmatch.h"
 #include "hashtab.h"
 #include "typeprint.h"
@@ -4168,7 +4169,7 @@ output_source_filename (const char *name,
   data->first = 0;
 
   wrap_here ("");
-  fputs_filtered (name, gdb_stdout);
+  fputs_styled (name, file_name_style.style (), gdb_stdout);
 }
 
 /* A callback for map_partial_symbol_filenames.  */
@@ -4620,7 +4621,7 @@ print_symbol_info (enum search_domain kind,
       if (filename_cmp (last, s_filename) != 0)
 	{
 	  fputs_filtered ("\nFile ", gdb_stdout);
-	  fputs_filtered (s_filename, gdb_stdout);
+	  fputs_styled (s_filename, file_name_style.style (), gdb_stdout);
 	  fputs_filtered (":\n", gdb_stdout);
 	}
 
@@ -4667,8 +4668,9 @@ print_msymbol_info (struct bound_minimal_symbol msymbol)
   else
     tmp = hex_string_custom (BMSYMBOL_VALUE_ADDRESS (msymbol),
 			     16);
-  printf_filtered ("%s  %s\n",
-		   tmp, MSYMBOL_PRINT_NAME (msymbol.minsym));
+  fputs_styled (tmp, address_style.style (), gdb_stdout);
+  printf_filtered ("  %s\n",
+		   MSYMBOL_PRINT_NAME (msymbol.minsym));
 }
 
 /* This is the guts of the commands "info functions", "info types", and
-- 
2.20.1

Re: [RFA 3/3] Make symtab.c better styled.

[Tom Tromey]

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> 2019-01-10  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
Philippe> 	* symtab.c (output_source_filename): Use file name style
Philippe> 	to print file name.
Philippe> 	(print_symbol_info): Likewise.
Philippe> 	(print_msymbol_info): Use address style to print addresses.

Thanks, this is ok.

Philippe> +  printf_filtered ("  %s\n",
Philippe> +		   MSYMBOL_PRINT_NAME (msymbol.minsym));

I suppose this could look at the minsymbol type and use either the
function or variable styling.

Tom

Re: [RFA 3/3] Make symtab.c better styled.

[Philippe Waroquiers]

On Sat, 2019-01-12 at 09:26 -0700, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Philippe> 2019-01-10  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> Philippe> 	* symtab.c (output_source_filename): Use file name style
> Philippe> 	to print file name.
> Philippe> 	(print_symbol_info): Likewise.
> Philippe> 	(print_msymbol_info): Use address style to print addresses.
> 
> Thanks, this is ok.
> 
> Philippe> +  printf_filtered ("  %s\n",
> Philippe> +		   MSYMBOL_PRINT_NAME (msymbol.minsym));
> 
> I suppose this could look at the minsymbol type and use either the
> function or variable styling.
That looks relatively easy to do.
To differentiate between data and text, would the condition
in expand_symtab_containing_pc be ok ?

i.e.  variable styling would be used for data, i.e. :
        (MSYMBOL_TYPE (msymbol.minsym) == mst_data
	  || MSYMBOL_TYPE (msymbol.minsym) == mst_bss
	  || MSYMBOL_TYPE (msymbol.minsym) == mst_abs
	  || MSYMBOL_TYPE (msymbol.minsym) == mst_file_data
	  || MSYMBOL_TYPE (msymbol.minsym) == mst_file_bss))

and function name styling would be used for the rest i.e.:
  mst_text,			/* Generally executable instructions */
  mst_text_gnu_ifunc,           /* Executable code returning address
				   of executable code */

  mst_data_gnu_ifunc,		/* Executable code returning address
				   of executable code */

  mst_slot_got_plt,		/* GOT entries for .plt sections */
  mst_solib_trampoline,		/* Shared library trampoline code */
  mst_file_text,		/* Static version of mst_text */


Philippe

Re: [RFA 3/3] Make symtab.c better styled.

[Tom Tromey]

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> To differentiate between data and text, would the condition
Philippe> in expand_symtab_containing_pc be ok ?

Philippe> i.e.  variable styling would be used for data, i.e. :
Philippe>         (MSYMBOL_TYPE (msymbol.minsym) == mst_data
Philippe> 	  || MSYMBOL_TYPE (msymbol.minsym) == mst_bss
Philippe> 	  || MSYMBOL_TYPE (msymbol.minsym) == mst_abs
Philippe> 	  || MSYMBOL_TYPE (msymbol.minsym) == mst_file_data
Philippe> 	  || MSYMBOL_TYPE (msymbol.minsym) == mst_file_bss))

Philippe> and function name styling would be used for the rest i.e.:
Philippe>   mst_text,			/* Generally executable instructions */
Philippe>   mst_text_gnu_ifunc,           /* Executable code returning address
Philippe> 				   of executable code */

Philippe>   mst_data_gnu_ifunc,		/* Executable code returning address
Philippe> 				   of executable code */

Philippe>   mst_slot_got_plt,		/* GOT entries for .plt sections */
Philippe>   mst_solib_trampoline,		/* Shared library trampoline code */
Philippe>   mst_file_text,		/* Static version of mst_text */

Yes, I think that's reasonable.

Tom