[patch/rfc] Add a sentinel frame

[patch/rfc] Add a sentinel frame

[Andrew Cagney]

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

Hello,

This patch modifies^D^D^D^Dsimplifies the frame code so that there is an 
extra frame below the innermost frame.  That extra frame maps any 
request for a register onto the regcache.

The thing to notice with this patch is how it eliminates the need to 
special case a level zero frame when fetching a frame's register. 
Instead, a register can be fetched using the recursive frame_register() 
method.

I'll look to commit this in a few days.

Follow on patches include:
- convert d10v to the current frame unwind mechanims
- deprecate methods such as POP_FRAME.

Andrew

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

2003-01-23  Andrew Cagney  <ac131313@redhat.com>

	* sentinel-frame.h, sentinel-frame.c: New files.
	* Makefile.in (frame.o): Update dependencies.
	(SFILES): Add sentinel-frame.c.
	(sentinel_frame_h): Define.
	(COMMON_OBS): Add sentinel-frame.o.
	(sentinel-frame.o): Specify dependencies.
	* frame.c: Include "sentinel-frame.h".
	(frame_register_unwind): Rewrite assuming that there is always a a
	->next frame.
	(frame_register, generic_unwind_get_saved_register): Ditto.
	(frame_read_unsigned_register, frame_read_signed_register): Ditto.
	(create_sentinel_frame, unwind_to_current_frame): New functions.
	(get_current_frame): Rewrite using create_sentinel_frame and
	unwind_to_current_frame.  When possible, always create a frame.
	(create_new_frame): Set next to the sentinel frame.
	(get_next_frame): Rewrite.  Don't go below the level 0 frame.
	(deprecated_update_frame_pc_hack): Update the next frame's PC and
	ID cache when necessary.
	(frame_saved_regs_id_unwind): Use frame_relative_level.
	(deprecated_generic_get_saved_register): Use frame_relative_level,
	get_frame_saved_regs, get_frame_pc, get_frame_base and
	get_next_frame.
	(frame_saved_regs_register_unwind): Use get_frame_saved_regs and
	frame_register.

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.317
diff -u -r1.317 Makefile.in
--- Makefile.in	23 Jan 2003 07:30:17 -0000	1.317
+++ Makefile.in	23 Jan 2003 20:46:59 -0000
@@ -528,7 +528,9 @@
 	objfiles.c osabi.c \
 	p-exp.y p-lang.c p-typeprint.c p-valprint.c parse.c printcmd.c \
 	regcache.c reggroups.c remote.c \
-	scm-exp.c scm-lang.c scm-valprint.c serial.c ser-unix.c source.c \
+	scm-exp.c scm-lang.c scm-valprint.c \
+	sentinel-frame.c \
+	serial.c ser-unix.c source.c \
 	stabsread.c stack.c std-regs.c symfile.c symmisc.c symtab.c \
 	target.c thread.c top.c tracepoint.c typeprint.c \
 	tui/tui.c tui/tui.h tui/tuiCommand.c tui/tuiCommand.h \
@@ -686,6 +688,7 @@
 remote_h = remote.h
 scm_lang_h = scm-lang.h $(scm_tags_h)
 scm_tags_h = scm-tags.h
+sentinel_frame_h = sentinel-frame.h
 ser_unix_h = ser-unix.h
 serial_h = serial.h
 sh_tdep_h = sh-tdep.h
@@ -832,7 +835,9 @@
 	varobj.o wrapper.o \
 	jv-lang.o jv-valprint.o jv-typeprint.o \
 	m2-lang.o p-lang.o p-typeprint.o p-valprint.o \
-	scm-exp.o scm-lang.o scm-valprint.o complaints.o typeprint.o \
+	scm-exp.o scm-lang.o scm-valprint.o \
+	sentinel-frame.o \
+	complaints.o typeprint.o \
 	c-typeprint.o f-typeprint.o m2-typeprint.o \
 	c-valprint.o cp-valprint.o f-valprint.o m2-valprint.o \
 	nlmread.o serial.o mdebugread.o top.o utils.o \
@@ -1692,7 +1697,8 @@
 frame.o: frame.c $(defs_h) $(frame_h) $(target_h) $(value_h) $(inferior_h) \
 	$(regcache_h) $(gdb_assert_h) $(gdb_string_h) $(builtin_regs_h) \
 	$(gdb_obstack_h) $(dummy_frame_h) $(gdbcore_h) $(annotate_h) \
-	$(language_h) $(frame_unwind_h) $(command_h) $(gdbcmd_h)
+	$(language_h) $(frame_unwind_h) $(command_h) $(gdbcmd_h) \
+	$(sentinel_frame_h)
 frame-unwind.o: frame-unwind.c $(defs_h) $(frame_h) $(frame_unwind_h) \
 	$(gdb_assert_h) $(dummy_frame_h) $(legacy_frame_h)
 frv-tdep.o: frv-tdep.c $(defs_h) $(inferior_h) $(symfile_h) $(gdbcore_h) \
@@ -2125,6 +2131,8 @@
 scm-valprint.o: scm-valprint.c $(defs_h) $(symtab_h) $(gdbtypes_h) \
 	$(expression_h) $(parser_defs_h) $(language_h) $(value_h) \
 	$(scm_lang_h) $(valprint_h) $(gdbcore_h)
+sentinel-frame.o: sentinel-frame.c $(defs_h) $(regcache_h) \
+	$(sentinel_frame_h) $(inferior_h) $(frame_unwind_h)
 ser-e7kpc.o: ser-e7kpc.c $(defs_h) $(serial_h) $(gdb_string_h)
 ser-go32.o: ser-go32.c $(defs_h) $(gdbcmd_h) $(serial_h) $(gdb_string_h)
 ser-pipe.o: ser-pipe.c $(defs_h) $(serial_h) $(ser_unix_h) $(gdb_vfork_h) \
Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.62
diff -u -r1.62 frame.c
--- frame.c	21 Jan 2003 19:32:42 -0000	1.62
+++ frame.c	23 Jan 2003 20:46:59 -0000
@@ -31,6 +31,7 @@
 #include "builtin-regs.h"
 #include "gdb_obstack.h"
 #include "dummy-frame.h"
+#include "sentinel-frame.h"
 #include "gdbcore.h"
 #include "annotate.h"
 #include "language.h"
@@ -179,29 +180,11 @@
   gdb_assert (realnump != NULL);
   /* gdb_assert (bufferp != NULL); */
 
-  /* NOTE: cagney/2002-04-14: It would be nice if, instead of a
-     special case, there was always an inner frame dedicated to the
-     hardware registers.  Unfortunatly, there is too much unwind code
-     around that looks up/down the frame chain while making the
-     assumption that each frame level is using the same unwind code.  */
-
-  if (frame == NULL)
-    {
-      /* We're in the inner-most frame, get the value direct from the
-	 register cache.  */
-      *optimizedp = 0;
-      *lvalp = lval_register;
-      /* ULGH!  Code uses the offset into the raw register byte array
-         as a way of identifying a register.  */
-      *addrp = REGISTER_BYTE (regnum);
-      /* Should this code test ``register_cached (regnum) < 0'' and do
-         something like set realnum to -1 when the register isn't
-         available?  */
-      *realnump = regnum;
-      if (bufferp)
-	deprecated_read_register_gen (regnum, bufferp);
-      return;
-    }
+  /* NOTE: cagney/2002-11-27: A program trying to unwind a NULL frame
+     is broken.  There is always a frame.  If there, for some reason,
+     isn't, there is some pretty busted code as it should have
+     detected the problem before calling here.  */
+  gdb_assert (frame != NULL);
 
   /* Ask this frame to unwind its register.  */
   frame->unwind->reg (frame, &frame->unwind_cache, regnum,
@@ -247,25 +230,11 @@
       return;
     }
 
-  /* Reached the the bottom (youngest, inner most) of the frame chain
-     (youngest, inner most) frame, go direct to the hardware register
-     cache (do not pass go, do not try to cache the value, ...).  The
-     unwound value would have been cached in frame->next but that
-     doesn't exist.  This doesn't matter as the hardware register
-     cache is stopping any unnecessary accesses to the target.  */
-
-  /* NOTE: cagney/2002-04-14: It would be nice if, instead of a
-     special case, there was always an inner frame dedicated to the
-     hardware registers.  Unfortunatly, there is too much unwind code
-     around that looks up/down the frame chain while making the
-     assumption that each frame level is using the same unwind code.  */
-
-  if (frame == NULL)
-    frame_register_unwind (NULL, regnum, optimizedp, lvalp, addrp, realnump,
-			   bufferp);
-  else
-    frame_register_unwind (frame->next, regnum, optimizedp, lvalp, addrp,
-			   realnump, bufferp);
+  /* Obtain the register value by unwinding the register from the next
+     (more inner frame).  */
+  gdb_assert (frame != NULL && frame->next != NULL);
+  frame_register_unwind (frame->next, regnum, optimizedp, lvalp, addrp,
+			 realnump, bufferp);
 }
 
 void
@@ -317,17 +286,17 @@
      tests like ``if get_next_frame() == NULL'' and instead just rely
      on recursive frame calls (like the below code) when manipulating
      a frame chain.  */
-  gdb_assert (frame != NULL);
-  frame_unwind_unsigned_register (get_next_frame (frame), regnum, val);
+  gdb_assert (frame != NULL && frame->next != NULL);
+  frame_unwind_unsigned_register (frame->next, regnum, val);
 }
 
 void
 frame_read_signed_register (struct frame_info *frame, int regnum,
 			    LONGEST *val)
 {
-  /* See note in frame_read_unsigned_register().  */
-  gdb_assert (frame != NULL);
-  frame_unwind_signed_register (get_next_frame (frame), regnum, val);
+  /* See note above in frame_read_unsigned_register().  */
+  gdb_assert (frame != NULL && frame->next != NULL);
+  frame_unwind_signed_register (frame->next, regnum, val);
 }
 
 static void
@@ -355,25 +324,9 @@
   if (addrp == NULL)
     addrp = &addrx;
 
-  /* Reached the the bottom (youngest, inner most) of the frame chain
-     (youngest, inner most) frame, go direct to the hardware register
-     cache (do not pass go, do not try to cache the value, ...).  The
-     unwound value would have been cached in frame->next but that
-     doesn't exist.  This doesn't matter as the hardware register
-     cache is stopping any unnecessary accesses to the target.  */
-
-  /* NOTE: cagney/2002-04-14: It would be nice if, instead of a
-     special case, there was always an inner frame dedicated to the
-     hardware registers.  Unfortunatly, there is too much unwind code
-     around that looks up/down the frame chain while making the
-     assumption that each frame level is using the same unwind code.  */
-
-  if (frame == NULL)
-    frame_register_unwind (NULL, regnum, optimizedp, lvalp, addrp, &realnumx,
-			   raw_buffer);
-  else
-    frame_register_unwind (frame->next, regnum, optimizedp, lvalp, addrp,
-			   &realnumx, raw_buffer);
+  gdb_assert (frame != NULL && frame->next != NULL);
+  frame_register_unwind (frame->next, regnum, optimizedp, lvalp, addrp,
+			 &realnumx, raw_buffer);
 }
 
 void
