[delayed-symfile] [commit] Fix a regression on delayed retrieving of the unwinding debug info.

[delayed-symfile] [commit] Fix a regression on delayed retrieving of the unwinding debug info.

[Jan Kratochvil]

commit 6a37c2b9962258ecf9299cc34a650e64a06acaa5

There was a regression on gdb.base/savedregs.exp.

quick_addrmap/require_partial_symbols should be used even for the unwind debug
info checking as its load has been also delayed by this branch.

Disputable is whether the delayed reading of partial symtabs and unwind info
should not be split in half and read separately on their specific demand.
Assuming their current load together has been decided for the GDB code
simplicity and it has no real performance impact.

	* dwarf2-frame.c: Include addrmap.h.
	(dwarf2_frame_find_fde): Check quick_addrmap, call
	require_partial_symbols.
---
 gdb/dwarf2-frame.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index ce11d89..eaa6a13 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -38,6 +38,7 @@
 
 #include "complaints.h"
 #include "dwarf2-frame.h"
+#include "addrmap.h"
 
 struct comp_unit;
 
@@ -1499,6 +1500,14 @@ dwarf2_frame_find_fde (CORE_ADDR *pc)
       struct dwarf2_fde *fde;
       CORE_ADDR offset;
 
+      if (objfile->quick_addrmap)
+	{
+	  if (!addrmap_find (objfile->quick_addrmap, *pc))
+	    continue;
+	}
+      /* FIXME: Read-in only .debug_frame/.eh_frame without .debug_info?  */
+      require_partial_symbols (objfile);
+
       fde = objfile_data (objfile, dwarf2_frame_objfile_data);
       if (fde == NULL)
 	continue;
-- 
1.6.0.6

Re: [delayed-symfile] [commit] Fix a regression on delayed retrieving of the unwinding debug info.

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> Disputable is whether the delayed reading of partial symtabs and
Jan> unwind info should not be split in half and read separately on
Jan> their specific demand.  Assuming their current load together has
Jan> been decided for the GDB code simplicity and it has no real
Jan> performance impact.

It might not be as deep as that.  Maybe nobody ever looked at this
idea before :-)

I occasionally think that in the future we will end up redoing all the
symbol table and dwarf parsing stuff to be much more incremental.  But
I think this is probably still a lower priority than fixing all the
C++ problems.  And, maybe we'll never really feel the need, I don't
know.

[ type lookup patch ]
Jan> Assuming (did not measure it so far) this patch will nullify any
Jan> performance effect of this archer-tromey-delayed-symfile branch.

I think we'll still see decent benefits in some scenarios.

My canonical test case concerns two things: first, "attach"
performance, and second, "bt" performance.  I do this by running OO.o
Writer (with all debuginfo installed) and then attaching to it; then I
run "thread apply all bt full".

The "attach" part shows us the biggest benefit; I think your patch
won't affect that.  The second thing helps quantify the slowdown
during a typical operation; it also lets us see whether incremental
reading is really working; that is, if you do both of these things
with -batch, you can measure performance gained by not reading some
objfiles.

I'm still interested to hear your results.


I'm very happy that you fixed these bugs, but I also wanted to address
a couple process things here.

I know you have limited time to work on your own major patches, so if
you find bugs like this, feel free to just push them back to the
branch author for fixing.  It is ok to do this publicly.  If the
branch author can't get to the bug in a timely way, we can work
something out.

It is also ok by me if you also want to just fix the bugs like you did
this time.  I just want to make sure that you don't feel trapped into
doing this.


The other process thing is that I obviously made a mess of this
branch.  I must have not run the test suite, or something careless
like that.  I think we need to go back to our original plan of patch
review for everybody.  Let's discuss exactly how to do this, and when
in the process.

Tom

Re: [delayed-symfile] [commit] Fix a regression on delayed retrieving of the unwinding debug info.

[Jan Kratochvil]

On Wed, 25 Feb 2009 02:47:20 +0100, Tom Tromey wrote:
> I know you have limited time to work on your own major patches, so if
> you find bugs like this, feel free to just push them back to the
> branch author for fixing.  It is ok to do this publicly.  If the
> branch author can't get to the bug in a timely way, we can work
> something out.
> 
> It is also ok by me if you also want to just fix the bugs like you did
> this time.  I just want to make sure that you don't feel trapped into
> doing this.

