[RFAv2,3/3] Make symtab.c better styled.
Commit Message
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(-)
Comments
>>>>> "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
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
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
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
@@ -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