[PATCH 0/2] fix multi-threaded unwinding on AArch64

[PATCH 0/2] fix multi-threaded unwinding on AArch64

[Tom Tromey]

This patch series fixes PR 16155, a hang when unwinding in a
multi-threaded program on AArch64.

There were two bugs in gdb to be addressed here; plus a bug in glibc
that I filed as:

    https://sourceware.org/bugzilla/show_bug.cgi?id=16169

Neither patch here is actually specific to AArch64.

Built and regtested on x86-64 Fedora 18.  I also verified that I could
see the bug using a simple multi-threaded program on AArch64; and that
these patches fix the problem there.

Tom

[PATCH 2/2] handle an unspecified return address column

[Tom Tromey]

Debugging PR 16155 further, I found that the DWARF unwinder found the
function in question, but thought it had no registers saved
(fs->regs.num_regs == 0).

It seems to me that if a frame does not specify the return address
column, or if the return address column is explicitly marked as
DWARF2_FRAME_REG_UNSPECIFIED, then we should set the
"undefined_retaddr" flag and let the DWARF unwinder gracefully stop.

This patch implements that idea.

With this patch the backtrace works properly:

    (gdb) bt
    #0  0x0000007fb7ed485c in nanosleep () from /lib64/libc.so.6
    #1  0x0000007fb7ed4508 in sleep () from /lib64/libc.so.6
    #2  0x00000000004008bc in thread_function (arg=0x4) at threadapply.c:73
    #3  0x0000007fb7fad950 in start_thread () from /lib64/libpthread.so.0
    #4  0x0000007fb7f0956c in clone () from /lib64/libc.so.6

2013-11-13  Tom Tromey  <tromey@redhat.com>

	PR backtrace/16155:
	* dwarf2-frame.c (dwarf2_frame_cache): Set undefined_retaddr if
	the return address column is unspecified.

2013-11-13  Tom Tromey  <tromey@redhat.com>

	* gdb.dwarf2/dw2-bad-cfi.c: New file.
	* gdb.dwarf2/dw2-bad-cfi.exp: New file.
	* gdb.dwarf2/dw2-bad-cfi.S: New file.
---
 gdb/ChangeLog                            |   6 +
 gdb/dwarf2-frame.c                       |   4 +
 gdb/testsuite/ChangeLog                  |   6 +
 gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.S   | 216 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.c   |  28 ++++
 gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.exp |  42 ++++++
 6 files changed, 302 insertions(+)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.S
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.exp

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index 8f55e9f..2f235b9 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1227,6 +1227,10 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"),
   if (fs->retaddr_column < fs->regs.num_regs
       && fs->regs.reg[fs->retaddr_column].how == DWARF2_FRAME_REG_UNDEFINED)
     cache->undefined_retaddr = 1;
+  else if (fs->retaddr_column >= fs->regs.num_regs
+  	  || (fs->regs.reg[fs->retaddr_column].how
+  	      == DWARF2_FRAME_REG_UNSPECIFIED))
+    cache->undefined_retaddr = 1;
 
   do_cleanups (old_chain);
 
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.S b/gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.S
new file mode 100644
index 0000000..9a97889
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.S
@@ -0,0 +1,216 @@
+# This file was created by running dw2-bad-cfi.c
+# through 'gcc -g -dA -S', and then hand-editing
+# the CFI in "callee" to suit.
+
+	.file	"dw2-bad-cfi.c"
+	.text
+.Ltext0:
+	.globl	callee
+	.type	callee, @function
+callee:
+.LFB0:
+	.file 1 "dw2-bad-cfi.c"
+	# dw2-bad-cfi.c:20
+	.loc 1 20 0
+	.cfi_startproc
+# BLOCK 2 seq:0
+# PRED: ENTRY (fallthru)
+	pushq	%rbp
+/*
+	.cfi_def_cfa_offset 16
+	.cfi_offset 6, -16
+*/
+	movq	%rsp, %rbp
+/*	.cfi_def_cfa_register 6 */
+	# dw2-bad-cfi.c:21
+	.loc 1 21 0
+	popq	%rbp
+/*	.cfi_def_cfa 7, 8 */
+	# Our little lies.
+	.cfi_undefined 6
+	.cfi_return_column 6
+# SUCC: EXIT [100.0%] 
+	ret
+	.cfi_endproc
+.LFE0:
+	.size	callee, .-callee
+	.globl	main
+	.type	main, @function
+main:
+.LFB1:
+	# dw2-bad-cfi.c:25
+	.loc 1 25 0
+	.cfi_startproc
+# BLOCK 2 seq:0
+# PRED: ENTRY (fallthru)
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset 6, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register 6
+	# dw2-bad-cfi.c:26
+	.loc 1 26 0
+	call	callee
+	# dw2-bad-cfi.c:27
+	.loc 1 27 0
+	movl	$0, %eax
+	# dw2-bad-cfi.c:28
+	.loc 1 28 0
+	popq	%rbp
+	.cfi_def_cfa 7, 8
+# SUCC: EXIT [100.0%] 
+	ret
+	.cfi_endproc
+.LFE1:
+	.size	main, .-main
+.Letext0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.long	0x67	# Length of Compilation Unit Info
+	.value	0x4	# DWARF version number
+	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
+	.byte	0x8	# Pointer Size (in bytes)
+	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
+	.long	.LASF0	# DW_AT_producer: "GNU C 4.7.2 20121109 (Red Hat 4.7.2-8) -mtune=generic -march=x86-64 -g"
+	.byte	0x1	# DW_AT_language
+	.long	.LASF1	# DW_AT_name: "dw2-bad-cfi.c"
+	.long	.LASF2	# DW_AT_comp_dir: "/home/tromey/gnu/gdb/devel/binutils-gdb/gdb/testsuite/gdb.dwarf2"
+	.quad	.Ltext0	# DW_AT_low_pc
+	.quad	.Letext0	# DW_AT_high_pc
+	.long	.Ldebug_line0	# DW_AT_stmt_list
+	.uleb128 0x2	# (DIE (0x2d) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF3	# DW_AT_name: "callee"
+	.byte	0x1	# DW_AT_decl_file (dw2-bad-cfi.c)
+	.byte	0x13	# DW_AT_decl_line
+			# DW_AT_prototyped
+	.quad	.LFB0	# DW_AT_low_pc
+	.quad	.LFE0	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_call_sites
+	.uleb128 0x3	# (DIE (0x46) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF4	# DW_AT_name: "main"
+	.byte	0x1	# DW_AT_decl_file (dw2-bad-cfi.c)
+	.byte	0x18	# DW_AT_decl_line
+			# DW_AT_prototyped
+	.long	0x63	# DW_AT_type
+	.quad	.LFB1	# DW_AT_low_pc
+	.quad	.LFE1	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_tail_call_sites
+	.uleb128 0x4	# (DIE (0x63) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.byte	0	# end of children of DIE 0xb
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.uleb128 0x1	# (abbrev code)
+	.uleb128 0x11	# (TAG: DW_TAG_compile_unit)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x25	# (DW_AT_producer)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x13	# (DW_AT_language)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x1b	# (DW_AT_comp_dir)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x10	# (DW_AT_stmt_list)
+	.uleb128 0x17	# (DW_FORM_sec_offset)
+	.byte	0
+	.byte	0
+	.uleb128 0x2	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0	# DW_children_no
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.uleb128 0x2117	# (DW_AT_GNU_all_call_sites)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.byte	0
+	.byte	0
+	.uleb128 0x3	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0	# DW_children_no
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.uleb128 0x2116	# (DW_AT_GNU_all_tail_call_sites)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.byte	0
+	.byte	0
+	.uleb128 0x4	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.byte	0
+	.byte	0
+	.byte	0
+	.section	.debug_aranges,"",@progbits
+	.long	0x2c	# Length of Address Ranges Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.byte	0x8	# Size of Address
+	.byte	0	# Size of Segment Descriptor
+	.value	0	# Pad to 16 byte boundary
+	.value	0
+	.quad	.Ltext0	# Address
+	.quad	.Letext0-.Ltext0	# Length
+	.quad	0
+	.quad	0
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.section	.debug_str,"MS",@progbits,1
+.LASF3:
+	.string	"callee"
+.LASF1:
+	.string	"dw2-bad-cfi.c"
+.LASF2:
+	.string	"/home/tromey/gnu/gdb/devel/binutils-gdb/gdb/testsuite/gdb.dwarf2"
+.LASF4:
+	.string	"main"
+.LASF0:
+	.string	"GNU C 4.7.2 20121109 (Red Hat 4.7.2-8) -mtune=generic -march=x86-64 -g"
+	.ident	"GCC: (GNU) 4.7.2 20121109 (Red Hat 4.7.2-8)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.c b/gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.c
new file mode 100644
index 0000000..f5b415b
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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/>.  */
+
+void
+callee (void)
+{
+}
+
+int
+main(void)
+{
+  callee ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.exp b/gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.exp
new file mode 100644
index 0000000..12b1275
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-bad-cfi.exp
@@ -0,0 +1,42 @@
+# Copyright 2013 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 dwarf.exp
+
+standard_testfile .S
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if ![dwarf2_support] {
+    return 0  
+}
+
+# This test can only be run on x86-64 targets.
+if {![istarget x86_64-*] || ![is_lp64_target]} {
+    return 0
+}
+
+if {[prepare_for_testing "$testfile.exp" "$testfile" $srcfile {nodebug}]} {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_breakpoint "callee"
+gdb_continue_to_breakpoint "callee"
+
+# This test ensures that the backtrace does not go past "callee".
+gdb_test "bt" ".*callee .. at [file rootname $srcfile]\\.c:\[0-9\]*"
-- 
1.8.1.4

[PATCH 1/2] avoid infinite loop with bad debuginfo

[Tom Tromey]

The immediate failure in PR 16155 is an infinite loop in
value_fetch_lazy.  Each iteration of the loop inside the lval_register
branch computes a value with the same frame id and same register
number as the previous iteration.  It never makes progress, using
progressively more memory creating new values.

It seems to me that it never makes sense to let this loop run
indefinitely.  This patch adds a check and throws an exception if the
same register is returned.  I intentionally did not use an internal
error, because this situation can be caused by bad debuginfo.

I did not go the full distance and have the code check all previous
values.  I could do that if folks want.

With this patch at least the infinite loop is gone.  Now the test case
yields:

    (gdb) bt
    #0  0x0000007fb7ed485c in nanosleep () from /lib64/libc.so.6
    #1  0x0000007fb7ed4508 in sleep () from /lib64/libc.so.6
    #2  0x00000000004008bc in thread_function (arg=0x4) at threadapply.c:73
    #3  0x0000007fb7fad950 in start_thread () from /lib64/libpthread.so.0
    #4  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
    #5  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
    Backtrace stopped: previous frame identical to this frame (corrupt stack?)

2013-11-13  Tom Tromey  <tromey@redhat.com>

	PR backtrace/16155:
	* value.c (value_fetch_lazy): Throw exception if
	get_frame_register_value returns the same register.
---
 gdb/ChangeLog | 6 ++++++
 gdb/value.c   | 9 ++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/gdb/value.c b/gdb/value.c
index 1f562f5..f8831ae 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3507,7 +3507,9 @@ value_fetch_lazy (struct value *val)
 
       while (VALUE_LVAL (new_val) == lval_register && value_lazy (new_val))
 	{
-	  frame = frame_find_by_id (VALUE_FRAME_ID (new_val));
+	  struct frame_id last_frame_id = VALUE_FRAME_ID (new_val);
+
+	  frame = frame_find_by_id (last_frame_id);
 	  regnum = VALUE_REGNUM (new_val);
 
 	  gdb_assert (frame != NULL);
@@ -3521,6 +3523,11 @@ value_fetch_lazy (struct value *val)
 						   regnum, type));
 
 	  new_val = get_frame_register_value (frame, regnum);
+	  if (VALUE_LVAL (new_val) == lval_register
+	      && value_lazy (new_val)
+	      && frame_id_eq (VALUE_FRAME_ID (new_val), last_frame_id))
+	    error (_("infinite loop while fetching a register; "
+		     "probably bad debug info"));
 	}
 
       /* If it's still lazy (for instance, a saved register on the
-- 
1.8.1.4

Re: [PATCH 1/2] avoid infinite loop with bad debuginfo

[Pedro Alves]

On 11/13/2013 08:51 PM, Tom Tromey wrote:
>        while (VALUE_LVAL (new_val) == lval_register && value_lazy (new_val))
>  	{
> -	  frame = frame_find_by_id (VALUE_FRAME_ID (new_val));
> +	  struct frame_id last_frame_id = VALUE_FRAME_ID (new_val);
> +
> +	  frame = frame_find_by_id (last_frame_id);
>  	  regnum = VALUE_REGNUM (new_val);
>  
>  	  gdb_assert (frame != NULL);
> @@ -3521,6 +3523,11 @@ value_fetch_lazy (struct value *val)
>  						   regnum, type));
>  
>  	  new_val = get_frame_register_value (frame, regnum);
> +	  if (VALUE_LVAL (new_val) == lval_register
> +	      && value_lazy (new_val)
> +	      && frame_id_eq (VALUE_FRAME_ID (new_val), last_frame_id))

I think this should also check the regnum:

	  if (VALUE_LVAL (new_val) == lval_register
	      && value_lazy (new_val)
	      && last_regnum == VALUE_REGNUM (new_val);
	      && frame_id_eq (VALUE_FRAME_ID (new_val), last_frame_id))

Makes sense to me with that change.  But see below.  It seems very odd
to me that we'd get into a situation where we have two frames with the
same id.

> +	    error (_("infinite loop while fetching a register; "
> +		     "probably bad debug info"));

What swallows this error?

As it leads to:

    Backtrace stopped: previous frame identical to this frame (corrupt stack?)

I'd mildly suggest changing the new error to match (corrupt stack?)

	    error (_("infinite loop while fetching a register (corrupt stack?)"));

However,

>     #4  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>     #5  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>     Backtrace stopped: previous frame identical to this frame (corrupt stack?)

Doesn't this all then mean that we somehow ended up with two identical
frames with the same id on the frame chain (#4 and #5) ?
That seems very wrong to me.

It seems to be a better fix would be to make
get_prev_frame_1/get_prev_frame_raw discard frame #5 before it
was ever linked in.  Either that, or, if we really need to keep
#5 linked in, we should find a way for frame_id_eq (#4, #5) to
return false.

-- 
Pedro Alves

Re: [PATCH 1/2] avoid infinite loop with bad debuginfo

[Tom Tromey]

>> +	  if (VALUE_LVAL (new_val) == lval_register
>> +	      && value_lazy (new_val)
>> +	      && frame_id_eq (VALUE_FRAME_ID (new_val), last_frame_id))

Pedro> I think this should also check the regnum:

Barf.  I have a memory of actually writing that.  False memory I guess.
Sigh.

>> #4  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>> #5  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>> Backtrace stopped: previous frame identical to this frame (corrupt stack?)

Pedro> Doesn't this all then mean that we somehow ended up with two identical
Pedro> frames with the same id on the frame chain (#4 and #5) ?
Pedro> That seems very wrong to me.

Pedro> It seems to be a better fix would be to make
Pedro> get_prev_frame_1/get_prev_frame_raw discard frame #5 before it
Pedro> was ever linked in.  Either that, or, if we really need to keep
Pedro> #5 linked in, we should find a way for frame_id_eq (#4, #5) to
Pedro> return false.

I will look into it, but my recollection is that last time we got into
this area, it was somehow undesirable to undo whatever changes were done
by existing frame sniffers.

Tom

Re: [PATCH 1/2] avoid infinite loop with bad debuginfo

[Pedro Alves]

On 11/18/2013 06:23 PM, Tom Tromey wrote:
>>> +	  if (VALUE_LVAL (new_val) == lval_register
>>> +	      && value_lazy (new_val)
>>> +	      && frame_id_eq (VALUE_FRAME_ID (new_val), last_frame_id))
> 
> Pedro> I think this should also check the regnum:
> 
> Barf.  I have a memory of actually writing that.  False memory I guess.
> Sigh.
> 
>>> #4  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>>> #5  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>>> Backtrace stopped: previous frame identical to this frame (corrupt stack?)
> 
> Pedro> Doesn't this all then mean that we somehow ended up with two identical
> Pedro> frames with the same id on the frame chain (#4 and #5) ?
> Pedro> That seems very wrong to me.
> 
> Pedro> It seems to be a better fix would be to make
> Pedro> get_prev_frame_1/get_prev_frame_raw discard frame #5 before it
> Pedro> was ever linked in.  Either that, or, if we really need to keep
> Pedro> #5 linked in, we should find a way for frame_id_eq (#4, #5) to
> Pedro> return false.
> 
> I will look into it, but my recollection is that last time we got into
> this area, it was somehow undesirable to undo whatever changes were done
> by existing frame sniffers.

Hmm, I don't think that's the same issue.   I'm just talking about
something like moving the frame_id_eq check to the end of
get_prev_frame_1, after calling get_prev_frame_raw.  Something
like:

  prev_frame = get_prev_frame_raw (this_frame);

  /* Check that this and the prev frame are not identical.  If they
     are, there is most likely a stack cycle.  */
  if (prev_frame != NULL
      && frame_id_eq (get_frame_id (prev_frame),
		      get_frame_id (this_frame)))
    {
      if (frame_debug)
	{
	  fprintf_unfiltered (gdb_stdlog, "-> ");
	  fprint_frame (gdb_stdlog, NULL);
	  fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
	}
      this_frame->stop_reason = UNWIND_SAME_ID;
      /* Unlink.  */
      this_frame->prev = NULL;
      return NULL;
    }

  return prev_frame;
}