I was close to push the branch back to you but fortunately it got
regression-free before that in this case.  The fixing was fine for me.


Regards,
Jan

Re: [delayed-symfile] [commit] Fix a regression on delayed retrieving of the unwinding debug info.

[Jan Kratochvil]

On Wed, 25 Feb 2009 02:47:20 +0100, Tom Tromey wrote:
> [ type lookup patch ]
> Jan> Assuming (did not measure it so far) this patch will nullify any
> Jan> performance effect of this archer-tromey-delayed-symfile branch.
> 
> I think we'll still see decent benefits in some scenarios.
> 
> My canonical test case concerns two things: first, "attach"
> performance, and second, "bt" performance.  I do this by running OO.o
> Writer (with all debuginfo installed) and then attaching to it; then I
> run "thread apply all bt full".
> 
> The "attach" part shows us the biggest benefit; I think your patch
> won't affect that.  The second thing helps quantify the slowdown
> during a typical operation; it also lets us see whether incremental
> reading is really working; that is, if you do both of these things
> with -batch, you can measure performance gained by not reading some
> objfiles.
> 
> I'm still interested to hear your results.

Here is a fast (no multiple runs etc.) benchmark:

delayed-prelink-at         real 0m 7.289s
delayed-prelink-bt         real 0m26.426s
delayed-old-prelink-at     real 0m 7.795s
delayed-old-prelink-bt     real 0m26.254s
delayed-oldest-prelink-at  real 0m 2.492s
delayed-oldest-prelink-bt  real 0m27.346s
master-prelink-at          real 0m26.510s
master-prelink-bt          real 0m31.900s
fedora-prelink-at          real 0m30.343s
fedora-prelink-bt          real 0m38.688s

delayed = archer-tromey-delayed-symfile branch
master = origin/master branch, FSF GDB
         that is 2bc2becfab3f5c1771f5a344fcb678f4b559e1d7
fedora = gdb-6.8-29.fc10
-at = `attach' only
-bt = `attach' followed by `thread apply all bt full'
delayed:
(-nothing) = HEAD
-old = without: Fix a regression on forgotten delayed read of a type info.
       that is without 0b71a223391a3f8304943b24956b1a6fcaba2a16
-oldest = last Tom Tromey's commit, that is FSF GDB of Sep 16 2008
          that is 289465b302e07e4cfa44857320ddcffb670197b6

Test box: 1GB RAM, 2GB SWAP, dual Xeon(R) 5110 @ 1.60GHz

All the results above are with the fix 
  http://cvs.fedora.redhat.com/viewvc/rpms/gdb/devel/gdb-6.8-bz466901-backtrace-full-prelinked.patch?view=co
as otherwise each `bt full' backtrace gets very soon terminated with:
	layer2 = Could not find the frame base for "desktop::Desktop::Main()".
without this -prelink patch IMO the results are not useful representatives:
delayed-at                 real 0m 7.106s
delayed-bt                 real 0m17.515s
delayed-old-at             real 0m 6.427s
delayed-old-bt             real 0m 4.222s
delayed-oldest-at          real 0m 8.087s
delayed-oldest-bt          real 0m 4.282s
master-at                  real 0m28.091s
master-bt                  real 0m30.814s
(there is no fedora without -prelink)

The time difference of `master-prelink' vs. `fedora-prelink' (slower) may be
due to some other minor missed debug info, the fedora backtrace has more rich
displayed type casts.

(for RH people the box is 10.34.35.50, for non-RH people may ask for more info)


Regards,
Jan

[delayed-symfile] [commit] Fix a regression on CFI without DIE [Re: [delayed-symfile] [commit] Fix a regression on delayed retrieving of the unwinding debug info.]

[Jan Kratochvil]