@@ -463,6 +416,32 @@
   return builtin_reg_map_regnum_to_name (regnum);
 }
 
+/* Create a sentinel frame.  */
+
+struct frame_info *
+create_sentinel_frame (struct regcache *regcache)
+{
+  struct frame_info *frame = FRAME_OBSTACK_ZALLOC (struct frame_info);
+  frame->type = NORMAL_FRAME;
+  frame->level = -1;
+  /* Explicitly initialize the sentinel frame's cache.  Provide it
+     with the underlying regcache.  In the future additional
+     information, such as the frame's thread will be added.  */
+  frame->unwind_cache = sentinel_frame_cache (regcache);
+  /* For the moment there is only one sentinel frame implementation.  */
+  frame->unwind = sentinel_frame_unwind;
+  /* Link this frame back to itself.  The frame is self referential
+     (the unwound PC is the same as the pc), so make it so.  */
+  frame->next = frame;
+  /* Always unwind the PC as part of creating this frame.  This
+     ensures that the frame's PC points at something valid.  */
+  /* FIXME: cagney/2003-01-10: Problem here.  Unwinding a sentinel
+     frame's PC may require information such as the frame's thread's
+     stop reason.  Is it possible to get to that?  */
+  frame->pc = frame_pc_unwind (frame);
+  return frame;
+}
+
 /* Info about the innermost stack frame (contents of FP register) */
 
 static struct frame_info *current_frame;
@@ -495,17 +474,43 @@
   return fi->saved_regs;
 }
 
-/* Return the innermost (currently executing) stack frame.  */
+/* Return the innermost (currently executing) stack frame.  This is
+   split into two functions.  The function unwind_to_current_frame()
+   is wrapped in catch exceptions so that, even when the unwind of the
+   sentinel frame fails, the function still returns a stack frame.  */
+
+static int
+unwind_to_current_frame (struct ui_out *ui_out, void *args)
+{
+  struct frame_info *frame = get_prev_frame (args);
+  /* A sentinel frame can fail to unwind, eg, because it's PC value
+     lands in somewhere like start.  */
+  if (frame == NULL)
+    return 1;
+  current_frame = frame;
+  return 0;
+}
 
 struct frame_info *
 get_current_frame (void)
 {
+  if (!target_has_stack)
+    error ("No stack.");
+  if (!target_has_registers)
+    error ("No registers.");
+  if (!target_has_memory)
+    error ("No memory.");
   if (current_frame == NULL)
     {
-      if (target_has_stack)
-	current_frame = create_new_frame (read_fp (), read_pc ());
-      else
-	error ("No stack.");
+      struct frame_info *sentinel_frame =
+	create_sentinel_frame (current_regcache);
+      if (catch_exceptions (uiout, unwind_to_current_frame, sentinel_frame,
+			    NULL, RETURN_MASK_ERROR) != 0)
+	{
+	  /* Oops! Fake a current frame?  Is this useful?  It has a PC
+             of zero, for instance.  */
+	  current_frame = sentinel_frame;
+	}
     }
   return current_frame;
 }
@@ -593,11 +598,11 @@
   gdb_assert (FRAME_INIT_SAVED_REGS_P ());
 
   /* Load the saved_regs register cache.  */
-  if (frame->saved_regs == NULL)
+  if (get_frame_saved_regs (frame) == NULL)
     FRAME_INIT_SAVED_REGS (frame);
 
-  if (frame->saved_regs != NULL
-      && frame->saved_regs[regnum] != 0)
+  if (get_frame_saved_regs (frame) != NULL
+      && get_frame_saved_regs (frame)[regnum] != 0)
     {
       if (regnum == SP_REGNUM)
 	{
@@ -608,7 +613,7 @@
 	  *realnump = -1;
 	  if (bufferp != NULL)
 	    store_address (bufferp, REGISTER_RAW_SIZE (regnum),
-			   frame->saved_regs[regnum]);
+			   get_frame_saved_regs (frame)[regnum]);
 	}
       else
 	{
@@ -616,7 +621,7 @@
              a local copy of its value.  */
 	  *optimizedp = 0;
 	  *lvalp = lval_memory;
-	  *addrp = frame->saved_regs[regnum];
+	  *addrp = get_frame_saved_regs (frame)[regnum];
 	  *realnump = -1;
 	  if (bufferp != NULL)
 	    {
@@ -635,13 +640,13 @@
 		{
 		  regs[regnum]
 		    = frame_obstack_zalloc (REGISTER_RAW_SIZE (regnum));
-		  read_memory (frame->saved_regs[regnum], regs[regnum],
+		  read_memory (get_frame_saved_regs (frame)[regnum], regs[regnum],
 			       REGISTER_RAW_SIZE (regnum));
 		}
 	      memcpy (bufferp, regs[regnum], REGISTER_RAW_SIZE (regnum));
 #else
 	      /* Read the value in from memory.  */
-	      read_memory (frame->saved_regs[regnum], bufferp,
+	      read_memory (get_frame_saved_regs (frame)[regnum], bufferp,
 			   REGISTER_RAW_SIZE (regnum));
 #endif
 	    }
@@ -650,21 +655,11 @@
     }
 
   /* No luck, assume this and the next frame have the same register
-     value.  If a value is needed, pass the request on down the chain;
-     otherwise just return an indication that the value is in the same
-     register as the next frame.  */
-  if (bufferp == NULL)
-    {
-      *optimizedp = 0;
-      *lvalp = lval_register;
-      *addrp = 0;
-      *realnump = regnum;
-    }
-  else
-    {
-      frame_register_unwind (frame->next, regnum, optimizedp, lvalp, addrp,
-			     realnump, bufferp);
-    }
+     value.  Pass the request down the frame chain to the next frame.
+     Hopefully that will find the register's location, either in a
+     register or in memory.  */
+  frame_register (frame, regnum, optimizedp, lvalp, addrp, realnump,
+		  bufferp);
 }
 
 static CORE_ADDR
@@ -684,7 +679,7 @@
   /* Start out by assuming it's NULL.  */
   (*id) = null_frame_id;
 
-  if (next_frame->next == NULL)
+  if (frame_relative_level (next_frame) <= 0)
     /* FIXME: 2002-11-09: Frameless functions can occure anywhere in
        the frame chain, not just the inner most frame!  The generic,
        per-architecture, frame code should handle this and the below
@@ -797,44 +792,50 @@
      the current frame itself: otherwise, we would be getting the
      previous frame's registers which were saved by the current frame.  */
 
-  while (frame && ((frame = frame->next) != NULL))
+  if (frame != NULL)
     {
-      if (get_frame_type (frame) == DUMMY_FRAME)
+      for (frame = get_next_frame (frame);
+	   frame_relative_level (frame) >= 0;
+	   frame = get_next_frame (frame))
 	{
-	  if (lval)		/* found it in a CALL_DUMMY frame */
-	    *lval = not_lval;
-	  if (raw_buffer)
-	    /* FIXME: cagney/2002-06-26: This should be via the
-	       gdbarch_register_read() method so that it, on the fly,
-	       constructs either a raw or pseudo register from the raw
-	       register cache.  */
-	    regcache_raw_read (generic_find_dummy_frame (frame->pc,
-							 frame->frame),
-			       regnum, raw_buffer);
-	  return;
-	}
-
-      FRAME_INIT_SAVED_REGS (frame);
-      if (frame->saved_regs != NULL
-	  && frame->saved_regs[regnum] != 0)
-	{
-	  if (lval)		/* found it saved on the stack */
-	    *lval = lval_memory;
-	  if (regnum == SP_REGNUM)
+	  if (get_frame_type (frame) == DUMMY_FRAME)
 	    {
-	      if (raw_buffer)	/* SP register treated specially */
-		store_address (raw_buffer, REGISTER_RAW_SIZE (regnum),
-			       frame->saved_regs[regnum]);
+	      if (lval)		/* found it in a CALL_DUMMY frame */
+		*lval = not_lval;
+	      if (raw_buffer)
+		/* FIXME: cagney/2002-06-26: This should be via the
+		   gdbarch_register_read() method so that it, on the
+		   fly, constructs either a raw or pseudo register
+		   from the raw register cache.  */
+		regcache_raw_read
+		  (generic_find_dummy_frame (get_frame_pc (frame),
+					     get_frame_base (frame)),
+		   regnum, raw_buffer);
+	      return;
 	    }
-	  else
+
+	  FRAME_INIT_SAVED_REGS (frame);
+	  if (get_frame_saved_regs (frame) != NULL
+	      && get_frame_saved_regs (frame)[regnum] != 0)
 	    {
-	      if (addrp)	/* any other register */
-		*addrp = frame->saved_regs[regnum];
-	      if (raw_buffer)
-		read_memory (frame->saved_regs[regnum], raw_buffer,
-			     REGISTER_RAW_SIZE (regnum));
+	      if (lval)		/* found it saved on the stack */
+		*lval = lval_memory;
+	      if (regnum == SP_REGNUM)
+		{
+		  if (raw_buffer)	/* SP register treated specially */
+		    store_address (raw_buffer, REGISTER_RAW_SIZE (regnum),
+				   get_frame_saved_regs (frame)[regnum]);
+		}
+	      else
+		{
+		  if (addrp)	/* any other register */
+		    *addrp = get_frame_saved_regs (frame)[regnum];
+		  if (raw_buffer)
+		    read_memory (get_frame_saved_regs (frame)[regnum], raw_buffer,
+				 REGISTER_RAW_SIZE (regnum));
+		}
+	      return;
 	    }
-	  return;
 	}
     }
 
@@ -884,6 +885,7 @@
 
   fi->frame = addr;
   fi->pc = pc;
+  fi->next = create_sentinel_frame (current_regcache);
   fi->type = frame_type_from_pc (pc);
 
   if (INIT_EXTRA_FRAME_INFO_P ())
@@ -896,12 +898,16 @@
 }
 
 /* Return the frame that FRAME calls (NULL if FRAME is the innermost
-   frame).  */
+   frame).  Be careful to not fall off the bottom of the frame chain
+   and onto the sentinel frame.  */
 
 struct frame_info *
 get_next_frame (struct frame_info *frame)
 {
-  return frame->next;
+  if (frame->level > 0)
+    return frame->next;
+  else
+    return NULL;
 }
 
 /* Flush the entire frame cache.  */
@@ -1415,6 +1421,7 @@
 deprecated_update_frame_pc_hack (struct frame_info *frame, CORE_ADDR pc)
 {
   /* See comment in "frame.h".  */
+  gdb_assert (frame->next != NULL);
   frame->pc = pc;
 }
 