-- 
Pedro Alves

Re: [PATCH 1/2] avoid infinite loop with bad debuginfo

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Hmm, I don't think that's the same issue.   I'm just talking about
Pedro> something like moving the frame_id_eq check to the end of
Pedro> get_prev_frame_1, after calling get_prev_frame_raw.  Something
Pedro> like:

Ok, I see now.

I think the issue is moot because your dwarf2_frame_cache-recursion
branch makes the problem disappear, and because the problem, as I
understand it, is that the DWARF unwinding code violates the unwinder
contract in a subtle way (this is what your patch fixes).

That said, even once your change is in, I think both of these patches
should go in.  Patch #1 still prevents an infinite loop -- I can
probably find another test case -- and patch #2 seems like a
straightforward correctness fix.

I'll fix up the tests and see what happens.

Tom

Re: [PATCH 1/2] avoid infinite loop with bad debuginfo

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> +	    error (_("infinite loop while fetching a register; "
>> +		     "probably bad debug info"));

Pedro> What swallows this error?

Just getting back to answering this question...

It is caught in dwarf2_tailcall_sniffer_first.  The straightforward idea
here of removing the TRY_CATCH from this function (and fixing up
cleanups in dwarf2_frame_cache) fails because something (I didn't try to
find what) modifies the frame before the sniffer fails; causing
frame_cleanup_after_sniffer to fail an assertion.

This is all moot if I apply your patches in this area, because they move
the call to dwarf2_tailcall_sniffer_first out of the spot where it can
cause trouble.

Tom

Re: [PATCH 1/2] avoid infinite loop with bad debuginfo

[Pedro Alves]

On 11/19/2013 03:43 PM, Tom Tromey wrote:

> That said, even once your change is in, I think both of these patches
> should go in.  Patch #1 still prevents an infinite loop -- 

...

> I can probably find another test case

Yes, probably by manually creating a corrupted stack.

      while (VALUE_LVAL (new_val) == lval_register && value_lazy (new_val))
	{
	  frame = frame_find_by_id (VALUE_FRAME_ID (new_val));
...
	  new_val = get_frame_register_value (frame, regnum);
	}

get_frame_register_value can return a lazy register value pointing
to the next frame (in the dwarf unwinder, that's
DWARF2_FRAME_REG_SAME_VALUE).  That's perfectly normal.

But say we have a corrupted stack like this:

 #0 - frame_id_1
 #1 - frame_id_2
 #2 - frame_id_3
 #3 - frame_id_4
 #4 - frame_id_4  <<<< outermost (UNWIND_SAME_ID).

So, get_frame_register_value in frame #4, can return
a lazy value pointing to frame #3.  What's not normal is having two
frames with the same id.  So the next iteration, frame_find_by_id
tries to look for frame #3.  But, since it has the
same id as frame #4, frame #4 is returned, rinse, repeat.

I think this is an old latent problem.  We shouldn't ever have
two frames with the same id in the frame chain, lots of things
break otherwise.  But somehow, we managed to get this far
in this particular case.

If we can indeed trigger this with a real corruption test
case, I believe the reason is that the recent-ish addition
of the frame stash exposes the latent bug:

struct frame_info *
frame_find_by_id (struct frame_id id)
{
  struct frame_info *frame, *prev_frame;

  /* ZERO denotes the null frame, let the caller decide what to do
     about it.  Should it instead return get_current_frame()?  */
  if (!frame_id_p (id))
    return NULL;

  /* Try using the frame stash first.  Finding it there removes the need
     to perform the search by looping over all frames, which can be very
     CPU-intensive if the number of frames is very high (the loop is O(n)
     and get_prev_frame performs a series of checks that are relatively
     expensive).  This optimization is particularly useful when this function
     is called from another function (such as value_fetch_lazy, case
     VALUE_LVAL (val) == lval_register) which already loops over all frames,
     making the overall behavior O(n^2).  */
  frame = frame_stash_find (id);
  if (frame)
    return frame;

  for (frame = get_current_frame (); ; frame = prev_frame)
    {


Before we had a stash, frame_find_by_id(frame_id_4) would
always find #3 first.  But now, it's possible that
the stash returns #4 instead.

I still think that such a loop should be broken by never
having two frames with the same id in the frame chain in the
first place.  This potential infinite loop in value_fetch_lazy
is really an internal error, IMO.

-- 
Pedro Alves

Re: [PATCH 1/2] avoid infinite loop with bad debuginfo

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Tom> I can probably find another test case

Pedro> Yes, probably by manually creating a corrupted stack.

Actually I was thinking of something simpler...

Pedro> I think this is an old latent problem.  We shouldn't ever have
Pedro> two frames with the same id in the frame chain, lots of things
Pedro> break otherwise.  But somehow, we managed to get this far
Pedro> in this particular case.

Ok, thanks for this analysis.

Pedro> If we can indeed trigger this with a real corruption test
Pedro> case, I believe the reason is that the recent-ish addition
Pedro> of the frame stash exposes the latent bug:

Nice insight.

To me this is another internal constraint of the unwinder design that is
being violated "somewhere".  It seems like we can't really write an
ordinary test case for it -- since it is a "shouldn't happen" scenario.

Pedro> I still think that such a loop should be broken by never
Pedro> having two frames with the same id in the frame chain in the
Pedro> first place.  This potential infinite loop in value_fetch_lazy
Pedro> is really an internal error, IMO.

It seems to me that the loop in question could perhaps be reached from
some path outside the unwinder.  If so, bad DWARF would be able to cause
an internal error -- clearly incorrect.  I suppose one idea is that the
DWARF code should do the checking itself, though.

Tom

Re: [PATCH 1/2] avoid infinite loop with bad debuginfo

[Pedro Alves]

On 11/19/2013 06:06 PM, Tom Tromey wrote:
> It seems to me that the loop in question could perhaps be reached from
> some path outside the unwinder.  

Yes, that's actually what my example was about.  I was assuming the
recursion fixed already.

> If so, bad DWARF would be able to cause
> an internal error -- clearly incorrect.  

I don't think so, because get_prev_frame_1 would not link in
the dup frame.  The loop in question would never see it.

Hmm, I think one of us is missing something.

      while (VALUE_LVAL (new_val) == lval_register && value_lazy (new_val))
	{
	  frame = frame_find_by_id (VALUE_FRAME_ID (new_val));
...
	  new_val = get_frame_register_value (frame, regnum);
	}

get_frame_register_value unwinds the value in question from the
next frame.

struct value *
get_frame_register_value (struct frame_info *frame, int regnum)
{
  return frame_unwind_register_value (frame->next, regnum);
                                      ^^^^^^^^^^^
}

IOW, if we get a lazy lval_register, it should have
the frame ID of the _next_ frame, never of FRAME.

At this point, the whole relevant chunk of the stack has
already been unwound -- note the loop always "unlazies"
lval_registers in the "next/innermost" direction, not in
the "prev/unwind further/outermost" direction.

So the bad loop can only ever happen (outside the unwinder code)
if we ever let outselves get in the dup frame_id situation:

>     #4  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>     #5  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>     Backtrace stopped: previous frame identical to this frame (corrupt stack?)

At least, I'm not seeing any other way.

-- 
Pedro Alves

Re: [PATCH 1/2] avoid infinite loop with bad debuginfo

[Tom Tromey]

Pedro> I don't think so, because get_prev_frame_1 would not link in
Pedro> the dup frame.  The loop in question would never see it.

Pedro> Hmm, I think one of us is missing something.

Haha, yeah, that usually means me :-)

No worries.  I think I understand this bit now.

Pedro> So the bad loop can only ever happen (outside the unwinder code)
Pedro> if we ever let outselves get in the dup frame_id situation:

>> #4  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>> #5  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>> Backtrace stopped: previous frame identical to this frame (corrupt stack?)

Pedro> At least, I'm not seeing any other way.

Yes, I see now.

Really not looking forward to writing the test.

Tom

[PATCH] Don't let two frames with the same id end up in the frame chain. (Re: [PATCH 1/2] avoid infinite loop with bad debuginfo)

[Pedro Alves]

On 11/19/2013 08:24 PM, Tom Tromey wrote:
> Pedro> I don't think so, because get_prev_frame_1 would not link in
> Pedro> the dup frame.  The loop in question would never see it.
> 
> Pedro> Hmm, I think one of us is missing something.
> 
> Haha, yeah, that usually means me :-)