On Wed, 25 Feb 2009 00:14:29 +0100, Jan Kratochvil wrote:
> commit 6a37c2b9962258ecf9299cc34a650e64a06acaa5
> 
> There was a regression on gdb.base/savedregs.exp.
> 
> quick_addrmap/require_partial_symbols should be used even for the unwind debug
> info checking as its load has been also delayed by this branch.
[...]
> --- a/gdb/dwarf2-frame.c
> +++ b/gdb/dwarf2-frame.c
[...]
> @@ -1499,6 +1500,14 @@ dwarf2_frame_find_fde (CORE_ADDR *pc)
>        struct dwarf2_fde *fde;
>        CORE_ADDR offset;
>  
> +      if (objfile->quick_addrmap)
> +	{
> +	  if (!addrmap_find (objfile->quick_addrmap, *pc))
> +	    continue;
> +	}
> +      /* FIXME: Read-in only .debug_frame/.eh_frame without .debug_info?  */
> +      require_partial_symbols (objfile);
> +

but this has caused a different regression (as discussed in the confcall).

QUICK_ADDRMAP is built only from .debug_aranges.  But we can have existing
built .debug_aranges for CUs in OBJFILE but still some CUs do not need to have
DWARF at all while they can feature CFIs (.eh_frame or .debug_frame).
It has been described by Daniel Jacobowitz at:
	Re: [2/4] RFC: check psymtabs_addrmap before reading FDEs
	http://sourceware.org/ml/gdb-patches/2010-07/msg00012.html

Sorry for this regression by me (in that fix of a different regression).

Fixed it the "slow way" as this branch is now obsoleted by .gdb-index.

No regressions on {x86_64,x86_64-m32,i686}-fedora13-linux-gnu.

Checked-in.


Thanks,
Jan


eb8df8566acc1ed963e3e9b77c13b9c2c3db03fb

Test CFI is parsed even for range (function) not described by any DIE.

https://bugzilla.redhat.com/show_bug.cgi?id=614028

gdb/
	* dwarf2-frame.c (dwarf2_frame_find_fde): Remove the
	OBJFILE->QUICK_ADDRMAP check.  New comment why.

gdb/testsuite/
	* gdb.base/cfi-without-die.exp, gdb.base/cfi-without-die-main.c,
	gdb.base/cfi-without-die-caller.c: New files.
---
 gdb/dwarf2-frame.c                              |    8 +--
 gdb/testsuite/gdb.base/cfi-without-die-caller.c |   28 ++++++++++
 gdb/testsuite/gdb.base/cfi-without-die-main.c   |   32 +++++++++++
 gdb/testsuite/gdb.base/cfi-without-die.exp      |   67 +++++++++++++++++++++++
 4 files changed, 130 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/cfi-without-die-caller.c
 create mode 100644 gdb/testsuite/gdb.base/cfi-without-die-main.c
 create mode 100644 gdb/testsuite/gdb.base/cfi-without-die.exp

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index 5915249..1dc2754 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1583,11 +1583,9 @@ dwarf2_frame_find_fde (CORE_ADDR *pc)
       CORE_ADDR offset;
       CORE_ADDR seek_pc;
 
-      if (objfile->quick_addrmap)
-	{
-	  if (!addrmap_find (objfile->quick_addrmap, *pc))
-	    continue;
-	}
+      /* OBJFILE->QUICK_ADDRMAP contains offsets only for DIEs.  It does not
+         contain ranges of CFIs.  */
+
       /* FIXME: Read-in only .debug_frame/.eh_frame without .debug_info?  */
       require_partial_symbols (objfile);
 
