[RFC] Calling mips-tdep.c:read_next_frame_reg with null frame?

[RFC] Calling mips-tdep.c:read_next_frame_reg with null frame?

[Joel Brobecker]

I am wondering what would happen if we were to call mips32_scan_prologue
with a NULL frame, which I think is something that we are doing on a
regular basis. See for instance how after_prologue that calls
heuristic_proc_desc with a NULL frame, which gets passed to scan_prologue.

There is some code in that function that does:

    frame_addr = read_next_frame_reg (next_frame, NUM_REGS + 30);

Looking at read_next_frame_reg():

        static CORE_ADDR
        read_next_frame_reg (struct frame_info *fi, int regno)
        {
          /* Always a pseudo.  */
          gdb_assert (regno >= NUM_REGS);
          if (fi == NULL)
            {
              LONGEST val;
              regcache_cooked_read_signed (current_regcache, regno, &val);
              return val;
            } 
          else
            return frame_unwind_register_signed (fi, regno);

So when the frame is null, we fetch some value from the regcache.
But what if we don't have any inferior? Looks like we have to be
a bit careful with the following code:

          else if (frame_reg == MIPS_SP_REGNUM)
            {
              unsigned alloca_adjust;

              frame_reg = 30;
              frame_addr = read_next_frame_reg (next_frame, NUM_REGS + 30);
              alloca_adjust = (unsigned) (frame_addr - (sp + low_word));
              if (alloca_adjust > 0)
                {
                  /* FP > SP + frame_size. This may be because of
                     an alloca or somethings similar.  Fix sp to
                     "pre-alloca" value, and try again.  */
                  sp += alloca_adjust;
                  /* Need to reset the status of all registers.  Otherwise,
                     we will hit a guard that prevents the new address
                     for each register to be recomputed during the second
                     pass.  */
                  reset_saved_regs (this_cache);
                  goto restart;
                }
            }

Same for

      /* move $30,$sp.  With different versions of gas this will be either
         `addu $30,$sp,$zero' or `or $30,$sp,$zero' or `daddu 30,sp,$0'.
         Accept any one of these.  */
      else if (inst == 0x03A0F021 || inst == 0x03a0f025 || inst == 0x03a0f02d)
        {
          /* New gcc frame, virtual frame pointer is at r30 + frame_size.  */
          if (frame_reg == MIPS_SP_REGNUM)
            {
              unsigned alloca_adjust;

              frame_reg = 30;
              frame_addr = read_next_frame_reg (next_frame, NUM_REGS + 30);
(etc...)

I suggest modifying the (frame_reg == SP_REGNUM) check into

        (frame_reg == SP_REGNUM && next_frame != NULL)

with a comment saying that if NEXT_FRAME is null, then we're probably
not analyzing a live frame, but just scanning the prologue. So we don't
need to do anything special for this instruction.

Thoughts?
-- 
Joel

Re: [RFC] Calling mips-tdep.c:read_next_frame_reg with null frame?

[Andrew Cagney]

> I am wondering what would happen if we were to call mips32_scan_prologue
> with a NULL frame, which I think is something that we are doing on a
> regular basis. See for instance how after_prologue that calls
> heuristic_proc_desc with a NULL frame, which gets passed to scan_prologue.
> 
> There is some code in that function that does:
> 
>     frame_addr = read_next_frame_reg (next_frame, NUM_REGS + 30);
> 
> Looking at read_next_frame_reg():
> 
>         static CORE_ADDR
>         read_next_frame_reg (struct frame_info *fi, int regno)
>         {
>           /* Always a pseudo.  */
>           gdb_assert (regno >= NUM_REGS);
>           if (fi == NULL)
>             {
>               LONGEST val;
>               regcache_cooked_read_signed (current_regcache, regno, &val);
>               return val;
>             } 
>           else
>             return frame_unwind_register_signed (fi, regno);
> 
> So when the frame is null, we fetch some value from the regcache.
> But what if we don't have any inferior? Looks like we have to be
> a bit careful with the following code:
> 
>           else if (frame_reg == MIPS_SP_REGNUM)
>             {
>               unsigned alloca_adjust;
> 
>               frame_reg = 30;
>               frame_addr = read_next_frame_reg (next_frame, NUM_REGS + 30);
>               alloca_adjust = (unsigned) (frame_addr - (sp + low_word));
>               if (alloca_adjust > 0)
>                 {
>                   /* FP > SP + frame_size. This may be because of
>                      an alloca or somethings similar.  Fix sp to
>                      "pre-alloca" value, and try again.  */
>                   sp += alloca_adjust;
>                   /* Need to reset the status of all registers.  Otherwise,
>                      we will hit a guard that prevents the new address
>                      for each register to be recomputed during the second
>                      pass.  */
>                   reset_saved_regs (this_cache);
>                   goto restart;
>                 }
>             }

> Same for
> 
>       /* move $30,$sp.  With different versions of gas this will be either
>          `addu $30,$sp,$zero' or `or $30,$sp,$zero' or `daddu 30,sp,$0'.
>          Accept any one of these.  */
>       else if (inst == 0x03A0F021 || inst == 0x03a0f025 || inst == 0x03a0f02d)
>         {
>           /* New gcc frame, virtual frame pointer is at r30 + frame_size.  */
>           if (frame_reg == MIPS_SP_REGNUM)
>             {
>               unsigned alloca_adjust;
> 
>               frame_reg = 30;
>               frame_addr = read_next_frame_reg (next_frame, NUM_REGS + 30);
> (etc...)
> 
> I suggest modifying the (frame_reg == SP_REGNUM) check into
> 
>         (frame_reg == SP_REGNUM && next_frame != NULL)
> 
> with a comment saying that if NEXT_FRAME is null, then we're probably
> not analyzing a live frame, but just scanning the prologue. So we don't
> need to do anything special for this instruction.

Yes (it's definitly not a live frame so the information extracted from 
the instructions doesn't need to be saved).  Btw, if you replace:

                  reset_saved_regs (this_cache);
                   goto restart;

with

                  reset_saved_regs (this_cache);
                  frame_offset = 0;

I believe that you can blow away the GOTO.

Andrew