Re: [PATCH] arm software watchpoint: return to epilogue

[Will Newton <will.newton@linaro.org>]

On 6 August 2014 04:18, Yao Qi <yao@codesourcery.com> wrote:
> Hi,
> This patch is to handle a software watchpoint case that program returns
> to caller's epilogue, and it causes the fail in thumb mode,
>
> finish^M
> Run till exit from #0  func () at gdb/testsuite/gdb.base/watchpoint-cond-gone.c:26^M
> 0x000001f6 in jumper ()^M
> (gdb) FAIL: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
>
> In the test, jumper calls func, and programs returns from func to
> jumper's epilogue, IOW, the branch instruction is the last instruction
> of jumper's function body.
>
>     jumper:
>     .....
>     0x000001f2 <+10>:    bl      0x200   [1] <---- indirect call to func
>     0x000001f6 <+14>:    mov     sp, r7  [2] <---- start of the epilogue
>     0x000001f8 <+16>:    add     sp, #8
>     0x000001fa <+18>:    pop     {r7}
>     0x000001fc <+20>:    pop     {r0}
>     0x000001fe <+22>:    bx      r0
>
> When the inferior returns from func back to jumper, it is expected
> that an expression of a software watchpoint becomes out-of-scope.
> GDB validates the expression by checking the corresponding frame,
> but this check is guarded by gdbarch_in_function_epilogue_p.  See
> breakpoint.c:watchpoint_check.
>
> It doesn't work in this case, because program returns from func's
> epilogue back to jumper's epilogue [2], GDB thinks the program is
> still within the epilogue, but in fact it goes to a different one.
> When PC points at [2], the sp-restore instruction is to be
> executed, so the stack frame isn't destroyed yet and we can still
> use the frame mechanism reliably.
>
> Note that when PC points to the first instruction of restoring SP,
> it is part of epilogue, but we still return zero.  When goes to
> the next instruction, the backward scan will still match the
> epilogue sequence correctly.  The reason for doing this is to
> handle the "return-to-epilogue" case.
>
> What this patch does is to restrict the epilogue matching that let
> GDB think the first SP restore instruction isn't part of the epilogue,
> and fall back to use frame mechanism.  We set 'found_stack_adjust'
> zero before backward scan (although found_stack_adjust is initialized
> to zero, it is safe to set it again before using it), and we've done
> this for arm mode counterpart (arm_in_function_epilogue_p) too.
>
> The patch is tested in arm-none-eabi and arm-none-linux-gnueabi with
> various multilibs.  OK to apply?
>
> gdb:
>
> 2014-08-06  Yao Qi  <yao@codesourcery.com>
>
>         * arm-tdep.c (thumb_in_function_epilogue_p): Don't set
>         found_stack_adjust in forward scan.  Set it zero before
>         backward scan.    Remove condition check on
>         found_stack_adjust which is always true.  Indent the code.

This looks ok to me, although I don't see the need for the redundant
zero assignment to found_stack_adjust.

> ---
>  gdb/arm-tdep.c | 43 +++++++++++++++++++------------------------
>  1 file changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index cb0030c..4e223cb 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3275,7 +3275,6 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>         found_return = 1;
>        else if (thumb_instruction_restores_sp (insn))
>         {
> -         found_stack_adjust = 1;
>           if ((insn & 0xfe00) == 0xbd00)  /* pop <registers, PC> */
>             found_return = 1;
>         }
> @@ -3289,20 +3288,18 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>
>           if (insn == 0xe8bd)  /* ldm.w sp!, <registers> */
>             {
> -             found_stack_adjust = 1;
>               if (insn2 & 0x8000)  /* <registers> include PC.  */
>                 found_return = 1;
>             }
>           else if (insn == 0xf85d  /* ldr.w <Rt>, [sp], #4 */
>                    && (insn2 & 0x0fff) == 0x0b04)
>             {
> -             found_stack_adjust = 1;
>               if ((insn2 & 0xf000) == 0xf000) /* <Rt> is PC.  */
>                 found_return = 1;
>             }
>           else if ((insn & 0xffbf) == 0xecbd  /* vldm sp!, <list> */
>                    && (insn2 & 0x0e00) == 0x0a00)
> -           found_stack_adjust = 1;
> +           ;
>           else
>             break;
>         }
> @@ -3318,28 +3315,26 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>       scan backwards for at most one instruction.  Try either a 16-bit or
>       a 32-bit instruction.  This is just a heuristic, so we do not worry
>       too much about false positives.  */
> +  found_stack_adjust = 0;
>
> -  if (!found_stack_adjust)
> -    {
> -      if (pc - 4 < func_start)
> -       return 0;
> -      if (target_read_memory (pc - 4, buf, 4))
> -       return 0;
> -
> -      insn = extract_unsigned_integer (buf, 2, byte_order_for_code);
> -      insn2 = extract_unsigned_integer (buf + 2, 2, byte_order_for_code);
> +  if (pc - 4 < func_start)
> +    return 0;
> +  if (target_read_memory (pc - 4, buf, 4))
> +    return 0;
>
> -      if (thumb_instruction_restores_sp (insn2))
> -       found_stack_adjust = 1;
> -      else if (insn == 0xe8bd)  /* ldm.w sp!, <registers> */
> -       found_stack_adjust = 1;
> -      else if (insn == 0xf85d  /* ldr.w <Rt>, [sp], #4 */
> -              && (insn2 & 0x0fff) == 0x0b04)
> -       found_stack_adjust = 1;
> -      else if ((insn & 0xffbf) == 0xecbd  /* vldm sp!, <list> */
> -              && (insn2 & 0x0e00) == 0x0a00)
> -       found_stack_adjust = 1;
> -    }
> +  insn = extract_unsigned_integer (buf, 2, byte_order_for_code);
> +  insn2 = extract_unsigned_integer (buf + 2, 2, byte_order_for_code);
> +
> +  if (thumb_instruction_restores_sp (insn2))
> +    found_stack_adjust = 1;
> +  else if (insn == 0xe8bd)  /* ldm.w sp!, <registers> */
> +    found_stack_adjust = 1;
> +  else if (insn == 0xf85d  /* ldr.w <Rt>, [sp], #4 */
> +          && (insn2 & 0x0fff) == 0x0b04)
> +    found_stack_adjust = 1;
> +  else if ((insn & 0xffbf) == 0xecbd  /* vldm sp!, <list> */
> +          && (insn2 & 0x0e00) == 0x0a00)
> +    found_stack_adjust = 1;
>
>    return found_stack_adjust;
>  }
> --
> 1.9.0
>



-- 
Will Newton
Toolchain Working Group, Linaro