diff --git a/gdb/testsuite/gdb.base/cfi-without-die-caller.c b/gdb/testsuite/gdb.base/cfi-without-die-caller.c
new file mode 100644
index 0000000..afdfd53
--- /dev/null
+++ b/gdb/testsuite/gdb.base/cfi-without-die-caller.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2005, 2007, 2008, 2009, 2010 Free Software Foundation, Inc.
+
+   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/>.  */
+
+typedef int (*callback_t) (void);
+
+int
+caller (callback_t callback)
+{
+  /* Ensure some frame content to push away the return address.  */
+  volatile const long one = 1;
+
+  /* Modify the return value to prevent any tail-call optimization.  */
+  return (*callback) () - one;
+}
diff --git a/gdb/testsuite/gdb.base/cfi-without-die-main.c b/gdb/testsuite/gdb.base/cfi-without-die-main.c
new file mode 100644
index 0000000..8451c4b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/cfi-without-die-main.c
@@ -0,0 +1,32 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2005, 2007, 2008, 2009, 2010 Free Software Foundation, Inc.
+
+   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/>.  */
+
+typedef int (*callback_t) (void);
+
+extern int caller (callback_t callback);
+
+int
+callback (void)
+{
+  return 1;
+}
+
+int
+main (void)
+{
+  return caller (callback);
+}
diff --git a/gdb/testsuite/gdb.base/cfi-without-die.exp b/gdb/testsuite/gdb.base/cfi-without-die.exp
new file mode 100644
index 0000000..db6d248
--- /dev/null
+++ b/gdb/testsuite/gdb.base/cfi-without-die.exp
@@ -0,0 +1,67 @@
+# Copyright 2010 Free Software Foundation, Inc.
+
+# 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/>.
+
+# Test CFI is parsed even for range (function) not described by any DIE.
+
+set testfile cfi-without-die
+set srcmainfile ${testfile}-main.c
+set srccallerfile ${testfile}-caller.c
+set executable ${testfile}
+set objmainfile ${objdir}/${subdir}/${testfile}-main.o
+set objcallerfile ${objdir}/${subdir}/${testfile}-caller.o
+set binfile ${objdir}/${subdir}/${executable}
+
+if { [gdb_compile "${srcdir}/${subdir}/${srccallerfile}" ${objcallerfile} \
+      object [list {additional_flags=-fomit-frame-pointer -fno-unwind-tables -fno-asynchronous-unwind-tables}]] != ""
+     || [gdb_compile "${srcdir}/${subdir}/${srcmainfile}" ${objmainfile} object {debug}] != ""
+     || [gdb_compile "${objmainfile} ${objcallerfile}" ${binfile} executable {}] != "" } {
+     untested ${testfile}.exp
+     return -1
+}
+
+clean_restart $executable
+
+if ![runto callback] then {
+   fail "verify unwinding: Can't run to callback"
+   return 0
+}
+set test "verify unwinding breaks without CFI"
+gdb_test_multiple "bt" $test {
+    -re " in main .*\r\n$gdb_prompt $" {
+	fail $test
+    }
+    -re "\r\n$gdb_prompt $" {
+	pass $test
+    }
+}
+
+if { [gdb_compile "${srcdir}/${subdir}/${srccallerfile}" ${objcallerfile} \
+      object [list {additional_flags=-fomit-frame-pointer -funwind-tables -fasynchronous-unwind-tables}]] != ""
+     || [gdb_compile "${srcdir}/${subdir}/${srcmainfile}" ${objmainfile} object {debug}] != ""
+     || [gdb_compile "${objmainfile} ${objcallerfile}" ${binfile} executable {}] != "" } {
+     untested ${testfile}.exp
+     return -1
+}
+
+clean_restart $executable
+
+if ![runto callback] then {
+   fail "test CFI without DIEs: Can't run to callback"
+   return 0
+}
+# #0  callback () at ...
+# #1  0x00000000004004e9 in caller ()
+# #2  0x00000000004004cd in main () at ...
+gdb_test "bt" "#0 +callback \[^\r\n\]+\r\n#1 \[^\r\n\]+ in caller \[^\r\n\]+\r\n#2 \[^\r\n\]+ in main \[^\r\n\]+" "verify unwindin works for CFI without DIEs"
-- 
1.7.1.1

Re: [delayed-symfile] [commit] Fix a regression on CFI without DIE [Re: [delayed-symfile] [commit] Fix a regression on delayed retrieving of the unwinding debug info.]

[Tom Tromey]

Jan> QUICK_ADDRMAP is built only from .debug_aranges.  But we can have
Jan> existing built .debug_aranges for CUs in OBJFILE but still some CUs
Jan> do not need to have DWARF at all while they can feature CFIs
Jan> (.eh_frame or .debug_frame).

Yeah, oops on me.

This is an area where we could use your idea of looking at where the
objfiles are actually mapped in the inferior.

Tom