[RFA] DWARF frame unwinder executes one too many rows

[RFA] DWARF frame unwinder executes one too many rows

[Joel Brobecker]

The problem is trying to unwind from a function where %ebp is NOT
used as the frame pointer, and the size of the frame changes over
the lifetime of that function.

For instance, trying to unwind past the GNAT runtime function
called system.tasking.rendezvous.timed_selective_wait on x86-linux,
one can get:

    (gdb) bt
    [...]
    #3  0x0805364b in system.tasking.rendezvous.timed_selective_wait ()
    #4  0xb7fe5068 in ?? ()
    Backtrace stopped: previous frame inner to this frame (corrupt stack?)

Looking at the CFI, we find the following initial instructions...

>   DW_CFA_def_cfa: %esp+4 (r4 ofs 4)
>   DW_CFA_offset: %eip at cfa-4  (r8 = %eip)

... and the associated FDE:

> 00001be4 00000054 00001be8 FDE cie=00000000 pc=08053310..08053951
[...]
>   DW_CFA_advance_loc: 8 to 080534ad
>   DW_CFA_def_cfa_offset: 112
>   DW_CFA_advance_loc2: 414 to 0805364b
>   DW_CFA_def_cfa_offset: 108
[...]

The problem is that the DWARF frame unwinder executed the FDE until
the row for PC == 0x0805364b. But in reality, our program hasn't
executed the instruction at that address yet (it is the return address).
So GDB executed a little too much of the FDE, giving us the wrong
offset for the frame base, and thus the wrong address where %eip
got saved.

This patch fixes the problem by using a more correct PC as the bound
for executing the FDE.

gdb/ChangeLog:

        * dwarf2-frame.c (dwarf2_frame_cache): Use
        get_frame_address_in_block instead of get_frame_pc as
        the bound for executing the frame's FDE.

gdb/testsuite/ChangeLog:

        * gdb.ada/rdv_wait: New testcase.

Tested on x86-linux, no regression.
OK to apply?

Thanks,
-- 
Joel

---
 gdb/dwarf2-frame.c                     |    5 ++-
 gdb/testsuite/gdb.ada/rdv_wait.exp     |   32 +++++++++++++++++++
 gdb/testsuite/gdb.ada/rdv_wait/foo.adb |   53 ++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/rdv_wait/pck.adb |   23 ++++++++++++++
 gdb/testsuite/gdb.ada/rdv_wait/pck.ads |   22 +++++++++++++
 5 files changed, 133 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/rdv_wait.exp
 create mode 100644 gdb/testsuite/gdb.ada/rdv_wait/foo.adb
 create mode 100644 gdb/testsuite/gdb.ada/rdv_wait/pck.adb
 create mode 100644 gdb/testsuite/gdb.ada/rdv_wait/pck.ads

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index 01786ef..5d614a5 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1054,7 +1054,8 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
 
   /* First decode all the insns in the CIE.  */
   execute_cfa_program (fde, fde->cie->initial_instructions,
-		       fde->cie->end, gdbarch, get_frame_pc (this_frame), fs);
+		       fde->cie->end, gdbarch,
+		       get_frame_address_in_block (this_frame), fs);
 
   /* Save the initialized register set.  */
   fs->initial = fs->regs;