Index: sentinel-frame.c
===================================================================
RCS file: sentinel-frame.c
diff -N sentinel-frame.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ sentinel-frame.c	23 Jan 2003 20:46:59 -0000
@@ -0,0 +1,113 @@
+/* Code dealing with register stack frames, for GDB, the GNU debugger.
+
+   Copyright 1986, 1987, 1988, 1989, 1990, 1991, 1992, 1993, 1994,
+   1995, 1996, 1997, 1998, 1999, 2000, 2001, 2002 Free Software
+   Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 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.  */
+
+
+#include "defs.h"
+#include "regcache.h"
+#include "sentinel-frame.h"
+#include "inferior.h"
+#include "frame-unwind.h"
+
+struct frame_unwind_cache
+{
+  struct regcache *regcache;
+};
+
+void *
+sentinel_frame_cache (struct regcache *regcache)
+{
+  struct frame_unwind_cache *cache = 
+    FRAME_OBSTACK_ZALLOC (struct frame_unwind_cache);
+  cache->regcache = regcache;
+  return cache;
+}
+
+/* Here the register value is taken direct from the register cache.  */
+
+void
+sentinel_frame_register_unwind (struct frame_info *frame,
+				void **unwind_cache,
+				int regnum, int *optimized,
+				enum lval_type *lvalp, CORE_ADDR *addrp,
+				int *realnum, void *bufferp)
+{
+  struct frame_unwind_cache *cache = *unwind_cache;
+  /* Describe the register's location.  A reg-frame maps all registers
+     onto the corresponding hardware register.  */
+  *optimized = 0;
+  *lvalp = lval_register;
+  *addrp = REGISTER_BYTE (regnum);
+  *realnum = regnum;
+
+  /* If needed, find and return the value of the register.  */
+  if (bufferp != NULL)
+    {
+      /* Return the actual value.  */
+      /* Use the regcache_cooked_read() method so that it, on the fly,
+         constructs either a raw or pseudo register from the raw
+         register cache.  */
+      regcache_cooked_read (cache->regcache, regnum, bufferp);
+    }
+}
+
+CORE_ADDR
+sentinel_frame_pc_unwind (struct frame_info *frame,
+			  void **cache)
+{
+  /* FIXME: cagney/2003-01-08: This should be using a per-architecture
+     method that doesn't suffer from DECR_PC_AFTER_BREAK problems.
+     Such a method would take unwind_cache, regcache and stop reason
+     parameters.  */
+  return read_pc ();
+}
+
+void
+sentinel_frame_id_unwind (struct frame_info *frame,
+			  void **cache,
+			  struct frame_id *id)
+{
+  /* FIXME: cagney/2003-01-08: This should be using a per-architecture
+     method that doesn't suffer from DECR_PC_AFTER_BREAK problems.
+     Such a method would take unwind_cache, regcache and stop reason
+     parameters.  */
+  id->base = read_fp ();
+  id->pc = read_pc ();
+}
+
+static void
+sentinel_frame_pop (struct frame_info *frame,
+		    void **cache,
+		    struct regcache *regcache)
+{
+  internal_error (__FILE__, __LINE__, "Function sentinal_frame_pop called");
+}
+
+const struct frame_unwind sentinel_frame_unwinder =
+{
+  sentinel_frame_pop,
+  sentinel_frame_pc_unwind,
+  sentinel_frame_id_unwind,
+  sentinel_frame_register_unwind
+};
+
+const struct frame_unwind *const sentinel_frame_unwind = &sentinel_frame_unwinder;
Index: sentinel-frame.h
===================================================================
RCS file: sentinel-frame.h
diff -N sentinel-frame.h
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ sentinel-frame.h	23 Jan 2003 20:46:59 -0000
@@ -0,0 +1,41 @@
+/* Code dealing with register stack frames, for GDB, the GNU debugger.
+
+   Copyright 2003 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 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 !defined (SENTINEL_FRAME_H)
+#define SENTINEL_FRAME_H 1
+
+struct frame_unwind;
+struct regcache;
+
+/* Implement the sentinel frame.  The sentinel frame terminates the
+   inner most end of the frame chain.  If unwound, it returns the
+   information need to construct an inner-most frame.  */
+
+/* Pump prime the sentinel frame's cache.  Since this needs the
+   REGCACHE provide that here.  */
+
+extern void *sentinel_frame_cache (struct regcache *regcache);
+
+/* At present there is only one type of sentinel frame.  */
+
+extern const struct frame_unwind *const sentinel_frame_unwind;
+
+#endif /* !defined (SENTINEL_FRAME_H)  */

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

I've checked this in.

> 2003-01-23  Andrew Cagney  <ac131313@redhat.com>
> 
> 	* sentinel-frame.h, sentinel-frame.c: New files.
> 	* Makefile.in (frame.o): Update dependencies.
> 	(SFILES): Add sentinel-frame.c.
> 	(sentinel_frame_h): Define.
> 	(COMMON_OBS): Add sentinel-frame.o.
> 	(sentinel-frame.o): Specify dependencies.
> 	* frame.c: Include "sentinel-frame.h".
> 	(frame_register_unwind): Rewrite assuming that there is always a a
> 	->next frame.
> 	(frame_register, generic_unwind_get_saved_register): Ditto.
> 	(frame_read_unsigned_register, frame_read_signed_register): Ditto.
> 	(create_sentinel_frame, unwind_to_current_frame): New functions.
> 	(get_current_frame): Rewrite using create_sentinel_frame and
> 	unwind_to_current_frame.  When possible, always create a frame.
> 	(create_new_frame): Set next to the sentinel frame.
> 	(get_next_frame): Rewrite.  Don't go below the level 0 frame.
> 	(deprecated_update_frame_pc_hack): Update the next frame's PC and
> 	ID cache when necessary.
> 	(frame_saved_regs_id_unwind): Use frame_relative_level.
> 	(deprecated_generic_get_saved_register): Use frame_relative_level,
> 	get_frame_saved_regs, get_frame_pc, get_frame_base and
> 	get_next_frame.
> 	(frame_saved_regs_register_unwind): Use get_frame_saved_regs and
> 	frame_register.

now back to kevin's question.

Andrew

Re: [patch/rfc] Add a sentinel frame

[Michal Ludvig]

Andrew Cagney wrote:
> Hello,
> 
> This patch modifies^D^D^D^Dsimplifies the frame code so that there is an 
> extra frame below the innermost frame.  That extra frame maps any 
> request for a register onto the regcache.
> 
> The thing to notice with this patch is how it eliminates the need to 
> special case a level zero frame when fetching a frame's register. 
> Instead, a register can be fetched using the recursive frame_register() 
> method.
> 
> I'll look to commit this in a few days.
> 
> Follow on patches include:
> - convert d10v to the current frame unwind mechanims
> - deprecate methods such as POP_FRAME.
> 
> Andrew

Hi Andrew,
I'm getting lots of internal errors due to the fact that this function 
is called while running x86-64 testsuite on mainline.

> +static void
> +sentinel_frame_pop (struct frame_info *frame,
> +		    void **cache,
> +		    struct regcache *regcache)
> +{
> +  internal_error (__FILE__, __LINE__, "Function sentinal_frame_pop called");
> +}

How can I avoid these errors?

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* (+420) 296.545.373 * http://www.suse.cz

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

> Andrew
> 
> Hi Andrew,
> I'm getting lots of internal errors due to the fact that this function is called while running x86-64 testsuite on mainline.
> 
> +static void
> +sentinel_frame_pop (struct frame_info *frame,
> +            void **cache,
> +            struct regcache *regcache)
> +{
> +  internal_error (__FILE__, __LINE__, "Function sentinal_frame_pop called");
> +}
> 
> How can I avoid these errors?

