[RFC] About arm-tdep.c arm_in_function_epilogue_p function

[RFC] About arm-tdep.c arm_in_function_epilogue_p function

[Pierre Muller]

  In the second part of that function,
for which II just committed an obvious
compilation failure fix, I found something
strange:


  found_stack_adjust = 0;
  insn = read_memory_unsigned_integer (pc - 4, 4, byte_order_for_code);
  if (bits (insn, 28, 31) != INST_NV)
    {
      if ((insn & 0x0df0f000) == 0x0080d000)
        /* ADD SP (register or immediate).  */
        found_stack_adjust = 1;
      else if ((insn & 0x0df0f000) == 0x0040d000)
        /* SUB SP (register or immediate).  */
        found_stack_adjust = 1;
      else if ((insn & 0x0ffffff0) == 0x01a0d000)
        /* MOV SP.  */
>>> This line seems weird:
>>> why should a MOV SP be considered as a return?
>>> It should rather be a change in stack, no?
>>> (but I know about nothing about ARM instructions...)
>>> Furthermore, found_return isn't used
>>> anymore in that function.
>>> Isn't the correct code
>>>     found_stack_adjust = 1;

        found_return = 1;
      else if ((insn & 0x0fff0000) == 0x08bd0000)
        /* POP (LDMIA).  */
        found_stack_adjust = 1;
    }

  if (found_stack_adjust)
    return 1;

  return 0;
}

  Ulrich or Daniel,
could one of you two check this and 
commit a fix if I am right?

  Thanks in advance,
  
Pierre Muller
GDB pascal language maintainer

Re: [RFC] About arm-tdep.c arm_in_function_epilogue_p function

[Daniel Jacobowitz]

On Sun, Nov 14, 2010 at 01:19:29PM +0100, Pierre Muller wrote:
> >>> Furthermore, found_return isn't used
> >>> anymore in that function.
> >>> Isn't the correct code
> >>>     found_stack_adjust = 1;

I'm pretty sure you're right.  I'm not set up at the moment to test
patches on ARM, but if anyone reading this is, then I'd approve
that change.

-- 
Daniel Jacobowitz
CodeSourcery

RE: [RFC] About arm-tdep.c arm_in_function_epilogue_p function

[Pierre Muller]

Hi,


On Sun, Nov 14, 2010 at 01:19:29PM +0100, Pierre Muller wrote:
> >>> Furthermore, found_return isn't used
> >>> anymore in that function.
> >>> Isn't the correct code
> >>>     found_stack_adjust = 1;

> I'm pretty sure you're right.  I'm not set up at the moment to test
> patches on ARM, but if anyone reading this is, then I'd approve
> that change.

  I was able to compile and run the testsuite on an arm linux
machine, processor type armv7l.
  Unfortunately, the testsuite seems to never trigger this instruction.
(Checked by adding some specific output if the code triggers,
not found in gdb.log after testsuite completion.)
  So I think we should rather leave the decision to Ulrich,
as he seems much more involved in the arm target, no?


Pierre Muller
GDB pascal language maintainer

RE: [RFC] About arm-tdep.c arm_in_function_epilogue_p function

[Richard Earnshaw]

On Thu, 2010-11-18 at 12:07 +0100, Pierre Muller wrote:
> Hi,
> 
> 
> On Sun, Nov 14, 2010 at 01:19:29PM +0100, Pierre Muller wrote:
> > >>> Furthermore, found_return isn't used
> > >>> anymore in that function.
> > >>> Isn't the correct code
> > >>>     found_stack_adjust = 1;
> 
> > I'm pretty sure you're right.  I'm not set up at the moment to test
> > patches on ARM, but if anyone reading this is, then I'd approve
> > that change.
> 
>   I was able to compile and run the testsuite on an arm linux
> machine, processor type armv7l.
>   Unfortunately, the testsuite seems to never trigger this instruction.
> (Checked by adding some specific output if the code triggers,
> not found in gdb.log after testsuite completion.)
>   So I think we should rather leave the decision to Ulrich,
> as he seems much more involved in the arm target, no?

Why don't you just post a patch?  Then we can assess it as we would all
other patches.  The comments in the code at this point clearly don't
match the code, so something certainly needs fixing.

R.