Haha, I wish.  :-)

> 
> No worries.  I think I understand this bit now.
> 
> Pedro> So the bad loop can only ever happen (outside the unwinder code)
> Pedro> if we ever let outselves get in the dup frame_id situation:
> 
>>> #4  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>>> #5  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>>> Backtrace stopped: previous frame identical to this frame (corrupt stack?)
> 
> Pedro> At least, I'm not seeing any other way.
> 
> Yes, I see now.

OK, here's the same in patch form.

> 
> Really not looking forward to writing the test.

Yeah, me neither. :-P

---------
Subject: Don't let two frames with the same id end up in the frame chain.

The UNWIND_SAME_ID check is done between THIS_FRAME and the next
frame.  But at this point, it's already too late -- we ended up with
two frames with the same ID in the frame chain.  Each frame having its
own ID is an invariant assumed throughout GDB.  So this patch applies
the UNWIND_SAME_ID detection earlier, right after the previous frame
is unwond, discarding the dup frame if a cycle is detected.

gdb/
2013-11-20  Pedro Alves  <palves@redhat.com>

	* frame.c (get_prev_frame_1): Do the UNWIND_SAME_ID check between
	this frame and the new previous frame, not between this frame and
	the next frame.
---

 gdb/frame.c |   43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 63f20d5..535a5a6 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1666,6 +1666,7 @@ get_prev_frame_1 (struct frame_info *this_frame)
 {
   struct frame_id this_id;
   struct gdbarch *gdbarch;
+  struct frame_info *prev_frame;
 
   gdb_assert (this_frame != NULL);
   gdbarch = get_frame_arch (this_frame);
@@ -1767,22 +1768,6 @@ get_prev_frame_1 (struct frame_info *this_frame)
 	}
     }
 
-  /* Check that this and the next frame are not identical.  If they
-     are, there is most likely a stack cycle.  As with the inner-than
-     test above, avoid comparing the inner-most and sentinel frames.  */
-  if (this_frame->level > 0
-      && frame_id_eq (this_id, get_frame_id (this_frame->next)))
-    {
-      if (frame_debug)
-	{
-	  fprintf_unfiltered (gdb_stdlog, "-> ");
-	  fprint_frame (gdb_stdlog, NULL);
-	  fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
-	}
-      this_frame->stop_reason = UNWIND_SAME_ID;
-      return NULL;
-    }
-
   /* Check that this and the next frame do not unwind the PC register
      to the same memory location.  If they do, then even though they
      have different frame IDs, the new frame will be bogus; two
@@ -1830,7 +1815,31 @@ get_prev_frame_1 (struct frame_info *this_frame)
 	}
     }
 
-  return get_prev_frame_raw (this_frame);
+  prev_frame = get_prev_frame_raw (this_frame);
+
+  /* Check that this and the prev frame are not identical.  If they
+     are, there is most likely a stack cycle.  Unlike the tests above,
+     we do this right after creating the prev frame, to avoid ever
+     ending up with two frames with the same id in the frame
+     chain.  */
+  if (prev_frame != NULL
+      && frame_id_eq (get_frame_id (prev_frame),
+		      get_frame_id (this_frame)))
+    {
+      if (frame_debug)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "-> ");
+	  fprint_frame (gdb_stdlog, NULL);
+	  fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
+	}
+      this_frame->stop_reason = UNWIND_SAME_ID;
+      /* Unlink.  */
+      prev_frame->next = NULL;
+      this_frame->prev = NULL;
+      return NULL;
+    }
+
+  return prev_frame;
 }
 
 /* Construct a new "struct frame_info" and link it previous to

Re: [PATCH] Don't let two frames with the same id end up in the frame chain. (Re: [PATCH 1/2] avoid infinite loop with bad debuginfo)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Tom> Really not looking forward to writing the test.

Pedro> Yeah, me neither. :-P

Well, I took at stab at it today, and totally failed.
I will try to catch you on irc tomorrow to pick your brain, if that's ok
with you, to try to understand how I could get a test case.

Pedro> Subject: Don't let two frames with the same id end up in the frame chain.

Pedro> The UNWIND_SAME_ID check is done between THIS_FRAME and the next
Pedro> frame.  But at this point, it's already too late -- we ended up with
Pedro> two frames with the same ID in the frame chain.  Each frame having its
Pedro> own ID is an invariant assumed throughout GDB.  So this patch applies
Pedro> the UNWIND_SAME_ID detection earlier, right after the previous frame
Pedro> is unwond, discarding the dup frame if a cycle is detected.

s/unwond/unwound/

FWIW I have nearly the identical patch here :)
I think it's a good idea.

I also have the appended, which makes the frame stash behave a little
nicer if an unwinder gives a duplicate frame id.  Probably not needed in
addition to the patch you sent, but on the other hand, cheap.

Tom

diff --git a/gdb/frame.c b/gdb/frame.c
index 63f20d5..dd419c9 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -203,7 +203,13 @@ frame_stash_add (struct frame_info *frame)
       slot = (struct frame_info **) htab_find_slot (frame_stash,
 						    frame,
 						    INSERT);
-      *slot = frame;
+
+      /* While it is invalid for two frames to have the same ID, it
+	 may happen due to a bug elsewhere in gdb.  And, should this
+	 happen, it is better for the frame stash to return the newer
+	 frame.  So, ignore duplicates here.  */
+      if (*slot == NULL)
+	*slot = frame;
     }
 }
 

Re: [PATCH] Don't let two frames with the same id end up in the frame chain. (Re: [PATCH 1/2] avoid infinite loop with bad debuginfo)

[Pedro Alves]

On 11/20/2013 09:21 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Tom> Really not looking forward to writing the test.
> 
> Pedro> Yeah, me neither. :-P
> 
> Well, I took at stab at it today, and totally failed.
> I will try to catch you on irc tomorrow to pick your brain, if that's ok
> with you, to try to understand how I could get a test case.

I wonder if it would be as "simple" as coming up with
dwarf that says all registers are found via DWARF2_FRAME_REG_SAME_VALUE?

> 
> Pedro> Subject: Don't let two frames with the same id end up in the frame chain.
> 
> Pedro> The UNWIND_SAME_ID check is done between THIS_FRAME and the next
> Pedro> frame.  But at this point, it's already too late -- we ended up with
> Pedro> two frames with the same ID in the frame chain.  Each frame having its
> Pedro> own ID is an invariant assumed throughout GDB.  So this patch applies
> Pedro> the UNWIND_SAME_ID detection earlier, right after the previous frame
> Pedro> is unwond, discarding the dup frame if a cycle is detected.
> 
> s/unwond/unwound/
> 
> FWIW I have nearly the identical patch here :)
> I think it's a good idea.
> 
> I also have the appended, which makes the frame stash behave a little
> nicer if an unwinder gives a duplicate frame id.  Probably not needed in
> addition to the patch you sent, but on the other hand, cheap.

Yeah, that makes the stash behave like if there was no stash
in this scenario.

As the stash now hold the ids of all frames in the chain, this
looks like the place we can detect these bad issues, for easier
diagnosis of whatever problem this could cause.  Perhaps we
should complaint() or warn() ?

Hmm, wait...

Why not go the full mile?  We could use this to go one
step further, and detect corrupted stacks that create
stack cycles with non-consecutive dup frame ids:

 #0 frame_id1
 #1 frame_id2
 #2 frame_id3
 #3 frame_id1
 #4 frame_id2
 #5 frame_id3
 #6 frame_id1
 ... forever ...

Given we already have the stash, it's just as cheap as just
detecting the cycle in consecutive frames.  See below, for
a patch on top of the previous one.

----
Make use of the frame stash to detect wider stack cycles.

Tested on x86_64 Fedora 17.

gdb/
2013-11-21  Pedro Alves  <palves@redhat.com>
	    Tom Tromey  <tromey@redhat.com>

	* frame.c (frame_stash_add): Now returns whether a frame with the
	same ID was already known.
	(compute_frame_id): New function, factored out from get_frame_id.
	(get_frame_id): No longer lazilly compute the frame id here.
	(get_prev_frame_if_no_cycle): New function.  Detects wider stack
	cycles.
	(get_prev_frame_1): Use it instead of get_prev_frame_raw directly,
	and checking for stack cycles here.
---

 gdb/frame.c |  152 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 89 insertions(+), 63 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 535a5a6..891b4ea 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -188,23 +188,31 @@ frame_stash_create (void)
 			     NULL);
 }
 
-/* Internal function to add a frame to the frame_stash hash table.  Do
-   not store frames below 0 as they may not have any addresses to
-   calculate a hash.  */
+/* Internal function to add a frame to the frame_stash hash table.
+   Returns false if a frame with the same ID was already stashed, true
+   otherwise.  */
 
-static void
+static int
 frame_stash_add (struct frame_info *frame)
 {
-  /* Do not stash frames below level 0.  */
-  if (frame->level >= 0)
-    {
-      struct frame_info **slot;
+  struct frame_info **slot;
 
-      slot = (struct frame_info **) htab_find_slot (frame_stash,
-						    frame,
-						    INSERT);
-      *slot = frame;
-    }
+  /* Do not try to stash the sentinel frame.  */
+  gdb_assert (frame->level >= 0);
+
+  slot = (struct frame_info **) htab_find_slot (frame_stash,
+						frame,
+						INSERT);
+
+  /* If we already have a frame in the stack with the same id, we
+     either have a stack cycle (corrupted stack?), or some bug
+     elsewhere in GDB.  In any case, ignore the duplicate and return
+     an indication to the caller.  */
+  if (*slot != NULL)
+    return 0;
+
+  *slot = frame;
+  return 1;
 }
 
 /* Internal function to search the frame stash for an entry with the
@@ -389,6 +397,34 @@ skip_artificial_frames (struct frame_info *frame)
   return frame;
 }
 
+/* Compute the frame's uniq ID that can be used to, later, re-find the
+   frame.  */
+
+static void
+compute_frame_id (struct frame_info *fi)
+{
+  gdb_assert (!fi->this_id.p);
+
+  if (frame_debug)
+    fprintf_unfiltered (gdb_stdlog, "{ compute_frame_id (fi=%d) ",
+			fi->level);
+  /* Find the unwinder.  */
+  if (fi->unwind == NULL)
+    frame_unwind_find_by_frame (fi, &fi->prologue_cache);
+  /* Find THIS frame's ID.  */
+  /* Default to outermost if no ID is found.  */
+  fi->this_id.value = outer_frame_id;
+  fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
+  gdb_assert (frame_id_p (fi->this_id.value));
+  fi->this_id.p = 1;
+  if (frame_debug)
+    {
+      fprintf_unfiltered (gdb_stdlog, "-> ");
+      fprint_frame_id (gdb_stdlog, fi->this_id.value);
+      fprintf_unfiltered (gdb_stdlog, " }\n");
+    }
+}
+
 /* Return a frame uniq ID that can be used to, later, re-find the
    frame.  */
 
@@ -398,29 +434,7 @@ get_frame_id (struct frame_info *fi)
   if (fi == NULL)
     return null_frame_id;
 
