[patch/rfc] The off again, on again, PC == 0 in get_prev_frame

[patch/rfc] The off again, on again, PC == 0 in get_prev_frame

[Andrew Cagney]

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

Ref: [PATCH] Don't try to unwind the PC in frame.c:get_prev_frame()
http://sources.redhat.com/ml/gdb-patches/2004-01/msg00104.html

Having only recently posted:
> Can just you add a -ve comment stating why a test for PC==0 does not belong in that function? I'm pretty much certain that someone will eventually try to re-add a very similar test (via inside_entry_func for instance).

the attached adds a conditionalized check for "PC == 0" to 
get_prev_frame.  If there's a zero PC, _and_ it isn't the inner most 
frame, stop unwinding.  That should let mark's original case through.

Turns out that the old removed test was stopping this problem on amd64:

(gdb) PASS: gdb.threads/linux-dp.exp: continue to breakpoint: thread 5's 
print
where
#0  print_philosopher (n=3, left=33 '!', right=33 '!') at 
/home/cygnus/cagney/GD
B/src/gdb/testsuite/gdb.threads/linux-dp.c:105
#1  0x0000000000400e4a in philosopher (data=0x50177c) at 
/home/cygnus/cagney/GDB
/src/gdb/testsuite/gdb.threads/linux-dp.c:148
#2  0x0000002a95671c2b in start_thread () from /lib64/tls/libpthread.so.0
#3  0x0000002a959de040 in thread_start () from /lib64/tls/libc.so.6
#4  0x0000000000000000 in ?? ()
#5  0x0000000000000000 in ?? ()
#6  0x0000000000000000 in ?? ()
#7  0x0000000000000000 in ?? ()
#8  0x0000000000000000 in ?? ()
#9  0x0000000000000000 in ?? ()
#10 0x0000000000000000 in ?? ()
#11 0x0000000000000000 in ?? ()
#12 0x0000000000000000 in ?? ()
#13 0x0000000000000000 in ?? ()
#14 0x0000000000000000 in ?? ()
#15 0x0000000000000000 in ?? ()
#16 0x0000002a95b41580 in _nl_C_locobj () from /lib64/tls/libc.so.6
#17 0x00000000427ffdc8 in ?? ()

comments (...)
Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 1955 bytes --]

2004-02-20  Andrew Cagney  <cagney@redhat.com>

	* frame.c (get_prev_frame): With the exception of the inner-most
	frame, do not backtrace past a frame with a zero pc.

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.162
diff -u -r1.162 frame.c
--- frame.c	16 Feb 2004 21:49:21 -0000	1.162
+++ frame.c	20 Feb 2004 21:12:52 -0000
@@ -1721,14 +1721,9 @@
   return prev;
 }
 
-/* Return a structure containing various interesting information
-   about the frame that called THIS_FRAME.  Returns NULL
-   if there is no such frame.
-
-   This function tests some target-independent conditions that should
-   terminate the frame chain, such as unwinding past main().  It
-   should not contain any target-dependent tests, such as checking
-   whether the program-counter is zero.  */
+/* Return a structure containing various interesting information about
+   the frame that called THIS_FRAME.  Returns NULL if there is no such
+   frame.  */
 
 struct frame_info *
 get_prev_frame (struct frame_info *this_frame)
@@ -1843,6 +1838,24 @@
 	  fprint_frame (gdb_stdlog, NULL);
 	  fprintf_unfiltered (gdb_stdlog, "// inside entry func }\n");
 	}
+      return NULL;
+    }
+
+  /* Don't allow frames with a zero PC to unwind.  The exception is
+     the inner-most frame - a jump through a NULL pointer results in
+     an inner most frame that is both unwindable and has a zero
+     PC.  */
+  /* NOTE: cagney/2004-02-20: The old zero PC check, removed
+     2004-01-11, had the problem that it stopped backtraces from that
+     inner most zero PCed frame.  */
+  if (this_frame->level > 0 && get_frame_pc (this_frame) == 0)
+    {
+      if (frame_debug)
+        {
+          fprintf_unfiltered (gdb_stdlog, "-> ");
+          fprint_frame (gdb_stdlog, NULL);
+          fprintf_unfiltered (gdb_stdlog, " // PC zero }\n");
+        }
       return NULL;
     }
 