[RFA] About arm-tdep.c arm_in_function_epilogue_p function

[Pierre Muller]

  OK, here is the patch.

  As already said, this patch doesn't change anything on
the testsuite results for a armv7l linux machine.

> Why don't you just post a patch?  Then we can assess it as we would all
> other patches.  The comments in the code at this point clearly don't
> match the code, so something certainly needs fixing.
> 
> R.

OK to apply?

2010-11-18  Pierre Muller  <muller@ics.u-strasbg.fr>

        * arm-tdep.c (arm_in_function_epilogue_p): Fix code when "MOV SP"
        instruction is found.

Index: src/gdb/arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.312
diff -u -p -r1.312 arm-tdep.c
--- src/14 Nov 2010 12:10:59 -0000      1.312
+++ src/gdb/arm-tdep.c  18 Nov 2010 13:35:41 -0000
@@ -2245,7 +2245,7 @@ arm_in_function_epilogue_p (struct gdbar
        found_stack_adjust = 1;
       else if ((insn & 0x0ffffff0) == 0x01a0d000)
        /* MOV SP.  */
-       found_return = 1;
+       found_stack_adjust = 1;
       else if ((insn & 0x0fff0000) == 0x08bd0000)
        /* POP (LDMIA).  */
        found_stack_adjust = 1;

Re: [RFA] About arm-tdep.c arm_in_function_epilogue_p function

[Richard Earnshaw]

On Thu, 2010-11-18 at 14:43 +0100, Pierre Muller wrote:
> OK, here is the patch.
> 
>   As already said, this patch doesn't change anything on
> the testsuite results for a armv7l linux machine.
> 
> > Why don't you just post a patch?  Then we can assess it as we would all
> > other patches.  The comments in the code at this point clearly don't
> > match the code, so something certainly needs fixing.
> > 
> > R.
> 

Looking at a wider context I think I see what's happened.  The block
here is essentially a copy of the block earlier in the function with
s/pc/sp/ applied.  Except this one variable wasn't updated properly.

OK.  

R

> OK to apply?
> 
> 2010-11-18  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
>         * arm-tdep.c (arm_in_function_epilogue_p): Fix code when "MOV SP"
>         instruction is found.
> 
> Index: src/gdb/arm-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/arm-tdep.c,v
> retrieving revision 1.312
> diff -u -p -r1.312 arm-tdep.c
> --- src/14 Nov 2010 12:10:59 -0000      1.312
> +++ src/gdb/arm-tdep.c  18 Nov 2010 13:35:41 -0000
> @@ -2245,7 +2245,7 @@ arm_in_function_epilogue_p (struct gdbar
>         found_stack_adjust = 1;
>        else if ((insn & 0x0ffffff0) == 0x01a0d000)
>         /* MOV SP.  */
> -       found_return = 1;
> +       found_stack_adjust = 1;
>        else if ((insn & 0x0fff0000) == 0x08bd0000)
>         /* POP (LDMIA).  */
>         found_stack_adjust = 1;
> 

RE: [RFA] About arm-tdep.c arm_in_function_epilogue_p function

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Richard Earnshaw
> Envoyé : jeudi 18 novembre 2010 14:56
> À : Pierre Muller
> Cc : 'Daniel Jacobowitz'; 'Ulrich Weigand'; gdb-patches@sourceware.org
> Objet : Re: [RFA] About arm-tdep.c arm_in_function_epilogue_p function
> 
> 
> On Thu, 2010-11-18 at 14:43 +0100, Pierre Muller wrote:
> > OK, here is the patch.
> >
> >   As already said, this patch doesn't change anything on
> > the testsuite results for a armv7l linux machine.
> >
> > > Why don't you just post a patch?  Then we can assess it as we would
> all
> > > other patches.  The comments in the code at this point clearly
> don't
> > > match the code, so something certainly needs fixing.
> > >
> > > R.
> >
> 
> Looking at a wider context I think I see what's happened.  The block
> here is essentially a copy of the block earlier in the function with
> s/pc/sp/ applied.  Except this one variable wasn't updated properly.
> 
> OK.
  Thank you for the approval,
patch committed:
http://sourceware.org/ml/gdb-cvs/2010-11/msg00088.html

Pierre