[PATCH 0/2] btrace: perf improvements

[PATCH 0/2] btrace: perf improvements

[Markus Metzger]

The subsequent patches improve the performance of processing branch trace by
reducing the number of symbol table lookups.

For the currently hard-coded buffer size the impact should only be noticeable
for big programs.  When tracing GDB itself, for example, there is no noticeable
delay.

Subsequent patches will allow the buffer size to be configurable.  Bigger
buffers will require faster processing.

Markus Metzger (2):
  btrace: only search for lines in current symtab
  btrace: avoid symbol lookup

 gdb/btrace.c                                  | 128 +++++++++++++-------------
 gdb/btrace.h                                  |   3 +
 gdb/record-btrace.c                           |   8 +-
 gdb/testsuite/gdb.btrace/exception.exp        |   4 +-
 gdb/testsuite/gdb.btrace/static_functions.exp |  62 +++++++++++++
 5 files changed, 135 insertions(+), 70 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/static_functions.exp

-- 
1.8.3.1

[PATCH 1/2] btrace: only search for lines in current symtab

[Markus Metzger]

When computing the function trace, we lookup the line number from the PC
for each instruction in the trace.  This is very expensive.

Instead, lookup the symtab once when we create the btrace function and
then only search in the linetable for that symtab.

Also use the stored symtab to print the filename.

2014-03-07  Markus Metzger  <markus.t.metzger@intel.com>

	* btrace.h (btrace_function)<symtab>: New.
	* btrace.c (ftrace_skip_file): Remove.
	(ftrace_print_filename): Use symtab.
	(ftrace_new_function): Update parameters.  Lookup symtab.
	(ftrace_new_call, ftrace_new_tailcall, ftrace_new_return,
	ftrace_new_switch): Update parameters.
	(ftrace_update_function): Pass pc.
	(ftrace_update_lines): Only search in BFUN->SYMTAB.
	* record-btrace.c (btrace_call_history_src_line): Use symtab.