Re: [patch/rfc] The off again, on again, PC == 0 in get_prev_frame

[Mark Kettenis]

   Date: Fri, 20 Feb 2004 16:27:41 -0500
   From: Andrew Cagney <cagney@gnu.org>

   Ref: [PATCH] Don't try to unwind the PC in frame.c:get_prev_frame()
   http://sources.redhat.com/ml/gdb-patches/2004-01/msg00104.html

   Turns out that the old removed test was stopping this problem on amd64:

   (gdb) PASS: gdb.threads/linux-dp.exp: continue to breakpoint: thread 5's 
   print
   where
   #0  print_philosopher (n=3, left=33 '!', right=33 '!') at 
   /home/cygnus/cagney/GD
   B/src/gdb/testsuite/gdb.threads/linux-dp.c:105
   #1  0x0000000000400e4a in philosopher (data=0x50177c) at 
   /home/cygnus/cagney/GDB
   /src/gdb/testsuite/gdb.threads/linux-dp.c:148
   #2  0x0000002a95671c2b in start_thread () from /lib64/tls/libpthread.so.0
   #3  0x0000002a959de040 in thread_start () from /lib64/tls/libc.so.6
   #4  0x0000000000000000 in ?? ()
   #5  0x0000000000000000 in ?? ()
   #6  0x0000000000000000 in ?? ()

Andrew, I think that this case is completely broken.  The frame chain
isn't properly terminated.  The clone(2) system call that has created
this thread tries to mark frame #3 as the outermost frame by clearing
%rbp.  However, since most amd64 code doesn't use the frame pointer,
this is pointless; GDB doesn't look at it.  The fact that you're
seeing a zero PC here is merely accidental; it just happens to be that
(%rsp) contains zero.  Therefore I don't think we should consider a
zero PC as a marker for the end of the frame chain.

The Linux folks should add proper CFI to the implementation of
close(2) in glibc.  A DW_CFA_def_cfa rule that sets the CFA to %rbp
should do the trick.

Mark

Re: [patch/rfc] The off again, on again, PC == 0 in get_prev_frame

[Andrew Cagney]

> Andrew, I think that this case is completely broken.  The frame chain
> isn't properly terminated.  The clone(2) system call that has created
> this thread tries to mark frame #3 as the outermost frame by clearing
> %rbp.  However, since most amd64 code doesn't use the frame pointer,
> this is pointless; GDB doesn't look at it.  The fact that you're
> seeing a zero PC here is merely accidental; it just happens to be that
> (%rsp) contains zero.  Therefore I don't think we should consider a
> zero PC as a marker for the end of the frame chain.
> 
> The Linux folks should add proper CFI to the implementation of
> close(2) in glibc.  A DW_CFA_def_cfa rule that sets the CFA to %rbp
> should do the trick.

clone(2)?  How come the trad unwind code doesn't recover?

Thinking about it, the test would stop this backtrace:

	sigsegv_handler
	<signal trampoline>
	"null"
	foo (which called "null")
	main

is this tested anywhere?

Andrew

Re: [patch/rfc] The off again, on again, PC == 0 in get_prev_frame

[Mark Kettenis]

   Date: Sun, 22 Feb 2004 12:40:53 -0500
   From: Andrew Cagney <cagney@gnu.org>

   > The Linux folks should add proper CFI to the implementation of
   > close(2) in glibc.  A DW_CFA_def_cfa rule that sets the CFA to %rbp
   > should do the trick.

   clone(2)?

Yup.  Sorry.  Typo.  The thread_start you're seeing in the backtrace
is part of the clone(2) implementation in glibc.

See sysdeps/unix/sysv/linux/x86_64/clone.S.

   How come the trad unwind code doesn't recover?

There simply isn't a frame pointer.

   Thinking about it, the test would stop this backtrace:

	   sigsegv_handler
	   <signal trampoline>
	   "null"
	   foo (which called "null")
	   main

   is this tested anywhere?

No it isn't.

Mark