[PATCH] Re: Unwinding CFI gcc practice of assumed `same value' regs

[PATCH] Re: Unwinding CFI gcc practice of assumed `same value' regs

[Jan Kratochvil]

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

Hi,

On Tue, 12 Dec 2006 16:52:33 +0100, Jakub Jelinek wrote:
...
> Here is something that would handle by default same_value retaddr_column:
[ http://sources.redhat.com/ml/gdb/2006-12/msg00100.html ]

Thanks for this backward compatible glibc unwinder patch.  I wish to have it
accepted as a step in preparing the environment to use `.cfi_undefined PC'
sometimes in the future.

Attaching patch for current glibc CVS which removes the `.cfi_undefined PC'
unwinder handling requirement but which provides explicit return address 0 from
the `__clone' function.  Currently the 0 is already present there but it is
uninitialized value out of some TLS or 'struct pthread' area (did not check).

Attaching patch for current gdb CVS to properly terminate on return address 0.
The check was already present there but it got applied one backward step later.

I hope these three patches are be 100% reliable and also backward compatible.


Regards,
Jan

[-- Attachment #2: glibc-unwind-compat2.patch --]
[-- Type: text/plain, Size: 2810 bytes --]

2006-12-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* sysdeps/unix/sysv/linux/i386/clone.S: CFI `clone' unwinding
	outermost frame indicator replaced by more unwinders compatible
	termination indication of `PC == 0'.
	* sysdeps/unix/sysv/linux/x86_64/clone.S: Likewise.

--- libc/sysdeps/unix/sysv/linux/i386/clone.S	3 Dec 2006 23:12:36 -0000	1.27
+++ libc/sysdeps/unix/sysv/linux/i386/clone.S	13 Dec 2006 11:20:55 -0000
@@ -68,6 +68,8 @@ ENTRY (BP_SYM (__clone))
 	   thread is started with an alignment of (mod 16).  */
 	andl	$0xfffffff0, %ecx
 	subl	$28,%ecx
+	/* Terminate the stack frame by pretended return address 0.  */
+	movl	$0,16(%ecx)
 	movl	ARG(%esp),%eax		/* no negative argument counts */
 	movl	%eax,12(%ecx)
 
@@ -121,10 +123,15 @@ L(pseudo_end):
 
 L(thread_start):
 	cfi_startproc;
-	/* Clearing frame pointer is insufficient, use CFI.  */
-	cfi_undefined (eip);
-	/* Note: %esi is zero.  */
-	movl	%esi,%ebp	/* terminate the stack frame */
+	/* This CFI recommended way of unwindable function is incompatible
+	   across unwinders incl. the libgcc_s one.
+	   cfi_undefined (eip);
+	   */
+	/* Frame pointer 0 was considered as the stack frame termination
+	   before but it is no longer valid for -fomit-frame-pointer code.
+	   Still keep the backward compatibility and clear the register.
+	   Note: %esi is zero.  */
+	movl	%esi,%ebp
 #ifdef RESET_PID
 	testl	$CLONE_THREAD, %edi
 	je	L(newpid)
--- libc/sysdeps/unix/sysv/linux/x86_64/clone.S	3 Dec 2006 23:12:36 -0000	1.7
+++ libc/sysdeps/unix/sysv/linux/x86_64/clone.S	13 Dec 2006 11:20:55 -0000
@@ -61,8 +61,12 @@ ENTRY (BP_SYM (__clone))
 	testq	%rsi,%rsi		/* no NULL stack pointers */
 	jz	SYSCALL_ERROR_LABEL
 
+	/* Prepare the data located at %rsp after `syscall' below.
+	   Used only 3*8 bytes but the stack is 16 bytes aligned.  */
+	subq	$32,%rsi
+	/* Terminate the stack frame by pretended return address 0.  */
+	movq	$0,16(%rsi)
 	/* Insert the argument onto the new stack.  */
-	subq	$16,%rsi
 	movq	%rcx,8(%rsi)
 
 	/* Save the function pointer.  It will be popped off in the
@@ -90,10 +94,15 @@ L(pseudo_end):
 
 L(thread_start):
 	cfi_startproc;
-	/* Clearing frame pointer is insufficient, use CFI.  */
-	cfi_undefined (rip);
-	/* Clear the frame pointer.  The ABI suggests this be done, to mark
-	   the outermost frame obviously.  */
+	/* This CFI recommended way of unwindable function is incompatible
+	   across unwinders incl. the libgcc_s one.
+	   cfi_undefined (rip);
+	   */
+	/* Frame pointer 0 was considered as the stack frame termination
+	   before but it is no longer valid for -fomit-frame-pointer code.
+	   Still keep the backward compatibility and clear the register,
+	   the ABI suggests this be done, to mark the outermost frame
+	   obviously.  */
 	xorl	%ebp, %ebp
 
 #ifdef RESET_PID

[-- Attachment #3: gdb-bt-clone-stop.patch --]
[-- Type: text/plain, Size: 4875 bytes --]

2006-12-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb/frame.c (get_prev_frame): Already the first `PC == 0' stack frame
	is declared invalid, not the second one as before.

2006-12-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.threads/bt-clone-stop.exp, gdb.threads/bt-clone-stop.c:
	Backtraced `clone' must not have `PC == 0' as its previous frame.


--- ./gdb/frame.c	10 Nov 2006 20:11:35 -0000	1.215
+++ ./gdb/frame.c	13 Dec 2006 19:06:19 -0000
@@ -1390,19 +1390,28 @@ get_prev_frame (struct frame_info *this_
       return NULL;
     }
 
+  prev_frame = get_prev_frame_1 (this_frame);
+  if (!prev_frame)
+    return NULL;
+
   /* Assume that the only way to get a zero PC is through something
      like a SIGSEGV or a dummy frame, and hence that NORMAL frames
-     will never unwind a zero PC.  */
-  if (this_frame->level > 0
+     will never unwind a zero PC.
+     This check must be done after retrieving the previous frame as we
+     would report the last PC == 0 frame:
+     	#4  0x00000036dd0c68c3 in clone () from /lib64/tls/libc.so.6
+     	#5  0x0000000000000000 in ?? ()
+     */
+  if (prev_frame->level > 0
+      && get_frame_type (prev_frame) == NORMAL_FRAME
       && get_frame_type (this_frame) == NORMAL_FRAME
-      && get_frame_type (get_next_frame (this_frame)) == NORMAL_FRAME
-      && get_frame_pc (this_frame) == 0)
+      && get_frame_pc (prev_frame) == 0)
     {
-      frame_debug_got_null_frame (gdb_stdlog, this_frame, "zero PC");
+      frame_debug_got_null_frame (gdb_stdlog, prev_frame, "zero PC");
       return NULL;
     }
 
-  return get_prev_frame_1 (this_frame);
+  return prev_frame;
 }
 
 CORE_ADDR
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ./gdb/testsuite/gdb.threads/bt-clone-stop.c	13 Dec 2006 19:06:19 -0000
@@ -0,0 +1,39 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2006 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 2 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, write to the Free Software
+   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+   MA 02110-1301, USA.  */
+
+
+#include <pthread.h>
+#include <unistd.h>
+#include <assert.h>
+
+
+void *threader(void *arg)
+{
+	assert(0);
+	return NULL;
+}
+
+int main()
+{
+	pthread_t t1;
+
+	pthread_create(&t1,NULL,threader,(void *)NULL);
+	for (;;)
+		pause();
+}
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ./gdb/testsuite/gdb.threads/bt-clone-stop.exp	13 Dec 2006 19:06:19 -0000
@@ -0,0 +1,56 @@
+# Copyright 2006 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 2 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, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set testfile bt-clone-stop
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if  { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested "Couldn't compile test program"
+    return -1
+}
+
+# Get things started.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# threader: threader.c:8: threader: Assertion `0' failed.
+# Program received signal SIGABRT, Aborted.
+
+gdb_test "run" \
+     "Program received signal SIGABRT.*" \
+     "run"
+
+# #5  0x0000003421ecd62d in ?? () from /lib64/libc.so.6
+# #6  0x0000000000000000 in ?? ()
+# (gdb)
+# Line `#6' must not be present
+# Keep those two cases in this order!
+# Fallback is some invalid output (FAIL).
+gdb_test_multiple "bt" "0x0 entry output invalid" {
+    -re "in threader \\(.*\n#\[0-9\]* *0x0* in .*$gdb_prompt $" {
+    	fail "0x0 entry found there"
+    }
+    -re "in threader \\(.*$gdb_prompt $" {
+    	pass "0x0 entry not found there"
+    }
+}

Re: [PATCH] Re: Unwinding CFI gcc practice of assumed `same value' regs

[Daniel Jacobowitz]

On Wed, Dec 13, 2006 at 09:46:04PM +0100, Jan Kratochvil wrote:
> Attaching patch for current gdb CVS to properly terminate on return address 0.
> The check was already present there but it got applied one backward step later.
> 
> I hope these three patches are be 100% reliable and also backward compatible.

Please read the previous discussion and patch on this topic:
  http://sourceware.org/ml/gdb-patches/2006-07/msg00392.html

Mark, I don't suppose you find it convincing that the libgcc unwinder
terminates on PC == 0 too?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Re: Unwinding CFI gcc practice of assumed `same value' regs

[Mark Kettenis]

>  On Wed, Dec 13, 2006 at 09:46:04PM +0100, Jan Kratochvil wrote:
> > Attaching patch for current gdb CVS to properly terminate on return
> > address 0.
> > The check was already present there but it got applied one backward step
> > later.
> >
> > I hope these three patches are be 100% reliable and also backward
> > compatible.
>
>  Please read the previous discussion and patch on this topic:
>    http://sourceware.org/ml/gdb-patches/2006-07/msg00392.html
>
>  Mark, I don't suppose you find it convincing that the libgcc unwinder
>  terminates on PC == 0 too?

It's a perfectly reasonable thing for the libgcc unwinder to do (just
as it is for us in GDB).  But in a debugger I want to see that PC == 0
if it is an abnormal backtrace termination.

