[RFAv2 0/3] Have GDB better styled.

[RFAv2 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.

Compared to the first version, this handles the comments of Tom:
* when doing 'info functions', the non debugging min symbols are printed
  using function name style, when the non debugging min symbol corresponds
  to some text.
* the commit log for function name style in the Ada and C type print gives
  more details about how these are called to print a function type and
  a function name.

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

[Philippe Waroquiers]

Note that ada-typeprint.c print_func_type is called with
types representing functions and is also called to print
a function NAME and its type.  In such a case, the function
name will be printed using function name style.

Similarly, c_print_type_1 is called to print a type, optionally
with the name of an object of this type in the VARSTRING arg.
So, c_print_type_1 uses function name style to print varstring
when the type code indicates that c_print_type_1 TYPE is some
'real code'.

gdb/ChangeLog
2019-01-12  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..6690ca53bc 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: [RFAv2 1/3] Use function_name_style to print Ada and C function names

[Tom Tromey]

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

Philippe> Note that ada-typeprint.c print_func_type is called with
Philippe> types representing functions and is also called to print
Philippe> a function NAME and its type.  In such a case, the function
Philippe> name will be printed using function name style.

In this particular spot it still isn't clear to me if this will
sometimes style a type name.  So, I'm going to defer to Joel on the Ada
bits.

I'm actually in favor of styling types, but I think they should have a
separate setting from functions.

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.

The c-typeprint.c change is ok.  I re-read your last note on the other
thread and I think I finally understand.

Tom

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

[Philippe Waroquiers]

Thanks for the review.

On Thu, 2019-01-17 at 15:21 -0700, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Philippe> Note that ada-typeprint.c print_func_type is called with
> Philippe> types representing functions and is also called to print
> Philippe> a function NAME and its type.  In such a case, the function
> Philippe> name will be printed using function name style.
> 
> In this particular spot it still isn't clear to me if this will
> sometimes style a type name.  So, I'm going to defer to Joel on the Ada
> bits.

Would be nice to have Joel confirming, but I am quite confident that
this will only style function names and not type names.

If that can help, here is some more detailed investigations:
First, in Ada, after the keywords procedure/function, and before
the arg list (as printed by print_func_type), you must put
a procedure/function name, no type name can be put there,
so GDB would produce real strange/invalid Ada such as:
   procedure Some_Ada_Type_Name (Some_Arg : ...);
This explanation matches the comment of print_func_type:
/* Print function or procedure type TYPE on STREAM.  Make it a header
   for function or procedure NAME if NAME is not null.  */

Also, the type name of an Ada type is accessed via ada_type_name,
that accesses a member of TYPE containing the type name.

To the contrary: the NAME arg of print_func_type is received from
ada_print_type (the single caller of print_func_type),
that passes as NAME its VARSTRING arg.

There are only a few places where ada_print_type is called
with a non null VARSTRING arg, and these are for the names
of fields of record/unchecked unions.

ada_print_type is called (indirectly) by all calls to
print_type (dispatching on the language).
Doing a grep, I found only symtab.c:print_symbol_info
that calls type_print with a non
NULL/non "" VARSTRING arg,
and this piece of code gives "" when the symbol
is a TYPEDEF.

So, I really do not see how that piece of code could style
something else than a function/procedure name.

Thanks

Philippe

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

[Joel Brobecker]

> > Philippe> Note that ada-typeprint.c print_func_type is called with
> > Philippe> types representing functions and is also called to print
> > Philippe> a function NAME and its type.  In such a case, the function
> > Philippe> name will be printed using function name style.
> > 
> > In this particular spot it still isn't clear to me if this will
> > sometimes style a type name.  So, I'm going to defer to Joel on the Ada
> > bits.
> 
> Would be nice to have Joel confirming, but I am quite confident that
> this will only style function names and not type names.

I reviewed the situation, and I think Philippe is right in the sense
that when this function is called with a name, it's an actual function's
name.

When one declares a function type in Ada, it has to be an access
type (the Ada equivalent of a pointer). And the target (function)
declaration is anonymous. This is what it looks like:

   type FA is access procedure (A : System.Address);

So, while the access type has a name ("FA"), the function type
itself does not. From there, the only way I could see that
the function might be called with a name is if it is given
a symbol which is a procedure (not an access to that symbol).
In that case, it would be legitimate to stylize the name as
a function name. But in practice, it's hard to make GDB call
that function with a name, because you can't really put
the function name within an Ada expression without taking
it's address (in Ada, this is a parameterless function call).

The only way I could find that allowed me to trigger this function
call with a non-NULL name was with "maintenance print symbols FILENAME".
In that situation, we're back to the reasoning above that function
types have no name unless they are tied to a real function, and
thus "name" seems to always represent a function name.

With all that, I'm wondering if Philippe had other examples
where he can demonstrate the usefulness of this patch.
At the moment, if the maintenance command is the only case,
knowing that the output gets sent to a file, rather than
stdout, and thus should not be stylized, are there other
situations I couldn't think of where this patch would be
useful?

-- 
Joel

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

[Philippe Waroquiers]

On Sat, 2019-01-26 at 10:21 +0400, Joel Brobecker wrote:
> > > Philippe> Note that ada-typeprint.c print_func_type is called with
> > > Philippe> types representing functions and is also called to print
> > > Philippe> a function NAME and its type.  In such a case, the function
> > > Philippe> name will be printed using function name style.
> > > 
> > > In this particular spot it still isn't clear to me if this will
> > > sometimes style a type name.  So, I'm going to defer to Joel on the Ada
> > > bits.
> > 
> > Would be nice to have Joel confirming, but I am quite confident that
> > this will only style function names and not type names.
> 
> I reviewed the situation, and I think Philippe is right in the sense
> that when this function is called with a name, it's an actual function's
> name.
> 
> When one declares a function type in Ada, it has to be an access
> type (the Ada equivalent of a pointer). And the target (function)
> declaration is anonymous. This is what it looks like:
> 
>    type FA is access procedure (A : System.Address);
Yes, effectively, that is the reasoning.

> With all that, I'm wondering if Philippe had other examples
> where he can demonstrate the usefulness of this patch.
> At the moment, if the maintenance command is the only case,
> knowing that the output gets sent to a file, rather than
> stdout, and thus should not be stylized, are there other
> situations I couldn't think of where this patch would be
> useful?

This part of the patch ensures that the function names in the output of
  'info functions [-q] [-t TYPEREGEXP] [NAMEREGEXP]'
are stylized.

Thanks

Philippe

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

[Philippe Waroquiers]

On Sat, 2019-01-26 at 12:04 +0100, Philippe Waroquiers wrote:
> > With all that, I'm wondering if Philippe had other examples
> > where he can demonstrate the usefulness of this patch.
> > At the moment, if the maintenance command is the only case,
> > knowing that the output gets sent to a file, rather than
> > stdout, and thus should not be stylized, are there other
> > situations I couldn't think of where this patch would be
> > useful?
> 
> This part of the patch ensures that the function names in the output of
>   'info functions [-q] [-t TYPEREGEXP] [NAMEREGEXP]'
> are stylized.
Any additional comments ?
Ok to push ?

Thanks

Philippe

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

[Philippe Waroquiers]

gdb/ChangeLog
2019-01-12  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

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

[Philippe Waroquiers]

Note that print_msymbol_info does not (yet?) print data msymbol
using variable_name_style, as otherwise 'info variables'
would show the non debugging symbols in variable name style,
but 'real' variables would be not styled.

2019-01-12  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.
	Use function name style to print executable text symbols.
	(msymbol_type_data_p): New function.
	(msymbol_type_text_p): New function.
	(expand_symtab_containing_pc): Use msymbol_type_data_p.
	(find_pc_sect_compunit_symtab): Likewise.
---
 gdb/symtab.c | 57 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 29b24328fb..cc24ba81f4 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"
@@ -311,6 +312,33 @@ compunit_language (const struct compunit_symtab *cust)
   return SYMTAB_LANGUAGE (symtab);
 }
 
