Message ID | 1407295090-17296-1-git-send-email-yao@codesourcery.com |
---|---|
State | New |
Headers | show |
On 08/06/2014 11:18 AM, Yao Qi wrote: > 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. Ping.
Yao Qi <yao@codesourcery.com> writes: > 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. Ping^2 https://sourceware.org/ml/gdb-patches/2014-08/msg00060.html
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 >
On 08/06/2014 04:18 AM, Yao Qi wrote: > 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. > 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. This gdbarch hook's name is a bit misleading -- your comment above kind of makes it sound like the patch is doing some kind of target specific hack, while this is exactly how the gdbarch hook is specified: # A target might have problems with watchpoints as soon as the stack # frame of the current function has been destroyed. This mostly happens # as the first action in a funtion's epilogue. in_function_epilogue_p() # is defined to return a non-zero value if either the given addr is one ^^^^^^ # instruction after the stack destroying instruction up to the trailing ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ # return instruction or if we can figure out that the stack frame has # already been invalidated regardless of the value of addr. Targets # which don't suffer from that problem could just let this functionality # untouched. m:int:in_function_epilogue_p:CORE_ADDR addr:addr:0:generic_in_function_epilogue_p::0 > The patch is tested in arm-none-eabi and arm-none-linux-gnueabi with > various multilibs. OK to apply? This is OK with Will's comment addressed. Thanks, Pedro Alves
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; }