-  if (!fi->this_id.p)
-    {
-      if (frame_debug)
-	fprintf_unfiltered (gdb_stdlog, "{ get_frame_id (fi=%d) ",
-			    fi->level);
-      /* Find the unwinder.  */
-      if (fi->unwind == NULL)
-	frame_unwind_find_by_frame (fi, &fi->prologue_cache);
-      /* Find THIS frame's ID.  */
-      /* Default to outermost if no ID is found.  */
-      fi->this_id.value = outer_frame_id;
-      fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
-      gdb_assert (frame_id_p (fi->this_id.value));
-      fi->this_id.p = 1;
-      if (frame_debug)
-	{
-	  fprintf_unfiltered (gdb_stdlog, "-> ");
-	  fprint_frame_id (gdb_stdlog, fi->this_id.value);
-	  fprintf_unfiltered (gdb_stdlog, " }\n");
-	}
-      frame_stash_add (fi);
-    }
-
+  gdb_assert (fi->this_id.p);
   return fi->this_id.value;
 }
 
@@ -1655,6 +1669,43 @@ frame_register_unwind_location (struct frame_info *this_frame, int regnum,
     }
 }
 
+/* Get the previous raw frame, and check that it is not identical to
+   same other frame frame already in the chain.  If it is, there is
+   most likely a stack cycle, so we discard it, and mark THIS_FRAME as
+   outermost, with UNWIND_SAME_ID stop reason.  Unlike the other
+   validity tests, that compare THIS_FRAME and the next frame, we do
+   this right after creating the prev frame, to avoid ever ending up
+   with two frames with the same id in the frame chain.  */
+
+static struct frame_info *
+get_prev_frame_if_no_cycle (struct frame_info *this_frame)
+{
+  struct frame_info *prev_frame;
+
+  prev_frame = get_prev_frame_raw (this_frame);
+  if (prev_frame == NULL)
+    return NULL;
+
+  gdb_assert (!prev_frame->this_id.p);
+  compute_frame_id (prev_frame);
+  if (frame_stash_add (prev_frame))
+    return prev_frame;
+
+  /* Another frame with the same id was already in the stash.  We just
+     detected a cycle.  */
+  if (frame_debug)
+    {
+      fprintf_unfiltered (gdb_stdlog, "-> ");
+      fprint_frame (gdb_stdlog, NULL);
+      fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
+    }
+  this_frame->stop_reason = UNWIND_SAME_ID;
+  /* Unlink.  */
+  prev_frame->next = NULL;
+  this_frame->prev = NULL;
+  return NULL;
+}
+
 /* Return a "struct frame_info" corresponding to the frame that called
    THIS_FRAME.  Returns NULL if there is no such frame.
 
@@ -1666,7 +1717,6 @@ get_prev_frame_1 (struct frame_info *this_frame)
 {
   struct frame_id this_id;
   struct gdbarch *gdbarch;
-  struct frame_info *prev_frame;
 
   gdb_assert (this_frame != NULL);
   gdbarch = get_frame_arch (this_frame);
@@ -1709,7 +1759,7 @@ get_prev_frame_1 (struct frame_info *this_frame)
      until we have unwound all the way down to the previous non-inline
      frame.  */
   if (get_frame_type (this_frame) == INLINE_FRAME)
-    return get_prev_frame_raw (this_frame);
+    return get_prev_frame_if_no_cycle (this_frame);
 
   /* Check that this frame is unwindable.  If it isn't, don't try to
      unwind to the prev frame.  */
@@ -1815,31 +1865,7 @@ get_prev_frame_1 (struct frame_info *this_frame)
 	}
     }
 
-  prev_frame = get_prev_frame_raw (this_frame);
-
-  /* Check that this and the prev frame are not identical.  If they
-     are, there is most likely a stack cycle.  Unlike the tests above,
-     we do this right after creating the prev frame, to avoid ever
-     ending up with two frames with the same id in the frame
-     chain.  */
-  if (prev_frame != NULL
-      && frame_id_eq (get_frame_id (prev_frame),
-		      get_frame_id (this_frame)))
-    {
-      if (frame_debug)
-	{
-	  fprintf_unfiltered (gdb_stdlog, "-> ");
-	  fprint_frame (gdb_stdlog, NULL);
-	  fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
-	}
-      this_frame->stop_reason = UNWIND_SAME_ID;
-      /* Unlink.  */
-      prev_frame->next = NULL;
-      this_frame->prev = NULL;
-      return NULL;
-    }
-
-  return prev_frame;
+  return get_prev_frame_if_no_cycle (this_frame);
 }
 
 /* Construct a new "struct frame_info" and link it previous to

Re: [PATCH] Don't let two frames with the same id end up in the frame chain. (Re: [PATCH 1/2] avoid infinite loop with bad debuginfo)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Why not go the full mile?  We could use this to go one
Pedro> step further, and detect corrupted stacks that create
Pedro> stack cycles with non-consecutive dup frame ids:

Good idea.

The patch looked good to me.

Tom

[COMMITTED] Make use of the frame stash to detect wider stack cycles. (was: Re: [PATCH] Don't let two frames with the same id end up in the frame chain. (Re: [PATCH 1/2] avoid infinite loop with bad debuginfo))

[Pedro Alves]

On 11/21/2013 07:10 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Why not go the full mile?  We could use this to go one
> Pedro> step further, and detect corrupted stacks that create
> Pedro> stack cycles with non-consecutive dup frame ids:
> 
> Good idea.
> 
> The patch looked good to me.

I pushed this one too.

-- 
Pedro Alves

Re: [PATCH] Don't let two frames with the same id end up in the frame chain. (Re: [PATCH 1/2] avoid infinite loop with bad debuginfo)

[Pedro Alves]

On 11/21/2013 04:12 PM, Pedro Alves wrote:
> On 11/20/2013 09:21 PM, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>>
>> Tom> Really not looking forward to writing the test.
>>
>> Pedro> Yeah, me neither. :-P
>>
>> Well, I took at stab at it today, and totally failed.
>> I will try to catch you on irc tomorrow to pick your brain, if that's ok
>> with you, to try to understand how I could get a test case.
> 
> I wonder if it would be as "simple" as coming up with
> dwarf that says all registers are found via DWARF2_FRAME_REG_SAME_VALUE?

It is.  I adjusted dw2-reg-undefined.S to use DW_CFA_same_value
instead of DW_CFA_undefined, and to mark a different set of
registers (most importantly, RSP and RIP) so that GDB computes
the same ID for both frames.  That triggers the infinite loop,
even with the dwarf2_frame_cache recursion fix applied.

Now pushed.

----------
Subject: [PATCH] Don't let two frames with the same id end up in the frame
 chain.

The UNWIND_SAME_ID check is done between THIS_FRAME and the next frame
when we go try to unwind the previous frame.  But at this point, it's
already too late -- we ended up with two frames with the same ID in
the frame chain.  Each frame having its own ID is an invariant assumed
throughout GDB.  This patch applies the UNWIND_SAME_ID detection
earlier, right after the previous frame is unwound, discarding the dup
frame if a cycle is detected.

The patch includes a new test that fails before the change.  Before
the patch, the test causes an infinite loop in GDB, after the patch,
the UNWIND_SAME_ID logic kicks in and makes the backtrace stop with:

  Backtrace stopped: previous frame identical to this frame (corrupt stack?)

The test uses dwarf CFI to emulate a corrupted stack with a cycle.  It
has a function with registers marked DW_CFA_same_value (most
importantly RSP/RIP), so that GDB computes the same ID for that frame
and its caller.  IOW, something like this:

 #0 - frame_id_1
 #1 - frame_id_2
 #2 - frame_id_3
 #3 - frame_id_4
 #4 - frame_id_4  <<<< outermost (UNWIND_SAME_ID).

(The test's code is just a copy of dw2-reg-undefined.S /
dw2-reg-undefined.c, adjusted to use DW_CFA_same_value instead of
DW_CFA_undefined, and to mark a different set of registers.)

The infinite loop is here, in value_fetch_lazy:

      while (VALUE_LVAL (new_val) == lval_register && value_lazy (new_val))
	{
	  frame = frame_find_by_id (VALUE_FRAME_ID (new_val));
...
	  new_val = get_frame_register_value (frame, regnum);
	}

get_frame_register_value can return a lazy register value pointing to
the next frame.  This means that the register wasn't clobbered by
FRAME; the debugger should therefore retrieve its value from the next
frame.

To be clear, get_frame_register_value unwinds the value in question
from the next frame:

 struct value *
 get_frame_register_value (struct frame_info *frame, int regnum)
 {
   return frame_unwind_register_value (frame->next, regnum);
                                       ^^^^^^^^^^^
 }

In other words, if we get a lazy lval_register, it should have the
frame ID of the _next_ frame, never of FRAME.

At this point in value_fetch_lazy, the whole relevant chunk of the
stack up to frame #4 has already been unwound.  The loop always
"unlazies" lval_registers in the "next/innermost" direction, not in
the "prev/unwind further/outermost" direction.

So say we're looking at frame #4.  get_frame_register_value in frame
#4 can return a lazy register value of frame #3.  So the next
iteration, frame_find_by_id tries to read the register from frame #3.
But, since frame #4 happens to have same id as frame #3,
frame_find_by_id returns frame #4 instead.  Rinse, repeat, and we have
an infinite loop.

This is an old latent problem, exposed by the recent addition of the
frame stash.  Before we had a stash, frame_find_by_id(frame_id_4)
would walk over all frames starting at the current frame, and would
always find #3 first.  The stash happens to return #4 instead:

struct frame_info *
frame_find_by_id (struct frame_id id)
{
  struct frame_info *frame, *prev_frame;

...
  /* Try using the frame stash first.  Finding it there removes the need
     to perform the search by looping over all frames, which can be very
     CPU-intensive if the number of frames is very high (the loop is O(n)
     and get_prev_frame performs a series of checks that are relatively
     expensive).  This optimization is particularly useful when this function
     is called from another function (such as value_fetch_lazy, case
     VALUE_LVAL (val) == lval_register) which already loops over all frames,
     making the overall behavior O(n^2).  */
  frame = frame_stash_find (id);
  if (frame)
    return frame;

  for (frame = get_current_frame (); ; frame = prev_frame)
    {

gdb/
2013-11-22  Pedro Alves  <palves@redhat.com>

	PR 16155
	* frame.c (get_prev_frame_1): Do the UNWIND_SAME_ID check between
	this frame and the new previous frame, not between this frame and
	the next frame.

gdb/testsuite/
2013-11-22  Pedro Alves  <palves@redhat.com>

	PR 16155
	* gdb.dwarf2/dw2-dup-frame.S: New file.
	* gdb.dwarf2/dw2-dup-frame.c: New file.
	* gdb.dwarf2/dw2-dup-frame.exp: New file.
---
 gdb/ChangeLog                              |   7 +
 gdb/frame.c                                |  43 ++-
 gdb/testsuite/ChangeLog                    |   7 +
 gdb/testsuite/gdb.dwarf2/dw2-dup-frame.S   | 540 +++++++++++++++++++++++++++++
 gdb/testsuite/gdb.dwarf2/dw2-dup-frame.c   |  36 ++
 gdb/testsuite/gdb.dwarf2/dw2-dup-frame.exp |  44 +++
 6 files changed, 660 insertions(+), 17 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-dup-frame.S
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-dup-frame.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-dup-frame.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c4bcdf6..e72097f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,6 +1,13 @@
 2013-11-22  Pedro Alves  <palves@redhat.com>
 
 	PR 16155
+	* frame.c (get_prev_frame_1): Do the UNWIND_SAME_ID check between
+	this frame and the new previous frame, not between this frame and
+	the next frame.
+
+2013-11-22  Pedro Alves  <palves@redhat.com>
+
+	PR 16155
 	* dwarf2-frame.c (struct dwarf2_frame_cache)
 	<checked_tailcall_bottom, entry_cfa_sp_offset,
 	entry_cfa_sp_offset_p>: New fields.
diff --git a/gdb/frame.c b/gdb/frame.c
index 63f20d5..535a5a6 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1666,6 +1666,7 @@ get_prev_frame_1 (struct frame_info *this_frame)
 {
   struct frame_id this_id;
   struct gdbarch *gdbarch;
+  struct frame_info *prev_frame;
 
   gdb_assert (this_frame != NULL);
   gdbarch = get_frame_arch (this_frame);
@@ -1767,22 +1768,6 @@ get_prev_frame_1 (struct frame_info *this_frame)
 	}
     }
 