+/* True if MSYMBOL is of some data type.  */
+
+static bool
+msymbol_type_data_p (struct bound_minimal_symbol msymbol)
+{
+  return msymbol.minsym
+    && (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);
+}
+
+/* True if MSYMBOL is of some text type.  */
+
+static bool
+msymbol_type_text_p (struct bound_minimal_symbol msymbol)
+{
+  return msymbol.minsym
+    && (MSYMBOL_TYPE (msymbol.minsym) == mst_text
+	|| MSYMBOL_TYPE (msymbol.minsym) == mst_text_gnu_ifunc
+	|| MSYMBOL_TYPE (msymbol.minsym) == mst_data_gnu_ifunc
+	|| MSYMBOL_TYPE (msymbol.minsym) == mst_slot_got_plt
+	|| MSYMBOL_TYPE (msymbol.minsym) == mst_solib_trampoline
+	|| MSYMBOL_TYPE (msymbol.minsym) == mst_file_text);
+}
+
 /* See whether FILENAME matches SEARCH_NAME using the rule that we
    advertise to the user.  (The manual's description of linespecs
    describes what we advertise).  Returns true if they match, false
@@ -1039,12 +1067,7 @@ expand_symtab_containing_pc (CORE_ADDR pc, struct obj_section *section)
      necessary because we loop based on texthigh and textlow, which do
      not include the data ranges.  */
   msymbol = lookup_minimal_symbol_by_pc_section (pc, section);