What really needs to be done is making the DWARF2 unwinder recognize the
condition and return a suitable frame id for that.

Mark

Re: [PATCH] Re: Unwinding CFI gcc practice of assumed `same value' regs

[Daniel Jacobowitz]

On Wed, Dec 13, 2006 at 10:40:52PM +0100, Mark Kettenis wrote:
> It's a perfectly reasonable thing for the libgcc unwinder to do (just
> as it is for us in GDB).  But in a debugger I want to see that PC == 0
> if it is an abnormal backtrace termination.
> 
> What really needs to be done is making the DWARF2 unwinder recognize the
> condition and return a suitable frame id for that.

Sorry, but I'm confused again :-(

Do you mean making the DWARF2 unwinder decide that ra == 0 is a clean
terminator?  That seems like a bad thing to do if you want to ever show
PC == 0 as abnormal termination; then it will do so only if the last
frame left on the stack didn't have DWARF2 info.  Or do you mean some
other change to the unwinder?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Re: Unwinding CFI gcc practice of assumed `same value' regs

[Mark Kettenis]

>  On Wed, Dec 13, 2006 at 10:40:52PM +0100, Mark Kettenis wrote:
> > It's a perfectly reasonable thing for the libgcc unwinder to do (just
> > as it is for us in GDB).  But in a debugger I want to see that PC == 0
> > if it is an abnormal backtrace termination.
> >
> > What really needs to be done is making the DWARF2 unwinder recognize the
> > condition and return a suitable frame id for that.
>
>  Sorry, but I'm confused again :-(
>
>  Do you mean making the DWARF2 unwinder decide that ra == 0 is a clean
>  terminator?  That seems like a bad thing to do if you want to ever show
>  PC == 0 as abnormal termination; then it will do so only if the last
>  frame left on the stack didn't have DWARF2 info.  Or do you mean some
>  other change to the unwinder?

