This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] arm software watchpoint: return to epilogue
- From: Will Newton <will dot newton at linaro dot org>
- To: Yao Qi <yao at codesourcery dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Wed, 27 Aug 2014 09:17:00 +0100
- Subject: Re: [PATCH] arm software watchpoint: return to epilogue
- Authentication-results: sourceware.org; auth=none
- References: <1407295090-17296-1-git-send-email-yao at codesourcery dot com>
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