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

["Mark Kettenis" <mark.kettenis@xs4all.nl>]

>  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