---
 gdb/btrace.c        | 116 +++++++++++++++++++++++++---------------------------
 gdb/btrace.h        |   3 ++
 gdb/record-btrace.c |   8 ++--
 3 files changed, 63 insertions(+), 64 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 601eb41..b2e34b7 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -73,13 +73,12 @@ ftrace_print_function_name (const struct btrace_function *bfun)
 static const char *
 ftrace_print_filename (const struct btrace_function *bfun)
 {
-  struct symbol *sym;
+  struct symtab *symtab;
   const char *filename;
 
-  sym = bfun->sym;
-
-  if (sym != NULL)
-    filename = symtab_to_filename_for_display (sym->symtab);
+  symtab = bfun->symtab;
+  if (symtab != NULL)
+    filename = symtab_to_filename_for_display (symtab);
   else
     filename = "<unknown>";
 
@@ -168,26 +167,6 @@ ftrace_function_switched (const struct btrace_function *bfun,
   return 0;
 }
 
-/* Return non-zero if we should skip this file when generating the function
-   call history, zero otherwise.
-   We would want to do that if, say, a macro that is defined in another file
-   is expanded in this function.  */
-
-static int
-ftrace_skip_file (const struct btrace_function *bfun, const char *fullname)
-{
-  struct symbol *sym;
-  const char *bfile;
-
-  sym = bfun->sym;
-  if (sym == NULL)
-    return 1;
-
-  bfile = symtab_to_fullname (sym->symtab);
-
-  return (filename_cmp (bfile, fullname) != 0);
-}
-
 /* Allocate and initialize a new branch trace function segment.
    PREV is the chronologically preceding function segment.
    MFUN and FUN are the symbol information we have for this function.  */
@@ -195,7 +174,8 @@ ftrace_skip_file (const struct btrace_function *bfun, const char *fullname)
 static struct btrace_function *
 ftrace_new_function (struct btrace_function *prev,
 		     struct minimal_symbol *mfun,
-		     struct symbol *fun)
+		     struct symbol *fun,
+		     CORE_ADDR pc)
 {
   struct btrace_function *bfun;
 
@@ -205,6 +185,11 @@ ftrace_new_function (struct btrace_function *prev,
   bfun->sym = fun;
   bfun->flow.prev = prev;
 
+  if (fun != NULL)
+    bfun->symtab = SYMBOL_SYMTAB (fun);
+  else
+    bfun->symtab = find_pc_symtab (pc);
+
   /* We start with the identities of min and max, respectively.  */
   bfun->lbegin = INT_MAX;
   bfun->lend = INT_MIN;
@@ -270,11 +255,12 @@ ftrace_fixup_caller (struct btrace_function *bfun,
 static struct btrace_function *
 ftrace_new_call (struct btrace_function *caller,
 		 struct minimal_symbol *mfun,
-		 struct symbol *fun)
+		 struct symbol *fun,
+		 CORE_ADDR pc)
 {
   struct btrace_function *bfun;
 
-  bfun = ftrace_new_function (caller, mfun, fun);
+  bfun = ftrace_new_function (caller, mfun, fun, pc);
   bfun->up = caller;
   bfun->level = caller->level + 1;
 
@@ -290,11 +276,12 @@ ftrace_new_call (struct btrace_function *caller,
 static struct btrace_function *
 ftrace_new_tailcall (struct btrace_function *caller,
 		     struct minimal_symbol *mfun,
-		     struct symbol *fun)
+		     struct symbol *fun,
+		     CORE_ADDR pc)
 {
   struct btrace_function *bfun;
 
-  bfun = ftrace_new_function (caller, mfun, fun);
+  bfun = ftrace_new_function (caller, mfun, fun, pc);
   bfun->up = caller;
   bfun->level = caller->level + 1;
   bfun->flags |= BFUN_UP_LINKS_TO_TAILCALL;
@@ -358,11 +345,12 @@ static struct btrace_function *
 ftrace_new_return (struct gdbarch *gdbarch,
 		   struct btrace_function *prev,
 		   struct minimal_symbol *mfun,
-		   struct symbol *fun)
+		   struct symbol *fun,
+		   CORE_ADDR pc)
 {
   struct btrace_function *bfun, *caller;
 
-  bfun = ftrace_new_function (prev, mfun, fun);
+  bfun = ftrace_new_function (prev, mfun, fun, pc);
 
   /* It is important to start at PREV's caller.  Otherwise, we might find
      PREV itself, if PREV is a recursive function.  */
@@ -433,13 +421,14 @@ ftrace_new_return (struct gdbarch *gdbarch,
 static struct btrace_function *
 ftrace_new_switch (struct btrace_function *prev,
 		   struct minimal_symbol *mfun,
-		   struct symbol *fun)
+		   struct symbol *fun,
+		   CORE_ADDR pc)
 {
   struct btrace_function *bfun;
 
   /* This is an unexplained function switch.  The call stack will likely
      be wrong at this point.  */
-  bfun = ftrace_new_function (prev, mfun, fun);
+  bfun = ftrace_new_function (prev, mfun, fun, pc);
 
   /* We keep the function level.  */
   bfun->level = prev->level;
@@ -474,7 +463,7 @@ ftrace_update_function (struct gdbarch *gdbarch,
 
   /* If we didn't have a function before, we create one.  */
   if (bfun == NULL)
-    return ftrace_new_function (bfun, mfun, fun);
+    return ftrace_new_function (bfun, mfun, fun, pc);
 
   /* Check the last instruction, if we have one.
      We do this check first, since it allows us to fill in the call stack
@@ -491,7 +480,7 @@ ftrace_update_function (struct gdbarch *gdbarch,
 
       /* Check for returns.  */
       if (gdbarch_insn_is_ret (gdbarch, lpc))
-	return ftrace_new_return (gdbarch, bfun, mfun, fun);
+	return ftrace_new_return (gdbarch, bfun, mfun, fun, pc);
 
       /* Check for calls.  */
       if (gdbarch_insn_is_call (gdbarch, lpc))
@@ -502,7 +491,7 @@ ftrace_update_function (struct gdbarch *gdbarch,
 
 	  /* Ignore calls to the next instruction.  They are used for PIC.  */
 	  if (lpc + size != pc)
-	    return ftrace_new_call (bfun, mfun, fun);
+	    return ftrace_new_call (bfun, mfun, fun, pc);
 	}
     }
 
@@ -529,10 +518,10 @@ ftrace_update_function (struct gdbarch *gdbarch,
 
 	  /* Jumps indicate optimized tail calls.  */
 	  if (start == pc && gdbarch_insn_is_jump (gdbarch, lpc))
-	    return ftrace_new_tailcall (bfun, mfun, fun);
+	    return ftrace_new_tailcall (bfun, mfun, fun, pc);
 	}
 
-      return ftrace_new_switch (bfun, mfun, fun);
+      return ftrace_new_switch (bfun, mfun, fun, pc);
     }
 
   return bfun;
@@ -543,32 +532,39 @@ ftrace_update_function (struct gdbarch *gdbarch,
 static void
 ftrace_update_lines (struct btrace_function *bfun, CORE_ADDR pc)
 {
-  struct symtab_and_line sal;
-  const char *fullname;
+  struct linetable *lines;
+  struct symtab *symtab;
+  int i;
 
-  sal = find_pc_line (pc, 0);
-  if (sal.symtab == NULL || sal.line == 0)
-    {
-      DEBUG_FTRACE ("no lines at %s", core_addr_to_string_nz (pc));
-      return;
-    }
+  symtab = bfun->symtab;
+  if (symtab == NULL)
+    return;
+
+  lines = LINETABLE (symtab);
+  if (lines == NULL)
+    return;
 
-  /* Check if we switched files.  This could happen if, say, a macro that
-     is defined in another file is expanded here.  */
-  fullname = symtab_to_fullname (sal.symtab);
-  if (ftrace_skip_file (bfun, fullname))
+  for (i = 0; i < lines->nitems; ++i)
     {
-      DEBUG_FTRACE ("ignoring file at %s, file=%s",
-		    core_addr_to_string_nz (pc), fullname);
-      return;
-    }
+      struct linetable_entry *entry;
 
-  /* Update the line range.  */
-  bfun->lbegin = min (bfun->lbegin, sal.line);
-  bfun->lend = max (bfun->lend, sal.line);
+      entry = &lines->item[i];
+      if (entry->pc == pc)
+	{
+	  int line;
 
-  if (record_debug > 1)
-    ftrace_debug (bfun, "update lines");
+	  line = entry->line;
+
+	  /* Update the line range.  */
+	  bfun->lbegin = min (bfun->lbegin, line);
+	  bfun->lend = max (bfun->lend, line);
+
+	  if (record_debug > 1)
+	    ftrace_debug (bfun, "update lines");
+
+	  break;
+	}
+    }
 }
 
 /* Add the instruction at PC to BFUN's instructions.  */
diff --git a/gdb/btrace.h b/gdb/btrace.h
index f83a80f..67ba577 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -76,6 +76,9 @@ struct btrace_function
   struct minimal_symbol *msym;
   struct symbol *sym;
 
+  /* The symbol table.  May be NULL.  */
+  struct symtab *symtab;
+
   /* The previous and next segment belonging to the same function.
      If a function calls another function, the former will have at least
      two segments: one before the call and another after the return.  */
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index d4c0d42..1851823 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -507,15 +507,15 @@ static void
 btrace_call_history_src_line (struct ui_out *uiout,
 			      const struct btrace_function *bfun)
 {
-  struct symbol *sym;
+  struct symtab *symtab;
   int begin, end;
 
-  sym = bfun->sym;
-  if (sym == NULL)
+  symtab = bfun->symtab;
+  if (symtab == NULL)
     return;
 
   ui_out_field_string (uiout, "file",
-		       symtab_to_filename_for_display (sym->symtab));
+		       symtab_to_filename_for_display (symtab));
 
   begin = bfun->lbegin;
   end = bfun->lend;
-- 
1.8.3.1

Re: [PATCH 1/2] btrace: only search for lines in current symtab

[Jan Kratochvil]

On Fri, 07 Mar 2014 09:57:44 +0100, Markus Metzger wrote:
[...]
> @@ -543,32 +532,39 @@ ftrace_update_function (struct gdbarch *gdbarch,
>  static void
>  ftrace_update_lines (struct btrace_function *bfun, CORE_ADDR pc)
>  {
> -  struct symtab_and_line sal;
> -  const char *fullname;
> +  struct linetable *lines;
> +  struct symtab *symtab;
> +  int i;
>  
> -  sal = find_pc_line (pc, 0);
> -  if (sal.symtab == NULL || sal.line == 0)
> -    {
> -      DEBUG_FTRACE ("no lines at %s", core_addr_to_string_nz (pc));
> -      return;
> -    }
> +  symtab = bfun->symtab;
> +  if (symtab == NULL)
> +    return;
> +
> +  lines = LINETABLE (symtab);
> +  if (lines == NULL)
> +    return;
>  
> -  /* Check if we switched files.  This could happen if, say, a macro that
> -     is defined in another file is expanded here.  */
> -  fullname = symtab_to_fullname (sal.symtab);
> -  if (ftrace_skip_file (bfun, fullname))
> +  for (i = 0; i < lines->nitems; ++i)

The line table has ordered PC values so one can find the right line in
logarithmic time.


>      {
> -      DEBUG_FTRACE ("ignoring file at %s, file=%s",
> -		    core_addr_to_string_nz (pc), fullname);
> -      return;
> -    }
> +      struct linetable_entry *entry;
>  
> -  /* Update the line range.  */
> -  bfun->lbegin = min (bfun->lbegin, sal.line);
> -  bfun->lend = max (bfun->lend, sal.line);
> +      entry = &lines->item[i];
> +      if (entry->pc == pc)

This is not right, PC can be between two ENTRY->PC values and it should match
the former line.  This implementation would not find a matching line.



> +	{
> +	  int line;
>  
> -  if (record_debug > 1)
> -    ftrace_debug (bfun, "update lines");
> +	  line = entry->line;
> +
> +	  /* Update the line range.  */
> +	  bfun->lbegin = min (bfun->lbegin, line);
> +	  bfun->lend = max (bfun->lend, line);
> +
> +	  if (record_debug > 1)
> +	    ftrace_debug (bfun, "update lines");
> +
> +	  break;
> +	}
> +    }
>  }
>  
>  /* Add the instruction at PC to BFUN's instructions.  */


I do not think this is the right approach, find_pc_sect_line() can be improved
to at least look up the lines in logarithmic time by bisecting the range.
It could also use some cache of recent lookups.

Also I am not sure this new implementation handles correctly include files and
their boundaries, which find_pc_sect_line() has to take care of.

This is my personal opinion unrelated to GDB project maintainers' opinion.


Regards,
Jan

[PATCH 2/2] btrace: avoid symbol lookup

[Markus Metzger]

The debug symbol lookup is quite expensive, particularly if no symbol
is found for a given PC.

Avoid the lookup when computing the function trace if we already found
a minimal symbol.

The only different seems to be that some function names are now printed
without '()'.

2014-03-07  Markus Metzger  <markus.t.metzger@intel.com>

	* btrace.c (ftrace_update_function): Lookup symbol only if there
	is no minimal symbol.

testsuite/
	* gdb.btrace/exception.exp: Update.
	* gdb.btrace/static_functions.exp: New.
---
 gdb/btrace.c                                  | 12 ++++--
 gdb/testsuite/gdb.btrace/exception.exp        |  4 +-
 gdb/testsuite/gdb.btrace/static_functions.exp | 62 +++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 6 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/static_functions.exp

diff --git a/gdb/btrace.c b/gdb/btrace.c
index b2e34b7..feb7bea 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -451,13 +451,17 @@ ftrace_update_function (struct gdbarch *gdbarch,
   struct symbol *fun;
   struct btrace_insn *last;
 
-  /* Try to determine the function we're in.  We use both types of symbols
-     to avoid surprises when we sometimes get a full symbol and sometimes
-     only a minimal symbol.  */
-  fun = find_pc_function (pc);
+  /* Try to determine the function we're in.  */
   bmfun = lookup_minimal_symbol_by_pc (pc);
   mfun = bmfun.minsym;
 
+  /* We only lookup the symbol in the debug information if we have not found
+     a minimal symbol.  This avoids the expensive lookup for globally visible
+     symbols.  */
+  fun = NULL;
+  if (mfun == NULL)
+    fun = find_pc_function (pc);
+
   if (fun == NULL && mfun == NULL)
     DEBUG_FTRACE ("no symbol at %s", core_addr_to_string_nz (pc));
 
diff --git a/gdb/testsuite/gdb.btrace/exception.exp b/gdb/testsuite/gdb.btrace/exception.exp
index 0279a7f..71f54a2 100755
--- a/gdb/testsuite/gdb.btrace/exception.exp
+++ b/gdb/testsuite/gdb.btrace/exception.exp
@@ -47,7 +47,7 @@ gdb_continue_to_breakpoint "cont to bp.2" ".*$srcfile:$bp_2\r\n.*"
 send_gdb "record function-call-history 1\n"
 gdb_expect_list "flat" "\r\n$gdb_prompt $" [list \
   [join [list \
-    "1\tmain\\(\\)" \
+    "1\tmain" \
     "2\ttest\\(\\)" \
     "3\tfoo\\(\\)" \
     "4\tbar\\(\\)" \
@@ -60,7 +60,7 @@ gdb_expect_list "flat" "\r\n$gdb_prompt $" [list \
 send_gdb "record function-call-history /c 1\n"
 gdb_expect_list "indented" "\r\n$gdb_prompt $" [list \
   [join [list \
-    "1\tmain\\(\\)" \
+    "1\tmain" \
   "2\t  test\\(\\)" \
   "3\t    foo\\(\\)" \
   "4\t      bar\\(\\)" \
diff --git a/gdb/testsuite/gdb.btrace/static_functions.exp b/gdb/testsuite/gdb.btrace/static_functions.exp
new file mode 100644
index 0000000..34ed485
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/static_functions.exp
@@ -0,0 +1,62 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2013 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corp. <markus.t.metzger@intel.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# check for btrace support
+if { [skip_btrace_tests] } { return -1 }
+
+# start inferior
+standard_testfile unknown_functions.c
+
+# discard local symbols
+set ldflags "additional_flags=-Wl,-x"
+if [prepare_for_testing $testfile.exp $testfile $srcfile {$ldflags debug}] {
+    return -1
+}
+if ![runto test] {
+    return -1
+}
+
+# we want to see the full trace for this test
+gdb_test_no_output "set record function-call-history-size 0"
+
+# trace from one call of test to the next
+gdb_test_no_output "record btrace"
+gdb_continue_to_breakpoint "cont to test" ".*test.*"
+
+# show the flat branch trace
+gdb_test "record function-call-history 1" [join [list \
+  "1\ttest" \
+  "2\tfoo" \
+  "3\tbar" \
+  "4\tfoo" \
+  "5\ttest" \
+  "6\tmain" \
+  "7\ttest" \
+  ] "\r\n"] "flat"
+
+# show the branch trace with calls indented
+gdb_test "record function-call-history /c 1" [join [list \
+  "1\t  test" \
+  "2\t    foo" \
+  "3\t      bar" \
+  "4\t    foo" \
+  "5\t  test" \
+  "6\tmain" \
+  "7\t  test" \
+  ] "\r\n"] "indented"
-- 
1.8.3.1

Re: [PATCH 2/2] btrace: avoid symbol lookup

[Pedro Alves]

On 03/07/2014 08:57 AM, Markus Metzger wrote:
> --- a/gdb/testsuite/gdb.btrace/exception.exp
> +++ b/gdb/testsuite/gdb.btrace/exception.exp
> @@ -47,7 +47,7 @@ gdb_continue_to_breakpoint "cont to bp.2" ".*$srcfile:$bp_2\r\n.*"
>  send_gdb "record function-call-history 1\n"
>  gdb_expect_list "flat" "\r\n$gdb_prompt $" [list \
>    [join [list \
> -    "1\tmain\\(\\)" \
> +    "1\tmain" \
>      "2\ttest\\(\\)" \
>      "3\tfoo\\(\\)" \
>      "4\tbar\\(\\)" \

I wonder whether people will think the different looks are a bug...

Also, doesn't that mean we'll show mangled C++ names?  Won't that
be quite user unfriendly?

-- 
Pedro Alves

RE: [PATCH 2/2] btrace: avoid symbol lookup

[Metzger, Markus T]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Friday, March 07, 2014 4:56 PM

Thanks for your review.

> To: Metzger, Markus T
> Cc: jan.kratochvil@redhat.com; gdb-patches@sourceware.org
> Subject: Re: [PATCH 2/2] btrace: avoid symbol lookup
> 
> On 03/07/2014 08:57 AM, Markus Metzger wrote:
> > --- a/gdb/testsuite/gdb.btrace/exception.exp
> > +++ b/gdb/testsuite/gdb.btrace/exception.exp
> > @@ -47,7 +47,7 @@ gdb_continue_to_breakpoint "cont to bp.2"
> ".*$srcfile:$bp_2\r\n.*"
> >  send_gdb "record function-call-history 1\n"
> >  gdb_expect_list "flat" "\r\n$gdb_prompt $" [list \
> >    [join [list \
> > -    "1\tmain\\(\\)" \
> > +    "1\tmain" \
> >      "2\ttest\\(\\)" \
> >      "3\tfoo\\(\\)" \
> >      "4\tbar\\(\\)" \
> 
> I wonder whether people will think the different looks are a bug...
> 
> Also, doesn't that mean we'll show mangled C++ names?  Won't that
> be quite user unfriendly?

It would.  Fortunately, GDB already prints the demangled name also for
minimal symbols - see MSYMBOL_PRINT_NAME which calls
symbol_natural_name.  Minimal and full symbols seem to be treated the
same way.

This does not work for main, though, for which the minimal symbol has
language 'auto' and the full symbol has language 'cplus'.

Regards,
Markus.

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [PATCH 2/2] btrace: avoid symbol lookup

[Pedro Alves]

On 03/07/2014 08:57 AM, Markus Metzger wrote:
> +# Copyright 2013 Free Software Foundation, Inc.

BTW, "2013-2014".

-- 
Pedro Alves

Re: [PATCH 2/2] btrace: avoid symbol lookup

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 2493 bytes --]

On Fri, 07 Mar 2014 09:57:45 +0100, Markus Metzger wrote:
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -451,13 +451,17 @@ ftrace_update_function (struct gdbarch *gdbarch,
>    struct symbol *fun;
>    struct btrace_insn *last;
>  
> -  /* Try to determine the function we're in.  We use both types of symbols
> -     to avoid surprises when we sometimes get a full symbol and sometimes
> -     only a minimal symbol.  */
> -  fun = find_pc_function (pc);
> +  /* Try to determine the function we're in.  */
>    bmfun = lookup_minimal_symbol_by_pc (pc);
>    mfun = bmfun.minsym;
>  
> +  /* We only lookup the symbol in the debug information if we have not found
> +     a minimal symbol.  This avoids the expensive lookup for globally visible
> +     symbols.  */
> +  fun = NULL;
> +  if (mfun == NULL)
> +    fun = find_pc_function (pc);
> +
>    if (fun == NULL && mfun == NULL)
>      DEBUG_FTRACE ("no symbol at %s", core_addr_to_string_nz (pc));
>  
[...]

Behavior after the change is not the same.  DWARF functions symbols can span
over discontiguous ranges with DW_AT_ranges but their corresponding ELF
function symbols cannot, therefore those are just some approximation.

Testcase gdb.dwarf2/dw2-objfile-overlap.exp tests such a case, from:
	https://sourceware.org/ml/gdb-patches/2011-11/msg00166.html

For address of symbol "inner":
 * find_pc_function finds DWARF symbol "inner"
 * lookup_minimal_symbol_by_pc finds ELF symbol "outer_inner"

In few Fedora 20 x86_64 -O2 -g built libraries I have not found any
DW_TAG_subprogram using DW_AT_ranges, only DW_TAG_inlined_subroutine use
DW_AT_ranges.  But that is more a limitation of gcc, other compilers may
produce DW_TAG_subprogram using DW_AT_ranges with overlapping function parts.

Inlined functions are found by neither find_pc_function nor
lookup_minimal_symbol_by_pc (block_containing_function (block_for_pc ()) finds
inlined function).  Not sure if it is not a bug of find_pc_function but that
is off-topic here.

I do not think it will be hit in real world cases.  GDB would need some better
abstraction of the symbol kinds to be more systematic in what it outputs.

It would be good to know how much it helps your usecase as it is not a fully
clean/transparent change.


Thanks,
Jan


set confirm no
set trace-commands
file gdb.dwarf2/dw2-objfile-overlap-outer.x
add-symbol-file gdb.dwarf2/dw2-objfile-overlap-inner.x outer_inner
info sym inner
->
find_pc_function "inner".
lookup_minimal_symbol_by_pc "outer_inner".

[-- Attachment #2: 1 --]
[-- Type: text/plain, Size: 934 bytes --]

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 87b1448..f1517d2 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1107,6 +1107,27 @@ sym_info (char *arg, int from_tty)
     error_no_arg (_("address"));
 
   addr = parse_and_eval_address (arg);
+
+{
+struct symbol *sym=find_pc_function (addr);
+if (sym) {
+  printf_filtered ("find_pc_function \"");
+  fprintf_symbol_filtered (gdb_stdout, SYMBOL_PRINT_NAME (sym),
+			   current_language->la_language, DMGL_ANSI);
+  printf_filtered ("\".\n");
+}
+}
+{
+struct bound_minimal_symbol bmfun=lookup_minimal_symbol_by_pc(addr);
+if (bmfun.minsym) {
+  printf_filtered ("lookup_minimal_symbol_by_pc \"");
+  fprintf_symbol_filtered (gdb_stdout, MSYMBOL_PRINT_NAME (bmfun.minsym),
+			   current_language->la_language, DMGL_ANSI);
+  printf_filtered ("\".\n");
+}
+}
+
+
   ALL_OBJSECTIONS (objfile, osect)
   {
     /* Only process each object file once, even if there's a separate

RE: [PATCH 2/2] btrace: avoid symbol lookup

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Jan Kratochvil
> Sent: Monday, March 10, 2014 10:43 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 2/2] btrace: avoid symbol lookup
> 
> On Fri, 07 Mar 2014 09:57:45 +0100, Markus Metzger wrote:
> > --- a/gdb/btrace.c
> > +++ b/gdb/btrace.c
> > @@ -451,13 +451,17 @@ ftrace_update_function (struct gdbarch
> *gdbarch,
> >    struct symbol *fun;
> >    struct btrace_insn *last;
> >
> > -  /* Try to determine the function we're in.  We use both types of symbols
> > -     to avoid surprises when we sometimes get a full symbol and sometimes
> > -     only a minimal symbol.  */
> > -  fun = find_pc_function (pc);
> > +  /* Try to determine the function we're in.  */
> >    bmfun = lookup_minimal_symbol_by_pc (pc);
> >    mfun = bmfun.minsym;
> >
> > +  /* We only lookup the symbol in the debug information if we have not
> found
> > +     a minimal symbol.  This avoids the expensive lookup for globally visible
> > +     symbols.  */
> > +  fun = NULL;
> > +  if (mfun == NULL)
> > +    fun = find_pc_function (pc);
> > +
> >    if (fun == NULL && mfun == NULL)
> >      DEBUG_FTRACE ("no symbol at %s", core_addr_to_string_nz (pc));
> >
> [...]
> 
> Behavior after the change is not the same.  DWARF functions symbols can
> span
> over discontiguous ranges with DW_AT_ranges but their corresponding ELF
> function symbols cannot, therefore those are just some approximation.
> 
> Testcase gdb.dwarf2/dw2-objfile-overlap.exp tests such a case, from:
> 	https://sourceware.org/ml/gdb-patches/2011-11/msg00166.html
> 
> For address of symbol "inner":
>  * find_pc_function finds DWARF symbol "inner"
>  * lookup_minimal_symbol_by_pc finds ELF symbol "outer_inner"
> 
> In few Fedora 20 x86_64 -O2 -g built libraries I have not found any
> DW_TAG_subprogram using DW_AT_ranges, only
> DW_TAG_inlined_subroutine use
> DW_AT_ranges.  But that is more a limitation of gcc, other compilers may
> produce DW_TAG_subprogram using DW_AT_ranges with overlapping
> function parts.

I have not seen DW_AT_ranges used for split functions.  OpenMP is a good
candidate for this so I checked what gcc 4.8.2 and a recent icc are doing today.

Both generate separate functions for parallel regions and describe them as
artificial functions in DWARF.  Whereas GCC generates a separate local function
in ELF, ICC puts the parallel region function after the function it was extracted
from and describes both as a single function in ELF.

For GCC we get a function with a funny name in both cases.
For ICC we get a function with a funny name in the DWARF case and the name
of the function from which the parallel region has been extracted in the ELF case.

So the result is different in some cases.  I'm not sure whether it matters, though.

Inline functions are a different topic.  I would like to support them one day.
As you point out below, the existing version does not support inline functions
either, so some work is necessary.
We might need some more work in symbol handling to allow a fast lookup
covering only inline functions.  We could combine this with the minimal symbol
lookup to get both inline functions support and reasonable performance.


> Inlined functions are found by neither find_pc_function nor
> lookup_minimal_symbol_by_pc (block_containing_function (block_for_pc
> ()) finds
> inlined function).  Not sure if it is not a bug of find_pc_function but that
> is off-topic here.
> 
> I do not think it will be hit in real world cases.  GDB would need some better
> abstraction of the symbol kinds to be more systematic in what it outputs.
> 
> It would be good to know how much it helps your usecase as it is not a fully
> clean/transparent change.

As benchmark, I traced "gdb a.out -ex quit" with a breakpoint on quit_force
for debug versions of both gdb and a.out.  The tracing GDB was compiled
without any additional options; the traced GDB was compiled with "-g -O0".
I used a relatively big buffer that contained ~1.5 million instructions.
I used "maintenance time 1" to measure the execution time of "info record".

The first patch improves performance by ~2x.
This patch improves performance by ~3x on top of the first patch.

I first tried to cache the returned symbol and check whether the next PC
belongs to the same function.  My cache hit in ~50% of the cases but showed
no benefit.  For the majority of the other ~50% no symbol was found.  I can't
cache those.

What's missing is a "fast fail", i.e. quickly determine that we won't find any
symbol for this PC.  I won't be able to do this in a reasonable amount of time,
though, so I thought this patch is a compromise between functionality and
performance.


Regards,
Markus.

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [PATCH 2/2] btrace: avoid symbol lookup

[Jan Kratochvil]

On Tue, 11 Mar 2014 11:08:43 +0100, Metzger, Markus T wrote:
> What's missing is a "fast fail", i.e. quickly determine that we won't find any
> symbol for this PC.  I won't be able to do this in a reasonable amount of time,
> though, so I thought this patch is a compromise between functionality and
> performance.

I do not think providing incorrect behavior for performance reasons is a valid
tradeoff.  The right way would be to fix the DWARF lookups to be fast enough.

But I no longer approve GDB patches so this is just my personal opinion.


Jan

RE: [PATCH 2/2] btrace: avoid symbol lookup

[Metzger, Markus T]

> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Friday, March 21, 2014 6:22 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org; Pedro Alves (palves@redhat.com)
> Subject: Re: [PATCH 2/2] btrace: avoid symbol lookup
> 
> On Tue, 11 Mar 2014 11:08:43 +0100, Metzger, Markus T wrote:
> > What's missing is a "fast fail", i.e. quickly determine that we won't find any
> > symbol for this PC.  I won't be able to do this in a reasonable amount of
> time,
> > though, so I thought this patch is a compromise between functionality and
> > performance.
> 
> I do not think providing incorrect behavior for performance reasons is a valid
> tradeoff.  The right way would be to fix the DWARF lookups to be fast
> enough.

I realized after I sent this that the word 'functionality' was not chosen well.

The only actual change in functionality I was able to observe was missing
parens for the main function, i.e. it had been printed "main()" and is now
printed "main".  That's because its language is 'auto' and not 'c' or 'cpp'.

I believe that this could and should be fixed by fixing up the language of
the main mini-symbol to align with the language of other symbols in the
same object file.

I think the compromise is rather between a nice, general solution that
benefits everybody and a local solution that only benefits btrace and that
might make supporting inline functions more difficult in the future.


Regards,
Markus.

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [PATCH 2/2] btrace: avoid symbol lookup

[Jan Kratochvil]

On Mon, 24 Mar 2014 08:55:33 +0100, Metzger, Markus T wrote:
> > I do not think providing incorrect behavior for performance reasons is a valid
> > tradeoff.  The right way would be to fix the DWARF lookups to be fast
> > enough.
[...]
> The only actual change in functionality I was able to observe was missing
> parens for the main function,

I agree in 99.9% of usecases it will work the same.  This still does not prove
it is correct.

I believe I can create a reproducer with two overlapping functions:
  0..1: a()
  2..3: b()
  4..6: a()
  7..8: b()
properly describe by DW_AT_ranges which will work with former GDB but which
will no longer work with patched GDB.

This may definitely happen for some user .S code with hand-coded DWARF.
I do not say it necessarilly happens with any real world compiler output.

This may happen for gdb.dwarf2/dw2-objfile-overlap.exp which comes from a real
world case of Linux kernel modules mapping.

But maybe I miss something and I would fail to create the reproducer, if you
do not agree I can create a .S with hand coded DWARF I can try to create one.

Corner cases are the ones most difficult to debug and it is a pity when
debugger provides incorrect output in such corner cases.


As I said maybe this compromise is acceptable as it may not be hit in real
world usage cases but I do not want to make this decision.


> I think the compromise is rather between a nice, general solution that
> benefits everybody and a local solution that only benefits btrace

This is the second reason why I did not agree with the patch.  GDB needs to be
faster and if this PC->functionname mapping can be accelerated such way then
it should be done globally.


Regards,
Jan

RE: [PATCH 2/2] btrace: avoid symbol lookup

[Metzger, Markus T]

> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Monday, March 24, 2014 9:37 AM


> On Mon, 24 Mar 2014 08:55:33 +0100, Metzger, Markus T wrote:
> > > I do not think providing incorrect behavior for performance reasons is a
> valid
> > > tradeoff.  The right way would be to fix the DWARF lookups to be fast
> > > enough.
> [...]
> > The only actual change in functionality I was able to observe was missing
> > parens for the main function,
> 
> I agree in 99.9% of usecases it will work the same.  This still does not prove
> it is correct.
> 
> I believe I can create a reproducer with two overlapping functions:
>   0..1: a()
>   2..3: b()
>   4..6: a()
>   7..8: b()
> properly describe by DW_AT_ranges which will work with former GDB but
> which
> will no longer work with patched GDB.
> 
> This may definitely happen for some user .S code with hand-coded DWARF.
> I do not say it necessarilly happens with any real world compiler output.
> 
> This may happen for gdb.dwarf2/dw2-objfile-overlap.exp which comes from
> a real
> world case of Linux kernel modules mapping.
> 
> But maybe I miss something and I would fail to create the reproducer, if you
> do not agree I can create a .S with hand coded DWARF I can try to create one.

No need.  I certainly agree that one can write such an assembly file and that
one can describe this in DWARF but not in ELF.

To describe this in ELF one would split a and b and thus end up with 4 functions -
that's what compilers seem to be doing today.  And they also seem to emit
DWARF that describes it that way.


> Corner cases are the ones most difficult to debug and it is a pity when
> debugger provides incorrect output in such corner cases.

I agree.


> As I said maybe this compromise is acceptable as it may not be hit in real
> world usage cases but I do not want to make this decision.

If you just look hard enough, I guess you will find everything somewhere.

Those who use compilers, on the other hand, may not even be able to
tell the difference - except that it's faster which allows them to use
bigger buffers and thus record longer traces.


> > I think the compromise is rather between a nice, general solution that
> > benefits everybody and a local solution that only benefits btrace
> 
> This is the second reason why I did not agree with the patch.  GDB needs to
> be
> faster and if this PC->functionname mapping can be accelerated such way
> then
> it should be done globally.

The specific problem of btrace is that it needs to do a huge number of symbol
lookups within a single GDB command.  I do not know any other GDB command
that gets anywhere near to that.

Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052