-  if (msymbol.minsym
-      && (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))
+  if (msymbol_type_data_p (msymbol))
     return;
 
   for (objfile *objfile : all_objfiles (current_program_space))
@@ -2879,12 +2902,7 @@ find_pc_sect_compunit_symtab (CORE_ADDR pc, struct obj_section *section)
      we call find_pc_sect_psymtab which has a similar restriction based
      on the partial_symtab's texthigh and textlow.  */
   msymbol = lookup_minimal_symbol_by_pc_section (pc, section);
-  if (msymbol.minsym
-      && (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))
+  if (msymbol_type_data_p (msymbol))
     return NULL;
 
   /* Search all symtabs for the one whose file contains our address, and which
@@ -4168,7 +4186,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 +4638,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 +4685,15 @@ 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);
+  fputs_filtered ("  ", gdb_stdout);
+  if (msymbol_type_text_p (msymbol))
+    fputs_styled (MSYMBOL_PRINT_NAME (msymbol.minsym),
+		  function_name_style.style (),
+		  gdb_stdout);
+  else
+    fputs_filtered (MSYMBOL_PRINT_NAME (msymbol.minsym), gdb_stdout);
+  fputs_filtered ("\n", gdb_stdout);
 }
 
 /* This is the guts of the commands "info functions", "info types", and
-- 
2.20.1

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

[Tom Tromey]

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

Philippe> +/* True if MSYMBOL is of some data type.  */
Philippe> +
Philippe> +static bool
Philippe> +msymbol_type_data_p (struct bound_minimal_symbol msymbol)
Philippe> +{
Philippe> +  return msymbol.minsym
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> +}
Philippe> +
Philippe> +/* True if MSYMBOL is of some text type.  */
Philippe> +
Philippe> +static bool
Philippe> +msymbol_type_text_p (struct bound_minimal_symbol msymbol)
Philippe> +{
Philippe> +  return msymbol.minsym
Philippe> +    && (MSYMBOL_TYPE (msymbol.minsym) == mst_text
Philippe> +	|| MSYMBOL_TYPE (msymbol.minsym) == mst_text_gnu_ifunc
Philippe> +	|| MSYMBOL_TYPE (msymbol.minsym) == mst_data_gnu_ifunc
Philippe> +	|| MSYMBOL_TYPE (msymbol.minsym) == mst_slot_got_plt
Philippe> +	|| MSYMBOL_TYPE (msymbol.minsym) == mst_solib_trampoline
Philippe> +	|| MSYMBOL_TYPE (msymbol.minsym) == mst_file_text);
Philippe> +}

I think these would be better as (const) methods on minsym.

Philippe> +  if (msymbol_type_data_p (msymbol))
Philippe>      return;

This could then be

  if (msymbol.msymbol->data_p ())

Tom

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

[Pedro Alves]

On 01/12/2019 10:28 PM, Philippe Waroquiers wrote:

> @@ -4667,8 +4685,15 @@ 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);
> +  fputs_filtered ("  ", gdb_stdout);
> +  if (msymbol_type_text_p (msymbol))
> +    fputs_styled (MSYMBOL_PRINT_NAME (msymbol.minsym),
> +		  function_name_style.style (),
> +		  gdb_stdout);
> +  else
> +    fputs_filtered (MSYMBOL_PRINT_NAME (msymbol.minsym), gdb_stdout);
> +  fputs_filtered ("\n", gdb_stdout);

Should this use the existing msymbol_is_function instead?

Thanks,
Pedro Alves

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

[Philippe Waroquiers]