@@ -1079,7 +1080,7 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
 
   /* Then decode the insns in the FDE up to our target PC.  */
   execute_cfa_program (fde, instr, fde->end, gdbarch,
-		       get_frame_pc (this_frame), fs);
+		       get_frame_address_in_block (this_frame), fs);
 
   TRY_CATCH (ex, RETURN_MASK_ERROR)
     {
diff --git a/gdb/testsuite/gdb.ada/rdv_wait.exp b/gdb/testsuite/gdb.ada/rdv_wait.exp
new file mode 100644
index 0000000..67e0f1e
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/rdv_wait.exp
@@ -0,0 +1,32 @@
+# Copyright 2012 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/>.
+
+load_lib "ada.exp"
+
+standard_ada_testfile foo
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+runto "break_me"
+
+# Switch to task 2, and verify that GDB is able to unwind all the way
+# to foo.T.
+gdb_test "task 2" \
+         [join {"\\\[Switching to task 2\\\].*" \
+                ".*foo\\.t \\(.*\\).*foo\\.adb:.*"} ""]
diff --git a/gdb/testsuite/gdb.ada/rdv_wait/foo.adb b/gdb/testsuite/gdb.ada/rdv_wait/foo.adb
new file mode 100644
index 0000000..cc3c22c
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/rdv_wait/foo.adb
@@ -0,0 +1,53 @@
+--  Copyright 2012 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/>.
+
+with Pck; use Pck;
+
+procedure Foo is
+
+   task type T is
+      entry Finish;
+   end T;
+   type TA is access T;
+
+   task body T is
+   begin
+      --  Wait for up to 100 seconds for the Finish Rendez-Vous to occur.
+      select
+         accept Finish do
+            null;
+         end Finish;
+      or
+         delay 100.0;
+      end select;
+   end T;
+
+   MIT : TA;
+
+begin
+
+   --  Create a task, and give it some time to activate and then start
+   --  its execution.
+   MIT := new T;
+   delay 0.01;
+
+   --  Now, call our anchor. The task we just created should now be
+   --  blocked on a timed entry wait.
+   Break_Me;
+
+   --  Tell the task to finish before the 100 seconds are up.  The test
+   --  is now finished, no need to wait :).
+   MIT.Finish;
+end Foo;
diff --git a/gdb/testsuite/gdb.ada/rdv_wait/pck.adb b/gdb/testsuite/gdb.ada/rdv_wait/pck.adb
new file mode 100644
index 0000000..e5eebdc
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/rdv_wait/pck.adb
@@ -0,0 +1,23 @@
+--  Copyright 2012 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/>.
+
+package body Pck is
+
+   procedure Break_Me is
+   begin
+      null;
+   end Break_Me;
+
+end Pck;
diff --git a/gdb/testsuite/gdb.ada/rdv_wait/pck.ads b/gdb/testsuite/gdb.ada/rdv_wait/pck.ads
new file mode 100644
index 0000000..afe98f7
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/rdv_wait/pck.ads
@@ -0,0 +1,22 @@
+--  Copyright 2012 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/>.
+
+package Pck is
+
+   procedure Break_Me;
+   --  A procedure doing nothing, but which is a convenient anchor
+   --  for inserting breakpoints.
+
+end Pck;
-- 
1.7.1

Re: [RFA] DWARF frame unwinder executes one too many rows

[asmwarrior]

On 2012-7-27 6:51, Joel Brobecker wrote:
>      (gdb) bt
>      [...]
>      #3  0x0805364b in system.tasking.rendezvous.timed_selective_wait ()
>      #4  0xb7fe5068 in ?? ()
>      Backtrace stopped: previous frame inner to this frame (corrupt stack?)
Here is my testing result:
I see such kind of message when debugging under Windows before.
After applying your patch, those messages were gone. Sounds good.

Re: [RFA] DWARF frame unwinder executes one too many rows

[asmwarrior]

On 2012-7-27 19:07, asmwarrior wrote:
> After applying your patch, those messages were gone.
Oh, sorry. I still see such messages after applying the patch, like:

[debug]> bt 30
[debug]#0  NativeParserBase::BreakUpComponents (this=0x22fc44, actual=..., components=...) at nativeparser_base.cpp:716
[debug]#1  0x0041a38f in NativeParserTest::TestExpression (this=0x22fc44, expression=..., tree=0x16309b0, searchScope=..., result=...) at nativeparsertest.cpp:83
[debug]#2  0x0040ed74 in Frame::Start (this=0x16280d8) at frame.cpp:336
[debug]#3  0x004360ea in ParserTestApp::OnInit (this=0x161a028) at app.cpp:117
[debug]#4  0x0045da0c in wxAppConsole::CallOnInit (this=0x161a028) at E:/code/cb/wx/wxWidgets-2.8.12/include/wx/app.h:76
[debug]#5  0x0088a23d in wxEntryReal(int&, wchar_t**) () from E:\code\cb\wx\wxWidgets-2.8.12\lib\gcc_dll\wxmsw28u_gcc_custom.dll
[debug]#6  0x01616d28 in ?? ()
[debug]#7  0x00000001 in ?? ()
[debug]#8  0x01619f48 in ?? ()
[debug]Backtrace stopped: previous frame inner to this frame (corrupt stack?)

Re: [RFA] DWARF frame unwinder executes one too many rows

[Joel Brobecker]

> Oh, sorry. I still see such messages after applying the patch, like:

The same symptom can be caused by a number of issues. You'll have
to debug your problem in order to determine what the cause is.

-- 
Joel

Re: [RFA] DWARF frame unwinder executes one too many rows

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> gdb/ChangeLog:
Joel>         * dwarf2-frame.c (dwarf2_frame_cache): Use
Joel>         get_frame_address_in_block instead of get_frame_pc as
Joel>         the bound for executing the frame's FDE.
Joel> gdb/testsuite/ChangeLog:
Joel>         * gdb.ada/rdv_wait: New testcase.

This looks good to me.

I looked into the history a tiny bit.  It appears I introduced this in
revision 1.120, but looking at the diff it appears I just hoisted the
call to get_frame_pc out of execute_cfa_program.  Whew.

Before that it gets into stuff before my involvement:

-  CORE_ADDR pc = frame_pc_unwind (next_frame);
+  CORE_ADDR pc = get_frame_pc (this_frame);

Tom

Re: [RFA] DWARF frame unwinder executes one too many rows

[Jan Kratochvil]

Hi Joel,

On Fri, 27 Jul 2012 00:51:22 +0200, Joel Brobecker wrote:
> The problem is trying to unwind from a function where %ebp is NOT
> used as the frame pointer, and the size of the frame changes over
> the lifetime of that function.

according to Tom's reply it is a regression by:
	commit 8d35b3995d73e87826263f02da0fd05190d34638
	Author: Tom Tromey <tromey@redhat.com>
	Date:   Thu Feb 17 16:20:37 2011 +0000

but the Ada testcase does not work for me (it always PASSes for i386 and it
always FAILs for x86_64).  Do you have some precompiled binaries so that I can
look at them.

I will then code a gdb.dwarf2/ testcase as the gdb.ada/ testcases are IMO
dependent on too many factors and in Fedora environment they are not reliable.


Thanks,
Jan

Re: [RFA] DWARF frame unwinder executes one too many rows

[Tom Tromey]

Jan> according to Tom's reply it is a regression by:
Jan> 	commit 8d35b3995d73e87826263f02da0fd05190d34638
Jan> 	Author: Tom Tromey <tromey@redhat.com>
Jan> 	Date:   Thu Feb 17 16:20:37 2011 +0000

No, that isn't what I said, or meant.

Tom

Re: [RFA] DWARF frame unwinder executes one too many rows

[Jan Kratochvil]

On Mon, 06 Aug 2012 16:05:08 +0200, Tom Tromey wrote:
> Jan> according to Tom's reply it is a regression by:
> Jan> 	commit 8d35b3995d73e87826263f02da0fd05190d34638
> Jan> 	Author: Tom Tromey <tromey@redhat.com>
> Jan> 	Date:   Thu Feb 17 16:20:37 2011 +0000
> 
> No, that isn't what I said, or meant.

I spent some time trying to find a regression but as I had no FAIL/PASS
indicator (the Ada case did not work for me) I did not find any.

As the discussion was about dwarf2-frame.c and you said

