Re: [rfa] Mips heuristic_proc_desc vs. the stack pointer.

[Daniel Jacobowitz <drow at mvista dot com>]

On Mon, Nov 19, 2001 at 04:30:31PM -0500, Andrew Cagney wrote:
> >As HJ noticed, we try to read the stack pointer in heuristic_proc_desc.  
> >I'm
> >not sure why this normally works and fails with linuxthread support, but 
> >I'm
> >convinced it's sometimes wrong.  If we are called from after_prologue(), 
> >the
> >stack pointer has nothing to do with the function we're trying to generate 
> >a
> >desc for.  We shouldn't try to read it in this case.  The uses of it in
> >*_heuristic_proc_desc are harmless.
> 
> Regarding the threads, are you saying things still sometimes break with 
> your patch applied?  I suspect there was the usual GDB internal thread 
> coherency problem where different parts of GDB were debugging different 
> threads.

Things won't break with the patch applied.  We build saved argument
stack offsets in *_heuristic_proc_desc, even if we don't have a stack
pointer; they'll be bogus, but nothing uses them.

> >Is this OK, Andrew?
> 
> Yes, but can you please adjust the following before committing:
> 
> > * mips-tdep.c (find_proc_desc): Add read_sp argument.  Update all
> >callers.
> 
> Given the updates were not identical / mechanical, could you please list 
> each making it clear that after_prologue() was the exception.
> 
> Can you please add a comment to after_prologue() explaining why the SP 
> shouldn't be fetched in that case.
> 
> Can the argument be called read_sp_p (say?) rather than read_sp.  There 
> is a global function read_sp that is hiding behind that variable.

Oops, I didn't even notice.  Isn't there a -Wshadow somewhere?

Here's the patch I committed to the trunk.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.1797
diff -u -p -r1.1797 ChangeLog
--- ChangeLog	2001/11/19 21:44:46	1.1797
+++ ChangeLog	2001/11/19 22:11:21
@@ -1,3 +1,13 @@
+2001-11-19  Daniel Jacobowitz  <drow@mvista.com>
+
+	* mips-tdep.c (find_proc_desc): Add cur_frame argument.  Pass
+	cur_frame to heuristic_proc_desc.
+	(heuristic_proc_desc): Add cur_frame argument.  Do not read SP
+	if cur_frame == 0.
+	(after_prologue): Pass cur_frame == 0 to find_proc_desc.
+	(mips_frame_chain): Pass cur_frame == 1 to find_proc_desc.
+	(mips_init_extra_frame_info): Likewise.
+
 2001-11-19  Andrew Cagney  <ac131313@redhat.com>
 
 	* defs.h (return_to_top_level): Comment.
Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.60
diff -u -p -r1.60 mips-tdep.c
--- mips-tdep.c	2001/10/15 18:18:29	1.60
+++ mips-tdep.c	2001/11/19 22:11:21
@@ -239,7 +239,7 @@ int gdb_print_insn_mips (bfd_vma, disass
 static void mips_print_register (int, int);
 
 static mips_extra_func_info_t
-heuristic_proc_desc (CORE_ADDR, CORE_ADDR, struct frame_info *);
+heuristic_proc_desc (CORE_ADDR, CORE_ADDR, struct frame_info *, int);
 
 static CORE_ADDR heuristic_proc_start (CORE_ADDR);
 
@@ -252,7 +252,7 @@ static void mips_show_processor_type_com
 static void reinit_frame_cache_sfunc (char *, int, struct cmd_list_element *);
 
 static mips_extra_func_info_t
-find_proc_desc (CORE_ADDR pc, struct frame_info *next_frame);
+find_proc_desc (CORE_ADDR pc, struct frame_info *next_frame, int cur_frame);
 
 static CORE_ADDR after_prologue (CORE_ADDR pc,
 				 mips_extra_func_info_t proc_desc);
@@ -561,8 +561,13 @@ after_prologue (CORE_ADDR pc,
   struct symtab_and_line sal;
   CORE_ADDR func_addr, func_end;
 
+  /* Pass cur_frame == 0 to find_proc_desc.  We should not attempt
+     to read the stack pointer from the current machine state, because
+     the current machine state has nothing to do with the information
+     we need from the proc_desc; and the process may or may not exist
+     right now.  */
   if (!proc_desc)
-    proc_desc = find_proc_desc (pc, NULL);
+    proc_desc = find_proc_desc (pc, NULL, 0);
 
   if (proc_desc)
     {
@@ -1858,10 +1863,15 @@ restart:
 
 static mips_extra_func_info_t
 heuristic_proc_desc (CORE_ADDR start_pc, CORE_ADDR limit_pc,
-		     struct frame_info *next_frame)
+		     struct frame_info *next_frame, int cur_frame)
 {
-  CORE_ADDR sp = read_next_frame_reg (next_frame, SP_REGNUM);
+  CORE_ADDR sp;
 
+  if (cur_frame)
+    sp = read_next_frame_reg (next_frame, SP_REGNUM);
+  else
+    sp = 0;
+
   if (start_pc == 0)
     return NULL;
   memset (&temp_proc_desc, '\0', sizeof (temp_proc_desc));
@@ -1919,7 +1929,7 @@ non_heuristic_proc_desc (CORE_ADDR pc, C
 
 
 static mips_extra_func_info_t
-find_proc_desc (CORE_ADDR pc, struct frame_info *next_frame)
+find_proc_desc (CORE_ADDR pc, struct frame_info *next_frame, int cur_frame)
 {
   mips_extra_func_info_t proc_desc;
   CORE_ADDR startaddr;
@@ -1951,7 +1961,7 @@ find_proc_desc (CORE_ADDR pc, struct fra
 	    {
 	      mips_extra_func_info_t found_heuristic =
 	      heuristic_proc_desc (PROC_LOW_ADDR (proc_desc),
-				   pc, next_frame);
+				   pc, next_frame, cur_frame);
 	      if (found_heuristic)
 		proc_desc = found_heuristic;
 	    }
@@ -1975,7 +1985,7 @@ find_proc_desc (CORE_ADDR pc, struct fra
 	startaddr = heuristic_proc_start (pc);
 
       proc_desc =
-	heuristic_proc_desc (startaddr, pc, next_frame);
+	heuristic_proc_desc (startaddr, pc, next_frame, cur_frame);
     }
   return proc_desc;
 }
@@ -2007,7 +2017,7 @@ mips_frame_chain (struct frame_info *fra
     saved_pc = tmp;
 
   /* Look up the procedure descriptor for this PC.  */
-  proc_desc = find_proc_desc (saved_pc, frame);
+  proc_desc = find_proc_desc (saved_pc, frame, 1);
   if (!proc_desc)
     return 0;
 
@@ -2033,7 +2043,7 @@ mips_init_extra_frame_info (int fromleaf
 
   /* Use proc_desc calculated in frame_chain */
   mips_extra_func_info_t proc_desc =
-  fci->next ? cached_proc_desc : find_proc_desc (fci->pc, fci->next);
+  fci->next ? cached_proc_desc : find_proc_desc (fci->pc, fci->next, 1);
 
   fci->extra_info = (struct frame_extra_info *)
     frame_obstack_alloc (sizeof (struct frame_extra_info));