On Thu, 2019-02-07 at 18:58 +0000, Pedro Alves wrote:
> On 01/12/2019 10:28 PM, Philippe Waroquiers wrote:
> 
> > @@ -4667,8 +4685,15 @@ 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);
> > +  fputs_filtered ("  ", gdb_stdout);
> > +  if (msymbol_type_text_p (msymbol))
> > +    fputs_styled (MSYMBOL_PRINT_NAME (msymbol.minsym),
> > +		  function_name_style.style (),
> > +		  gdb_stdout);
> > +  else
> > +    fputs_filtered (MSYMBOL_PRINT_NAME (msymbol.minsym), gdb_stdout);
> > +  fputs_filtered ("\n", gdb_stdout);
> 
> Should this use the existing msymbol_is_function instead?
That is a good question.  Note that there was a v3 where
   if (msymbol_type_text_p (msymbol))
is replaced by
   if (msymbol.minsym->text_p ())

The below summarizes the behavior difference if we change the patch
to rather use msymbol_is_function instead of msymbol.minsym->text_p ():

                            patch   msymbol_is_function
                            -----   -------------------
  mst_unknown
  mst_text,		    text_p  function
  mst_text_gnu_ifunc,       text_p  function
  mst_data_gnu_ifunc,	    text_p  maybe
  mst_slot_got_plt,	    text_p  maybe
  mst_data,                 data_p  maybe
  mst_bss,		    data_p  maybe
  mst_abs,		    data_p  maybe
  mst_solib_trampoline,	    text_p  function
  mst_file_text,	    text_p  function
  mst_file_data,	    data_p  maybe
  mst_file_bss,		    data_p  maybe
  nr_minsym_types

The 'patch' first colummn indicates how the patch classifies a
msymbol type.  The 'msymbol_is_function' describes the behaviour
if msymbol_is_function is used instead of 'text_p'.
maybe indicates msymbol_is_function returns True if the msymbol address
can be converted to a different address using
gdbarch_convert_from_func_ptr_addr.

When trying to use msymbol_is_function, I however do not see
any difference of behavior on debian/amd64with a small executable.

The logic I used for the patch was: there are already 2 places
that define the set of msymbol types that are data, so I assumed
the rest was text, and the resulting type set looked plausible.

It is however somewhat bizarre to have msymbol_is_function that
will sometime say that data is a function (and then 'gives back'
another address than the msymbol address).

So, not very clear which one is better/correct, as this msymbol
concept is not very clear to me, and I see no
difference of behavior to decide which one looks better.

Thanks

Philippe

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

[Pedro Alves]

On 02/09/2019 10:35 AM, Philippe Waroquiers wrote:
> On Thu, 2019-02-07 at 18:58 +0000, Pedro Alves wrote:
>> On 01/12/2019 10:28 PM, Philippe Waroquiers wrote:
>>
>>> @@ -4667,8 +4685,15 @@ 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);
>>> +  fputs_filtered ("  ", gdb_stdout);
>>> +  if (msymbol_type_text_p (msymbol))
>>> +    fputs_styled (MSYMBOL_PRINT_NAME (msymbol.minsym),
>>> +		  function_name_style.style (),
>>> +		  gdb_stdout);
>>> +  else
>>> +    fputs_filtered (MSYMBOL_PRINT_NAME (msymbol.minsym), gdb_stdout);
>>> +  fputs_filtered ("\n", gdb_stdout);
>>
>> Should this use the existing msymbol_is_function instead?
> That is a good question.  Note that there was a v3 where
>    if (msymbol_type_text_p (msymbol))
> is replaced by
>    if (msymbol.minsym->text_p ())
> 
> The below summarizes the behavior difference if we change the patch
> to rather use msymbol_is_function instead of msymbol.minsym->text_p ():
> 
>                             patch   msymbol_is_function
>                             -----   -------------------
>   mst_unknown
>   mst_text,		    text_p  function
>   mst_text_gnu_ifunc,       text_p  function
>   mst_data_gnu_ifunc,	    text_p  maybe
>   mst_slot_got_plt,	    text_p  maybe
>   mst_data,                 data_p  maybe
>   mst_bss,		    data_p  maybe
>   mst_abs,		    data_p  maybe
>   mst_solib_trampoline,	    text_p  function
>   mst_file_text,	    text_p  function
>   mst_file_data,	    data_p  maybe
>   mst_file_bss,		    data_p  maybe
>   nr_minsym_types
> 
> The 'patch' first colummn indicates how the patch classifies a
> msymbol type.  The 'msymbol_is_function' describes the behaviour
> if msymbol_is_function is used instead of 'text_p'.
> maybe indicates msymbol_is_function returns True if the msymbol address
> can be converted to a different address using
> gdbarch_convert_from_func_ptr_addr.
> 
> When trying to use msymbol_is_function, I however do not see
> any difference of behavior on debian/amd64with a small executable.
> 
> The logic I used for the patch was: there are already 2 places
> that define the set of msymbol types that are data, so I assumed
> the rest was text, and the resulting type set looked plausible.
> 
> It is however somewhat bizarre to have msymbol_is_function that
> will sometime say that data is a function (and then 'gives back'
> another address than the msymbol address).
> 
> So, not very clear which one is better/correct, as this msymbol
> concept is not very clear to me, and I see no
> difference of behavior to decide which one looks better.

