your turn :-)

your turn :-)

[Andrew Cagney]

Joel

The theory is to enable this:

#ifdef NOT_YET
     proc_desc = heuristic_proc_desc (start_addr, pc, next_frame, 
this_cache);
#else
     proc_desc = heuristic_proc_desc (start_addr, pc, next_frame, NULL);
#endif

Then:

- delete the rest of the insn{32,16}_frame_cache code as its redundant, 
the heuristic code will have already updated the cache

- eliminate proc_desc from mips{32,16}_heuristic ... as its redundant, 
the code only needs to update this_cache

- either inline mips{32,16}_heuristic into mips_insn{32,16}_frame_cache 
or, instead, merge mips_insn{32,16}_frame_cache.

want to try it (the test results don't even need to vaguely pass).

Andrew

Re: your turn :-)

[Joel Brobecker]

I'll see if I can take care of this. Right now, GDB is completely
broken again. This change sort of patch things up a little bit
by preventing a SEGV. But we get some other problems that just
screw the testsuite run.


    Index: mips-tdep.c
    ===================================================================
    RCS file: /cvs/src/src/gdb/mips-tdep.c,v
    retrieving revision 1.320
    diff -u -p -r1.320 mips-tdep.c
    --- mips-tdep.c	4 Sep 2004 00:16:56 -0000	1.320
    +++ mips-tdep.c	4 Sep 2004 18:59:18 -0000
    @@ -2762,9 +2762,17 @@ mips32_heuristic_proc_desc (CORE_ADDR st
     {
       CORE_ADDR cur_pc;
       CORE_ADDR frame_addr = 0;	/* Value of $r30. Used by gcc for frame-pointer */
    +
    +  /* FIXME: brobecker/2004-09-04: We're in the middle of a transition,
    +     and this_cache may be NULL. In that case, then create a new one
    +     just for now. It means a memory leak, but oh well, I don't care.  */
    +  if (this_cache == NULL)
    +    this_cache = FRAME_OBSTACK_ZALLOC (struct mips_frame_cache);
    +
     restart:
    -  this_cache = xrealloc (this_cache, SIZEOF_FRAME_SAVED_REGS);
    -  memset (this_cache, '\0', SIZEOF_FRAME_SAVED_REGS);
    +  this_cache->saved_regs = xrealloc (this_cache->saved_regs,
    +                                     SIZEOF_FRAME_SAVED_REGS);
    +  memset (this_cache->saved_regs, '\0', SIZEOF_FRAME_SAVED_REGS);
       PROC_FRAME_OFFSET (&temp_proc_desc) = 0;
       PROC_FRAME_ADJUST (&temp_proc_desc) = 0;	/* offset of FP from SP */
       for (cur_pc = start_pc; cur_pc < limit_pc; cur_pc += MIPS_INSTLEN)

I guess you really don't want to see me incorporate my last RFA
until the cleanup is done, eh?


On Fri, Sep 03, 2004 at 08:11:36PM -0400, Andrew Cagney wrote:
> Joel
> 
> The theory is to enable this:
> 
> #ifdef NOT_YET
>     proc_desc = heuristic_proc_desc (start_addr, pc, next_frame, 
> this_cache);
> #else
>     proc_desc = heuristic_proc_desc (start_addr, pc, next_frame, NULL);
> #endif

What should heuristic_proc_desc do if the cache given is NULL.
Create a temporary one while scaning the function, and then
trash it?

Another question that's troubling me: I can't see how mips_frame_cache
structs are deallocated. I am guessing that this happens when the
obstack is reset, but then that would mean that we leak the save_regs
array. ???

> Then:
> 
> - delete the rest of the insn{32,16}_frame_cache code as its redundant, 
> the heuristic code will have already updated the cache
> 
> - eliminate proc_desc from mips{32,16}_heuristic ... as its redundant, 
> the code only needs to update this_cache
> 
> - either inline mips{32,16}_heuristic into mips_insn{32,16}_frame_cache 
> or, instead, merge mips_insn{32,16}_frame_cache.
> 
> want to try it (the test results don't even need to vaguely pass).
> 
> Andrew

Hafta run.
-- 
Joel

Re: your turn :-)

[Joel Brobecker]

> I'll see if I can take care of this. Right now, GDB is completely
> broken again. This change sort of patch things up a little bit
> by preventing a SEGV. But we get some other problems that just
> screw the testsuite run.

The first problem that I saw was the following. Using advance.exp:

        (gdb) b func
        Breakpoint 1 at 0x10001600: file advance.c, line 18.
        (gdb) run
        Starting program: /chico.a/brobecke/mips-inline/gdb-public/gdb/testsuite/gdb.bas
        e/advance 
        
        Breakpoint 1, func () at advance.c:18
        18        x = x + 5;
        (gdb) advance func3
        warning: GDB can't find the start of the function at 0x3.
        
            GDB is unable to find the start of the function at 0x3
        and thus can't determine the size of that function's stack frame.
        This means that GDB may be unable to access that stack frame, or
        the frames below it.
            This problem is most likely caused by an invalid program counter or
        stack pointer.
            However, if you think GDB should simply search farther back
        from 0x3 for code which looks like the beginning of a
        function, you can increase the range of the search using the `set
        heuristic-fence-post' command.
        zsh: 19433825 segmentation fault (core dumped)  ../../gdb advance

It turns out it was that heuristic_proc_desc problem I sent a patch for
striking again. Forgot to apply my patch before starting the testsuite
:-(.

-- 
Joel

Re: your turn :-)

[Andrew Cagney]

> I'll see if I can take care of this. Right now, GDB is completely
> broken again. This change sort of patch things up a little bit
> by preventing a SEGV. But we get some other problems that just
> screw the testsuite run.
> 
> 
>     Index: mips-tdep.c
>     ===================================================================
>     RCS file: /cvs/src/src/gdb/mips-tdep.c,v
>     retrieving revision 1.320
>     diff -u -p -r1.320 mips-tdep.c
>     --- mips-tdep.c	4 Sep 2004 00:16:56 -0000	1.320
>     +++ mips-tdep.c	4 Sep 2004 18:59:18 -0000
>     @@ -2762,9 +2762,17 @@ mips32_heuristic_proc_desc (CORE_ADDR st
>      {
>        CORE_ADDR cur_pc;
>        CORE_ADDR frame_addr = 0;	/* Value of $r30. Used by gcc for frame-pointer */
>     +
>     +  /* FIXME: brobecker/2004-09-04: We're in the middle of a transition,
>     +     and this_cache may be NULL. In that case, then create a new one
>     +     just for now. It means a memory leak, but oh well, I don't care.  */
>     +  if (this_cache == NULL)
>     +    this_cache = FRAME_OBSTACK_ZALLOC (struct mips_frame_cache);

This shouldn't be needed, hmm:

>      restart:

oops, this should be deleted.  An earlier change modified set_reg_offset 
to ignore NULL pointers.

>     -  this_cache = xrealloc (this_cache, SIZEOF_FRAME_SAVED_REGS);
>     -  memset (this_cache, '\0', SIZEOF_FRAME_SAVED_REGS);


>     +  this_cache->saved_regs = xrealloc (this_cache->saved_regs,
>     +                                     SIZEOF_FRAME_SAVED_REGS);
>     +  memset (this_cache->saved_regs, '\0', SIZEOF_FRAME_SAVED_REGS);
>        PROC_FRAME_OFFSET (&temp_proc_desc) = 0;
>        PROC_FRAME_ADJUST (&temp_proc_desc) = 0;	/* offset of FP from SP */
>        for (cur_pc = start_pc; cur_pc < limit_pc; cur_pc += MIPS_INSTLEN)
> 
> I guess you really don't want to see me incorporate my last RFA
> until the cleanup is done, eh?
> 
> 
> On Fri, Sep 03, 2004 at 08:11:36PM -0400, Andrew Cagney wrote:
> 
>>> Joel
>>> 
>>> The theory is to enable this:
>>> 
>>> #ifdef NOT_YET
>>>     proc_desc = heuristic_proc_desc (start_addr, pc, next_frame, 
>>> this_cache);
>>> #else
>>>     proc_desc = heuristic_proc_desc (start_addr, pc, next_frame, NULL);
>>> #endif
> 
> 
> What should heuristic_proc_desc do if the cache given is NULL.
> Create a temporary one while scaning the function, and then
> trash it?

No, the code should survive a NULL this_cache.

> Another question that's troubling me: I can't see how mips_frame_cache
> structs are deallocated. I am guessing that this happens when the
> obstack is reset, but then that would mean that we leak the save_regs
> array. ???

Right.  The deallocate occures each time the frame cache is flushed and 
that occures over and over.

>>> Then:
>>> 
>>> - delete the rest of the insn{32,16}_frame_cache code as its redundant, 
>>> the heuristic code will have already updated the cache
>>> 
>>> - eliminate proc_desc from mips{32,16}_heuristic ... as its redundant, 
>>> the code only needs to update this_cache

This might get changed.

I've this hunch that heuristic_proc_desc may need to be changed to 
return the last instruction - indicating the end of the prologue.  But 
worry about this latter.

>>> - either inline mips{32,16}_heuristic into mips_insn{32,16}_frame_cache 
>>> or, instead, merge mips_insn{32,16}_frame_cache.
>>> 
>>> want to try it (the test results don't even need to vaguely pass).
>>> 
>>> Andrew

Andrew

Re: your turn :-)

[Joel Brobecker]

> oops, this should be deleted.  An earlier change modified set_reg_offset 
> to ignore NULL pointers.
> 
> >    -  this_cache = xrealloc (this_cache, SIZEOF_FRAME_SAVED_REGS);
> >    -  memset (this_cache, '\0', SIZEOF_FRAME_SAVED_REGS);
> 
> 
> >    +  this_cache->saved_regs = xrealloc (this_cache->saved_regs,
> >    +                                     SIZEOF_FRAME_SAVED_REGS);
> >    +  memset (this_cache->saved_regs, '\0', SIZEOF_FRAME_SAVED_REGS);

Of course, this was OB - vious!

> >What should heuristic_proc_desc do if the cache given is NULL.
> >Create a temporary one while scaning the function, and then
> >trash it?
> 
> No, the code should survive a NULL this_cache.

OK.

> >Another question that's troubling me: I can't see how mips_frame_cache
> >structs are deallocated. I am guessing that this happens when the
> >obstack is reset, but then that would mean that we leak the save_regs
> >array. ???
> 
> Right.  The deallocate occures each time the frame cache is flushed and 
> that occures over and over.

I was confused by the saved_regs array re-allocation above. I thought
that was what you meant when I saw that code, so I was confused. Now,
with these lines gone, I see the code allocating this array and it
becomes obvious too. I shouldn't be doing things in a hurry...

Thanks,
-- 
Joel