-  /* Check that this and the next frame are not identical.  If they
-     are, there is most likely a stack cycle.  As with the inner-than
-     test above, avoid comparing the inner-most and sentinel frames.  */
-  if (this_frame->level > 0
-      && frame_id_eq (this_id, get_frame_id (this_frame->next)))
-    {
-      if (frame_debug)
-	{
-	  fprintf_unfiltered (gdb_stdlog, "-> ");
-	  fprint_frame (gdb_stdlog, NULL);
-	  fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
-	}
-      this_frame->stop_reason = UNWIND_SAME_ID;
-      return NULL;
-    }
-
   /* Check that this and the next frame do not unwind the PC register
      to the same memory location.  If they do, then even though they
      have different frame IDs, the new frame will be bogus; two
@@ -1830,7 +1815,31 @@ get_prev_frame_1 (struct frame_info *this_frame)
 	}
     }
 
-  return get_prev_frame_raw (this_frame);
+  prev_frame = get_prev_frame_raw (this_frame);
+
+  /* Check that this and the prev frame are not identical.  If they
+     are, there is most likely a stack cycle.  Unlike the tests above,
+     we do this right after creating the prev frame, to avoid ever
+     ending up with two frames with the same id in the frame
+     chain.  */
+  if (prev_frame != NULL
+      && frame_id_eq (get_frame_id (prev_frame),
+		      get_frame_id (this_frame)))
+    {
+      if (frame_debug)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "-> ");
+	  fprint_frame (gdb_stdlog, NULL);
+	  fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
+	}
+      this_frame->stop_reason = UNWIND_SAME_ID;
+      /* Unlink.  */
+      prev_frame->next = NULL;
+      this_frame->prev = NULL;
+      return NULL;
+    }
+
+  return prev_frame;
 }
 
 /* Construct a new "struct frame_info" and link it previous to
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 58d2bc7..0135ece 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2013-11-22  Pedro Alves  <palves@redhat.com>
+
+	PR 16155
+	* gdb.dwarf2/dw2-dup-frame.S: New file.
+	* gdb.dwarf2/dw2-dup-frame.c: New file.
+	* gdb.dwarf2/dw2-dup-frame.exp: New file.
+
 2013-11-22  Yao Qi  <yao@codesourcery.com>
 
 	* lib/mi-support.exp (mi_create_dynamic_varobj): Update
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-dup-frame.S b/gdb/testsuite/gdb.dwarf2/dw2-dup-frame.S
new file mode 100644
index 0000000..4c3bd76
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-dup-frame.S
@@ -0,0 +1,540 @@
+/*
+   Copyright 2013 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/>.  */
+
+	/* The FDE entry for "stop_frame" in the .debug_frame section has
+	been hand modified to mark a set of registers as DW_CFA_same_value.
+	Otherwise this file is as generated by gcc 4.7.2 for x86_64.  */
+	.file	"dw2-dup-frame.c"
+	.text
+.Ltext0:
+	.globl	stop_frame
+	.type	stop_frame, @function
+stop_frame:
+.LFB0:
+	.file 1 "dw2-dup-frame.c"
+	.loc 1 19 0
+	pushq	%rbp
+.LCFI0:
+	movq	%rsp, %rbp
+.LCFI1:
+	.loc 1 22 0
+	popq	%rbp
+.LCFI2:
+	ret
+.LFE0:
+	.size	stop_frame, .-stop_frame
+	.globl	first_frame
+	.type	first_frame, @function
+first_frame:
+.LFB1:
+	.loc 1 26 0
+	pushq	%rbp
+.LCFI3:
+	movq	%rsp, %rbp
+.LCFI4:
+	.loc 1 27 0
+	movl	$0, %eax
+	call	stop_frame
+	.loc 1 28 0
+	popq	%rbp
+.LCFI5:
+	ret
+.LFE1:
+	.size	first_frame, .-first_frame
+	.globl	main
+	.type	main, @function
+main:
+.LFB2:
+	.loc 1 32 0
+	pushq	%rbp
+.LCFI6:
+	movq	%rsp, %rbp
+.LCFI7:
+	.loc 1 33 0
+	movl	$0, %eax
+	call	first_frame
+	.loc 1 35 0
+	movl	$0, %eax
+	.loc 1 36 0
+	popq	%rbp
+.LCFI8:
+	ret
+.LFE2:
+	.size	main, .-main
+	.section	.debug_frame,"",@progbits
+.Lframe0:
+	.long	.LECIE0-.LSCIE0
+.LSCIE0:
+	.long	0xffffffff
+	.byte	0x1
+	.string	""
+	.uleb128 0x1
+	.sleb128 -8
+	.byte	0x10
+	.byte	0xc
+	.uleb128 0x7
+	.uleb128 0x8
+	.byte	0x90
+	.uleb128 0x1
+	.align 8
+.LECIE0:
+	/* This FDE entry, for stop_frame was modified to mark
+	   registers 0 -> 16 (rax..ra/rip) as being DW_CFA_same_value.  */
+.LSFDE0:
+	.long	.LEFDE0-.LASFDE0
+.LASFDE0:
+	.long	.Lframe0
+	.quad	.LFB0
+	.quad	.LFE0-.LFB0
+
+		/* START OF NEW CONTENT.  */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0x0			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0x1			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0x2			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0x3			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0x4			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0x5			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0x6			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0x7			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0x8			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0x9			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0xa			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0xb			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0xc			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0xd			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0xe			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0xf			/*   ULEB128 register */
+	.byte	0x8			/* DW_CFA_same_value */
+	.uleb128 0x10			 /*   ULEB128 register */
+		/* END OF NEW CONTENT.  */
+
+	.byte	0x4
+	.long	.LCFI0-.LFB0
+	.byte	0xe
+	.uleb128 0x10
+	.byte	0x86
+	.uleb128 0x2
+	.byte	0x4
+	.long	.LCFI1-.LCFI0
+	.byte	0xd
+	.uleb128 0x6
+	.byte	0x4
+	.long	.LCFI2-.LCFI1
+	.byte	0xc
+	.uleb128 0x7
+	.uleb128 0x8
+	.align 8
+.LEFDE0:
+.LSFDE2:
+	.long	.LEFDE2-.LASFDE2
+.LASFDE2:
+	.long	.Lframe0
+	.quad	.LFB1
+	.quad	.LFE1-.LFB1
+	.byte	0x4
+	.long	.LCFI3-.LFB1
+	.byte	0xe
+	.uleb128 0x10
+	.byte	0x86
+	.uleb128 0x2
+	.byte	0x4
+	.long	.LCFI4-.LCFI3
+	.byte	0xd
+	.uleb128 0x6
+	.byte	0x4
+	.long	.LCFI5-.LCFI4
+	.byte	0xc
+	.uleb128 0x7
+	.uleb128 0x8
+	.align 8
+.LEFDE2:
+.LSFDE4:
+	.long	.LEFDE4-.LASFDE4
+.LASFDE4:
+	.long	.Lframe0
+	.quad	.LFB2
+	.quad	.LFE2-.LFB2
+	.byte	0x4
+	.long	.LCFI6-.LFB2
+	.byte	0xe
+	.uleb128 0x10
+	.byte	0x86
+	.uleb128 0x2
+	.byte	0x4
+	.long	.LCFI7-.LCFI6
+	.byte	0xd
+	.uleb128 0x6
+	.byte	0x4
+	.long	.LCFI8-.LCFI7
+	.byte	0xc
+	.uleb128 0x7
+	.uleb128 0x8
+	.align 8
+.LEFDE4:
+	.section	.eh_frame,"a",@progbits
+.Lframe1:
+	.long	.LECIE1-.LSCIE1
+.LSCIE1:
+	.long	0
+	.byte	0x1
+	.string	"zR"
+	.uleb128 0x1
+	.sleb128 -8
+	.byte	0x10
+	.uleb128 0x1
+	.byte	0x3
+	.byte	0xc
+	.uleb128 0x7
+	.uleb128 0x8
+	.byte	0x90
+	.uleb128 0x1
+	.align 8
+.LECIE1:
+.LSFDE7:
+	.long	.LEFDE7-.LASFDE7
+.LASFDE7:
+	.long	.LASFDE7-.Lframe1
+	.long	.LFB0
+	.long	.LFE0-.LFB0
+	.uleb128 0
+	.byte	0x4
+	.long	.LCFI0-.LFB0
+	.byte	0xe
+	.uleb128 0x10
+	.byte	0x86
+	.uleb128 0x2
+	.byte	0x4
+	.long	.LCFI1-.LCFI0
+	.byte	0xd
+	.uleb128 0x6
+	.byte	0x4
+	.long	.LCFI2-.LCFI1
+	.byte	0xc
+	.uleb128 0x7
+	.uleb128 0x8
+	.align 8
+.LEFDE7:
+.LSFDE9:
+	.long	.LEFDE9-.LASFDE9
+.LASFDE9:
+	.long	.LASFDE9-.Lframe1
+	.long	.LFB1
+	.long	.LFE1-.LFB1
+	.uleb128 0
+	.byte	0x4
+	.long	.LCFI3-.LFB1
+	.byte	0xe
+	.uleb128 0x10
+	.byte	0x86
+	.uleb128 0x2
+	.byte	0x4
+	.long	.LCFI4-.LCFI3
+	.byte	0xd
+	.uleb128 0x6
+	.byte	0x4
+	.long	.LCFI5-.LCFI4
+	.byte	0xc
+	.uleb128 0x7
+	.uleb128 0x8
+	.align 8
+.LEFDE9:
+.LSFDE11:
+	.long	.LEFDE11-.LASFDE11
+.LASFDE11:
+	.long	.LASFDE11-.Lframe1
+	.long	.LFB2
+	.long	.LFE2-.LFB2
+	.uleb128 0
+	.byte	0x4
+	.long	.LCFI6-.LFB2
+	.byte	0xe
+	.uleb128 0x10
+	.byte	0x86
+	.uleb128 0x2
+	.byte	0x4
+	.long	.LCFI7-.LCFI6
+	.byte	0xd
+	.uleb128 0x6
+	.byte	0x4
+	.long	.LCFI8-.LCFI7
+	.byte	0xc
+	.uleb128 0x7
+	.uleb128 0x8
+	.align 8
+.LEFDE11:
+	.text
+.Letext0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.long	0x8c
+	.value	0x2
+	.long	.Ldebug_abbrev0
+	.byte	0x8
+	.uleb128 0x1
+	.long	.LASF2
+	.byte	0x1
+	.long	.LASF3
+	.long	.LASF4
+	.quad	.Ltext0
+	.quad	.Letext0
+	.long	.Ldebug_line0
+	.uleb128 0x2
+	.byte	0x1
+	.long	.LASF0
+	.byte	0x1
+	.byte	0x12
+	.quad	.LFB0
+	.quad	.LFE0
+	.long	.LLST0
+	.byte	0x1
+	.uleb128 0x3
+	.byte	0x1
+	.long	.LASF1
+	.byte	0x1
+	.byte	0x19
+	.quad	.LFB1
+	.quad	.LFE1
+	.long	.LLST1
+	.byte	0x1
+	.uleb128 0x4
+	.byte	0x1
+	.long	.LASF5
+	.byte	0x1
+	.byte	0x1f
+	.long	0x88
+	.quad	.LFB2
+	.quad	.LFE2
+	.long	.LLST2
+	.byte	0x1
+	.uleb128 0x5
+	.byte	0x4
+	.byte	0x5
+	.string	"int"
+	.byte	0
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.uleb128 0x1
+	.uleb128 0x11
+	.byte	0x1
+	.uleb128 0x25
+	.uleb128 0xe
+	.uleb128 0x13
+	.uleb128 0xb
+	.uleb128 0x3
+	.uleb128 0xe
+	.uleb128 0x1b
+	.uleb128 0xe
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x1
+	.uleb128 0x10
+	.uleb128 0x6
+	.byte	0
+	.byte	0
+	.uleb128 0x2
+	.uleb128 0x2e
+	.byte	0
+	.uleb128 0x3f
+	.uleb128 0xc
+	.uleb128 0x3
+	.uleb128 0xe
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x1
+	.uleb128 0x40
+	.uleb128 0x6
+	.uleb128 0x2117
+	.uleb128 0xc
+	.byte	0
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x2e
+	.byte	0
+	.uleb128 0x3f
+	.uleb128 0xc
+	.uleb128 0x3
+	.uleb128 0xe
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x1
+	.uleb128 0x40
+	.uleb128 0x6
+	.uleb128 0x2116
+	.uleb128 0xc
+	.byte	0
+	.byte	0
+	.uleb128 0x4
+	.uleb128 0x2e
+	.byte	0
+	.uleb128 0x3f
+	.uleb128 0xc
+	.uleb128 0x3
+	.uleb128 0xe
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x1
+	.uleb128 0x40
+	.uleb128 0x6
+	.uleb128 0x2116
+	.uleb128 0xc
+	.byte	0
+	.byte	0
+	.uleb128 0x5
+	.uleb128 0x24
+	.byte	0
+	.uleb128 0xb
+	.uleb128 0xb
+	.uleb128 0x3e
+	.uleb128 0xb
+	.uleb128 0x3
+	.uleb128 0x8
+	.byte	0
+	.byte	0
+	.byte	0
+	.section	.debug_loc,"",@progbits
+.Ldebug_loc0:
+.LLST0:
+	.quad	.LFB0-.Ltext0
+	.quad	.LCFI0-.Ltext0
+	.value	0x2
+	.byte	0x77
+	.sleb128 8
+	.quad	.LCFI0-.Ltext0
+	.quad	.LCFI1-.Ltext0
+	.value	0x2
+	.byte	0x77
+	.sleb128 16
+	.quad	.LCFI1-.Ltext0
+	.quad	.LCFI2-.Ltext0
+	.value	0x2
+	.byte	0x76
+	.sleb128 16
+	.quad	.LCFI2-.Ltext0
+	.quad	.LFE0-.Ltext0
+	.value	0x2
+	.byte	0x77
+	.sleb128 8
+	.quad	0
+	.quad	0
+.LLST1:
+	.quad	.LFB1-.Ltext0
+	.quad	.LCFI3-.Ltext0
+	.value	0x2
+	.byte	0x77
+	.sleb128 8
+	.quad	.LCFI3-.Ltext0
+	.quad	.LCFI4-.Ltext0
+	.value	0x2
+	.byte	0x77
+	.sleb128 16
+	.quad	.LCFI4-.Ltext0
+	.quad	.LCFI5-.Ltext0
+	.value	0x2
+	.byte	0x76
+	.sleb128 16
+	.quad	.LCFI5-.Ltext0
+	.quad	.LFE1-.Ltext0
+	.value	0x2
+	.byte	0x77
+	.sleb128 8
+	.quad	0
+	.quad	0
+.LLST2:
+	.quad	.LFB2-.Ltext0
+	.quad	.LCFI6-.Ltext0
+	.value	0x2
+	.byte	0x77
+	.sleb128 8
+	.quad	.LCFI6-.Ltext0
+	.quad	.LCFI7-.Ltext0
+	.value	0x2
+	.byte	0x77
+	.sleb128 16
+	.quad	.LCFI7-.Ltext0
+	.quad	.LCFI8-.Ltext0
+	.value	0x2
+	.byte	0x76
+	.sleb128 16
+	.quad	.LCFI8-.Ltext0
+	.quad	.LFE2-.Ltext0
+	.value	0x2
+	.byte	0x77
+	.sleb128 8
+	.quad	0
+	.quad	0
+	.section	.debug_aranges,"",@progbits
+	.long	0x2c
+	.value	0x2
+	.long	.Ldebug_info0
+	.byte	0x8
+	.byte	0
+	.value	0
+	.value	0
+	.quad	.Ltext0
+	.quad	.Letext0-.Ltext0
+	.quad	0
+	.quad	0
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.section	.debug_str,"MS",@progbits,1
+.LASF0:
+	.string	"stop_frame"
+.LASF3:
+	.string	"dw2-reg-undefined.c"
+.LASF2:
+	.string	"GNU C 4.7.2"
+.LASF1:
+	.string	"first_frame"
+.LASF5:
+	.string	"main"
+.LASF4:
+	.string	"/home/username/src/gdb/testsuite/gdb.dwarf2"
+	.ident	"GCC: (GNU) 4.7.2"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-dup-frame.c b/gdb/testsuite/gdb.dwarf2/dw2-dup-frame.c
new file mode 100644
index 0000000..4868d05
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-dup-frame.c
@@ -0,0 +1,36 @@
+/*
+   Copyright 2013 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/>.  */
+
+void
+stop_frame ()
+{
+  /* The debug information for this frame is modified in the accompanying
+     .S file, to mark a set of registers as being DW_CFA_same_value.  */
+}
+
+void
+first_frame ()
+{
+  stop_frame ();
+}
+
+int
+main ()
+{
+  first_frame ();
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-dup-frame.exp b/gdb/testsuite/gdb.dwarf2/dw2-dup-frame.exp
new file mode 100644
index 0000000..a57ab8a
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-dup-frame.exp
@@ -0,0 +1,44 @@
+# Copyright 2013 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 dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+# This test can only be run on x86_64 targets.
+if {![istarget "x86_64-*-*"] || ![is_lp64_target]} {
+    return 0
+}
+
+standard_testfile .S
+
+if { [prepare_for_testing $testfile.exp $testfile $srcfile {nodebug}] } {
+    return -1
+}
+
+if ![runto stop_frame] {
+    perror "Failed to stop in stop_frame"
+    return -1
+}
+
+gdb_test "bt" \
+    "#0  stop_frame \[^\r\n\]*\r\nBacktrace stopped: previous frame identical to this frame \\(corrupt stack\\?\\)" \
+    "backtrace from stop_frame"
+
+gdb_test "up" \
+    "Initial frame selected; you cannot go up\\\." \
+    "up from stop_frame"
-- 
1.7.11.7

Re: [PATCH 1/2] avoid infinite loop with bad debuginfo

[Pedro Alves]

On 11/18/2013 06:23 PM, Tom Tromey wrote:
>>> +	  if (VALUE_LVAL (new_val) == lval_register
>>> +	      && value_lazy (new_val)
>>> +	      && frame_id_eq (VALUE_FRAME_ID (new_val), last_frame_id))
> 
> Pedro> I think this should also check the regnum:
> 
> Barf.  I have a memory of actually writing that.  False memory I guess.
> Sigh.

Don't sigh.  :-)  I now believe the regnum check would be wrong.
This shouldn't return any register of the same frame.

WDYT of adjusting the patch like this?

------
From: Tom Tromey <tromey@redhat.com>
Subject: [PATCH] Detect lval_register handling infinite loop in
 value_fetch_lazy.

If value_fetch_lazy loops infinitely while unwrapping lval_register
values, it means we either somehow ended up with two frames with the
same ID in the frame chain, or some code is trying to unwind behind
get_prev_frame's back (e.g., a frame unwind sniffer trying to unwind).
In any case, it should always be an internal error to end up in this
situation.

This patch adds a check and throws an internal error if the same frame
is returned.

2013-11-22  Tom Tromey  <tromey@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	PR backtrace/16155
	* value.c (value_fetch_lazy): Internal error if
	get_frame_register_value returns the same register.
---
 gdb/value.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/gdb/value.c b/gdb/value.c
index 8c263ea..da7778f 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3507,7 +3507,9 @@ value_fetch_lazy (struct value *val)
 
       while (VALUE_LVAL (new_val) == lval_register && value_lazy (new_val))
 	{
-	  frame = frame_find_by_id (VALUE_FRAME_ID (new_val));
+	  struct frame_id frame_id = VALUE_FRAME_ID (new_val);
+
+	  frame = frame_find_by_id (frame_id);
 	  regnum = VALUE_REGNUM (new_val);
 
 	  gdb_assert (frame != NULL);
@@ -3521,6 +3523,22 @@ value_fetch_lazy (struct value *val)
 						   regnum, type));
 
 	  new_val = get_frame_register_value (frame, regnum);