No, sorry, it is me who is confused.  I didn't actually look at Jan's patch
and assumed it did the the right thing of marking the return address as
undefined.

Mark

Re: [PATCH] Re: Unwinding CFI gcc practice of assumed `same value' regs

[Jan Kratochvil]

On Wed, 13 Dec 2006 22:52:52 +0100, Mark Kettenis wrote:
...
> No, sorry, it is me who is confused.  I didn't actually look at Jan's patch
> and assumed it did the the right thing of marking the return address as
> undefined.

Therefore what should the patch do? Currently `.cfi_undefined' looks as too
radical.

GDB may check for `PC == 0 && strcmp (name, "clone") == 0' (more reliably).

That `strcmp (name, "clone")' needs to be coded in more trustworthy way.
This "outermost" framework is already present in GDB but it would mean to add
besides `set backtrace past-main' and `set backtrace past-entry' also some `set
backtrace past-clone' and several more others in the future.


Thanks,
Jan

Re: [PATCH] Re: Unwinding CFI gcc practice of assumed `same value' regs

[Jan Kratochvil]

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

On Wed, 13 Dec 2006 23:11:11 +0100, Jan Kratochvil wrote:
> On Wed, 13 Dec 2006 22:52:52 +0100, Mark Kettenis wrote:
> ...
> > No, sorry, it is me who is confused.  I didn't actually look at Jan's patch
> > and assumed it did the the right thing of marking the return address as
> > undefined.
> 
> Therefore what should the patch do? Currently `.cfi_undefined' looks as too
> radical.

Read all the past threads and I hope this patch complies with all the parties.

I would like if the `.cfi_undefined' patch
	http://sourceware.org/ml/libc-alpha/2006-11/msg00082.html
could stay in the development glibc with the unwinder patched by Jakub Jelinek
	http://sources.redhat.com/ml/gdb/2006-12/msg00100.html
as with such CFI patch gdb does not show any bogus 0x0 unwind of `clone'.

On the other hand as a legacy compatibility I wrote the attached patch to fix
the GDB excessive non-CFI amd64 `clone' unwind as 0x0.  Other platforms (and
functions?) could be carbon-copied from this one.

I consider it the same approach as the way GDB currently on amd64 detects
signal frames by checking the instructions - the CFI for signal frames has been
also recently checked to the development glibc as was the `clone' CFI.

Tested for regressions by the gdb testsuite, it is the most conservative way so
it should not have any sideeffects.  The 0x0 unwinding of any non-`clone'
functions remains the same so Mark should have no objections, I hope.



Regards,
Jan

[-- Attachment #2: gdb-6.5-clone-is-outermost.patch --]
[-- Type: text/plain, Size: 8151 bytes --]

2006-12-16  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb/amd64-linux-tdep.c (linux_clone_code, LINUX_CLONE_LEN,
	amd64_linux_clone_running, amd64_linux_outermost_frame): Detect the
	caller of the thread `start_routing' as the outermost frame to unwind.
	(amd64_linux_init_abi): New hooking for `amd64_linux_outermost_frame'.
	* gdb/i386-tdep.h (gdbarch_tdep): New hook `outermost_frame_p'.
	* gdb/i386-tdep.c (i386_gdbarch_init): Likewise.
	* gdb/amd64-tdep.c (amd64_frame_this_id): Call `outermost_frame_p'.

2006-12-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.threads/bt-clone-stop.exp, gdb.threads/bt-clone-stop.c:
	Backtraced `clone' must not have `PC == 0' as its previous frame.


