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

Message ID 20190608195434.26512-2-kevinb@redhat.com
State New
Headers show
Series
  • Non-contiguous address range bug fixes / improvements
Related show

Commit Message

Kevin Buettner June 8, 2019, 7:54 p.m.
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(-)

-- 
2.21.0

Comments

Pedro Alves June 21, 2019, 2:26 p.m. | #1
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

Patch

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))
 	{
 	  /* If this is a function (i.e. a code address), strip out any
 	     non-address bits.  For instance, display a pointer to the
diff --git a/gdb/stack.c b/gdb/stack.c
index 408c795e38..d511690a14 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -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