Re: patch: fix stack unwind through uClibc syscall() on mips

[Joel Brobecker <brobecker@adacore.com>]

Jan,

> uClibc syscall() is macro which modifies stack before syscall
> instruction, gdb is only looking at function prologue and misses the
> stack modification made in syscall(). Because of this unwind doesn't
> work. Attached is a patch, which is looking at actual $pc and $pc-4,
> and in case of syscall it modifies $sp, so mip32_scan_prologue finds
> correct values.

Can you give us a disassembly of the code, please, so that I can
understand a little better what your problem is?

> 2010-03-27  Jan Stancek  <jan.stancek@gmail.com>
>         * mips-tdep.c: fix stack unwind through uClibc syscall() on mips

Do you have a copyright assignment on file? I could not locate you in
the FSF database, so it looks like not. We cannot accept your patch
until you have one on file, so let me know.

You also need to indicate how you tested this change. In particular,
did you run the testsuite before and after your change?

> +/*
> + * fix the $sp by looking around actual $pc
> + * Currently this handles only uClibc syscalls,
> + * which adjust $sp before syscall itsels
> + */

This is not the GNU style (please have a  look at the GNU Coding
Standards, which is available at
http://www.gnu.org/prep/standards/standards.html).

  /* Fix the $sp by looking around actual $pc.  Currently this handles
     only uClibc syscalls, which adjust $sp before syscall itself.  */

In particular, use of capital letter for the first word in the sentence,
use of a period at the end of the a sentence.  And two spaces after a
period.

I think that the comment can be improved - it seems to be assuming some
form of context that the reader might not have.  But we'll see about
that later, if the patch is correct; right now, I really am not sure,
because you are doing the adjustment unconditionally, and perhaps we
should not.

> +int mips32_get_sp_adjustment(struct frame_info *this_frame, CORE_ADDR start_pc)

Style:

int
mips32_get_sp_adjustment (struct frame_info *this_frame, CORE_ADDR start_pc)

And the function should be declared static.

> +{
> +  CORE_ADDR pc = get_frame_address_in_block (this_frame);
> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +  unsigned long inst, high_word, low_word, ret;
> +  int reg;
> +
> +  inst = (unsigned long) mips_fetch_instruction (gdbarch, pc);
> +
> +  ret = 0;
> +  high_word = (inst >> 16) & 0xffff;
> +  low_word = inst & 0xffff;
> +  reg = high_word & 0x1f;
> +
> +  if (high_word == 0x27bd               /* addiu $sp,$sp,-i */
> +      || high_word == 0x23bd            /* addi $sp,$sp,-i */
> +      || high_word == 0x67bd)           /* daddiu $sp,$sp,-i */
> +    {
> +      if ( reg == MIPS_SP_REGNUM
           ^^^^
           Style: No space after '('

> +          && (low_word & 0x8000) == 0   /* positive stack adjustment */
> +          && (pc-4) > start_pc )
               ^^^^^^^^         ^^^
                  ||             Style: No space before ')'
                  ||   
                  ||   
            Style: remove useless parens, and add space around '-'.

> +        {
> +          pc = pc - 4;
> +          inst = (unsigned long) mips_fetch_instruction (gdbarch, pc);
> +          if (inst==0x0000000c)         /* syscall */
                    ^^^^ Style: spaces around all binary operators

> +            {
> +              ret = low_word;
> +            }

Style: Remove useless curly braces for if block.


> @@ -1920,9 +1958,12 @@ mips32_scan_prologue (struct gdbarch *gd
>    /* Can be called when there's no process, and hence when there's no
>       THIS_FRAME.  */
>    if (this_frame != NULL)
> -    sp = get_frame_register_signed (this_frame,
> -                                   gdbarch_num_regs (gdbarch)
> -                                   + MIPS_SP_REGNUM);
> +    {
> +      sp = get_frame_register_signed (this_frame,
> +              gdbarch_num_regs (gdbarch)
> +              + MIPS_SP_REGNUM);
> +      sp += mips32_get_sp_adjustment(this_frame, start_pc);
> +    }
>    else
>      sp = 0;

The formatting for the call to get_frame_register_signed is incorrect.

-- 
Joel