On some platforms (PPC64 v1), functions in the symbol tables are
actually function descriptors, which are data symbols.  Addresses of
functions (and thus C function pointers) are addresses of those
function descriptors.

See here:

https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-3956e82276f3/entry/deeply_understand_64_bit_powerpc_elf_abi_function_descriptors?lang=en

So even though function descriptors are data symbols, they're
basically treated as text/code symbols.  The msymbol_is_function
function returns true for them, and optionally returns the resolved
entry address (which is where we place a breakpoint).

Since these function descriptor symbols represent functions, even though
they're data symbols in the elf tables, it would seem natural to me to
print them using the function name style.

What I'm not sure is whether we'll reach print_msymbol_info with
data descriptor symbols when we do "info functions".  Seems like
search_symbols won't consider those symbols when looking for
functions for "info functions".  So we'd find those symbols only
with "info variables", and then we'd print them as functions?

I guess it could be argued that we should fix search_symbols
/ "info functions" to consider function descriptors, and use
msymbol_is_function in print_msymbol_info to decide whether
to use function style.  I'm not 100% sure it needs fixing, and whether
making "info functions" find the descriptors like that is the
desired behavior.

It wouldn't be fair to impose "info functions" on PPC64 on you,
though.

So if we don't fix that (and assuming it actually needs fixing, I'm
not sure), then I guess the question is whether we should still
print function descriptor symbols in function style, via 
"info variable" or whatever other paths could end up calling
print_msymbol_info in the future.

I'm not 100% sure what is preferred here.  Ulrich, thoughts?

Thanks,
Pedro Alves

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

[Ulrich Weigand]

Pedro Alves wrote:

> So even though function descriptors are data symbols, they're
> basically treated as text/code symbols.  The msymbol_is_function
> function returns true for them, and optionally returns the resolved
> entry address (which is where we place a breakpoint).
> 
> Since these function descriptor symbols represent functions, even though
> they're data symbols in the elf tables, it would seem natural to me to
> print them using the function name style.
> 
> What I'm not sure is whether we'll reach print_msymbol_info with
> data descriptor symbols when we do "info functions".  Seems like
> search_symbols won't consider those symbols when looking for
> functions for "info functions".  So we'd find those symbols only
> with "info variables", and then we'd print them as functions?
> 
> I guess it could be argued that we should fix search_symbols
> / "info functions" to consider function descriptors, and use
> msymbol_is_function in print_msymbol_info to decide whether
> to use function style.  I'm not 100% sure it needs fixing, and whether
> making "info functions" find the descriptors like that is the
> desired behavior.
> 
> It wouldn't be fair to impose "info functions" on PPC64 on you,
> though.
> 
> So if we don't fix that (and assuming it actually needs fixing, I'm
> not sure), then I guess the question is whether we should still
> print function descriptor symbols in function style, via 
> "info variable" or whatever other paths could end up calling
> print_msymbol_info in the future.
> 
> I'm not 100% sure what is preferred here.  Ulrich, thoughts?

In addition to the function descriptor, which is a data symbol,
we also get a function symbol for the actual code entry point,
prefixed with a dot ('.').  This is synthesized by BFD these
days.

I've just checked the current behavior on a ppc64 machine,
and we do indeed see the (synthetic) dot symbol listed under
"info functions" and the function descriptor symbol listed
under "info variables".

  (gdb) info functions
  [...]
  Non-debugging symbols:
  0x0000000010000514  .main

  (gdb) info variables
  [...]
  Non-debugging symbols:
  0x0000000010020088  main

On the other hand, "info symbol main" does dereference the
function descriptor and returns the code entry point:

 (gdb) info symbol main
 .main in section .text of /home/uweigand/a.out