Can you please post a stack backtrace and a transcript illustrating the 
problem?  The above assertion is correct - for some reason GDB is trying 
to pop the wrong frame :-(

Andrew

Re: [patch/rfc] Add a sentinel frame

[Michal Ludvig]

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

Andrew Cagney wrote:
> Michal Ludvig wrote:
>>
>> Hi Andrew,
>> I'm getting lots of internal errors due to the fact that this function 
>> is called while running x86-64 testsuite on mainline.
>>
>> +static void
>> +sentinel_frame_pop (struct frame_info *frame,
>> +            void **cache,
>> +            struct regcache *regcache)
>> +{
>> +  internal_error (__FILE__, __LINE__, "Function sentinal_frame_pop 
>> called");
>> +}
>>
>> How can I avoid these errors?
> 
> Can you please post a stack backtrace and a transcript illustrating the 
> problem?  The above assertion is correct - for some reason GDB is trying 
> to pop the wrong frame :-(

Hi again,
I got back to the mainline/"sentinel" problem and have found that it 
happens when a program's function is called from a gdb prompt.

To reproduce create a very simple program:
int func(int arg) { return 2*arg; }
int main(int argc) { return func(argc); }

Then run mainline GDB on x86-64:
(gdb) break main
(gdb) run
(gdb) print func(1)
../../gdb-head/gdb/sentinel-frame.c:102: internal-error: Function 
sentinal_frame_pop called
A problem internal to GDB has been detected.  Further
debugging may prove unreliable.
Quit this debugging session? (y or n)

Attached is a backtrace of this failing GDB.
Any ideas?

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* (+420) 296.545.373 * http://www.suse.cz

[-- Attachment #2: gdb-head-failure.txt --]
[-- Type: text/plain, Size: 5668 bytes --]

GNU gdb 5.3.0.90_2003-02-03-cvs
Copyright 2002 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "x86_64-unknown-linux-gnu"...
Core was generated by `./gdb b'.
Program terminated with signal 6, Aborted.
Reading symbols from /lib64/libncurses.so.5...done.
Loaded symbols for /lib64/libncurses.so.5
Reading symbols from /lib64/libm.so.6...done.
Loaded symbols for /lib64/libm.so.6
Reading symbols from /lib64/libdl.so.2...done.
Loaded symbols for /lib64/libdl.so.2
Reading symbols from /lib64/libc.so.6...done.
Loaded symbols for /lib64/libc.so.6
Reading symbols from /lib64/ld-linux-x86-64.so.2...done.
Loaded symbols for /lib64/ld-linux-x86-64.so.2
Reading symbols from /lib64/libthread_db.so.1...done.
Loaded symbols for /lib64/libthread_db.so.1
#0  0x0000002a95a84b39 in kill () from /lib64/libc.so.6
Setting up the environment for debugging gdb.
Breakpoint 1 at 0x4e28de: file ../../gdb-head/gdb/utils.c, line 800.
Breakpoint 2 at 0x43e907: file ../../gdb-head/gdb/cli/cli-cmds.c, line 202.
(top-gdb) bt
#0  0x0000002a95a84b39 in kill () from /lib64/libc.so.6
#1  0x0000002a95a84861 in raise () from /lib64/libc.so.6
#2  0x0000002a95a85e90 in abort () from /lib64/libc.so.6
#3  0x00000000004e27ea in internal_vproblem (problem=0x7a8d40, 
    file=0x66e740 "../../gdb-head/gdb/sentinel-frame.c", line=102, 
    fmt=0x66e780 "Function sentinal_frame_pop called", ap=0x7fbfffe430)
    at ../../gdb-head/gdb/utils.c:769
#4  0x00000000004e2847 in internal_verror (
    file=0x66e740 "../../gdb-head/gdb/sentinel-frame.c", line=102, 
    fmt=0x66e780 "Function sentinal_frame_pop called", ap=0x7fbfffe430)
    at ../../gdb-head/gdb/utils.c:792
#5  0x00000000004e292b in internal_error (
    file=0x66e740 "../../gdb-head/gdb/sentinel-frame.c", line=102, 
    string=0x66e780 "Function sentinal_frame_pop called")
    at ../../gdb-head/gdb/utils.c:801
#6  0x00000000005446d1 in sentinel_frame_pop (frame=0x82c100, cache=0x82c130, 
    regcache=0x850610) at ../../gdb-head/gdb/sentinel-frame.c:102
#7  0x00000000004e6692 in frame_pop (frame=0x82c100)
    at ../../gdb-head/gdb/frame.c:164
#8  0x000000000048a3c4 in normal_stop () at ../../gdb-head/gdb/infrun.c:3113
#9  0x000000000048727c in proceed (addr=4195336, siggnal=TARGET_SIGNAL_0, step=0)
    at ../../gdb-head/gdb/infrun.c:805
#10 0x0000000000484d62 in run_stack_dummy (addr=4195336, buffer=0x9b9ae0)
    at ../../gdb-head/gdb/infcmd.c:1041
#11 0x0000000000465c78 in hand_function_call (function=0x9b99e0, nargs=1, 
    args=0x7fbfffe8e8) at ../../gdb-head/gdb/valops.c:1684
#12 0x0000000000465dcd in call_function_by_hand (function=0x9b99e0, nargs=1, 
    args=0x7fbfffe8e8) at ../../gdb-head/gdb/valops.c:1808
#13 0x0000000000460566 in evaluate_subexp_standard (expect_type=0x0, exp=0x9b9900, 
    pos=0x7fbfffec04, noside=EVAL_NORMAL) at ../../gdb-head/gdb/eval.c:940
#14 0x000000000045e1df in evaluate_subexp (expect_type=0x0, exp=0x6, pos=0xd, 
    noside=EVAL_NORMAL) at ../../gdb-head/gdb/eval.c:70
#15 0x000000000045e3ba in evaluate_expression (exp=0x9b9900)
    at ../../gdb-head/gdb/eval.c:159
#16 0x0000000000470051 in print_command_1 (exp=0x800cd2 "func(1)", inspect=0, 
    voidprint=1) at ../../gdb-head/gdb/printcmd.c:907
#17 0x0000000000470198 in print_command (exp=0x800cd2 "func(1)", from_tty=1)
    at ../../gdb-head/gdb/printcmd.c:951
#18 0x000000000043a2e4 in do_cfunc (c=0x811960, args=0x800cd2 "func(1)", 
    from_tty=1) at ../../gdb-head/gdb/cli/cli-decode.c:53
#19 0x000000000043c921 in cmd_func (cmd=0x811960, args=0x800cd2 "func(1)", 
    from_tty=1) at ../../gdb-head/gdb/cli/cli-decode.c:1523
#20 0x00000000004dfb5c in execute_command (p=0x800cd8 ")", from_tty=1)
    at ../../gdb-head/gdb/top.c:711
#21 0x00000000004dfd34 in command_loop () at ../../gdb-head/gdb/top.c:792
#22 0x0000000000492824 in current_interp_command_loop ()
    at ../../gdb-head/gdb/interps.c:279
#23 0x0000000000437dcd in captured_command_loop (data=0x0)
    at ../../gdb-head/gdb/main.c:100
#24 0x00000000004df74f in do_catch_errors (uiout=0x82efb0, data=0x7fbfffefd0)
    at ../../gdb-head/gdb/top.c:492
#25 0x00000000004df5ae in catcher (func=0x4df726 <do_catch_errors>, 
    func_uiout=0x82efb0, func_args=0x7fbfffefd0, func_val=0x7fbfffeff0, 
    func_caught=0x7fbfffefec, errstring=0x60e62d "", mask=6)
    at ../../gdb-head/gdb/top.c:424
#26 0x00000000004df7a0 in catch_errors (func=0x437dbc <captured_command_loop>, 
    func_args=0x0, errstring=0x60e62d "", mask=6) at ../../gdb-head/gdb/top.c:504
#27 0x0000000000438b20 in captured_main (data=0x7fbffff490)
    at ../../gdb-head/gdb/main.c:797
#28 0x00000000004df74f in do_catch_errors (uiout=0x7a3860, data=0x7fbffff420)
    at ../../gdb-head/gdb/top.c:492
#29 0x00000000004df5ae in catcher (func=0x4df726 <do_catch_errors>, 
    func_uiout=0x7a3860, func_args=0x7fbffff420, func_val=0x7fbffff440, 
    func_caught=0x7fbffff43c, errstring=0x60e62d "", mask=6)
    at ../../gdb-head/gdb/top.c:424
#30 0x00000000004df7a0 in catch_errors (func=0x437dfc <captured_main>, 
    func_args=0x7fbffff490, errstring=0x60e62d "", mask=6)
    at ../../gdb-head/gdb/top.c:504
#31 0x0000000000438b53 in gdb_main (args=0x7fbffff490)
    at ../../gdb-head/gdb/main.c:806
#32 0x0000000000437db8 in main (argc=2, argv=0x7fbffff528)
    at ../../gdb-head/gdb/gdb.c:33
#33 0x0000002a95a74035 in __libc_start_main () from /lib64/libc.so.6
#34 0x0000000000437cda in _start ()
(top-gdb) 

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

(I hate jet lag, I'm never up at 7am, well ok ...)

> I got back to the mainline/"sentinel" problem and have found that it happens when a program's function is called from a gdb prompt.
> 
> To reproduce create a very simple program:
> int func(int arg) { return 2*arg; }
> int main(int argc) { return func(argc); }
> 
> Then run mainline GDB on x86-64:
> (gdb) break main
> (gdb) run
> (gdb) print func(1)
> ../../gdb-head/gdb/sentinel-frame.c:102: internal-error: Function sentinal_frame_pop called
> A problem internal to GDB has been detected.  Further
> debugging may prove unreliable.
> Quit this debugging session? (y or n)
> 
> Attached is a backtrace of this failing GDB.
> Any ideas?

Yes!


> #6  0x00000000005446d1 in sentinel_frame_pop (frame=0x82c100, cache=0x82c130, 
>     regcache=0x850610) at ../../gdb-head/gdb/sentinel-frame.c:102

At this point GDB is hosed.

As I mentioned before, popping the sentinal frame is meaningless so the 
question is, where did that frame come from.

A wild guess is that it is trying to pop the dummy frame having finished 
the inferior function call.  A confirmation is:

(gdb) break func
(gdb) print func(1)

It should manage to stop in func, the stack being something like 
(assuming bt doesn't also internal error :-):

(gdb) bt
.... func ...
.... <dummy-frame> ...
.... main ...

Returning from func(), causing the dummy-frame to be discarded should 
then trigger things:

(gdb) finish
... barf ...

Can you confirm that the PC for this frame is falling in the stack dummy?


> #7  0x00000000004e6692 in frame_pop (frame=0x82c100)
>     at ../../gdb-head/gdb/frame.c:164


> #8  0x000000000048a3c4 in normal_stop () at ../../gdb-head/gdb/infrun.c:3113

The code reads:

   if (stop_stack_dummy)
     {
       /* Pop the empty frame that contains the stack dummy.  POP_FRAME
          ends with a setting of the current frame, so we can use that
          next. */
       frame_pop (get_current_frame ());
       /* Set stop_pc to what it was before we called the function.
          Can't rely on restore_inferior_status because that only gets
          called if we don't stop in the called function.  */
       stop_pc = read_pc ();
       select_frame (get_current_frame ());
     }

Hmm, not so good.  I was expecting something involving sentinal frames. 
  Anyway ...

get_current_frame() does the sequence:

   if (current_frame == NULL)
     {

This is where the sentinel frame comes from.

       struct frame_info *sentinel_frame =
         create_sentinel_frame (current_regcache);

This is where it tries to unwind it back to the current frame (a dummy 
frame in this case). Note that unwind_to_current_frame() calls 
get_prev_frame().

       if (catch_exceptions (uiout, unwind_to_current_frame, sentinel_frame,
                             NULL, RETURN_MASK_ERROR) != 0)
         {

And if it fails, it does this:

           /* Oops! Fake a current frame?  Is this useful?  It has a PC
              of zero, for instance.  */
           current_frame = sentinel_frame;
         }
     }

So lets assume that get_prev_frame() is failing.  It can do it two ways:

- noisily via an error() call such as when a memory read fails
(I don't think it is this 'cos we'd see that error).
- silently because get_prev_frame() thinks that chaining is invalid
(more likely)

So I think it is one of these tests going awall:

   if (next_frame->level >= 0
       && !backtrace_below_main
       && inside_main_func (get_frame_pc (next_frame)))
     /* Don't unwind past main(), bug always unwind the sentinel frame.
        Note, this is done _before_ the frame has been marked as
        previously unwound.  That way if the user later decides to
        allow unwinds past main(), that just happens.  */
     return NULL;

   /* If we're inside the entry file, it isn't valid.  */
   /* NOTE: drow/2002-12-25: should there be a way to disable this
      check?  It assumes a single small entry file, and the way some
      debug readers (e.g.  dbxread) figure out which object is the
      entry file is somewhat hokey.  */
   /* NOTE: cagney/2003-01-10: If there is a way of disabling this test
      then it should probably be moved to before the ->prev_p test,
      above.  */
   if (inside_entry_file (get_frame_pc (next_frame)))
       return NULL;

The second looks worrying (the dummy frame breakpoint lives in the entry 
file ...).  Perhaphs something like:

if (dummy_frame_p (get_frame_pc (next_frame) != NULL
     && inside_entry_file (get_frame_pc (next_frame))
   return NULL;

Andrew

Re: [patch/rfc] Add a sentinel frame

[Daniel Jacobowitz]

On Wed, Feb 19, 2003 at 08:32:32AM -0500, Andrew Cagney wrote:
> So I think it is one of these tests going awall:
> 
>   if (next_frame->level >= 0
>       && !backtrace_below_main
>       && inside_main_func (get_frame_pc (next_frame)))
>     /* Don't unwind past main(), bug always unwind the sentinel frame.
>        Note, this is done _before_ the frame has been marked as
>        previously unwound.  That way if the user later decides to
>        allow unwinds past main(), that just happens.  */
>     return NULL;
> 
>   /* If we're inside the entry file, it isn't valid.  */
>   /* NOTE: drow/2002-12-25: should there be a way to disable this
>      check?  It assumes a single small entry file, and the way some
>      debug readers (e.g.  dbxread) figure out which object is the
>      entry file is somewhat hokey.  */
>   /* NOTE: cagney/2003-01-10: If there is a way of disabling this test
>      then it should probably be moved to before the ->prev_p test,
>      above.  */
>   if (inside_entry_file (get_frame_pc (next_frame)))
>       return NULL;
> 
> The second looks worrying (the dummy frame breakpoint lives in the entry 
> file ...).  Perhaphs something like:
> 
> if (dummy_frame_p (get_frame_pc (next_frame) != NULL
>     && inside_entry_file (get_frame_pc (next_frame))
>   return NULL;

Hrm, shouldn't we have already detected the dummy frame at this point? 
That's what happens on i386 IIRC...

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

> On Wed, Feb 19, 2003 at 08:32:32AM -0500, Andrew Cagney wrote:
> 
>> So I think it is one of these tests going awall:
>> 
>>   if (next_frame->level >= 0
>>       && !backtrace_below_main
>>       && inside_main_func (get_frame_pc (next_frame)))
>>     /* Don't unwind past main(), bug always unwind the sentinel frame.
>>        Note, this is done _before_ the frame has been marked as
>>        previously unwound.  That way if the user later decides to
>>        allow unwinds past main(), that just happens.  */
>>     return NULL;
>> 
>>   /* If we're inside the entry file, it isn't valid.  */
>>   /* NOTE: drow/2002-12-25: should there be a way to disable this
>>      check?  It assumes a single small entry file, and the way some
>>      debug readers (e.g.  dbxread) figure out which object is the
>>      entry file is somewhat hokey.  */
>>   /* NOTE: cagney/2003-01-10: If there is a way of disabling this test
>>      then it should probably be moved to before the ->prev_p test,
>>      above.  */
>>   if (inside_entry_file (get_frame_pc (next_frame)))
>>       return NULL;
>> 
>> The second looks worrying (the dummy frame breakpoint lives in the entry 
>> file ...).  Perhaphs something like:
>> 
>> if (dummy_frame_p (get_frame_pc (next_frame) != NULL
>>     && inside_entry_file (get_frame_pc (next_frame))
>>   return NULL;
> 
> 
> Hrm, shouldn't we have already detected the dummy frame at this point? 

No.  GDB is trying to perform:

	pop_frame (get_current_frame())

with the assumption that it has a dummy frame and get_current_frame() 
will return it.

> That's what happens on i386 IIRC...

Chance has it inside_entry_file(entry-point) returns false.  Knowing 
that function, I'd not be suprized.

The `bug' is an unexpected interaction between get_prev_frame's 
end-of-frame test and the change to unwind from the sentinel frame. 
Previously the end-of-frame test wasn't applied to the current frame.

Andrew

Re: [patch/rfc] Add a sentinel frame

[Daniel Jacobowitz]

On Wed, Feb 19, 2003 at 11:51:40AM -0500, Andrew Cagney wrote:
> >On Wed, Feb 19, 2003 at 08:32:32AM -0500, Andrew Cagney wrote:
> >
> >>So I think it is one of these tests going awall:
> >>
> >>  if (next_frame->level >= 0
> >>      && !backtrace_below_main
> >>      && inside_main_func (get_frame_pc (next_frame)))
> >>    /* Don't unwind past main(), bug always unwind the sentinel frame.
> >>       Note, this is done _before_ the frame has been marked as
> >>       previously unwound.  That way if the user later decides to
> >>       allow unwinds past main(), that just happens.  */
> >>    return NULL;
> >>
> >>  /* If we're inside the entry file, it isn't valid.  */
> >>  /* NOTE: drow/2002-12-25: should there be a way to disable this
> >>     check?  It assumes a single small entry file, and the way some
> >>     debug readers (e.g.  dbxread) figure out which object is the
> >>     entry file is somewhat hokey.  */
> >>  /* NOTE: cagney/2003-01-10: If there is a way of disabling this test
> >>     then it should probably be moved to before the ->prev_p test,
> >>     above.  */
> >>  if (inside_entry_file (get_frame_pc (next_frame)))
> >>      return NULL;
> >>
> >>The second looks worrying (the dummy frame breakpoint lives in the entry 
> >>file ...).  Perhaphs something like:
> >>
> >>if (dummy_frame_p (get_frame_pc (next_frame) != NULL
> >>    && inside_entry_file (get_frame_pc (next_frame))
> >>  return NULL;
> >
> >
> >Hrm, shouldn't we have already detected the dummy frame at this point? 
> 
> No.  GDB is trying to perform:
> 
> 	pop_frame (get_current_frame())
> 
> with the assumption that it has a dummy frame and get_current_frame() 
> will return it.
> 
> >That's what happens on i386 IIRC...

I thought that we wouldn't reach frame_chain_valid if the next frame
was a dummy frame.  Hmm, that only seems to happen for deprecated
generic dummy frames:

  if (DEPRECATED_USE_GENERIC_DUMMY_FRAMES
      && DEPRECATED_PC_IN_CALL_DUMMY (get_frame_pc (fi), 0, 0))
    return 1;

Oh I didn't realize the contents of frame_chain_valid had ended up
repeated in get_prev_frame, I've been looking at the wrong function.
That's why I didn't understand you.  Should the check above exist in
get_prev_frame also?

[Why does this logic need to be in more than one place?]

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

>> No.  GDB is trying to perform:
>> 
>> 	pop_frame (get_current_frame())
>> 
>> with the assumption that it has a dummy frame and get_current_frame() 
>> will return it.
>> 
> 
>> >That's what happens on i386 IIRC...
> 
> 
> I thought that we wouldn't reach frame_chain_valid if the next frame
> was a dummy frame.  Hmm, that only seems to happen for deprecated
> generic dummy frames:

The variable `use_generic_dummy_frames' is deprecated because it is 
redundant.  All targets should use generic dummy frames.  Yes, the 
variable name is screwed up :-(

>   if (DEPRECATED_USE_GENERIC_DUMMY_FRAMES
>       && DEPRECATED_PC_IN_CALL_DUMMY (get_frame_pc (fi), 0, 0))
>     return 1;

> Oh I didn't realize the contents of frame_chain_valid had ended up
> repeated in get_prev_frame, I've been looking at the wrong function.
> That's why I didn't understand you.  Should the check above exist in
> get_prev_frame also?

When you first committed that stuff, I warned you that would happen :-)
The above test handled differently.

> [Why does this logic need to be in more than one place?]

Because frame_chain_valid() is only there to keep legacy code working. 
Need to rename it, need to deprecate the rest of those old methods.

Andrew

Re: [patch/rfc] Add a sentinel frame

[Daniel Jacobowitz]

On Wed, Feb 19, 2003 at 12:15:55PM -0500, Andrew Cagney wrote:
> >  if (DEPRECATED_USE_GENERIC_DUMMY_FRAMES
> >      && DEPRECATED_PC_IN_CALL_DUMMY (get_frame_pc (fi), 0, 0))
> >    return 1;
> 
> >Oh I didn't realize the contents of frame_chain_valid had ended up
> >repeated in get_prev_frame, I've been looking at the wrong function.
> >That's why I didn't understand you.  Should the check above exist in
> >get_prev_frame also?
> 
> When you first committed that stuff, I warned you that would happen :-)
> The above test handled differently.

Hey, you can't blame me for this bit.  I didn't add that check for
DEPRECATED_PC_IN_CALL_DUMMY, it was already there in
generic_frame_chain_valid.

> >[Why does this logic need to be in more than one place?]
> 
> Because frame_chain_valid() is only there to keep legacy code working. 
> Need to rename it, need to deprecate the rest of those old methods.

That doesn't answer my question though.

I don't understand why you have to move the logic out of
frame_chain_valid instead of _using_ it from get_prev_frame.  Does it
not have the interface you want?  Does it do something grubby in frames
that it shouldn't?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

>> When you first committed that stuff, I warned you that would happen :-)
>> The above test handled differently.
> 
> 
> Hey, you can't blame me for this bit.  I didn't add that check for
> DEPRECATED_PC_IN_CALL_DUMMY, it was already there in
> generic_frame_chain_valid.

I'm refering to frame_chain_valid(), a small part of which you changed. 
  The useful bits (your changes) were copied to the rewritten 
get_prev_frame.  When frame_chain_valid() is deleted, that duplication 
will go away.  To see what's wrong with frame_chain_valid() see 
legacy_get_prev_frame.

Andrew

Re: [patch/rfc] Add a sentinel frame

[Daniel Jacobowitz]

On Wed, Feb 19, 2003 at 12:51:18PM -0500, Andrew Cagney wrote:
> 
> >>When you first committed that stuff, I warned you that would happen :-)
> >>The above test handled differently.
> >
> >
> >Hey, you can't blame me for this bit.  I didn't add that check for
> >DEPRECATED_PC_IN_CALL_DUMMY, it was already there in
> >generic_frame_chain_valid.
> 
> I'm refering to frame_chain_valid(), a small part of which you changed. 
>  The useful bits (your changes) were copied to the rewritten 
> get_prev_frame.  When frame_chain_valid() is deleted, that duplication 
> will go away.  To see what's wrong with frame_chain_valid() see 
> legacy_get_prev_frame.

I'm slow.  Could you explain the problem?  There's a comment about
things being deduced there which is no longer true, and a comment about
leaves of main that I can't make heads nor tails of but I don't think
it applies.

I still don't see why you copied the code instead of using
frame_chain_valid, or why you copied only some of it (not including the
dummy frame checks which I think would solve this).

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

> On Wed, Feb 19, 2003 at 12:51:18PM -0500, Andrew Cagney wrote:
> 
>> 
> 
>> >>When you first committed that stuff, I warned you that would happen :-)
>> >>The above test handled differently.
> 
>> >
>> >
>> >Hey, you can't blame me for this bit.  I didn't add that check for
>> >DEPRECATED_PC_IN_CALL_DUMMY, it was already there in
>> >generic_frame_chain_valid.
> 
>> 
>> I'm refering to frame_chain_valid(), a small part of which you changed. 
>>  The useful bits (your changes) were copied to the rewritten 
>> get_prev_frame.  When frame_chain_valid() is deleted, that duplication 
>> will go away.  To see what's wrong with frame_chain_valid() see 
>> legacy_get_prev_frame.
> 
> 
> I'm slow.  Could you explain the problem?  There's a comment about
> things being deduced there which is no longer true, and a comment about
> leaves of main that I can't make heads nor tails of but I don't think
> it applies.

Where does one start?  it calls pc_unwind; it calls get_frame_pc; it 
calls get_frame_base yet we passed in the frame base; it does tests in 
the wrong order, carefully compare it to get_prev_frame; the lack of 
frame-id; the fact that, on the sparc, the fp that is passed in was 
bogus; knowing that all the function was ment to the general confusion 
over unwinding the pc or fp first; knowing that frame_chain_valid() 
started out as an equivalent to frame_id_p().

Contrast that to the new get_prev_frame() were everything is handled at 
the one level.

As I said, to understand this, you're really going to have to study the 
code.

Andrew

Re: [patch/rfc] Add a sentinel frame

[Daniel Jacobowitz]

On Wed, Feb 19, 2003 at 01:40:56PM -0500, Andrew Cagney wrote:
> >On Wed, Feb 19, 2003 at 12:51:18PM -0500, Andrew Cagney wrote:
> >
> >>
> >
> >>>>When you first committed that stuff, I warned you that would happen :-)
> >>>>The above test handled differently.
> >
> >>>
> >>>
> >>>Hey, you can't blame me for this bit.  I didn't add that check for
> >>>DEPRECATED_PC_IN_CALL_DUMMY, it was already there in
> >>>generic_frame_chain_valid.
> >
> >>
> >>I'm refering to frame_chain_valid(), a small part of which you changed. 
> >> The useful bits (your changes) were copied to the rewritten 
> >>get_prev_frame.  When frame_chain_valid() is deleted, that duplication 
> >>will go away.  To see what's wrong with frame_chain_valid() see 
> >>legacy_get_prev_frame.
> >
> >
> >I'm slow.  Could you explain the problem?  There's a comment about
> >things being deduced there which is no longer true, and a comment about
> >leaves of main that I can't make heads nor tails of but I don't think
> >it applies.
> 
> Where does one start?  it calls pc_unwind; it calls get_frame_pc; it 
> calls get_frame_base yet we passed in the frame base; it does tests in 
> the wrong order, carefully compare it to get_prev_frame; the lack of 
> frame-id; the fact that, on the sparc, the fp that is passed in was 
> bogus; knowing that all the function was ment to the general confusion 
> over unwinding the pc or fp first; knowing that frame_chain_valid() 
> started out as an equivalent to frame_id_p().

Offhand, we do _not_ pass in the frame base - we base in the base for
the next frame.  get_prev_frame makes the same get_frame_pc call.

You've lost the call to inside_entry_func.  Why?  You've changed the
inside_entry_file check to check the PC for the next frame instead of
the forthcoming frame, which is not at all the same thing.  Why?

You've lost the hook for an architecture-specific FRAME_CHAIN_VALID_P. 
I've asked you about this before and I still don't understand where you
want that logic to go.  The impression I've gotten is that you want it
to vanish, and that doesn't make any sense.

I'm not sure why the new order is better.

Duplicating the code has just made it harder for me to spot what I
consider under-explained changes in the logic.

> Contrast that to the new get_prev_frame() were everything is handled at 
> the one level.
> 
> As I said, to understand this, you're really going to have to study the 
> code.

I have, at length.  It hasn't been helping.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

> Offhand, we do _not_ pass in the frame base - we base in the base for
> the next frame.  get_prev_frame makes the same get_frame_pc call.

Oops, yes.  Anyway.

The old code would `randomly' call either frame_saved_pc or frame_chain. 
  I mean randomly, you'd think you had it licked and then discover that 
for some edge case the two calls were reversed.

The new get_prev_frame carefully orders these calls so that the sequence:

	frame_pc_unwind()
then
	frame_id_unwind()

always occures.

> You've lost the call to inside_entry_func.  Why?  You've changed the
> inside_entry_file check to check the PC for the next frame instead of
> the forthcoming frame, which is not at all the same thing.  Why?

Dig up old notes.

This is the test you added.  It stops the unwind past main:

   if (next_frame->level >= 0
       && !backtrace_below_main
       && inside_main_func (get_frame_pc (next_frame)))
     /* Don't unwind past main(), bug always unwind the sentinel frame.
        Note, this is done _before_ the frame has been marked as
        previously unwound.  That way if the user later decides to
        allow unwinds past main(), that just happens.  */
     return NULL;

It occures first (as it should).  It occures before any 
frame_id_unwind() as needed by frame_chain_valid.  It also occures 
before the test:

   /* Only try to do the unwind once.  */
   if (next_frame->prev_p)
     return next_frame->prev;
   next_frame->prev_p = 1;

so that frame flush code was eliminated (ya!).

On the other hand, if GDB is to unwind past main (presumably, if 
s/backtrace_below_main/unwind_past_main/ is false) it does the test:

   if (inside_entry_file (get_frame_pc (next_frame)))

(note the comments about how, if this becomes optional, it should also 
be moved to before `next_frame->prev_p = 1').

Anyway, now that missing test.  frame_chain_valid() also contained:

   /* If we're inside the entry file, it isn't valid.  */
   if (inside_entry_file (frame_pc_unwind (fi)))
       return 0;

Note the frame_pc_unwind().  This test is looking one level along the 
stack frame to determine if it should unwind to that level.  That is, 
when FI->prev->pc is in the entry_file, don't unwind to FI->prev.  The 
problem is, FI->prev->pc is in entry_file when FI->pc is in main.

Even when unwind-past-main is disabled, GDB refuses to unwind past main! 
  Consequently, on the branch, I dropped the test.

(It also unwinds the PC when we're probably not ready).

--

As things progress, and more targets switch to the new code, the tests 
in get_prev_frame will most likely evolve.  However, I don't know that 
we want to be adding tests without hard evidence that they are needed :-/

Having said that, sanity checks that the frame didn't go backwards:
	!frame_id_inner (frame_id, get_frame_id (next_frame))?
and that they changed:
	!frame_id_eq (frame_id, get_frame_id (next_frame))?
probably wouldn't hurt.

> You've lost the hook for an architecture-specific FRAME_CHAIN_VALID_P. 
> I've asked you about this before and I still don't understand where you
> want that logic to go.  The impression I've gotten is that you want it
> to vanish, and that doesn't make any sense.

If the frame isn't valid, the per architecture frame_id_unwind() returns 
a null frame ID (tested using frame_id_p()).  No need for that redundant 
test.

Andrew

Re: [patch/rfc] Add a sentinel frame

[Daniel Jacobowitz]

On Wed, Feb 19, 2003 at 03:27:36PM -0500, Andrew Cagney wrote:
> >You've lost the call to inside_entry_func.  Why?  You've changed the
> >inside_entry_file check to check the PC for the next frame instead of
> >the forthcoming frame, which is not at all the same thing.  Why?
> 
> Dig up old notes.
> 
> This is the test you added.  It stops the unwind past main:
> 
>   if (next_frame->level >= 0
>       && !backtrace_below_main
>       && inside_main_func (get_frame_pc (next_frame)))
>     /* Don't unwind past main(), bug always unwind the sentinel frame.
>        Note, this is done _before_ the frame has been marked as
>        previously unwound.  That way if the user later decides to
>        allow unwinds past main(), that just happens.  */
>     return NULL;
> 
> It occures first (as it should).  It occures before any 
> frame_id_unwind() as needed by frame_chain_valid.  It also occures 
> before the test:
> 
>   /* Only try to do the unwind once.  */
>   if (next_frame->prev_p)
>     return next_frame->prev;
>   next_frame->prev_p = 1;
> 
> so that frame flush code was eliminated (ya!).

OK.

> On the other hand, if GDB is to unwind past main (presumably, if 
> s/backtrace_below_main/unwind_past_main/ is false) it does the test:
> 
>   if (inside_entry_file (get_frame_pc (next_frame)))
> 
> (note the comments about how, if this becomes optional, it should also 
> be moved to before `next_frame->prev_p = 1').
> 
> Anyway, now that missing test.  frame_chain_valid() also contained:
> 
>   /* If we're inside the entry file, it isn't valid.  */
>   if (inside_entry_file (frame_pc_unwind (fi)))
>       return 0;
> 
> Note the frame_pc_unwind().  This test is looking one level along the 
> stack frame to determine if it should unwind to that level.  That is, 
> when FI->prev->pc is in the entry_file, don't unwind to FI->prev.  The 
> problem is, FI->prev->pc is in entry_file when FI->pc is in main.
> 
> Even when unwind-past-main is disabled, GDB refuses to unwind past main! 
>  Consequently, on the branch, I dropped the test.
> 
> (It also unwinds the PC when we're probably not ready).

Oh, but you're misunderstanding.  There's more than one frame in there. 
The call stack in glibc looks like:
  _start
  calls __libc_start_main
  calls main

_start is written in assembly; it generally doesn't have a frame worth
talking about.  Even if we want to show __libc_start_main, we can't
safely backtrace into _start.  That's what the inside_entry_file
(frame_pc_unwind (fi)) is for.

Now, if we want to do this anyway, that's different.  But it's a
change, not a redundancy.


The missing test I mentioned above inside_entry_func, not
inside_entry_file.  Where'd that go?

> As things progress, and more targets switch to the new code, the tests 
> in get_prev_frame will most likely evolve.  However, I don't know that 
> we want to be adding tests without hard evidence that they are needed :-/
> 
> Having said that, sanity checks that the frame didn't go backwards:
> 	!frame_id_inner (frame_id, get_frame_id (next_frame))?

Yes.

> and that they changed:
> 	!frame_id_eq (frame_id, get_frame_id (next_frame))?

Can we do that?  Hmm, we probably can.  A frame ID has a PC in it and a
stack pointer, and if neither has changed we're probably stuck in a
rut.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

> Oh, but you're misunderstanding.  There's more than one frame in there. 
> The call stack in glibc looks like:
>   _start
>   calls __libc_start_main
>   calls main

Nope, that assumes glibc.  Remember, i debugged this using the d10v.

> _start is written in assembly; it generally doesn't have a frame worth
> talking about.  Even if we want to show __libc_start_main, we can't
> safely backtrace into _start.  That's what the inside_entry_file
> (frame_pc_unwind (fi)) is for.

Why not?  If someone wants to do that, why should we stand in their way :-)

> Now, if we want to do this anyway, that's different.  But it's a
> change, not a redundancy.

The entire get_prev_frame is change.  It should only affect targets 
converted to the new code though (if it wasn't for this sentinel edge 
condition).

> The missing test I mentioned above inside_entry_func, not
> inside_entry_file.  Where'd that go?

Left until something that does need it surfaces.

>> As things progress, and more targets switch to the new code, the tests 
>> in get_prev_frame will most likely evolve.  However, I don't know that 
>> we want to be adding tests without hard evidence that they are needed :-/
>> 
>> Having said that, sanity checks that the frame didn't go backwards:
>> 	!frame_id_inner (frame_id, get_frame_id (next_frame))?
> 
> 
> Yes.
> 
> 
>> and that they changed:
>> 	!frame_id_eq (frame_id, get_frame_id (next_frame))?
> 
> 
> Can we do that?  Hmm, we probably can.  A frame ID has a PC in it and a
> stack pointer, and if neither has changed we're probably stuck in a
> rut.

legacy_get_prev_frame() contains exactly that test.

Andrew

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

Anyway, do you now see why trying to directly call frame_chain_valid() 
wouldn't be helpful to the rewritten get_prev_frame?

Andrew

Re: [patch/rfc] Add a sentinel frame

[Daniel Jacobowitz]

On Wed, Feb 19, 2003 at 04:25:52PM -0500, Andrew Cagney wrote:
> 
> >Oh, but you're misunderstanding.  There's more than one frame in there. 
> >The call stack in glibc looks like:
> >  _start
> >  calls __libc_start_main
> >  calls main
> 
> Nope, that assumes glibc.  Remember, i debugged this using the d10v.

No, it doesn't assume glibc, it uses glibc as an example.  That check
would prevent backtraces entering _start; on the d10v, that would
override backtrace-beyond-main, but on a glibc system, it wouldn't.

> >_start is written in assembly; it generally doesn't have a frame worth
> >talking about.  Even if we want to show __libc_start_main, we can't
> >safely backtrace into _start.  That's what the inside_entry_file
> >(frame_pc_unwind (fi)) is for.
> 
> Why not?  If someone wants to do that, why should we stand in their way :-)

... uhm, I guess, I don't really find that convincing.  The check was
there because it won't work.  But now that it's not the default I don't
care if you want to break it.

> >The missing test I mentioned above inside_entry_func, not
> >inside_entry_file.  Where'd that go?
> 
> Left until something that does need it surfaces.

If you're going to yank code that way please add a comment saying so,
before you yank frame_chain_valid as dead and someone else discovers
all the inside_entry_func support is now dead code and purges it.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] Add a sentinel frame

[Daniel Jacobowitz]

On Wed, Feb 19, 2003 at 04:50:06PM -0500, Andrew Cagney wrote:
> Anyway, do you now see why trying to directly call frame_chain_valid() 
> wouldn't be helpful to the rewritten get_prev_frame?

Yes, I think I do now.  Thanks.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] Add a sentinel frame

[Michal Ludvig]

Andrew Cagney wrote:
>> Then run mainline GDB on x86-64:
>> (gdb) break main
>> (gdb) run
>> (gdb) print func(1)
>> ../../gdb-head/gdb/sentinel-frame.c:102: internal-error: Function 
>> sentinal_frame_pop called
>> A problem internal to GDB has been detected.  Further
>> debugging may prove unreliable.
>> Quit this debugging session? (y or n)
>>
>> Attached is a backtrace of this failing GDB.
>> Any ideas?
> 
> Yes!
> 
>> #6  0x00000000005446d1 in sentinel_frame_pop (frame=0x82c100, 
>> cache=0x82c130,     regcache=0x850610) at 
>> ../../gdb-head/gdb/sentinel-frame.c:102
> 
> 
> At this point GDB is hosed.
> 
> As I mentioned before, popping the sentinal frame is meaningless so the 
> question is, where did that frame come from.
> 
> A wild guess is that it is trying to pop the dummy frame having finished 
> the inferior function call.  A confirmation is:
> 
> (gdb) break func
> (gdb) print func(1)
> 
> It should manage to stop in func, the stack being something like 
> (assuming bt doesn't also internal error :-):
> 
> (gdb) bt
> .... func ...
> .... <dummy-frame> ...
> .... main ...

Hmm...

(gdb) b func
Breakpoint 1 at 0x4000040f: file prog.c, line 1.
(gdb) bt
#0  func (arg=1) at prog.c:1
#1  <function called from gdb>
#2  func (arg=1) at prog.c:1
Cannot access memory at address 0x3320303236383ae0

Ad #2 - I was in main before 'print func(1)', not in func()...

> Returning from func(), causing the dummy-frame to be discarded should 
> then trigger things:
> 
> (gdb) finish
> ... barf ...

(gdb) fin
Run till exit from #0  func (arg=1) at prog.c:1
/ttt/64/src/gdb/sentinel-frame.c:102: internal-error: Function 
sentinal_frame_pop called
[...]

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* (+420) 296.545.373 * http://www.suse.cz

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

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

Michael, try the attached patch.

Andrew

[-- Attachment #2: mailbox-message://ac131313@movemail/fsf/gdb/patches#31479902 --]
[-- Type: message/rfc822, Size: 6393 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 248 bytes --]

> Hello,
> 
> This improves the checks in get_prev_frame() that look for stuff like the top-of-stack or a corrupt stack.
> 
> d10v (which uses this) showed no regressions, neither did i386.
> 
> I'll commit `tomorrow'.
> 
> Andrew

With patch....


[-- Attachment #2.1.2: diffs --]
[-- Type: text/plain, Size: 2983 bytes --]

2003-02-24  Andrew Cagney  <cagney@redhat.com>

	* frame.c (get_prev_frame): Add comment on check for
	inside_entry_func. Only check for inside_entry_file when not a
	dummy and not a sentinel.  Check that the new frame is not inner
	to the old frame.

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.67
diff -u -r1.67 frame.c
--- frame.c	20 Feb 2003 16:35:51 -0000	1.67
+++ frame.c	25 Feb 2003 03:51:35 -0000
@@ -1230,7 +1230,6 @@
     return next_frame->prev;
   next_frame->prev_p = 1;
 
-  /* If we're inside the entry file, it isn't valid.  */
   /* NOTE: drow/2002-12-25: should there be a way to disable this
      check?  It assumes a single small entry file, and the way some
      debug readers (e.g.  dbxread) figure out which object is the
@@ -1238,8 +1237,26 @@
   /* NOTE: cagney/2003-01-10: If there is a way of disabling this test
      then it should probably be moved to before the ->prev_p test,
      above.  */
-  if (inside_entry_file (get_frame_pc (next_frame)))
-      return NULL;
+  /* If we're inside the entry file, it isn't valid.  Don't apply this
+     test to a dummy frame - dummy frame PC's typically land in the
+     entry file.  Don't apply this test to the sentinel frame.
+     Sentinel frames should always be allowed to unwind.  */
+  if (next_frame->type != DUMMY_FRAME && next_frame->level >= 0
+      && inside_entry_file (get_frame_pc (next_frame)))
+    return NULL;
+
+#if 0
+  /* NOTE: cagney/2003-02-25: Don't enable until someone has found
+     evidence that this is needed.  */
+  /* If we're already inside the entry function for the main objfile,
+     then it isn't valid.  Don't apply this test to a dummy frame -
+     dummy frame PC's typically land in the entry func.  Don't apply
+     this test to the sentinel frame.  Sentinel frames should always
+     be allowed to unwind.  */
+  if (next_frame->type != DUMMY_FRAME && next_frame->level >= 0
+      && inside_entry_func (get_frame_pc (fi)))
+    return 0;
+#endif
 
   /* If any of the old frame initialization methods are around, use
      the legacy get_prev_frame method.  Just don't try to unwind a
@@ -1301,6 +1318,16 @@
     struct frame_id id = frame_id_unwind (next_frame);
     if (!frame_id_p (id))
       return NULL;
+    /* Check that the new frame isn't inner to (younger, below, next)
+       the old frame - we've not gone backwards.  Ignore the sentinel
+       frame where weird things happen.  */
+    if (next_frame->level >= 0
+	&& frame_id_inner (id, get_frame_id (next_frame)))
+      error ("Unwound frame inner to selected frame (corrupt stack?)");
+    /* Note that, due to frameless functions, the stronger test of the
+       new frame being outer to the old frame can't be used -
+       frameless functions differ by only their PC value.  Ignore the
+       sentinel frame where weird things happen.  */
     prev_frame->frame = id.base;
   }
 

Re: [patch/rfc] Add a sentinel frame

[Michal Ludvig]

Andrew Cagney wrote:
> Michael, try the attached patch.

It makes no difference, unfortunately ;-(

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* (+420) 296.545.373 * http://www.suse.cz

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

So, the new broken behavior or the old broken behavior?

Anyway, I've got an x86-64 gdb.  You wouldn't have some patches to fix 
sware's expect would you?  Sware's expect doesn't appear to work :-(

Andrew

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

Michael,

A shallow analysis of the problem is that, when the PC is in the frame 
dummy, read_fp() (i.e., cfi_read_fp()) returns NULL (an invalid frame). 
  The new code is checking for this and bailing out.  Disabling the test 
doesn't exactly improve things:

(gdb) finish
Inside legacy frame
Run till exit from #0  marker2 (a=99) at 
/home/cagney/PENDING/2003-02-24-prev-checks/src/gdb/testsuite/gdb.base/break.c:49
cfi_read_fp -> 0x0000000000000000
Can't pop dummy frame!
(gdb)

since the search for a NULL frame in the dummy_frame_stack will fail 
(see dummy_frame_pop).

Andrew

Re: [patch/rfc] Add a sentinel frame

[Michal Ludvig]

Andrew Cagney wrote:
> Anyway, I've got an x86-64 gdb.  You wouldn't have some patches to fix 
> sware's expect would you?  Sware's expect doesn't appear to work :-(

I use expect-5.34 from our distribution.

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* (+420) 296.545.373 * http://www.suse.cz

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

Michal,

To give this x86-64 thread clear closure.  The internal-error you are 
seeing from the new frame code is now, officially, "not-a-frame-bug".

The underlying problem is caused by a design flaw (one of many) in the 
original CFI code (on which the x86-64 depends).  It's trying to use the 
CFI unwinder on a block of code that either: has no CFI information; or 
has CFI information that isn't relevant to the stack frame being 
unwound.  Using CFI to unwind such a frame is meaningless.

Please read the two cited posts below to get at least a feel for what 
the x86-64 needs to do for the problem to be fixed:

http://sources.redhat.com/ml/gdb/2003-02/msg00549.html
http://sources.redhat.com/ml/gdb-patches/2003-02/msg00668.html

I've already cooked up a draft patch that adds gdbarch_unwind_dummy_id() 
to the architecture vector (mumble something about needing documentation).

To fix this problem, the x86-64 will need to implement both that and the 
save_dummy_frame_tos() method.

Andrew

Re: [patch/rfc] Add a sentinel frame

[Michal Ludvig]

Andrew Cagney wrote:
> To give this x86-64 thread clear closure.  The internal-error you are 
> seeing from the new frame code is now, officially, "not-a-frame-bug".

Yes, I've already realised so. Thank you for confirmation.

> The underlying problem is caused by a design flaw (one of many) in the 
> original CFI code (on which the x86-64 depends).  It's trying to use the 
> CFI unwinder on a block of code that either: has no CFI information; or 
> has CFI information that isn't relevant to the stack frame being 
> unwound.  Using CFI to unwind such a frame is meaningless.
[...]
> To fix this problem, the x86-64 will need to implement both that and the 
> save_dummy_frame_tos() method.

OK. So, first I need to convert x86-64 target to use all the new 
frame-id stuff I think. And then implement handling of different frame 
types (normal (CFI), dummy, sigtramp, and specifically for x86-64 also 
normal frames without CFI debug info).
Basically all calls to cfi_*() functions from x86-64-*.c files should 
become x86_64_*() functions that call the appropriate cfi_*() functions 
if needed, or a frame-type specific thing otherwise.
Correct?

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* (+420) 296.545.373 * http://www.suse.cz

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

> Andrew Cagney wrote:
> To give this x86-64 thread clear closure.  The internal-error you are seeing from the new frame code is now, officially, "not-a-frame-bug".
> 
> Yes, I've already realised so. Thank you for confirmation.
> 
> The underlying problem is caused by a design flaw (one of many) in the original CFI code (on which the x86-64 depends).  It's trying to use the CFI unwinder on a block of code that either: has no CFI information; or has CFI information that isn't relevant to the stack frame being unwound.  Using CFI to unwind such a frame is meaningless.
> [...]
> To fix this problem, the x86-64 will need to implement both that and the save_dummy_frame_tos() method.
> 
> OK. So, first I need to convert x86-64 target to use all the new frame-id stuff I think. And then implement handling of different frame types (normal (CFI), dummy, sigtramp, and specifically for x86-64 also normal frames without CFI debug info).
> Basically all calls to cfi_*() functions from x86-64-*.c files should become x86_64_*() functions that call the appropriate cfi_*() functions if needed, or a frame-type specific thing otherwise.
> Correct?

Well, to fix this specific bug I think you'd just need to implement:

	save_dummy_frame_tos()
	unwind_dummy_id() (see uncommitted patch I posted).

And ensure that the top-of-stack value saved by save_dummy_frame_tos() 
matches the id.base value returned by unwind_dummy_id().

--

The cleanup is more substantial:.  The first shaky step is to implement 
a  cfi-frame.[hc] object (using dwarf2expr.[hc]?).  After that are the 
separate x86-64 specific unwinders: traditional, sigtramp.  The key 
difference is that with the old code the sequence:

	frame->get_saved_register ()
	->x86_64_get_saved_register ()
	->cfi_get_saved_register ()

where as the new code is more direct:

	frame->register_unwind()
	->cfi_register_unwind()

(the x86-64 code doesn't get a look in), and very recursive:

	frame->register_unwind()
	->cfi_register_unwind(frame)
	... determines that it needs the next frame's register
	... that frame happens to be a dummy
	frame->register ()
	frame->next->register_unwind()
	->dummy_frame_register_unwind(frame->next)

Andrew

Re: [patch/rfc] Add a sentinel frame

[Michal Ludvig]

Andrew Cagney wrote:
> Well, to fix this specific bug I think you'd just need to implement:
> 
>     save_dummy_frame_tos()
>     unwind_dummy_id() (see uncommitted patch I posted).
> 
> And ensure that the top-of-stack value saved by save_dummy_frame_tos() 
> matches the id.base value returned by unwind_dummy_id().

I added this:
Index: dummy-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dummy-frame.c,v
retrieving revision 1.10
diff -u -p -r1.10 dummy-frame.c
--- dummy-frame.c       19 Jan 2003 17:39:16 -0000      1.10
+++ dummy-frame.c       5 Mar 2003 17:30:56 -0000
@@ -246,6 +246,17 @@ generic_save_dummy_frame_tos (CORE_ADDR
    dummy_frame_stack->top = sp;
  }

+struct frame_id
+generic_unwind_dummy_id (struct gdbarch *gdbarch, struct frame_info *frame)
+{
+  static struct frame_id id;
+
+  id.base = dummy_frame_stack->top;
+  id.pc = frame->pc;
+
+  return id;
+}
+
  /* Record the upper/lower bounds on the address of the call dummy.  */

  void

and registered to the target:
set_gdbarch_save_dummy_frame_tos (gdbarch,generic_save_dummy_frame_tos);
set_gdbarch_unwind_dummy_id (gdbarch, generic_unwind_dummy_id);

I don't know if everything works as it should, but at least simple
(gdb) print func(123)
doesn't segfault and surprisingly even returns a result!

Does the above look correct to you?

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* (+420) 296.545.373 * http://www.suse.cz

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

> +struct frame_id
> +generic_unwind_dummy_id (struct gdbarch *gdbarch, struct frame_info *frame)
> +{
> +  static struct frame_id id;
> +
> +  id.base = dummy_frame_stack->top;
> +  id.pc = frame->pc;
> +
> +  return id;
> +}
> +

No.  That would make unwind dummy id's implementation circular.  The 
ID's value is needed to find the correct dummy frame in the 
dummy_frame_stack.

This method needs to unwind register value's from the NEXT_FRAME and 
then use that to determine the dummy frame's ID.  The d10v's 
implementation (not yet committed) looks like:

/* Assuming NEXT_FRAME->prev is a dummy, return the frame ID of that
    dummy frame.  The frame ID's base needs to match the TOS value
    saved by save_dummy_frame_tos(), and the PC match the dummy frame's
    breakpoint.  */

static struct frame_id
d10v_unwind_dummy_id (struct gdbarch *gdbarch, struct frame_info 
*next_frame)
{
   ULONGEST base;
   struct frame_id id;
   id.pc = frame_pc_unwind (next_frame);
   frame_unwind_unsigned_register (next_frame, SP_REGNUM, &base);
   id.base = d10v_make_daddr (base);
   return id;
}

Andrew

Re: [patch/rfc] Add a sentinel frame

[Michal Ludvig]

Andrew Cagney wrote:
> This method needs to unwind register value's from the NEXT_FRAME and 
> then use that to determine the dummy frame's ID.  The d10v's 
> implementation (not yet committed) looks like:

So could that be something like this? So far it works pretty well...

+static void
+x86_64_save_dummy_frame_tos (CORE_ADDR sp)
+{
+  /* We must add the size of the return address that is already
+     put on the stack.  */
+  generic_save_dummy_frame_tos (sp + sizeof(CORE_ADDR));
+}
+
+struct frame_id
+x86_64_unwind_dummy_id (struct gdbarch *gdbarch, struct frame_info *frame)
+{
+  struct frame_id id;
+
+  id.pc = frame_pc_unwind (frame);
+  frame_unwind_unsigned_register (frame, SP_REGNUM, &id.base);
+
+  return id;
+}
+

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* (+420) 296.545.373 * http://www.suse.cz

Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney]

> +static void
> +x86_64_save_dummy_frame_tos (CORE_ADDR sp)
> +{
> +  /* We must add the size of the return address that is already
> +     put on the stack.  */
> +  generic_save_dummy_frame_tos (sp + sizeof(CORE_ADDR));

Yes, but use TYPE_LENGTH (builtin_type_void_{code/data}_ptr).

> +}
> +
> +struct frame_id
> +x86_64_unwind_dummy_id (struct gdbarch *gdbarch, struct frame_info *frame)
> +{
> +  struct frame_id id;
> +
> +  id.pc = frame_pc_unwind (frame);
> +  frame_unwind_unsigned_register (frame, SP_REGNUM, &id.base);

Andrew

> +  return id;
> +}
> + 

[RFA] Dummy frames on x86-64

[Michal Ludvig]

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

Andrew Cagney wrote:
>> +static void
>> +x86_64_save_dummy_frame_tos (CORE_ADDR sp)
>> +{
>> +  /* We must add the size of the return address that is already
>> +     put on the stack.  */
>> +  generic_save_dummy_frame_tos (sp + sizeof(CORE_ADDR));
> 
> Yes, but use TYPE_LENGTH (builtin_type_void_{code/data}_ptr).

Great, thanks for your help!
Can I commit the attached patch to mainline, please?

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* (+420) 296.545.373 * http://www.suse.cz

[-- Attachment #2: dummy-unwind-1.diff --]
[-- Type: text/plain, Size: 1561 bytes --]

2002-03-06  Michal Ludvig  <mludvig@suse.cz>

	* x86-64-tdep.c (x86_64_save_dummy_frame_tos)
	(x86_64_unwind_dummy_id): New functions.
	(x86_64_init_abi): Register these two new functions.

Index: x86-64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/x86-64-tdep.c,v
retrieving revision 1.51
diff -u -p -r1.51 x86-64-tdep.c
--- x86-64-tdep.c	2 Mar 2003 04:02:25 -0000	1.51
+++ x86-64-tdep.c	6 Mar 2003 22:38:14 -0000
@@ -913,6 +913,26 @@ x86_64_breakpoint_from_pc (CORE_ADDR *pc
   return breakpoint;
 }
 
+static void
+x86_64_save_dummy_frame_tos (CORE_ADDR sp)
+{
+  /* We must add the size of the return address that is already 
+     put on the stack.  */
+  generic_save_dummy_frame_tos (sp + 
+				TYPE_LENGTH (builtin_type_void_func_ptr));
+}
+
+static struct frame_id
+x86_64_unwind_dummy_id (struct gdbarch *gdbarch, struct frame_info *frame)
+{
+  struct frame_id id;
+  
+  id.pc = frame_pc_unwind (frame);
+  frame_unwind_unsigned_register (frame, SP_REGNUM, &id.base);
+
+  return id;
+}
+
 void
 x86_64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
@@ -1023,6 +1043,10 @@ x86_64_init_abi (struct gdbarch_info inf
      since all supported x86-64 targets are ELF, but that might change
      in the future.  */
   set_gdbarch_in_solib_call_trampoline (gdbarch, in_plt_section);
+  
+  /* Dummy frame helper functions.  */
+  set_gdbarch_save_dummy_frame_tos (gdbarch, x86_64_save_dummy_frame_tos);
+  set_gdbarch_unwind_dummy_id (gdbarch, x86_64_unwind_dummy_id);
 }
 
 void

Re: [RFA] Dummy frames on x86-64

[Andrew Cagney]

> 2002-03-06  Michal Ludvig  <mludvig@suse.cz>
> 
> 	* x86-64-tdep.c (x86_64_save_dummy_frame_tos)
> 	(x86_64_unwind_dummy_id): New functions.
> 	(x86_64_init_abi): Register these two new functions.
> 
Yep!

Andrew

Re: [patch/rfc] Add a sentinel frame

[Michael Elizabeth Chastain]

> Anyway, I've got an x86-64 gdb.  You wouldn't have some patches to fix 
> sware's expect would you?  Sware's expect doesn't appear to work :-(

Would it be feasible to try stock expect?  That's what I test with anyways.

  ftp://ftp2.sourceforge.net/pub/sourceforge/tcl/tcl8.4.1-src.tar.gz
  http://expect.nist.gov/src/expect-5.38.0.tar.gz
  ftp://ftp.gnu.org/dejagnu/dejagnu-1.4.3.tar.gz

Sware expect dates to 1998-06-15, but sware tcl and sware dejagnu are
actually almost identical to the latest released version.

If it's not feasible to do this just forget I said anything.

Michael C