The 'cold' function attribute and GDB

Message ID 20190505130403.56de7f08@f29-4.lan
State New, archived
Headers

Commit Message

Kevin Buettner May 5, 2019, 8:04 p.m. UTC
  On Sat, 04 May 2019 11:30:14 +0300
Eli Zaretskii <eliz@gnu.org> wrote:

> Do you want me to test some patch?

See below.
 
> And why doesn't the GNU/Linux executable have this minimal symbol in
> the first place, btw?

It turns out that the GNU/Linux executable does have this symbol.  The
difference is in its placement. The GNU/Linux executable placed the
.cold address before that of the entry pc for the function.  The windows
executable places it after the entry pc.  The code mentioned in my
earlier analysis prefers to use the minsym only when it's greater than
the entry pc.

My patch is below.  If it works for you and the approach seems sound,
I'll work on a test case which doesn't depend on gcc placing the
.cold section after the entry pc.

- - -

Prefer symtab symbol over minsym for function names in discontiguous blocks

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 info.

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.

gdb/ChangeLog:

	* stack.c (find_frame_funname): Disable use of minsym for function
	name in functions comprised of non-contiguous blocks.
---
 gdb/stack.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
  

Comments

Eli Zaretskii May 6, 2019, 3:33 p.m. UTC | #1
> Date: Sun, 5 May 2019 13:04:03 -0700
> From: Kevin Buettner <kevinb@redhat.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, simark@simark.ca
> 
> My patch is below.  If it works for you and the approach seems sound,
> I'll work on a test case which doesn't depend on gcc placing the
> .cold section after the entry pc.

The patch fixes the backtrace, thanks.

The output from "info line" still references the .cold symbol,
though.  I think this was not so on GNU/Linux?  Is it also by sheer
luck, because of the order of the 'cold' symbols in the executable?
  
Kevin Buettner May 6, 2019, 4:24 p.m. UTC | #2
On Mon, 06 May 2019 18:33:37 +0300
Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Sun, 5 May 2019 13:04:03 -0700
> > From: Kevin Buettner <kevinb@redhat.com>
> > Cc: Eli Zaretskii <eliz@gnu.org>, simark@simark.ca
> > 
> > My patch is below.  If it works for you and the approach seems sound,
> > I'll work on a test case which doesn't depend on gcc placing the
> > .cold section after the entry pc.  
> 
> The patch fixes the backtrace, thanks.

Thanks for testing it.  I'll begin working on a test case.

> The output from "info line" still references the .cold symbol,
> though.  I think this was not so on GNU/Linux?  Is it also by sheer
> luck, because of the order of the 'cold' symbols in the executable?

The address printed by "info line" is constructed by
build_address_symbolic in printcmd.c.  It contains code similar
to that in find_frame_funname.

You recall correctly - GNU/Linux did not display the .cold symbol when
using the "info line" command.  The GNU/Linux executable that I
examined does have .cold symbols, but (at least at the examples I saw)
placed them at addresses lower than the corresponding function.

[ .cold symbols at lower addresses is also an interesting case, causing
all sorts of problems when I first started looking at the non-contiguous
range problem.  GDB had assumed that the lowest address might also
be a start address. ]

I'll amend my patch to also  address the "info line" problem too.
(Though for that one, it doesn't bother my quite as much to see the
minimal symbol.)

Kevin
  

Patch

diff --git a/gdb/stack.c b/gdb/stack.c
index e5de10949d..66ba5ab43c 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