On Mon, 30 Jul 2012 19:43:19 +0200, Tom Tromey wrote:
> I looked into the history a tiny bit.  It appears I introduced this in
> revision 1.120, 

this GIT commit corresponds to dwarf2-frame.c CVS revision 1.120.

But this commit does not seem to contain the regression to me so you maybe
meant revision 1.120 of a different file.


Regards,
Jan

Re: [RFA] DWARF frame unwinder executes one too many rows

[Tom Tromey]

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

Jan> As the discussion was about dwarf2-frame.c and you said

>> I looked into the history a tiny bit.  It appears I introduced this in
>> revision 1.120, 

You left out an important part of my message:

"but looking at the diff it appears I just hoisted the call to
get_frame_pc out of execute_cfa_program."

Tom

Re: [RFA] DWARF frame unwinder executes one too many rows

[Jan Kratochvil]

On Mon, 06 Aug 2012 17:10:01 +0200, Tom Tromey wrote:
> You left out an important part of my message:
> 
> "but looking at the diff it appears I just hoisted the call to
> get_frame_pc out of execute_cfa_program."

That's true but I do not see any change with it.  get_frame_pc was just called
in the callers instead, not as the first statement of the callee.

8d35b3995d73e87826263f02da0fd05190d34638

 execute_cfa_program (struct dwarf2_fde *fde, const gdb_byte *insn_ptr,
-                    const gdb_byte *insn_end, struct frame_info *this_frame,
-                    struct dwarf2_frame_state *fs)
+                    const gdb_byte *insn_end, struct gdbarch *gdbarch,
+                    CORE_ADDR pc, struct dwarf2_frame_state *fs)
 {
   int eh_frame_p = fde->eh_frame_p;
-  CORE_ADDR pc = get_frame_pc (this_frame);
   int bytes_read;
and
   execute_cfa_program (fde, fde->cie->initial_instructions,
-                      fde->cie->end, this_frame, fs);
+                      fde->cie->end, gdbarch, get_frame_pc (this_frame), fs);
and
-  execute_cfa_program (fde, fde->instructions, fde->end, this_frame, fs);
+  execute_cfa_program (fde, fde->instructions, fde->end, gdbarch,
+                      get_frame_pc (this_frame), fs);


Moreover without a reproducer I do not feel comfortable to say anything.


Jan

Re: [RFA] DWARF frame unwinder executes one too many rows

[Joel Brobecker]

> Joel> gdb/ChangeLog:
> Joel>         * dwarf2-frame.c (dwarf2_frame_cache): Use
> Joel>         get_frame_address_in_block instead of get_frame_pc as
> Joel>         the bound for executing the frame's FDE.
> Joel> gdb/testsuite/ChangeLog:
> Joel>         * gdb.ada/rdv_wait: New testcase.
> 
> This looks good to me.

Thanks, Tom. The patch is now in...

-- 
Joel

[patch] testsuite: Test the unwinder off-by-one [Re: [RFA] DWARF frame unwinder executes one too many rows]

[Jan Kratochvil]

On Sat, 04 Aug 2012 14:30:05 +0200, Jan Kratochvil wrote:
> but the Ada testcase does not work for me (it always PASSes for i386 and it
> always FAILs for x86_64).  Do you have some precompiled binaries so that I can
> look at them.

Joel has sent them off-list, I found the Ada testcase works for me on i386
with the attached update.

I will check it in.


Thanks,
Jan


gdb/testsuite/
2012-08-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.ada/rdv_wait.exp (set debug-file-directory): New command.
	* gdb.arch/i386-cfi-notcurrent.S: New file.
	* gdb.arch/i386-cfi-notcurrent.exp: New file.

