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

Daniel Jacobowitz
2001-11-19T23:15:16Z

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));