This all is maybe not perfect, but seems reasonable enough to me.
I'm not sure it's worth spending much effort trying to "fix"
anything here.

I haven't followed the patch series in detail, do you think it
would break anything I've outlined above?


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

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

[Pedro Alves]

On 02/12/2019 02:04 PM, Ulrich Weigand wrote:

> I've just checked the current behavior on a ppc64 machine,
> and we do indeed see the (synthetic) dot symbol listed under
> "info functions" and the function descriptor symbol listed
> under "info variables".
> 
>   (gdb) info functions
>   [...]
>   Non-debugging symbols:
>   0x0000000010000514  .main
> 
>   (gdb) info variables
>   [...]
>   Non-debugging symbols:
>   0x0000000010020088  main
> 
> On the other hand, "info symbol main" does dereference the
> function descriptor and returns the code entry point:
> 
>  (gdb) info symbol main
>  .main in section .text of /home/uweigand/a.out
> 
> This all is maybe not perfect, but seems reasonable enough to me.
> I'm not sure it's worth spending much effort trying to "fix"
> anything here.
> 

OK.

> I haven't followed the patch series in detail, do you think it
> would break anything I've outlined above?

So the question becomes a simple cosmetic one.  In this case:

>   (gdb) info variables
>   [...]
>   Non-debugging symbols:
>   0x0000000010020088  main

Should "main" be printed with function style, or variable style.
This basically affects the color used to print the symbol.

In Philippe's patch, we'd print it in variable style.  If we used
msymbol_is_function instead of his "is text symbol" check, we'd
print it in function style.

Thanks,
Pedro Alves

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

[Ulrich Weigand]

Pedro Alves wrote:

> So the question becomes a simple cosmetic one.  In this case:
> 
> >   (gdb) info variables
> >   [...]
> >   Non-debugging symbols:
> >   0x0000000010020088  main
> 
> Should "main" be printed with function style, or variable style.
> This basically affects the color used to print the symbol.
> 
> In Philippe's patch, we'd print it in variable style.  If we used
> msymbol_is_function instead of his "is text symbol" check, we'd
> print it in function style.

Ah, I see.  I guess that doesn't really matter that much at this
point.  Since we're already showing it under "variables" we might
as well use the variable style.  But in the end either way would
be fine with me ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

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

[Pedro Alves]

On 02/12/2019 02:54 PM, Ulrich Weigand wrote:
> Pedro Alves wrote:
> 
>> So the question becomes a simple cosmetic one.  In this case:
>>
>>>   (gdb) info variables
>>>   [...]
>>>   Non-debugging symbols:
>>>   0x0000000010020088  main
>>
>> Should "main" be printed with function style, or variable style.
>> This basically affects the color used to print the symbol.
>>
>> In Philippe's patch, we'd print it in variable style.  If we used
>> msymbol_is_function instead of his "is text symbol" check, we'd
>> print it in function style.
> 
> Ah, I see.  I guess that doesn't really matter that much at this
> point.  Since we're already showing it under "variables" we might
> as well use the variable style.  But in the end either way would
> be fine with me ...

OK, in that case, let's just use with Philippe already has.

Thanks,
Pedro Alves

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

[Philippe Waroquiers]

On Tue, 2019-02-12 at 15:53 +0000, Pedro Alves wrote:
> On 02/12/2019 02:54 PM, Ulrich Weigand wrote:
> > Pedro Alves wrote:
> > 
> > > So the question becomes a simple cosmetic one.  In this case:
> > > 
> > > >   (gdb) info variables
> > > >   [...]
> > > >   Non-debugging symbols:
> > > >   0x0000000010020088  main
> > > 
> > > Should "main" be printed with function style, or variable style.
> > > This basically affects the color used to print the symbol.
> > > 
> > > In Philippe's patch, we'd print it in variable style.  If we used
> > > msymbol_is_function instead of his "is text symbol" check, we'd
> > > print it in function style.
> > 
> > Ah, I see.  I guess that doesn't really matter that much at this
> > point.  Since we're already showing it under "variables" we might
> > as well use the variable style.  But in the end either way would
> > be fine with me ...
> 
> OK, in that case, let's just use with Philippe already has.
Thanks for the review and discussion.
With all this + the feedback of Joel
(in https://sourceware.org/ml/gdb-patches/2019-01/msg00560.html
that confirmed that the ada-lang.c code will effectively use
function style only to style function names), I am assuming
I can push the series (I will retest before that).

Thanks

Philippe