[RFA/mips] mips32 frame code simplification...

[Joel Brobecker <brobecker at gnat dot com>]

Hello,

This implements an idea that Andrew suggested a few days ago.
Basically, we're trying to get rid of the faked proc_desc and
compute the frame cache directly. The goal is to simplify the
code, and get rid of a lot of code duplication.

2004-09-14  Joel Brobecker  <brobecker@gnat.com>

        * mips-tdep.c (mips_insn32_frame_cache): Pass frame cache in
        call to heuristic_proc_desc. Remove code that became redundant
        as a consequence.
        (read_next_frame_reg): Remove special case for SP_REGNUM.
        (set_reg_offset): Fix small typo.
        (reset_saved_regs): New procedure.
        (mips32_heuristic_proc_desc): No longer compute a fake
        procedure descriptor. Compute the full frame cache instead.
        Some minor comment reformatting.

Tested on mips-irix. I think it is a considerable improvement in
terms of testsuite results if you compare them to the result one
would obtain before this patch is applied. I didn't have the patience
to run it though, because I knew it would take forever to complete
and return poor results.

If I compare the new results to the ones I obtained with a debugger
containing the equivalent of the patch I submitted a few weeks back
(the one that cured a heuristic-proc-desc warning by fixing the faked
proc_desc), then the results are slightly worse. I think the area were
we regressed in mostly signal handlers, where I have noticed an infinite
loop while doing a backtrace inside one of the signal testcases. We can
fix that later. The good news is that the heuristic-proc-desc problem
I mentioned is now fixed as a side-effect, since we no longer rely on
the synthesized proc desc anymore.

I also have to say that I am saying that they are slight worse, but
that's only a guess. In reality, I had to deactivate the signal tests
because they were causing some "buffer full" error messages, and they
seem to screw dejaGNU big time. So much so that dejagnu is just useless
during the rest of the run. Also, I now get an premature abortion of
the testsuite run like this:

        Running ./gdb.threads/killed.exp ...
        ERROR: (DejaGnu) proc "(gdb) {$}" does not exist.
        The error code is NONE
        The info on the error is:
        close: spawn id exp8 not open
            while executing
        "close -i exp8"
            invoked from within
        "catch "close -i $spawn_id""

So the number if PASSes I am getting is not as high as if I had
been able to run the entire testsuite.

Frankly, I have reached a point with dejagnu where I just want to
throw it out the window, trample it, chop it, burn it, curse it,
bury it face down, etc. I think it's OK for now, but Michael might
want to crucify me for suggesting that it's OK.

I should also note that I am not sure of the effect of this change
to mips16, or the cases when a proc_desc *IS* available:

    @@ -2373,12 +2259,6 @@ read_next_frame_reg (struct frame_info *
           regcache_cooked_read_signed (current_regcache, regno, &val);
           return val;
         }
    -  else if ((regno % NUM_REGS) == MIPS_SP_REGNUM)
    -    /* MIPS_SP_REGNUM is special, its value is stored in saved_regs.
    -       In fact, it is so special that it can even only be fetched
    -       using a raw register number!  Once this code as been converted
    -       to frame-unwind the problem goes away.  */
    -    return frame_unwind_register_signed (fi, regno % NUM_REGS);
       else
         return frame_unwind_register_signed (fi, regno);
 
This chance is necessary once we stop relying on synthesized proc
descriptors for mips32. But I am guessing it breaks at least mips16.
We need to convert mips16 too, but I can't test it. Andrew?

And the last part I'd like to emphasize is the following new function:

    +/* Mark all the registers as unset in the saved_regs array
    +   of THIS_CACHE.  Do nothing if THIS_CACHE is null.  */
    +
    +void
    +reset_saved_regs (struct mips_frame_cache *this_cache)
    +{
    +  if (this_cache == NULL || this_cache->saved_regs == NULL)
    +    return;
    +
    +  {
    +    const int num_regs = NUM_REGS;
    +    int i;
    +
    +    for (i = 0; i < num_regs; i++)
    +      {
    +        this_cache->saved_regs[i].addr = -1;
    +      }
    +  }
    +}
    +

This function is called when we notice a frame using alloca.
The algorithm we use to scan the function prologue actually
re-computes a new frame address, and restarts the scan of
the prologue. If we don't reset the address of the saved_regs,
we hit a small gard inside set_reg_offset that prevents us from
storing the new register addresses when we hit instructions
saving registers on the stack.

My question is: Should we also reset the realreg?

All in all, I am pretty satisfied with this iteration. I think it's
a clear improvement, but that may be a controversial opinion.

Ahem. Ok to apply?

Next for me is to cleanup a bit the testsuite results, by fixing
the bugs that cause testsuite havoc. Once this is done, I can continue
the code cleanup more serenely.

-- 
Joel

Attachment: mips-tdep.c.diff
Description: Text document