[1/4] Prefer symtab symbol over minsym for function names in non-contiguous blocks
Commit Message
The discussion on gdb-patches which led to this patch may be found
here:
https://www.sourceware.org/ml/gdb-patches/2019-05/msg00018.html
Here's a brief synopsis/analysis:
Eli Zaretskii, while debugging a Windows emacs executable, found
that functions comprised of more than one (non-contiguous)
address range were not being displayed correctly in a backtrace. This
is the example that Eli provided:
(gdb) bt
#0 0x76a63227 in KERNELBASE!DebugBreak ()
from C:\Windows\syswow64\KernelBase.dll
#1 0x012e7b89 in emacs_abort () at w32fns.c:10768
#2 0x012e1f3b in print_vectorlike.cold () at print.c:1824
#3 0x011d2dec in print_object (obj=<optimized out>, printcharfun=XIL(0),
escapeflag=true) at print.c:2150
The function print_vectorlike consists of two address ranges, one of
which contains "cold" code which is expected to not execute very often.
There is a minimal symbol, print_vectorlike.cold.65, which is the address
of the "cold" range.
GDB is prefering this minsym over the the name provided by the
DWARF info due to some really old code in GDB which handles
"certain pathological cases". See the first big block comment
in find_frame_funname for more information.
I considered removing the code for this corner case entirely, but it
seems as though it might still be useful, so I left it intact.
That code is already disabled for inline functions. I added a
condition which disables it for non-contiguous functions as well.
The change to find_frame_funname in stack.c fixes this problem for
stack traces. A similar change to print_address_symbolic in
printcmd.c fixes this problem for the "x" command and other commands
which use print_address_symbolic().
gdb/ChangeLog:
* stack.c (find_frame_funname): Disable use of minsym for function
name in functions comprised of non-contiguous blocks.
* printcmd.c (print_address_symbolic): Likewise.
---
gdb/printcmd.c | 4 +++-
gdb/stack.c | 15 ++++++++++++---
2 files changed, 15 insertions(+), 4 deletions(-)
Comments
On 6/8/19 8:54 PM, Kevin Buettner wrote:
> The discussion on gdb-patches which led to this patch may be found
> here:
>
> https://www.sourceware.org/ml/gdb-patches/2019-05/msg00018.html
>
> Here's a brief synopsis/analysis:
>
> Eli Zaretskii, while debugging a Windows emacs executable, found
> that functions comprised of more than one (non-contiguous)
> address range were not being displayed correctly in a backtrace. This
> is the example that Eli provided:
>
> (gdb) bt
> #0 0x76a63227 in KERNELBASE!DebugBreak ()
> from C:\Windows\syswow64\KernelBase.dll
> #1 0x012e7b89 in emacs_abort () at w32fns.c:10768
> #2 0x012e1f3b in print_vectorlike.cold () at print.c:1824
> #3 0x011d2dec in print_object (obj=<optimized out>, printcharfun=XIL(0),
> escapeflag=true) at print.c:2150
>
> The function print_vectorlike consists of two address ranges, one of
> which contains "cold" code which is expected to not execute very often.
> There is a minimal symbol, print_vectorlike.cold.65, which is the address
> of the "cold" range.
>
> GDB is prefering this minsym over the the name provided by the
> DWARF info due to some really old code in GDB which handles
> "certain pathological cases". See the first big block comment
> in find_frame_funname for more information.
Yuck!
>
> I considered removing the code for this corner case entirely, but it
> seems as though it might still be useful, so I left it intact.
>
Yeah, I'd be inclined to try removing it too. The comment
smells of a.out or stabs limitations. But I'm not 100% sure,
and I'm sympathetic with forward incremental progress.
> That code is already disabled for inline functions. I added a
> condition which disables it for non-contiguous functions as well.
>
> The change to find_frame_funname in stack.c fixes this problem for
> stack traces. A similar change to print_address_symbolic in
> printcmd.c fixes this problem for the "x" command and other commands
> which use print_address_symbolic().
>
> gdb/ChangeLog:
>
> * stack.c (find_frame_funname): Disable use of minsym for function
> name in functions comprised of non-contiguous blocks.
> * printcmd.c (print_address_symbolic): Likewise.
> ---
> gdb/printcmd.c | 4 +++-
> gdb/stack.c | 15 ++++++++++++---
> 2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index 9e84594fe6..e00a9c671a 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -627,7 +627,9 @@ build_address_symbolic (struct gdbarch *gdbarch,
>
> if (msymbol.minsym != NULL)
> {
> - if (BMSYMBOL_VALUE_ADDRESS (msymbol) > name_location || symbol == NULL)
> + if (symbol == NULL
> + || (BLOCK_CONTIGUOUS_P (SYMBOL_BLOCK_VALUE (symbol))
> + && BMSYMBOL_VALUE_ADDRESS (msymbol) > name_location))
I think this warrants a comment somewhere. Maybe a bit higher up,
where it reads:
/* First try to find the address in the symbol table, then
in the minsyms. Take the closest one. */
Or better yet, abstract out the relevant bits in
build_address_symbolic and find_frame_funname to a separate
function, and then use that function in both places?
IIUC, in both cases, we're looking to see if there's a minimal
symbol that would be better than the debug symbol we start with.
> @@ -1067,9 +1067,18 @@ find_frame_funname (struct frame_info *frame, enum language *funlang,
>
> struct bound_minimal_symbol msymbol;
>
> - /* Don't attempt to do this for inlined functions, which do not
> - have a corresponding minimal symbol. */
> - if (!block_inlined_p (SYMBOL_BLOCK_VALUE (func)))
> + /* Don't attempt to do this for two cases:
> +
> + 1) Inlined functions, which do not have a corresponding minimal
> + symbol.
> +
> + 2) Functions which are comprised of non-contiguous blocks.
> + Such functions often contain a minimal symbol for a
> + "cold" range, i.e. code which is not expected to execute
> + very often. It is incorrect to use the minimal symbol
> + associated with this range. */
I don't find this "incorrect" here very useful -- why is it incorrect
is my immediate question when I read this.
Maybe that old comment:
So look in the minimal symbol tables as well, and if it comes
up with a larger address for the function use that instead.
I don't think this can ever cause any problems; there
shouldn't be any minimal symbols in the middle of a function;
should be updated.
Thanks,
Pedro Alves
>> GDB is prefering this minsym over the the name provided by the
>> DWARF info due to some really old code in GDB which handles
>> "certain pathological cases". See the first big block comment
>> in find_frame_funname for more information.
Pedro> Yuck!
>> I considered removing the code for this corner case entirely, but it
>> seems as though it might still be useful, so I left it intact.
Pedro> Yeah, I'd be inclined to try removing it too. The comment
Pedro> smells of a.out or stabs limitations. But I'm not 100% sure,
Pedro> and I'm sympathetic with forward incremental progress.
That code dates to the creation of the sourceware repository.
I think it could be deleted. And, if it's still a bug somehow, it seems
better to fix it some other way, and not let stabs and/or a.out
weirdness into the generic code.
Tom
On Wed, 26 Jun 2019 11:30:51 -0600
Tom Tromey <tom@tromey.com> wrote:
> >> GDB is prefering this minsym over the the name provided by the
> >> DWARF info due to some really old code in GDB which handles
> >> "certain pathological cases". See the first big block comment
> >> in find_frame_funname for more information.
>
> Pedro> Yuck!
>
> >> I considered removing the code for this corner case entirely, but it
> >> seems as though it might still be useful, so I left it intact.
>
> Pedro> Yeah, I'd be inclined to try removing it too. The comment
> Pedro> smells of a.out or stabs limitations. But I'm not 100% sure,
> Pedro> and I'm sympathetic with forward incremental progress.
>
> That code dates to the creation of the sourceware repository.
>
> I think it could be deleted. And, if it's still a bug somehow, it seems
> better to fix it some other way, and not let stabs and/or a.out
> weirdness into the generic code.
Okay, for v2 of this patch series, I'll get rid of that code.
Thanks,
Kevin
@@ -627,7 +627,9 @@ build_address_symbolic (struct gdbarch *gdbarch,
if (msymbol.minsym != NULL)
{
- if (BMSYMBOL_VALUE_ADDRESS (msymbol) > name_location || symbol == NULL)
+ if (symbol == NULL
+ || (BLOCK_CONTIGUOUS_P (SYMBOL_BLOCK_VALUE (symbol))
+ && BMSYMBOL_VALUE_ADDRESS (msymbol) > name_location))
{
/* If this is a function (i.e. a code address), strip out any
non-address bits. For instance, display a pointer to the
@@ -1067,9 +1067,18 @@ find_frame_funname (struct frame_info *frame, enum language *funlang,
struct bound_minimal_symbol msymbol;
- /* Don't attempt to do this for inlined functions, which do not
- have a corresponding minimal symbol. */
- if (!block_inlined_p (SYMBOL_BLOCK_VALUE (func)))
+ /* Don't attempt to do this for two cases:
+
+ 1) Inlined functions, which do not have a corresponding minimal
+ symbol.
+
+ 2) Functions which are comprised of non-contiguous blocks.
+ Such functions often contain a minimal symbol for a
+ "cold" range, i.e. code which is not expected to execute
+ very often. It is incorrect to use the minimal symbol
+ associated with this range. */
+ if (!block_inlined_p (SYMBOL_BLOCK_VALUE (func))
+ && BLOCK_CONTIGUOUS_P (SYMBOL_BLOCK_VALUE (func)))
msymbol
= lookup_minimal_symbol_by_pc (get_frame_address_in_block (frame));
else