diff --git a/gdb/testsuite/gdb.ada/rdv_wait.exp b/gdb/testsuite/gdb.ada/rdv_wait.exp
index 67e0f1e..f2b00b7 100644
--- a/gdb/testsuite/gdb.ada/rdv_wait.exp
+++ b/gdb/testsuite/gdb.ada/rdv_wait.exp
@@ -23,6 +23,9 @@ if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" }
 
 clean_restart ${testfile}
 
+# This test won't work properly if system debuginfo is installed.
+gdb_test_no_output "set debug-file-directory"
+
 runto "break_me"
 
 # Switch to task 2, and verify that GDB is able to unwind all the way
diff --git a/gdb/testsuite/gdb.arch/i386-cfi-notcurrent.S b/gdb/testsuite/gdb.arch/i386-cfi-notcurrent.S
new file mode 100644
index 0000000..eae94b0
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-cfi-notcurrent.S
@@ -0,0 +1,76 @@
+/* Copyright 2012 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+/* Testcase compiled by gcc -m32 -S -fasynchronous-unwind-tables -O2:
+
+static int __attribute__ ((regparm (0), stdcall, noinline, noclone))
+f (int x)
+{
+  return x;
+}
+static int __attribute__ ((noinline, noclone))
+g (void)
+{
+  return f (1) + 1;
+}
+int
+main (void)
+{
+  return g () + 1;
+}
+
+ */
+
+	.text
+	.type	f, @function
+f:
+.LFB0:
+	.cfi_startproc
+	movl	4(%esp), %eax
+	ret	$4
+	.cfi_endproc
+.LFE0:
+	.size	f, .-f
+	.type	g, @function
+g:
+.LFB1:
+	.cfi_startproc
+	subl	$4, %esp
+	.cfi_def_cfa_offset 8
+	movl	$1, (%esp)
+	call	f
+	.cfi_def_cfa_offset 4
+	subl	$4, %esp
+	.cfi_def_cfa_offset 8
+	addl	$1, %eax
+	addl	$4, %esp
+	.cfi_def_cfa_offset 4
+	ret
+	.cfi_endproc
+.LFE1:
+	.size	g, .-g
+	.globl	main
+	.type	main, @function
+main:
+.LFB2:
+	.cfi_startproc
+	call	g
+	addl	$1, %eax
+	ret
+	.cfi_endproc
+.LFE2:
+	.size	main, .-main
diff --git a/gdb/testsuite/gdb.arch/i386-cfi-notcurrent.exp b/gdb/testsuite/gdb.arch/i386-cfi-notcurrent.exp
new file mode 100644
index 0000000..cf41c57
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-cfi-notcurrent.exp
@@ -0,0 +1,34 @@
+# Copyright 2012 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/>.
+
+# [RFA] DWARF frame unwinder executes one too many rows
+# http://sourceware.org/ml/gdb-patches/2012-07/msg00650.html
+
+if {(![istarget "x86_64-*-*"] && ![istarget "i?86-*-*"]) || ![is_ilp32_target] } {
+    return 0
+}
+
+set testfile "i386-cfi-notcurrent"
+set srcfile ${testfile}.S
+
+if [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} {nodebug}] {
+    return -1
+}
+
+if ![runto f] {
+    return -1
+}
+
+gdb_test "backtrace" "#0 \[^\r\n\]* f \[^\r\n\]*\r\n#1 \[^\r\n\]* g \[^\r\n\]*\r\n#2 \[^\r\n\]* main \[^\r\n\]*"

[commit] [patch] testsuite: Test the unwinder off-by-one [Re: [RFA] DWARF frame unwinder executes one too many rows]

[Jan Kratochvil]

On Fri, 17 Aug 2012 20:14:25 +0200, Jan Kratochvil wrote:
> gdb/testsuite/
> 2012-08-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.ada/rdv_wait.exp (set debug-file-directory): New command.
> 	* gdb.arch/i386-cfi-notcurrent.S: New file.
> 	* gdb.arch/i386-cfi-notcurrent.exp: New file.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2012-08/msg00197.html


Jan