Re: [PATCH 1/4] Prefer symtab symbol over minsym for function names in non-contiguous blocks

[Pedro Alves <palves at redhat dot com>]

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