From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27612 invoked by alias); 27 Aug 2014 08:17:04 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 27601 invoked by uid 89); 27 Aug 2014 08:17:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ie0-f180.google.com Received: from mail-ie0-f180.google.com (HELO mail-ie0-f180.google.com) (209.85.223.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 27 Aug 2014 08:17:02 +0000 Received: by mail-ie0-f180.google.com with SMTP id at20so12562209iec.25 for ; Wed, 27 Aug 2014 01:17:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=Mpmk2R+3CRdjon7eoOpilFzIq95/9MPgGe+jwAfR//Q=; b=aH7Y4s3g9ZyOrSDtanHFojhVqXWyJx6VxxwpCSOLc8pgXbzvKe9eeskkjFGt8qthTb WntWGr6hJuAkOSjuX8X4c01eV1KVznv6XJCyiD9nz9Hnl56dhajCQl+xVHL4U3my+buP +LFKSyOBSIFTlce4NQ8eixw+ciDyk5uVg8Ja1Qo8pFINjv8WrO0jUpWj+06gVLFb+JAC W43VphKLQB65E5CPP0QyscDhbkk/YBdHWBirXqMqRJJEJeVhXG4p6WNqy4f1xKHfdKZW bA3fSmZhM+J8lKH+XltlU+o4I8sq28/GZ2cYHO4hK3lIKlX8T3l1zJCQzAPrQ5P7768Q KDhw== X-Gm-Message-State: ALoCoQnjA/FB9HKP6BrdCE6bO8Uei6yOyYMER65y6d8n5fDobvLU1pIK9NpUbuLOdNRpjkezYOFc MIME-Version: 1.0 X-Received: by 10.42.101.77 with SMTP id d13mr17595274ico.53.1409127420465; Wed, 27 Aug 2014 01:17:00 -0700 (PDT) Received: by 10.64.142.116 with HTTP; Wed, 27 Aug 2014 01:17:00 -0700 (PDT) In-Reply-To: <1407295090-17296-1-git-send-email-yao@codesourcery.com> References: <1407295090-17296-1-git-send-email-yao@codesourcery.com> Date: Wed, 27 Aug 2014 08:17:00 -0000 Message-ID: Subject: Re: [PATCH] arm software watchpoint: return to epilogue From: Will Newton To: Yao Qi Cc: "gdb-patches@sourceware.org" Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-08/txt/msg00545.txt.bz2 On 6 August 2014 04:18, Yao Qi 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 > > * 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 */ > found_return = 1; > } > @@ -3289,20 +3288,18 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc) > > if (insn == 0xe8bd) /* ldm.w sp!, */ > { > - found_stack_adjust = 1; > if (insn2 & 0x8000) /* include PC. */ > found_return = 1; > } > else if (insn == 0xf85d /* ldr.w , [sp], #4 */ > && (insn2 & 0x0fff) == 0x0b04) > { > - found_stack_adjust = 1; > if ((insn2 & 0xf000) == 0xf000) /* is PC. */ > found_return = 1; > } > else if ((insn & 0xffbf) == 0xecbd /* vldm sp!, */ > && (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!, */ > - found_stack_adjust = 1; > - else if (insn == 0xf85d /* ldr.w , [sp], #4 */ > - && (insn2 & 0x0fff) == 0x0b04) > - found_stack_adjust = 1; > - else if ((insn & 0xffbf) == 0xecbd /* vldm sp!, */ > - && (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!, */ > + found_stack_adjust = 1; > + else if (insn == 0xf85d /* ldr.w , [sp], #4 */ > + && (insn2 & 0x0fff) == 0x0b04) > + found_stack_adjust = 1; > + else if ((insn & 0xffbf) == 0xecbd /* vldm sp!, */ > + && (insn2 & 0x0e00) == 0x0a00) > + found_stack_adjust = 1; > > return found_stack_adjust; > } > -- > 1.9.0 > -- Will Newton Toolchain Working Group, Linaro