--- ./gdb/amd64-linux-tdep.c	19 Aug 2006 15:15:18 -0000	1.12
+++ ./gdb/amd64-linux-tdep.c	16 Dec 2006 20:15:16 -0000
@@ -235,6 +235,72 @@ amd64_linux_register_reggroup_p (struct 
 
 /* Set the program counter for process PTID to PC.  */
 
+/* Detect the outermost frame as of:
+   #5  0x000000305cec68c3 in clone () from /lib64/tls/libc.so.6
+   We compare the `linux_clone_code' block to be _before_ the unwound PC.  */
+
+static const unsigned char linux_clone_code[] =
+{
+/* libc/sysdeps/unix/sysv/linux/x86_64/clone.S */
+/* #ifdef RESET_PID */
+/* ... */
+/* 	movl	$SYS_ify(getpid), %eax */
+  0x48, 0xc7, 0xc0, 0x27, 0x00, 0x00, 0x00,
+/* 	syscall */
+  0x0f, 0x05,
+/* 	movl	%eax, %fs:PID */
+  0x64, 0x89, 0x04, 0x25, 0x94, 0x00, 0x00, 0x00,
+/* 	movl	%eax, %fs:TID */
+  0x64, 0x89, 0x04, 0x25, 0x90, 0x00, 0x00, 0x00,
+/* #endif */
+/* 	|* Set up arguments for the function call.  *| */
+/* 	popq	%rax		|* Function to call.  *| */
+  0x58,
+/* 	popq	%rdi		|* Argument.  *| */
+  0x5f,
+/* 	call	*%rax$   */
+  0xff, 0xd0
+};
+
+#define LINUX_CLONE_LEN (sizeof linux_clone_code)
+
+static int
+amd64_linux_clone_running (struct frame_info *next_frame)
+{
+  CORE_ADDR pc = frame_pc_unwind (next_frame);
+  unsigned char buf[LINUX_CLONE_LEN];
+
+  if (!safe_frame_unwind_memory (next_frame, pc - LINUX_CLONE_LEN, buf,
+				 LINUX_CLONE_LEN))
+    return 0;
+
+  if (memcmp (buf, linux_clone_code, LINUX_CLONE_LEN) != 0)
+    return 0;
+
+  return 1;
+}
+
+static int
+amd64_linux_outermost_frame (struct frame_info *next_frame)
+{
+  CORE_ADDR pc = frame_pc_unwind (next_frame);
+  char *name;
+
+  find_pc_partial_function (pc, &name, NULL, NULL);
+
+  /* If we have NAME, we can optimize the search.
+     Check the code even in the case of the unambigious `__clone'
+     as before the `clone' syscall we still have a valid stack there.
+     FIXME: The whole part after the successful syscall there no longer has the
+     valid stack but we still try to unwind it, fortunately it applies only for
+     the case of tracing through `__clone'.  */
+  if (name == NULL || strcmp (name, "clone") == 0
+      || strcmp ("__clone", name) == 0)
+    return (amd64_linux_clone_running (next_frame) != 0);
+
+  return 0;
+}
+
 static void
 amd64_linux_write_pc (CORE_ADDR pc, ptid_t ptid)
 {
@@ -273,6 +339,8 @@ amd64_linux_init_abi (struct gdbarch_inf
   tdep->sc_reg_offset = amd64_linux_sc_reg_offset;
   tdep->sc_num_regs = ARRAY_SIZE (amd64_linux_sc_reg_offset);
 
+  tdep->outermost_frame_p = amd64_linux_outermost_frame;
+
   /* GNU/Linux uses SVR4-style shared libraries.  */
   set_solib_svr4_fetch_link_map_offsets
     (gdbarch, svr4_lp64_fetch_link_map_offsets);
--- ./gdb/amd64-tdep.c	19 Aug 2006 15:15:18 -0000	1.32
+++ ./gdb/amd64-tdep.c	16 Dec 2006 20:15:16 -0000
@@ -879,11 +879,16 @@ amd64_frame_this_id (struct frame_info *
 {
   struct amd64_frame_cache *cache =
     amd64_frame_cache (next_frame, this_cache);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
 
   /* This marks the outermost frame.  */
   if (cache->base == 0)
     return;
 
+  /* Detect OS dependent outermost frames; such as `clone'.  */
+  if (tdep->outermost_frame_p && tdep->outermost_frame_p (next_frame))
+    return;
+
   (*this_id) = frame_id_build (cache->base + 16, cache->pc);
 }
 
--- ./gdb/i386-tdep.h	21 Jan 2006 20:59:50 -0000	1.46
+++ ./gdb/i386-tdep.h	16 Dec 2006 20:15:16 -0000
@@ -104,6 +104,9 @@ struct gdbarch_tdep
      is deprecated, please use `sc_reg_offset' instead.  */
   int sc_pc_offset;
   int sc_sp_offset;
+
+  /* Detect OS dependent outermost frames; such as `clone'.  */
+  int (*outermost_frame_p) (struct frame_info *next_frame);
 };
 
 /* Floating-point registers.  */
--- ./gdb/i386-tdep.c	8 Aug 2006 21:36:46 -0000	1.225
+++ ./gdb/i386-tdep.c	16 Dec 2006 21:13:19 -0000
@@ -2313,6 +2313,9 @@ i386_gdbarch_init (struct gdbarch_info i
   tdep->sc_pc_offset = -1;
   tdep->sc_sp_offset = -1;
 
+  /* Unwinding stops on i386 automatically.  */
+  tdep->outermost_frame_p = NULL;
+
   /* The format used for `long double' on almost all i386 targets is
      the i387 extended floating-point format.  In fact, of all targets
      in the GCC 2.95 tree, only OSF/1 does it different, and insists
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ./gdb/testsuite/gdb.threads/bt-clone-stop.c	13 Dec 2006 19:06:19 -0000
@@ -0,0 +1,39 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2006 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 2 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, write to the Free Software
+   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+   MA 02110-1301, USA.  */
+
+
+#include <pthread.h>
+#include <unistd.h>
+#include <assert.h>
+
+
+void *threader(void *arg)
+{
+	assert(0);
+	return NULL;
+}
+
+int main()
+{
+	pthread_t t1;
+
+	pthread_create(&t1,NULL,threader,(void *)NULL);
+	for (;;)
+		pause();
+}
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ./gdb/testsuite/gdb.threads/bt-clone-stop.exp	13 Dec 2006 19:06:19 -0000
@@ -0,0 +1,56 @@
+# Copyright 2006 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 2 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, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set testfile bt-clone-stop
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if  { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested "Couldn't compile test program"
+    return -1
+}
+
+# Get things started.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# threader: threader.c:8: threader: Assertion `0' failed.
+# Program received signal SIGABRT, Aborted.
+
+gdb_test "run" \
+     "Program received signal SIGABRT.*" \
+     "run"
+
+# #5  0x0000003421ecd62d in ?? () from /lib64/libc.so.6
+# #6  0x0000000000000000 in ?? ()
+# (gdb)
+# Line `#6' must not be present
+# Keep those two cases in this order!
+# Fallback is some invalid output (FAIL).
+gdb_test_multiple "bt" "0x0 entry output invalid" {
+    -re "in threader \\(.*\n#\[0-9\]* *0x0* in .*$gdb_prompt $" {
+    	fail "0x0 entry found there"
+    }
+    -re "in threader \\(.*$gdb_prompt $" {
+    	pass "0x0 entry not found there"
+    }
+}

Re: [PATCH] Re: Unwinding CFI gcc practice of assumed `same value' regs

[Mark Kettenis]

>  On Wed, 13 Dec 2006 23:11:11 +0100, Jan Kratochvil wrote:
>
>  On the other hand as a legacy compatibility I wrote the attached patch to
>  fix the GDB excessive non-CFI amd64 `clone' unwind as 0x0.  Other
>  platforms(and functions?) could be carbon-copied from this one.

Do people indeed think the legacy support is worth the extra complication?

>  I consider it the same approach as the way GDB currently on amd64 detects
>  signal frames by checking the instructions - the CFI for signal frames
>  has been also recently checked to the development glibc as was the
> `clone' CFI.

Hmm, now if the CFI on 'clone' is incorrect and doesn't mark the return
address as undefined, things will still fail.  You're sure there are no
official glibc releases with bogus CFI for 'clone'?

>  2006-12-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>  	* gdb/amd64-linux-tdep.c (linux_clone_code, LINUX_CLONE_LEN,
>  	amd64_linux_clone_running, amd64_linux_outermost_frame): Detect the
>  	caller of the thread `start_routing' as the outermost frame to unwind.
>  	(amd64_linux_init_abi): New hooking for `amd64_linux_outermost_frame'.
>  	* gdb/i386-tdep.h (gdbarch_tdep): New hook `outermost_frame_p'.
>  	* gdb/i386-tdep.c (i386_gdbarch_init): Likewise.
>  	* gdb/amd64-tdep.c (amd64_frame_this_id): Call `outermost_frame_p'.
>
>  2006-12-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>  	* gdb.threads/bt-clone-stop.exp, gdb.threads/bt-clone-stop.c:
>  	Backtraced `clone' must not have `PC == 0' as its previous frame.

Hmm, I really have problems understanding your english Jan.  Anyway, most
of it is irrelevant for ChangeLog.  New functions, defines, etc. should
just be listed as New function, new define, etc.  Only when you change
an existing function you need to describe what you changed (but not why,
since that should be a comment in your code).  So instead of "New hooking
for 'amd64_linux_outermost_frame' I'd just write: initialize
outermost_frame_p.

>  --- ./gdb/amd64-linux-tdep.c	19 Aug 2006 15:15:18 -0000	1.12
>  +++ ./gdb/amd64-linux-tdep.c	16 Dec 2006 20:15:16 -0000
>  @@ -235,6 +235,72 @@ amd64_linux_register_reggroup_p (struct
>
>   /* Set the program counter for process PTID to PC.  */
>
>  +/* Detect the outermost frame as of:
>  +   #5  0x000000305cec68c3 in clone () from /lib64/tls/libc.so.6
>  +   We compare the `linux_clone_code' block to be _before_ the unwound PC.
>   */
>  +
>  +static const unsigned char linux_clone_code[] =
>  +{
>  +/* libc/sysdeps/unix/sysv/linux/x86_64/clone.S */
>  +/* #ifdef RESET_PID */
>  +/* ... */
>  +/* 	movl	$SYS_ify(getpid), %eax */
>  +  0x48, 0xc7, 0xc0, 0x27, 0x00, 0x00, 0x00,
>  +/* 	syscall */
>  +  0x0f, 0x05,
>  +/* 	movl	%eax, %fs:PID */
>  +  0x64, 0x89, 0x04, 0x25, 0x94, 0x00, 0x00, 0x00,
>  +/* 	movl	%eax, %fs:TID */
>  +  0x64, 0x89, 0x04, 0x25, 0x90, 0x00, 0x00, 0x00,
>  +/* #endif */
>  +/* 	|* Set up arguments for the function call.  *| */
>  +/* 	popq	%rax		|* Function to call.  *| */
>  +  0x58,
>  +/* 	popq	%rdi		|* Argument.  *| */
>  +  0x5f,
>  +/* 	call	*%rax$   */
>  +  0xff, 0xd0
>  +};

The #ifdefs here make me a bit nervous

>  +  /* If we have NAME, we can optimize the search.
>  +     Check the code even in the case of the unambigious `__clone'
>  +     as before the `clone' syscall we still have a valid stack there.
>  +     FIXME: The whole part after the successful syscall there no longer
>  has the
>  +     valid stack but we still try to unwind it, fortunately it applies
>  only for
>  +     the case of tracing through `__clone'.  */

Sorry, again I don't understand at all what you're saying here.  Do you
mean that the parent thread may also be in clone() and in that case we
don't want to terminate the backtrace?  But in that case I don't
understand why this is a FIXME.

>  +  /* Detect OS dependent outermost frames; such as `clone'.  */
>  +  if (tdep->outermost_frame_p && tdep->outermost_frame_p (next_frame))
>  +    return;
>  +
>  +# #5  0x0000003421ecd62d in ?? () from /lib64/libc.so.6
>  +# #6  0x0000000000000000 in ?? ()
>  +# (gdb)
>  +# Line `#6' must not be present
>  +# Keep those two cases in this order!
>  +# Fallback is some invalid output (FAIL).
>  +gdb_test_multiple "bt" "0x0 entry output invalid" {
>  +    -re "in threader \\(.*\n#\[0-9\]* *0x0* in .*$gdb_prompt $" {
>  +    	fail "0x0 entry found there"
>  +    }
>  +    -re "in threader \\(.*$gdb_prompt $" {
>  +    	pass "0x0 entry not found there"
>  +    }
>  +}

Sorry, again I don't understand your explanation here.  Do you have a
collegue who is a native english speaker who can help you with writing
english?

Mark

Re: [PATCH] Re: Unwinding CFI gcc practice of assumed `same value' regs

[Jan Kratochvil]

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

Hi Mark,

attached updated patch; no code changes were made, only wording got updated.


On Sun, 17 Dec 2006 14:06:08 +0100, Mark Kettenis wrote:
> >  On Wed, 13 Dec 2006 23:11:11 +0100, Jan Kratochvil wrote:
> >
> >  On the other hand as a legacy compatibility I wrote the attached patch to
> >  fix the GDB excessive non-CFI amd64 `clone' unwind as 0x0.  Other
> >  platforms(and functions?) could be carbon-copied from this one.
> 
> Do people indeed think the legacy support is worth the extra complication?

The patch was provided AS IS.  It is a mandatory and useful part of the
customer support.  It may not be worth the maintenance for the upstream
development but this decision does not belong to me.


> >  I consider it the same approach as the way GDB currently on amd64 detects
> >  signal frames by checking the instructions - the CFI for signal frames
> >  has been also recently checked to the development glibc as was the
> > `clone' CFI.
> 
> Hmm, now if the CFI on 'clone' is incorrect and doesn't mark the return
> address as undefined, things will still fail.  You're sure there are no
> official glibc releases with bogus CFI for 'clone'?

This question is very intentionally out of the scope of this patch.
This patch only fixes clearly broken unwind of ``clone' without any CFI'.

Another patch may address a broken `clone' CFI but I do not have
a bugreport/reproducibility for such case.


> Anyway, most of it is irrelevant for ChangeLog.  New functions, defines, etc.
> should just be listed as New function, new define, etc.

Thanks [6.8.1 Change Log Concepts]; it is really not easy to perceive all the
GNU Coding Standards details.


> >  +static const unsigned char linux_clone_code[] =
> >  +{
> >  +/* libc/sysdeps/unix/sysv/linux/x86_64/clone.S */
> >  +/* #ifdef RESET_PID */
> >  +/* ... */
> >  +/* 	movl	$SYS_ify(getpid), %eax */
> >  +  0x48, 0xc7, 0xc0, 0x27, 0x00, 0x00, 0x00,
> >  +/* 	syscall */
> >  +  0x0f, 0x05,
> >  +/* 	movl	%eax, %fs:PID */
> >  +  0x64, 0x89, 0x04, 0x25, 0x94, 0x00, 0x00, 0x00,
> >  +/* 	movl	%eax, %fs:TID */
> >  +  0x64, 0x89, 0x04, 0x25, 0x90, 0x00, 0x00, 0x00,
> >  +/* #endif */
> >  +/* 	|* Set up arguments for the function call.  *| */
> >  +/* 	popq	%rax		|* Function to call.  *| */
> >  +  0x58,
> >  +/* 	popq	%rdi		|* Argument.  *| */
> >  +  0x5f,
> >  +/* 	call	*%rax$   */
> >  +  0xff, 0xd0
> >  +};
> 
> The #ifdefs here make me a bit nervous

Regarding GDB compilation or GLIBC compatibility?

I tried to verbatim copy the original source code.  It suggests the possible
point of a failure for systems with glibc compiled `!RESET_PID'.


> Sorry, again I don't understand your explanation here.  Do you have a
> collegue who is a native english speaker who can help you with writing
> english?

My code comments always have been a second grade text as nobody was reading it
before.  I understand it should change if submitting the code for review.



Regards,
Jan

[-- Attachment #2: gdb-6.5-clone-is-outermost2.patch --]
[-- Type: text/plain, Size: 8504 bytes --]

2006-12-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb/amd64-linux-tdep.c (linux_clone_code): New variable.
	(LINUX_CLONE_LEN): New definition.
	(amd64_linux_clone_running, amd64_linux_outermost_frame): New function.
	(amd64_linux_init_abi): Initialize `outermost_frame_p'.
	* gdb/i386-tdep.c (i386_gdbarch_init): Likewise.
	* gdb/i386-tdep.h (gdbarch_tdep): Add `outermost_frame_p' member.
	* gdb/amd64-tdep.c (amd64_frame_this_id): Call `outermost_frame_p'.

2006-12-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.threads/bt-clone-stop.exp, gdb.threads/bt-clone-stop.c:
	New file.


--- ./gdb/amd64-linux-tdep.c	19 Aug 2006 15:15:18 -0000	1.12
+++ ./gdb/amd64-linux-tdep.c	17 Dec 2006 17:59:05 -0000
@@ -235,6 +235,75 @@ amd64_linux_register_reggroup_p (struct 
 
 /* Set the program counter for process PTID to PC.  */
 
+/* Detect the outermost frame; during unwind of
+   	#5  0x000000305cec68c3 in clone () from /lib64/tls/libc.so.6
+   avoid the additional bogus frame
+   	#6  0x0000000000000000 in ??
+   We compare if the `linux_clone_code' block is _before_ unwound PC.  */
+
+static const unsigned char linux_clone_code[] =
+{
+/* libc/sysdeps/unix/sysv/linux/x86_64/clone.S */
+/* #ifdef RESET_PID */
+/* ... */
+/* 	movl	$SYS_ify(getpid), %eax */
+  0x48, 0xc7, 0xc0, 0x27, 0x00, 0x00, 0x00,
+/* 	syscall */
+  0x0f, 0x05,
+/* 	movl	%eax, %fs:PID */
+  0x64, 0x89, 0x04, 0x25, 0x94, 0x00, 0x00, 0x00,
+/* 	movl	%eax, %fs:TID */
+  0x64, 0x89, 0x04, 0x25, 0x90, 0x00, 0x00, 0x00,
+/* #endif */
+/* 	|* Set up arguments for the function call.  *| */
+/* 	popq	%rax		|* Function to call.  *| */
+  0x58,
+/* 	popq	%rdi		|* Argument.  *| */
+  0x5f,
+/* 	call	*%rax$   */
+  0xff, 0xd0
+};
+
+#define LINUX_CLONE_LEN (sizeof linux_clone_code)
+
+static int
+amd64_linux_clone_running (struct frame_info *next_frame)
+{
+  CORE_ADDR pc = frame_pc_unwind (next_frame);
+  unsigned char buf[LINUX_CLONE_LEN];
+
+  if (!safe_frame_unwind_memory (next_frame, pc - LINUX_CLONE_LEN, buf,
+				 LINUX_CLONE_LEN))
+    return 0;
+
+  if (memcmp (buf, linux_clone_code, LINUX_CLONE_LEN) != 0)
+    return 0;
+
+  return 1;
+}
+
+static int
+amd64_linux_outermost_frame (struct frame_info *next_frame)
+{
+  CORE_ADDR pc = frame_pc_unwind (next_frame);
+  char *name;
+
+  find_pc_partial_function (pc, &name, NULL, NULL);
+
+  /* If we have NAME, we can optimize the search.
+     `clone' NAME still needs to have the code checked as its name may be
+     present in the user code.
+     `__clone' NAME should not be present in the user code but in the initial
+     parts of the `__clone' implementation the unwind still makes sense.
+     More detailed unwinding decision would be too much sensitive to possible
+     subtle changes in specific glibc revisions.  */
+  if (name == NULL || strcmp (name, "clone") == 0
+      || strcmp ("__clone", name) == 0)
+    return (amd64_linux_clone_running (next_frame) != 0);
+
+  return 0;
+}
+
 static void
 amd64_linux_write_pc (CORE_ADDR pc, ptid_t ptid)
 {
@@ -273,6 +342,8 @@ amd64_linux_init_abi (struct gdbarch_inf
   tdep->sc_reg_offset = amd64_linux_sc_reg_offset;
   tdep->sc_num_regs = ARRAY_SIZE (amd64_linux_sc_reg_offset);
 
+  tdep->outermost_frame_p = amd64_linux_outermost_frame;
+
   /* GNU/Linux uses SVR4-style shared libraries.  */
   set_solib_svr4_fetch_link_map_offsets
     (gdbarch, svr4_lp64_fetch_link_map_offsets);
--- ./gdb/amd64-tdep.c	19 Aug 2006 15:15:18 -0000	1.32
+++ ./gdb/amd64-tdep.c	17 Dec 2006 17:59:05 -0000
@@ -879,11 +879,16 @@ amd64_frame_this_id (struct frame_info *
 {
   struct amd64_frame_cache *cache =
     amd64_frame_cache (next_frame, this_cache);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
 
   /* This marks the outermost frame.  */
   if (cache->base == 0)
     return;
 
+  /* Detect OS dependent outermost frames; such as `clone'.  */
+  if (tdep->outermost_frame_p && tdep->outermost_frame_p (next_frame))
+    return;
+
   (*this_id) = frame_id_build (cache->base + 16, cache->pc);
 }
 
--- ./gdb/i386-tdep.c	8 Aug 2006 21:36:46 -0000	1.225
+++ ./gdb/i386-tdep.c	17 Dec 2006 17:59:10 -0000
@@ -2313,6 +2313,9 @@ i386_gdbarch_init (struct gdbarch_info i
   tdep->sc_pc_offset = -1;
   tdep->sc_sp_offset = -1;
 
+  /* Unwinding stops on i386 automatically.  */
+  tdep->outermost_frame_p = NULL;
+
   /* The format used for `long double' on almost all i386 targets is
      the i387 extended floating-point format.  In fact, of all targets
      in the GCC 2.95 tree, only OSF/1 does it different, and insists
--- ./gdb/i386-tdep.h	21 Jan 2006 20:59:50 -0000	1.46
+++ ./gdb/i386-tdep.h	17 Dec 2006 17:59:10 -0000
@@ -104,6 +104,9 @@ struct gdbarch_tdep
      is deprecated, please use `sc_reg_offset' instead.  */
   int sc_pc_offset;
   int sc_sp_offset;
+
+  /* Detect OS dependent outermost frames; such as `clone'.  */
+  int (*outermost_frame_p) (struct frame_info *next_frame);
 };
 
 /* Floating-point registers.  */
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ./gdb/testsuite/gdb.threads/bt-clone-stop.c	17 Dec 2006 17:59:19 -0000
@@ -0,0 +1,39 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2006 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 2 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, write to the Free Software
+   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+   MA 02110-1301, USA.  */
+
+
+#include <pthread.h>
+#include <unistd.h>
+#include <assert.h>
+
+
+void *threader (void *arg)
+{
+	assert (0);
+	return NULL;
+}
+
+int main (void)
+{
+	pthread_t t1;
+
+	pthread_create (&t1, NULL, threader, (void *) NULL);
+	for (;;)
+		pause();
+}
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ./gdb/testsuite/gdb.threads/bt-clone-stop.exp	17 Dec 2006 17:59:19 -0000
@@ -0,0 +1,61 @@
+# Copyright 2006 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 2 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, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+
+# Backtraced `clone' must not have `PC == 0' as its previous frame.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set testfile bt-clone-stop
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if  { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested "Couldn't compile test program"
+    return -1
+}
+
+# Get things started.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# threader: threader.c:8: threader: Assertion `0' failed.
+# Program received signal SIGABRT, Aborted.
+
+gdb_test "run" \
+     "Program received signal SIGABRT.*" \
+     "run"
+
+# Former gdb unwind (the first function is `clone'):
+# #5  0x0000003421ecd62d in ?? () from /lib64/libc.so.6
+# #6  0x0000000000000000 in ?? ()
+# (gdb)
+# Tested `amd64_linux_outermost_frame' functionality should omit the line `#6'.
+# 
+# Two `-re' cases below must be in this order (1st is a subset of the 2nd one).
+# Unhandled case below should not happen and it is fortunately handled by
+# `amd64_linux_outermost_frame' as FAIL (and result `0x0 entry output invalid').
+gdb_test_multiple "bt" "0x0 entry output invalid" {
+    -re "in threader \\(.*\n#\[0-9\]* *0x0* in .*$gdb_prompt $" {
+    	fail "0x0 entry found"
+    }
+    -re "in threader \\(.*$gdb_prompt $" {
+    	pass "0x0 entry not found"
+    }
+}