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

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

> 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.


> 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.  */" ...

> 
> +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.

You’ll probably have to move the call to gdbarch_breakpoint_from_pc into here too.


> +}
> +
> void
> default_skip_permanent_breakpoint (struct regcache *regcache)
> {
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index 48ff3bb9a1..77ffe8190c 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -227,6 +227,9 @@ extern int default_return_in_first_hidden_param_p (struct gdbarch *,
> extern int default_insn_is_call (struct gdbarch *, CORE_ADDR);
> extern int default_insn_is_ret (struct gdbarch *, CORE_ADDR);
> extern int default_insn_is_jump (struct gdbarch *, CORE_ADDR);

... plus a brief comment here.


> +extern bool default_insn_is_breakpoint (struct gdbarch *gdbarch,
> +					const gdb_byte *insn,
> +					unsigned int insn_size);
> 
> /* Do-nothing version of vsyscall_range.  Returns false.  */
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 904abda8db..ffb7f7f8be 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -8599,14 +8599,23 @@ program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address)
>   target_mem = (gdb_byte *) alloca (len);
> 
>   /* Enable the automatic memory restoration from breakpoints while
> -     we read the memory.  Otherwise we could say about our temporary
> -     breakpoints they are permanent.  */
> +     we read the memory.  Otherwise we may find temporary breakpoints, ones
> +     inserted by GDB, and flag them as permanent breakpoints.  */
>   scoped_restore restore_memory
>     = make_scoped_restore_show_memory_breakpoints (0);
> 
> -  if (target_read_memory (address, target_mem, len) == 0
> -      && memcmp (target_mem, bpoint, len) == 0)
> -    return 1;
> +  if (target_read_memory (address, target_mem, len) == 0)
> +    {
> +      /* Check if this is a breakpoint instruction for this architecture,
> +	 including ones used by GDB. 
> +
> +	 Some architectures have more than one possible breakpoint
> +	 instruction, but GDB does not use all of them.  We should detect those
> +	 as well.  */
> +      if (gdbarch_insn_is_breakpoint (gdbarch, target_mem, len)
> +	  ||  memcmp (target_mem, bpoint, len) == 0)
> +	return 1;
> +    }
> 
>   return 0;
> }
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 59c97da985..9e21ba7eca 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -345,6 +345,7 @@ struct gdbarch
>   gdbarch_insn_is_call_ftype *insn_is_call;
>   gdbarch_insn_is_ret_ftype *insn_is_ret;
>   gdbarch_insn_is_jump_ftype *insn_is_jump;
> +  gdbarch_insn_is_breakpoint_ftype *insn_is_breakpoint;
>   gdbarch_auxv_parse_ftype *auxv_parse;
>   gdbarch_print_auxv_entry_ftype *print_auxv_entry;
>   gdbarch_vsyscall_range_ftype *vsyscall_range;
> @@ -464,6 +465,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
>   gdbarch->insn_is_call = default_insn_is_call;
>   gdbarch->insn_is_ret = default_insn_is_ret;
>   gdbarch->insn_is_jump = default_insn_is_jump;
> +  gdbarch->insn_is_breakpoint = default_insn_is_breakpoint;
>   gdbarch->print_auxv_entry = default_print_auxv_entry;
>   gdbarch->vsyscall_range = default_vsyscall_range;
>   gdbarch->infcall_mmap = default_infcall_mmap;
> @@ -708,6 +710,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
>   /* Skip verify of insn_is_call, invalid_p == 0 */
>   /* Skip verify of insn_is_ret, invalid_p == 0 */
>   /* Skip verify of insn_is_jump, invalid_p == 0 */
> +  /* Skip verify of insn_is_breakpoint, invalid_p == 0 */
>   /* Skip verify of auxv_parse, has predicate.  */
>   /* Skip verify of print_auxv_entry, invalid_p == 0 */
>   /* Skip verify of vsyscall_range, invalid_p == 0 */
> @@ -1137,6 +1140,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
>   fprintf_unfiltered (file,
>                       "gdbarch_dump: inner_than = <%s>\n",
>                       host_address_to_string (gdbarch->inner_than));
> +  fprintf_unfiltered (file,
> +                      "gdbarch_dump: insn_is_breakpoint = <%s>\n",
> +                      host_address_to_string (gdbarch->insn_is_breakpoint));
>   fprintf_unfiltered (file,
>                       "gdbarch_dump: insn_is_call = <%s>\n",
>                       host_address_to_string (gdbarch->insn_is_call));
> @@ -4928,6 +4934,23 @@ set_gdbarch_insn_is_jump (struct gdbarch *gdbarch,
>   gdbarch->insn_is_jump = insn_is_jump;
> }
> 
> +bool
> +gdbarch_insn_is_breakpoint (struct gdbarch *gdbarch, const gdb_byte *insn, unsigned int insn_size)
> +{
> +  gdb_assert (gdbarch != NULL);
> +  gdb_assert (gdbarch->insn_is_breakpoint != NULL);
> +  if (gdbarch_debug >= 2)
> +    fprintf_unfiltered (gdb_stdlog, "gdbarch_insn_is_breakpoint called\n");
> +  return gdbarch->insn_is_breakpoint (gdbarch, insn, insn_size);
> +}
> +
> +void
> +set_gdbarch_insn_is_breakpoint (struct gdbarch *gdbarch,
> +                                gdbarch_insn_is_breakpoint_ftype insn_is_breakpoint)
> +{
> +  gdbarch->insn_is_breakpoint = insn_is_breakpoint;
> +}
> +
> int
> gdbarch_auxv_parse_p (struct gdbarch *gdbarch)
> {
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 78e05ecdcb..d94950b8f2 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -1545,6 +1545,13 @@ typedef int (gdbarch_insn_is_jump_ftype) (struct gdbarch *gdbarch, CORE_ADDR add
> extern int gdbarch_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr);
> extern void set_gdbarch_insn_is_jump (struct gdbarch *gdbarch, gdbarch_insn_is_jump_ftype *insn_is_jump);
> 
> +/* Return true if INSN of size INSN_SIZE acts like a permanent breakpoint and
> +   false otherwise. */
> +
> +typedef bool (gdbarch_insn_is_breakpoint_ftype) (struct gdbarch *gdbarch, const gdb_byte *insn, unsigned int insn_size);
> +extern bool gdbarch_insn_is_breakpoint (struct gdbarch *gdbarch, const gdb_byte *insn, unsigned int insn_size);
> +extern void set_gdbarch_insn_is_breakpoint (struct gdbarch *gdbarch, gdbarch_insn_is_breakpoint_ftype *insn_is_breakpoint);
> +
> /* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
>    Return 0 if *READPTR is already at the end of the buffer.
>    Return -1 if there is insufficient buffer for a whole entry.
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 331eb39278..dbf31ee8ae 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1152,6 +1152,10 @@ m;int;insn_is_ret;CORE_ADDR addr;addr;;default_insn_is_ret;;0
> # Return non-zero if the instruction at ADDR is a jump; zero otherwise.
> m;int;insn_is_jump;CORE_ADDR addr;addr;;default_insn_is_jump;;0
> 
> +# Return true if INSN of size INSN_SIZE acts like a permanent breakpoint and
> +# false otherwise.
> +m;bool;insn_is_breakpoint;const gdb_byte *insn, unsigned int insn_size;insn, insn_size;;default_insn_is_breakpoint;;0
> +
> # Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
> # Return 0 if *READPTR is already at the end of the buffer.
> # Return -1 if there is insufficient buffer for a whole entry.
> — 
> 2.17.1
>