+
+	  /* If we get another lazy lval_register value, it means the
+	     register is found by reading it from the next frame.
+	     get_frame_register_value should never return a value with
+	     the frame id pointing to FRAME.  If it does, it means we
+	     either have two consecutive frames with the same frame id
+	     in the frame chain, or some code is trying to unwind
+	     behind get_prev_frame's back (e.g., a frame unwind
+	     sniffer trying to unwind), bypassing its validations.  In
+	     any case, it should always be an internal error to end up
+	     in this situation.  */
+	  if (VALUE_LVAL (new_val) == lval_register
+	      && value_lazy (new_val)
+	      && frame_id_eq (VALUE_FRAME_ID (new_val), frame_id))
+	    internal_error (__FILE__, __LINE__,
+			    _("infinite loop while fetching a register"));
 	}
 
       /* If it's still lazy (for instance, a saved register on the
-- 
1.7.11.7

Re: [PATCH 1/2] avoid infinite loop with bad debuginfo

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Tom> Barf.  I have a memory of actually writing that.  False memory I guess.
Tom> Sigh.

Pedro> Don't sigh.  :-)  I now believe the regnum check would be wrong.

Haha, then it is double wrong, since I thought I wrote it that way :)

Pedro> This shouldn't return any register of the same frame.

Pedro> WDYT of adjusting the patch like this?

It looks good to me.  Please put it in.

I tried current git master with my original test case and I see it is
fixed.  So, thanks :)

Tom

Re: [PATCH 1/2] avoid infinite loop with bad debuginfo

[Pedro Alves]

On 11/22/2013 05:06 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Tom> Barf.  I have a memory of actually writing that.  False memory I guess.
> Tom> Sigh.
> 
> Pedro> Don't sigh.  :-)  I now believe the regnum check would be wrong.
> 
> Haha, then it is double wrong, since I thought I wrote it that way :)

:-)

> Pedro> This shouldn't return any register of the same frame.
> 
> Pedro> WDYT of adjusting the patch like this?
> 
> It looks good to me.  Please put it in.

Pushed.

> I tried current git master with my original test case and I see it is
> fixed.  So, thanks :)

Awesome!

-- 
Pedro Alves

Re: [PATCH 2/2] handle an unspecified return address column

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> Debugging PR 16155 further, I found that the DWARF unwinder found the
Tom> function in question, but thought it had no registers saved
Tom> (fs->regs.num_regs == 0).

Tom> It seems to me that if a frame does not specify the return address
Tom> column, or if the return address column is explicitly marked as
Tom> DWARF2_FRAME_REG_UNSPECIFIED, then we should set the
Tom> "undefined_retaddr" flag and let the DWARF unwinder gracefully stop.

Tom> This patch implements that idea.

I'm going to check this in shortly.

Tom

Re: [PATCH 2/2] handle an unspecified return address column

[Joel Brobecker]

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

Hi Tom,

On Wed, Nov 13, 2013 at 01:51:13PM -0700, Tom Tromey wrote:
> Debugging PR 16155 further, I found that the DWARF unwinder found the
> function in question, but thought it had no registers saved
> (fs->regs.num_regs == 0).
> 
> It seems to me that if a frame does not specify the return address
> column, or if the return address column is explicitly marked as
> DWARF2_FRAME_REG_UNSPECIFIED, then we should set the
> "undefined_retaddr" flag and let the DWARF unwinder gracefully stop.
> 
> This patch implements that idea.
> 
> With this patch the backtrace works properly:
> 
>     (gdb) bt
>     #0  0x0000007fb7ed485c in nanosleep () from /lib64/libc.so.6
>     #1  0x0000007fb7ed4508 in sleep () from /lib64/libc.so.6
>     #2  0x00000000004008bc in thread_function (arg=0x4) at threadapply.c:73
>     #3  0x0000007fb7fad950 in start_thread () from /lib64/libpthread.so.0
>     #4  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
> 
> 2013-11-13  Tom Tromey  <tromey@redhat.com>
> 
> 	PR backtrace/16155:
> 	* dwarf2-frame.c (dwarf2_frame_cache): Set undefined_retaddr if
> 	the return address column is unspecified.

I just found out that this patch causes some problems on at least
arm-elf and ppc-elf. Attached is a proposed patch, with associated
analysis.

gdb/ChangeLog:

        PR backtrace/16155:
        * dwarf2-frame.c (dwarf2_frame_cache): Remove condition that
        sets cache->undefined_retaddr to 1 if there is no column in
        the frame info for the return register.

Tested on x86_64-linux, no regression. It also fixes all regressions
observed on arm-elf and ppc-elf.

Can you tell me what you think?

Thank you!
-- 
Joel

[-- Attachment #2: 0001-DWARF-cannot-unwind-from-leaf-functions-that-do-not-.patch --]
[-- Type: text/x-diff, Size: 3581 bytes --]

From f01867f9f4fdd55d7692d09ae1d4622968e2bb04 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Tue, 26 Nov 2013 17:08:03 +0400
Subject: [PATCH] DWARF: cannot unwind from leaf functions that do not touch
 the link register

I noticed this problem on arm-eabi and on ppc-elf. Consider this
simple function:

   function F return Num is
   begin
      return 1.0 + 2.0;
   end F;

Trying to unwind from it on ppc-elf, no longer works. Consider for
instance the following setup, where we are about to call our function
F above:

    (gdb) tar sim -e bug -r 0x400000
    (gdb) load x
    (gdb) start
    [...]
    Temporary breakpoint 1, x () at x.adb:4
    4          Z : constant Num := F;

At this point, let's see what happens to our backtrace once we've
stepped inside F:

    (gdb) s
    pck.f () at pck.adb:2
    2          function F return Num is
    (gdb) bt
    #0  pck.f () at pck.adb:2

As you can see, the backtrace is truncated. We're missing the frame
corresponding to the main subprogram "x".

This is due to a recent change, which added the following check:

    +  else if (fs->retaddr_column >= fs->regs.num_regs
    +     || (fs->regs.reg[fs->retaddr_column].how
    +         == DWARF2_FRAME_REG_UNSPECIFIED))
    +    cache->undefined_retaddr = 1;

The rationale was:

    It seems to me that if a frame does not specify the return address
    column, or if the return address column is explicitly marked as
    DWARF2_FRAME_REG_UNSPECIFIED, then we should set the
    "undefined_retaddr" flag and let the DWARF unwinder gracefully stop.

In our case, we're tripping the first half of the condition.
Looking at the frame info with readelf, for instance, we see:

| 00000000 0000000c ffffffff CIE "" cf=4 df=-4 ra=65
|    LOC   CFA
| 00000000 r1+0
|
| 00000010 00000020 00000000 FDE cie=00000000 pc=00000000..0000002c
|    LOC   CFA      r31
| 00000000 r1+0     u
| 00000004 r1+16    u
| [etc]

Here, the return address register is number 65, and there is no
column for it. I think that this meant to be decoded as "ra never
changes throughout the lifetime of the function, so no column
needed for it in the CFI". Regardless of correctness, this appears
to be a practice that's established enough that we should handle it
gracefully, as we used to.

This patch therefore changes a bit the logic to remove the part
of the condition that would cause the unwinding to terminate when
the CFI has no column corresponding to the return address register.

gdb/ChangeLog:

        PR backtrace/16155:
        * dwarf2-frame.c (dwarf2_frame_cache): Remove condition that
        sets cache->undefined_retaddr to 1 if there is no column in
        the frame info for the return register.
---
 gdb/dwarf2-frame.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index f185ca6..9e9c32b 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1243,11 +1243,9 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"),
   }
 
   if (fs->retaddr_column < fs->regs.num_regs
-      && fs->regs.reg[fs->retaddr_column].how == DWARF2_FRAME_REG_UNDEFINED)
-    cache->undefined_retaddr = 1;
-  else if (fs->retaddr_column >= fs->regs.num_regs
-  	  || (fs->regs.reg[fs->retaddr_column].how
-  	      == DWARF2_FRAME_REG_UNSPECIFIED))
+      && (fs->regs.reg[fs->retaddr_column].how == DWARF2_FRAME_REG_UNDEFINED
+	  || (fs->regs.reg[fs->retaddr_column].how
+	      == DWARF2_FRAME_REG_UNSPECIFIED)))
     cache->undefined_retaddr = 1;
 
   do_cleanups (old_chain);
-- 
1.8.1.2

Re: [PATCH 2/2] handle an unspecified return address column

[Mark Kettenis]

> Date: Tue, 26 Nov 2013 17:34:46 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> 
> Hi Tom,
> 
> On Wed, Nov 13, 2013 at 01:51:13PM -0700, Tom Tromey wrote:
> > Debugging PR 16155 further, I found that the DWARF unwinder found the
> > function in question, but thought it had no registers saved
> > (fs->regs.num_regs == 0).
> > 
> > It seems to me that if a frame does not specify the return address
> > column, or if the return address column is explicitly marked as
> > DWARF2_FRAME_REG_UNSPECIFIED, then we should set the
> > "undefined_retaddr" flag and let the DWARF unwinder gracefully stop.
> > 
> > This patch implements that idea.
> > 
> > With this patch the backtrace works properly:
> > 
> >     (gdb) bt
> >     #0  0x0000007fb7ed485c in nanosleep () from /lib64/libc.so.6
> >     #1  0x0000007fb7ed4508 in sleep () from /lib64/libc.so.6
> >     #2  0x00000000004008bc in thread_function (arg=0x4) at threadapply.c:73
> >     #3  0x0000007fb7fad950 in start_thread () from /lib64/libpthread.so.0
> >     #4  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
> > 
> > 2013-11-13  Tom Tromey  <tromey@redhat.com>
> > 
> > 	PR backtrace/16155:
> > 	* dwarf2-frame.c (dwarf2_frame_cache): Set undefined_retaddr if
> > 	the return address column is unspecified.
> 
> I just found out that this patch causes some problems on at least
> arm-elf and ppc-elf. Attached is a proposed patch, with associated
> analysis.
> 
> gdb/ChangeLog:
> 
>         PR backtrace/16155:
>         * dwarf2-frame.c (dwarf2_frame_cache): Remove condition that
>         sets cache->undefined_retaddr to 1 if there is no column in
>         the frame info for the return register.
> 
> Tested on x86_64-linux, no regression. It also fixes all regressions
> observed on arm-elf and ppc-elf.
> 
> Can you tell me what you think?

Please start with backing out the original change.

Re: [PATCH 2/2] handle an unspecified return address column

[Joel Brobecker]

> > gdb/ChangeLog:
> > 
> >         PR backtrace/16155:
> >         * dwarf2-frame.c (dwarf2_frame_cache): Remove condition that
> >         sets cache->undefined_retaddr to 1 if there is no column in
> >         the frame info for the return register.
> > 
> > Tested on x86_64-linux, no regression. It also fixes all regressions
> > observed on arm-elf and ppc-elf.
> > 
> > Can you tell me what you think?
> 
> Please start with backing out the original change.

I would gladly do so, but can you explain the rationale behind your
request? Is it to facilitate review of this patch? Or is it because
you think all of the original patch needs to go? I felt that the patch
just overachieved a bit from what it initially set out to do (detect
unspecified return registers), and so I felt it was ok to send a
followup rather than redo it entirely.

-- 
Joel

Re: [PATCH 2/2] handle an unspecified return address column

[Mark Kettenis]

> Date: Tue, 26 Nov 2013 17:55:19 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > > gdb/ChangeLog:
> > > 
> > >         PR backtrace/16155:
> > >         * dwarf2-frame.c (dwarf2_frame_cache): Remove condition that
> > >         sets cache->undefined_retaddr to 1 if there is no column in
> > >         the frame info for the return register.
> > > 
> > > Tested on x86_64-linux, no regression. It also fixes all regressions
> > > observed on arm-elf and ppc-elf.
> > > 
> > > Can you tell me what you think?
> > 
> > Please start with backing out the original change.
> 
> I would gladly do so, but can you explain the rationale behind your
> request? Is it to facilitate review of this patch? Or is it because
> you think all of the original patch needs to go? I felt that the patch
> just overachieved a bit from what it initially set out to do (detect
> unspecified return registers), and so I felt it was ok to send a
> followup rather than redo it entirely.

Pretty much both.  The original diff was clearly wrong, and it is best
to have the history reflect that clearly.  But it will also make the
review easier.  To be honest, I think the conditional, after your
modification, is too confusing.

Didn't realize that the tests were part of the same commit though.  So
you probably can't simply use git revert.  Perhaps that means we
should commit testsuite changes seperately in the future.

Re: [PATCH 2/2] handle an unspecified return address column

[Joel Brobecker]

> Pretty much both.  The original diff was clearly wrong, and it is best
> to have the history reflect that clearly.  But it will also make the
> review easier.  To be honest, I think the conditional, after your
> modification, is too confusing.

OK. It's Tom's commit, so I will wait for his input before undoing
the change.

> Didn't realize that the tests were part of the same commit though.  So
> you probably can't simply use git revert.  Perhaps that means we
> should commit testsuite changes seperately in the future.

It's easy to undo just part of the commit, still keeping the testcase.
We can't use "git revert" anyways, because of those $(&! ChangeLog
files, nearly always triggerring revert conflict. And the sad part
is that we actually want those, because we do not want to undo the
ChangeLog entry!

-- 
Joel

Re: [PATCH 2/2] handle an unspecified return address column

[Tom Tromey]

>>>>> "Mark" == Mark Kettenis <mark.kettenis@xs4all.nl> writes:

Mark> Didn't realize that the tests were part of the same commit though.  So
Mark> you probably can't simply use git revert.  Perhaps that means we
Mark> should commit testsuite changes seperately in the future.

I think it's correct to revert the test suite.
It's a bad idea to leave failing tests in the tree.

I'll revert it shortly.

Tom

Re: [PATCH 2/2] handle an unspecified return address column

[Tom Tromey]

Mark> Didn't realize that the tests were part of the same commit though.  So
Mark> you probably can't simply use git revert.  Perhaps that means we
Mark> should commit testsuite changes seperately in the future.

Note we can't use git revert anyway as that also reverts the ChangeLog
entries.

Tom

Re: [PATCH 2/2] handle an unspecified return address column

[Tom Tromey]

Mark> Please start with backing out the original change.

Here's the reversion.
Committed.

Tom

commit 158599681f7c57e4d233a3e14c2e01faeaae55aa
Author: Tom Tromey <tromey@redhat.com>
Date:   Tue Nov 26 07:47:56 2013 -0700

    revert patch from 2013-11-22
    
    This reverts da2b2fdf57a96f7a5b6b153e94afb747e212b17f and some
    follow-up patches.  They were incorrect.
    
    2013-11-26  Tom Tromey  <tromey@redhat.com>
    
    	* dwarf2-frame.c (dwarf2_frame_cache): Revert patch from
    	2013-11-22.
    
    2013-11-26  Tom Tromey  <tromey@redhat.com>
    
    	* gdb.dwarf2/dw2-unspecified-ret-addr.S: Remove.
    	* gdb.dwarf2/dw2-unspecified-ret-addr.c: Remove.
    	* gdb.dwarf2/dw2-unspecified-ret-addr.exp: Remove.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index eb30c9d..7e28436 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2013-11-26  Tom Tromey  <tromey@redhat.com>
+
+	* dwarf2-frame.c (dwarf2_frame_cache): Revert patch from
+	2013-11-22.
+
 2013-11-26  Walfred Tedeschi  <walfred.tedeschi@intel.com>
 
 	* i386-xstate.h (I386_XSTATE_MPX): New Macro.
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index f185ca6..c4f8771 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1245,10 +1245,6 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"),
   if (fs->retaddr_column < fs->regs.num_regs
       && fs->regs.reg[fs->retaddr_column].how == DWARF2_FRAME_REG_UNDEFINED)
     cache->undefined_retaddr = 1;
-  else if (fs->retaddr_column >= fs->regs.num_regs
-  	  || (fs->regs.reg[fs->retaddr_column].how
-  	      == DWARF2_FRAME_REG_UNSPECIFIED))
-    cache->undefined_retaddr = 1;
 
   do_cleanups (old_chain);
   discard_cleanups (reset_cache_cleanup);
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 7d03d49..9438f48 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2013-11-26  Tom Tromey  <tromey@redhat.com>
+
+	* gdb.dwarf2/dw2-unspecified-ret-addr.S: Remove.
+	* gdb.dwarf2/dw2-unspecified-ret-addr.c: Remove.
+	* gdb.dwarf2/dw2-unspecified-ret-addr.exp: Remove.
+
 2013-11-25  Keith Seitz  <keiths@redhat.com>
 
 	PR c++/14819
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-unspecified-ret-addr.S b/gdb/testsuite/gdb.dwarf2/dw2-unspecified-ret-addr.S
deleted file mode 100644
index d9c2809..0000000
--- a/gdb/testsuite/gdb.dwarf2/dw2-unspecified-ret-addr.S
+++ /dev/null
@@ -1,217 +0,0 @@
-# This file was created by running dw2-unspecified-ret-addr.c
-# through 'gcc -g -dA -S', and then hand-editing
-# the CFI in "callee" to suit.
-
-	.file	"dw2-unspecified-ret-addr.c"
-	.text
-.Ltext0:
-	.globl	callee
-	.type	callee, @function
-callee:
-.LFB0:
-	.file 1 "dw2-unspecified-ret-addr.c"
-	# dw2-unspecified-ret-addr.c:20
-	.loc 1 20 0
-	.cfi_startproc
-# BLOCK 2 seq:0
-# PRED: ENTRY (fallthru)
-	pushq	%rbp
-/*
-	.cfi_def_cfa_offset 16
-	.cfi_offset 6, -16
-*/
-	movq	%rsp, %rbp
-/*	.cfi_def_cfa_register 6 */
-	# dw2-unspecified-ret-addr.c:21
-	.loc 1 21 0
-	popq	%rbp
-/*	.cfi_def_cfa 7, 8 */
-	/* The bug we introduce is that the return address column is
-	   unspecified.  In this case, there is no way to unwind.  */
-	.cfi_undefined 6
-	.cfi_return_column 6
-# SUCC: EXIT [100.0%] 
-	ret
-	.cfi_endproc
-.LFE0:
-	.size	callee, .-callee
-	.globl	main
-	.type	main, @function
-main:
-.LFB1:
-	# dw2-unspecified-ret-addr.c:25
-	.loc 1 25 0
-	.cfi_startproc
-# BLOCK 2 seq:0
-# PRED: ENTRY (fallthru)
-	pushq	%rbp
-	.cfi_def_cfa_offset 16
-	.cfi_offset 6, -16
-	movq	%rsp, %rbp
-	.cfi_def_cfa_register 6
-	# dw2-unspecified-ret-addr.c:26
-	.loc 1 26 0
-	call	callee
-	# dw2-unspecified-ret-addr.c:27
-	.loc 1 27 0
-	movl	$0, %eax
-	# dw2-unspecified-ret-addr.c:28
-	.loc 1 28 0
-	popq	%rbp
-	.cfi_def_cfa 7, 8
-# SUCC: EXIT [100.0%] 
-	ret
-	.cfi_endproc
-.LFE1:
-	.size	main, .-main
-.Letext0:
-	.section	.debug_info,"",@progbits
-.Ldebug_info0:
-	.long	0x67	# Length of Compilation Unit Info
-	.value	0x4	# DWARF version number
-	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
-	.byte	0x8	# Pointer Size (in bytes)
-	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
-	.long	.LASF0	# DW_AT_producer: "GNU C 4.7.2 20121109 (Red Hat 4.7.2-8) -mtune=generic -march=x86-64 -g"
-	.byte	0x1	# DW_AT_language
-	.long	.LASF1	# DW_AT_name: "dw2-unspecified-ret-addr.c"
-	.long	.LASF2	# DW_AT_comp_dir: "/home/tromey/gnu/gdb/devel/binutils-gdb/gdb/testsuite/gdb.dwarf2"
-	.quad	.Ltext0	# DW_AT_low_pc
-	.quad	.Letext0	# DW_AT_high_pc
-	.long	.Ldebug_line0	# DW_AT_stmt_list
-	.uleb128 0x2	# (DIE (0x2d) DW_TAG_subprogram)
-			# DW_AT_external
-	.long	.LASF3	# DW_AT_name: "callee"
-	.byte	0x1	# DW_AT_decl_file (dw2-unspecified-ret-addr.c)
-	.byte	0x13	# DW_AT_decl_line
-			# DW_AT_prototyped
-	.quad	.LFB0	# DW_AT_low_pc
-	.quad	.LFE0	# DW_AT_high_pc
-	.uleb128 0x1	# DW_AT_frame_base
-	.byte	0x9c	# DW_OP_call_frame_cfa
-			# DW_AT_GNU_all_call_sites
-	.uleb128 0x3	# (DIE (0x46) DW_TAG_subprogram)
-			# DW_AT_external
-	.long	.LASF4	# DW_AT_name: "main"
-	.byte	0x1	# DW_AT_decl_file (dw2-unspecified-ret-addr.c)
-	.byte	0x18	# DW_AT_decl_line
-			# DW_AT_prototyped
-	.long	0x63	# DW_AT_type
-	.quad	.LFB1	# DW_AT_low_pc
-	.quad	.LFE1	# DW_AT_high_pc
-	.uleb128 0x1	# DW_AT_frame_base
-	.byte	0x9c	# DW_OP_call_frame_cfa
-			# DW_AT_GNU_all_tail_call_sites
-	.uleb128 0x4	# (DIE (0x63) DW_TAG_base_type)
-	.byte	0x4	# DW_AT_byte_size
-	.byte	0x5	# DW_AT_encoding
-	.ascii "int\0"	# DW_AT_name
-	.byte	0	# end of children of DIE 0xb
-	.section	.debug_abbrev,"",@progbits
-.Ldebug_abbrev0:
-	.uleb128 0x1	# (abbrev code)
-	.uleb128 0x11	# (TAG: DW_TAG_compile_unit)
-	.byte	0x1	# DW_children_yes
-	.uleb128 0x25	# (DW_AT_producer)
-	.uleb128 0xe	# (DW_FORM_strp)
-	.uleb128 0x13	# (DW_AT_language)
-	.uleb128 0xb	# (DW_FORM_data1)
-	.uleb128 0x3	# (DW_AT_name)
-	.uleb128 0xe	# (DW_FORM_strp)
-	.uleb128 0x1b	# (DW_AT_comp_dir)
-	.uleb128 0xe	# (DW_FORM_strp)
-	.uleb128 0x11	# (DW_AT_low_pc)
-	.uleb128 0x1	# (DW_FORM_addr)
-	.uleb128 0x12	# (DW_AT_high_pc)
-	.uleb128 0x1	# (DW_FORM_addr)
-	.uleb128 0x10	# (DW_AT_stmt_list)
-	.uleb128 0x17	# (DW_FORM_sec_offset)
-	.byte	0
-	.byte	0
-	.uleb128 0x2	# (abbrev code)
-	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
-	.byte	0	# DW_children_no
-	.uleb128 0x3f	# (DW_AT_external)
-	.uleb128 0x19	# (DW_FORM_flag_present)
-	.uleb128 0x3	# (DW_AT_name)
-	.uleb128 0xe	# (DW_FORM_strp)
-	.uleb128 0x3a	# (DW_AT_decl_file)
-	.uleb128 0xb	# (DW_FORM_data1)
-	.uleb128 0x3b	# (DW_AT_decl_line)
-	.uleb128 0xb	# (DW_FORM_data1)
-	.uleb128 0x27	# (DW_AT_prototyped)
-	.uleb128 0x19	# (DW_FORM_flag_present)
-	.uleb128 0x11	# (DW_AT_low_pc)
-	.uleb128 0x1	# (DW_FORM_addr)
-	.uleb128 0x12	# (DW_AT_high_pc)
-	.uleb128 0x1	# (DW_FORM_addr)
-	.uleb128 0x40	# (DW_AT_frame_base)
-	.uleb128 0x18	# (DW_FORM_exprloc)
-	.uleb128 0x2117	# (DW_AT_GNU_all_call_sites)
-	.uleb128 0x19	# (DW_FORM_flag_present)
-	.byte	0
-	.byte	0
-	.uleb128 0x3	# (abbrev code)
-	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
-	.byte	0	# DW_children_no
-	.uleb128 0x3f	# (DW_AT_external)
-	.uleb128 0x19	# (DW_FORM_flag_present)
-	.uleb128 0x3	# (DW_AT_name)
-	.uleb128 0xe	# (DW_FORM_strp)
-	.uleb128 0x3a	# (DW_AT_decl_file)
-	.uleb128 0xb	# (DW_FORM_data1)
-	.uleb128 0x3b	# (DW_AT_decl_line)
-	.uleb128 0xb	# (DW_FORM_data1)
-	.uleb128 0x27	# (DW_AT_prototyped)
-	.uleb128 0x19	# (DW_FORM_flag_present)
-	.uleb128 0x49	# (DW_AT_type)
-	.uleb128 0x13	# (DW_FORM_ref4)
-	.uleb128 0x11	# (DW_AT_low_pc)
-	.uleb128 0x1	# (DW_FORM_addr)
-	.uleb128 0x12	# (DW_AT_high_pc)
-	.uleb128 0x1	# (DW_FORM_addr)
-	.uleb128 0x40	# (DW_AT_frame_base)
-	.uleb128 0x18	# (DW_FORM_exprloc)
-	.uleb128 0x2116	# (DW_AT_GNU_all_tail_call_sites)
-	.uleb128 0x19	# (DW_FORM_flag_present)
-	.byte	0
-	.byte	0
-	.uleb128 0x4	# (abbrev code)
-	.uleb128 0x24	# (TAG: DW_TAG_base_type)
-	.byte	0	# DW_children_no
-	.uleb128 0xb	# (DW_AT_byte_size)
-	.uleb128 0xb	# (DW_FORM_data1)
-	.uleb128 0x3e	# (DW_AT_encoding)
-	.uleb128 0xb	# (DW_FORM_data1)
-	.uleb128 0x3	# (DW_AT_name)
-	.uleb128 0x8	# (DW_FORM_string)
-	.byte	0
-	.byte	0
-	.byte	0
-	.section	.debug_aranges,"",@progbits
-	.long	0x2c	# Length of Address Ranges Info
-	.value	0x2	# DWARF Version
-	.long	.Ldebug_info0	# Offset of Compilation Unit Info
-	.byte	0x8	# Size of Address
-	.byte	0	# Size of Segment Descriptor
-	.value	0	# Pad to 16 byte boundary
-	.value	0
-	.quad	.Ltext0	# Address
-	.quad	.Letext0-.Ltext0	# Length
-	.quad	0
-	.quad	0
-	.section	.debug_line,"",@progbits
-.Ldebug_line0:
-	.section	.debug_str,"MS",@progbits,1
-.LASF3:
-	.string	"callee"
-.LASF1:
-	.string	"dw2-unspecified-ret-addr.c"
-.LASF2:
-	.string	"/home/tromey/gnu/gdb/devel/binutils-gdb/gdb/testsuite/gdb.dwarf2"
-.LASF4:
-	.string	"main"
-.LASF0:
-	.string	"GNU C 4.7.2 20121109 (Red Hat 4.7.2-8) -mtune=generic -march=x86-64 -g"
-	.ident	"GCC: (GNU) 4.7.2 20121109 (Red Hat 4.7.2-8)"
-	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-unspecified-ret-addr.c b/gdb/testsuite/gdb.dwarf2/dw2-unspecified-ret-addr.c
deleted file mode 100644
index f5b415b..0000000
--- a/gdb/testsuite/gdb.dwarf2/dw2-unspecified-ret-addr.c
+++ /dev/null
@@ -1,28 +0,0 @@
-/* This testcase is part of GDB, the GNU debugger.
-
-   Copyright 2013 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/>.  */
-
-void
-callee (void)
-{
-}
-
-int
-main(void)
-{
-  callee ();
-  return 0;
-}
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-unspecified-ret-addr.exp b/gdb/testsuite/gdb.dwarf2/dw2-unspecified-ret-addr.exp
deleted file mode 100644
index 12b1275..0000000
--- a/gdb/testsuite/gdb.dwarf2/dw2-unspecified-ret-addr.exp
+++ /dev/null
@@ -1,42 +0,0 @@
-# Copyright 2013 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 dwarf.exp
-
-standard_testfile .S
-
-# This test can only be run on targets which support DWARF-2 and use gas.
-if ![dwarf2_support] {
-    return 0  
-}
-
-# This test can only be run on x86-64 targets.
-if {![istarget x86_64-*] || ![is_lp64_target]} {
-    return 0
-}
-
-if {[prepare_for_testing "$testfile.exp" "$testfile" $srcfile {nodebug}]} {
-    return -1
-}
-
-if ![runto_main] {
-    return -1
-}
-
-gdb_breakpoint "callee"
-gdb_continue_to_breakpoint "callee"
-
-# This test ensures that the backtrace does not go past "callee".
-gdb_test "bt" ".*callee .. at [file rootname $srcfile]\\.c:\[0-9\]*"

Re: [PATCH 2/2] handle an unspecified return address column

[Joel Brobecker]

> Mark> Please start with backing out the original change.
> 
> Here's the reversion.
> Committed.

Thanks, Tom. Do not hesitate to ping me if you'd like to revisit
this issue.

-- 
Joel