Re: [PATCH] [AArch64] Recognize more program breakpoint patterns

[Alan Hayward <Alan dot Hayward at arm dot com>]

> On 10 Jan 2020, at 14:58, Luis Machado <luis.machado@linaro.org> wrote:
> 
> 
> 
> On 1/9/20 1:25 PM, Alan Hayward wrote:
>>> On 9 Jan 2020, at 15:45, Luis Machado <luis.machado@linaro.org> wrote:
>>> 
>>> On 1/9/20 11:52 AM, Alan Hayward wrote:
>>>>> On 23 Dec 2019, at 17:34, Luis Machado <luis.machado@linaro.org> wrote:
>>>>> 
>>>>> It was reported to me that program breakpoints (permanent ones inserted into
>>>>> the code itself) other than the one GDB uses for AArch64 (0xd4200000) do not
>>>>> generate visible stops when continuing, and GDB will continue spinning
>>>>> infinitely.
>>>>> 
>>>>> This happens because GDB, upon hitting one of those program breakpoints, thinks
>>>>> the SIGTRAP came from a delayed breakpoint hit...
>>>>> 
>>>>> (gdb) x/i $pc
>>>>> => 0x4005c0 <problem_function>: brk     #0x90f
>>>>> (gdb) c
>>>>> Continuing.
>>>>> infrun: clear_proceed_status_thread (process 14198)
>>>>> infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
>>>>> infrun: proceed: resuming process 14198
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
>>>>> infrun: infrun_async(1)
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun:   14198.14198.0 [process 14198],
>>>>> infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: stop_pc = 0x4005c0
>>>>> infrun: delayed software breakpoint trap, ignoring
>>>>> infrun: no stepping, continue
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun:   14198.14198.0 [process 14198],
>>>>> infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: stop_pc = 0x4005c0
>>>>> infrun: delayed software breakpoint trap, ignoring
>>>>> infrun: no stepping, continue
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun:   14198.14198.0 [process 14198],
>>>>> infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: stop_pc = 0x4005c0
>>>>> infrun: delayed software breakpoint trap, ignoring
>>>>> infrun: no stepping, continue
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun:   14198.14198.0 [process 14198],
>>>>> infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: stop_pc = 0x4005c0
>>>>> infrun: delayed software breakpoint trap, ignoring
>>>>> infrun: no stepping, continue
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun:   14198.14198.0 [process 14198],
>>>>> infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> ...
>>>>> 
>>>>> ... which is not the case.
>>>>> 
>>>>> If the program breakpoint is one GDB recognizes, then it will stop when it
>>>>> hits it.
>>>>> 
>>>>> (gdb) x/i $pc
>>>>> => 0x4005c0 <problem_function>: brk     #0x0
>>>>> (gdb) c
>>>>> Continuing.
>>>>> infrun: clear_proceed_status_thread (process 14193)
>>>>> infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
>>>>> infrun: proceed: resuming process 14193
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14193] at 0x4005c0
>>>>> infrun: infrun_async(1)
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun:   14193.14193.0 [process 14193],
>>>>> infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: stop_pc = 0x4005c0
>>>>> infrun: random signal (GDB_SIGNAL_TRAP)
>>>>> infrun: stop_waiting
>>>>> infrun: stop_all_threads
>>>>> infrun: stop_all_threads, pass=0, iterations=0
>>>>> infrun:   process 14193 not executing
>>>>> infrun: stop_all_threads, pass=1, iterations=1
>>>>> infrun:   process 14193 not executing
>>>>> infrun: stop_all_threads done
>>>>> 
>>>>> Program received signal SIGTRAP, Trace/breakpoint trap.
>>>>> problem_function () at brk_0.c:7
>>>>> 7        asm("brk %0\n\t" ::"n"(0x0));
>>>>> infrun: infrun_async(0)
>>>>> 
>>>>> Otherwise GDB will keep trying to resume the inferior and will keep
>>>>> seeing the SIGTRAP's, without stopping.
>>>>> 
>>>>> To the user it appears GDB has gone into an infinite loop, interruptible only
>>>>> by Ctrl-C.
>>>>> 
>>>>> Also, windbg seems to use a different variation of AArch64 breakpoint compared
>>>>> to GDB. This causes problems when debugging Windows on ARM binaries, when
>>>>> program breakpoints are being used.
>>>>> 
>>>>> The proposed patch creates a new gdbarch method (gdbarch_insn_is_breakpoint)
>>>>> that tells GDB whether the underlying instruction is a breakpoint instruction
>>>>> or not.
>>>>> 
>>>>> This is more general than only checking for the instruction GDB uses as
>>>>> breakpoint.
>>>>> 
>>>>> The existing logic is still preserved for targets that do not implement this
>>>>> new gdbarch method.
>>>>> 
>>>>> The end result is like so:
>>>>> 
>>>>> (gdb) x/i $pc
>>>>> => 0x4005c0 <problem_function>: brk     #0x90f
>>>>> (gdb) c
>>>>> Continuing.
>>>>> infrun: clear_proceed_status_thread (process 16417)
>>>>> infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
>>>>> infrun: proceed: resuming process 16417
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 16417] at 0x4005c0
>>>>> infrun: infrun_async(1)
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun:   16417.16417.0 [process 16417],
>>>>> infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: stop_pc = 0x4005c0
>>>>> infrun: random signal (GDB_SIGNAL_TRAP)
>>>>> infrun: stop_waiting
>>>>> infrun: stop_all_threads
>>>>> infrun: stop_all_threads, pass=0, iterations=0
>>>>> infrun:   process 16417 not executing
>>>>> infrun: stop_all_threads, pass=1, iterations=1
>>>>> infrun:   process 16417 not executing
>>>>> infrun: stop_all_threads done
>>>>> 
>>>>> Program received signal SIGTRAP, Trace/breakpoint trap.
>>>>> problem_function () at brk.c:7
>>>>> 7        asm("brk %0\n\t" ::"n"(0x900 + 0xf));
>>>>> infrun: infrun_async(0)
>>>>> 
>>>>> Does this change look ok?
>>>>> 
>>>>> gdb/ChangeLog:
>>>>> 
>>>>> 2019-12-23  Luis Machado  <luis.machado@linaro.org>
>>>>> 
>>>>> 	* aarch64-tdep.c (BRK_INSN_MASK): Define to 0xd4200000.
>>>>> 	(aarch64_insn_is_breakpoint): New function.
>>>>> 	(aarch64_gdbarch_init): Set gdbarch_insn_is_breakpoint hook.
>>>>> 	* arch-utils.c (default_insn_is_breakpoint): New function.
>>>>> 	* arch-utils.h (default_insn_is_breakpoint): New prototype.
>>>>> 	* breakpoint.c (program_breakpoint_here): Updated to use
>>>>> 	  gdbarch_insn_is_breakpoint.
>>>>> 	Update documentation to clarify behavior.
>>>>> 	* gdbarch.c: Regenerate.
>>>>> 	* gdbarch.h: Regenerate.
>>>>> 	* gdbarch.sh (gdbarch_insn_is_breakpoint): New method.
>>>>> 
>>>>> Change-Id: I96eb27151442f435560a58c87eac48b0f68432bc
>>>>> ---
>>>>> gdb/aarch64-tdep.c | 25 +++++++++++++++++++++++++
>>>>> gdb/arch-utils.c   |  7 +++++++
>>>>> gdb/arch-utils.h   |  3 +++
>>>>> gdb/breakpoint.c   | 19 ++++++++++++++-----
>>>>> gdb/gdbarch.c      | 23 +++++++++++++++++++++++
>>>>> gdb/gdbarch.h      |  7 +++++++
>>>>> gdb/gdbarch.sh     |  4 ++++
>>>>> 7 files changed, 83 insertions(+), 5 deletions(-)
>>>>> 
>>>> Do you have a test case for this? It could go in the gdb.arch/ directory.
>>>> It’d be fairly easy to check all the different brk patterns.
>>> 
>>> I'll work on this. I'm thinking we could auto-generate a number of brk patterns and verify we can continue and hit each of them.
>>> 
>>>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>>>> index 1d5fb2001d..c69361d4ea 100644
>>>>> --- a/gdb/aarch64-tdep.c
>>>>> +++ b/gdb/aarch64-tdep.c
>>>>> @@ -1201,6 +1201,28 @@ aarch64_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_byte op,
>>>>>   return false;
>>>>> }
>>>>> 
>>>>> +#define BRK_INSN_MASK 0xd4200000
>>>>> +
>>>>> +/* Implementation of gdbarch_insn_is_breakpoint for aarch64.  */
>>>>> +
>>>>> +static bool
>>>>> +aarch64_insn_is_breakpoint (gdbarch *gdbarch,
>>>>> +			    const gdb_byte *insn,
>>>>> +			    unsigned int insn_size)
>>>>> +{
>>>>> +  gdb_assert (insn != nullptr);
>>>>> +
>>>>> +  uint32_t i;
>>>>> +
>>>>> +  i = (uint32_t) extract_unsigned_integer (insn, insn_size,
>>>>> +					   gdbarch_byte_order (gdbarch));
>>>>> +
>>>>> +  /* Check if INSN is a BRK instruction pattern.  There are multiple choices
>>>>> +     of such instructions with different immediate values.  Different OS' may
>>>>> +     use a different variation, but they have the same outcome.  */
>>>>> +  return (i & BRK_INSN_MASK) == BRK_INSN_MASK;
>>>>> +}
>>>>> +
>>>>> /* When arguments must be pushed onto the stack, they go on in reverse
>>>>>    order.  The code below implements a FILO (stack) to do this.  */
>>>>> 
>>>>> @@ -3357,6 +3379,9 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>>>>   set_gdbarch_execute_dwarf_cfa_vendor_op (gdbarch,
>>>>> 					   aarch64_execute_dwarf_cfa_vendor_op);
>>>>> 
>>>>> +  /* Permanent/Program breakpoint handling.  */
>>>>> +  set_gdbarch_insn_is_breakpoint (gdbarch, aarch64_insn_is_breakpoint);
>>>>> +
>>>>>   /* Add some default predicates.  */
>>>>>   frame_unwind_append_unwinder (gdbarch, &aarch64_stub_unwind);
>>>>>   dwarf2_append_unwinders (gdbarch);
>>>>> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
>>>>> index a1a003f91f..99c9f281be 100644
>>>>> --- a/gdb/arch-utils.c
>>>>> +++ b/gdb/arch-utils.c
>>>>> @@ -876,6 +876,13 @@ int default_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr)
>>>>>   return 0;
>>>>> }
>>>> Add "/* See arch-utils.h.  */" ...
>>> 
>>> Fixed.
>>> 
>>>>> 
>>>>> +bool default_insn_is_breakpoint (struct gdbarch *gdbarch,
>>>>> +				 const gdb_byte  *insn,
>>>>> +				 unsigned int insn_size)
>>>>> +{
>>>>> +  return false;
>>>> I don’t like that this is just returning false, as it’s not really doing what the function name says.
>>>> How about if the function did this:
>>>>   return (memcmp (target_mem, bpoint, len) == 0);
>>>> Then remove the memcmp from program_breakpoint_here_p.
>>> 
>>> I agree it would be cleaner, but ...
>>> 
>>>> You’ll probably have to move the call to gdbarch_breakpoint_from_pc into here too.
>>> 
>>> ... this depends on calling gdbarch_breakpoint_from_pc to fetch bpoint, and gdbarch_breakpoint_from_pc requires the address information so it can determine the breakpoint kind.
>>> 
>>> Passing in the address is a bit out of scope for what the function is supposed to do (verify if a particular instruction is a breakpoint).
>>> 
>>> I don't have a strong objection towards passing in the address (or NULL if no address) if others are OK with it.
>> How about replacing program_breakpoint_here_p with
>> gdbarch_program_breakpoint_here_p(struct gdbarch *gdbarch, CORE_ADDR address) ?
>> default_program_breakpoint_here_p would be an exact copy of the existing program_breakpoint_here_p.
>> aarch64_program_breakpoint_here_p would do the copy to memory then the same as aarch64_insn_is_breakpoint.
> 
> It sounds interesting at first, but i'm not sure about it. We'll save a memory comparison but will duplicate the code to gdbarch_breakpoint_from_pc to every arch that sets its own gdbarch_program_breakpoint_here_p.
> 
> At least the old function dealt with just arch-specific bits, whereas the <arch>_program_breakpoint_here_p implementation will need to do a generic check + generic memory read as well, then do the arch-specific checks.
> 
> Thoughts?


It should be rare that any other architecture needs to define their own
gdbarch_program_breakpoint_here_p function.

However, let's rewind a little. My concern was that in the existing code, the common
program_breakpoint_here_p is assuming that the target has a single breakpoint instruction,
which it obtains by calling gdbarch_breakpoint_from_pc. On AArch64, we have multiple patterns
to match. With your patch, program_breakpoint_here_p now checks both the single pattern and
multiple pattern cases, which feels wrong. Instead the common code should just ask the target
code “does this address contain a breakpoint?”

I suggested creating gdbarch_program_breakpoint_here_p as that would reduce the number of
functions called, which helps when trying to follow the code logic.
However, I’m happy for the target_read_memory to remain in common code, then pass that down
to the arch specific versions.

I’m not sure if anything else uses it, but maybe the call to set_gdbarch_breakpoint_kind_from_pc
in aarch64-tdep.c should probably be removed too.


Alan.