[PATCH 0/2,v3][AArch64]Handle additional brk instruction patterns

[PATCH 0/2,v3][AArch64]Handle additional brk instruction patterns

[Luis Machado]

This patch teaches GDB how to handle additional breakpoint instruction
patterns for aarch64.

Luis Machado (2):
  [AArch64] Recognize more program breakpoint patterns
  [AArch64] Test handling of additional brk instruction patterns

 gdb/aarch64-tdep.c                            | 37 ++++++++++
 gdb/arch-utils.c                              | 32 ++++++++
 gdb/arch-utils.h                              |  4 +
 gdb/breakpoint.c                              | 46 ++----------
 gdb/breakpoint.h                              |  5 --
 gdb/gdbarch.c                                 | 23 ++++++
 gdb/gdbarch.h                                 |  7 ++
 gdb/gdbarch.sh                                |  4 +
 gdb/infrun.c                                  |  4 +-
 gdb/testsuite/gdb.arch/aarch64-brk-patterns.c | 30 ++++++++
 .../gdb.arch/aarch64-brk-patterns.exp         | 74 +++++++++++++++++++
 11 files changed, 220 insertions(+), 46 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp

-- 
2.17.1

[PATCH 1/2,v3] [AArch64] Recognize more program breakpoint patterns

[Luis Machado]

New in v3:

- Code cleanups based on reviews.

New in v2:

- Fixed misc problems based on reviews.
- Switched to using gdbarch_program_breakpoint_here_p as opposed to
gdbarch_insn_is_breakpoint.
- Fixed matching of brk instructions. Previously the mask was incorrect, which
was showing up as a few failures in the testsuite. Now it is clean.
- New testcase (separate patch).
- Moved program_breakpoint_here () to arch-utils.c and made it the default
implementation of gdbarch_program_breakpoint_here_p.

--

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_program_breakpoint_here_p)
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)

gdb/ChangeLog:

2020-01-15  Luis Machado  <luis.machado@linaro.org>

	* aarch64-tdep.c (BRK_INSN_MASK): Define to 0xffe0001f.
	(BRK_INSN_MASK): Define to 0xd4200000.
	(aarch64_program_breakpoint_here_p): New function.
	(aarch64_gdbarch_init): Set gdbarch_program_breakpoint_here_p hook.
	* arch-utils.c (default_program_breakpoint_here_p): Moved from
	breakpoint.c.
	* arch-utils.h (default_program_breakpoint_here_p): Moved from
	breakpoint.h
	* breakpoint.c (bp_loc_is_permanent): Changed return type to bool and
	call gdbarch_program_breakpoint_here_p.
	(program_breakpoint_here): Moved to arch-utils.c, renamed to
	default_program_breakpoint_here_p, changed return type to bool and
	simplified.
	* breakpoint.h (program_breakpoint_here): Moved prototype to
	arch-utils.h, renamed to default_program_breakpoint_here_p and changed
	return type to bool.
	* gdbarch.c: Regenerate.
	* gdbarch.h: Regenerate.
	* gdbarch.sh (program_breakpoint_here_p): New method.
	* infrun.c (handle_signal_stop): Call
	gdbarch_program_breakpoint_here_p.
---
 gdb/aarch64-tdep.c | 37 +++++++++++++++++++++++++++++++++++++
 gdb/arch-utils.c   | 32 ++++++++++++++++++++++++++++++++
 gdb/arch-utils.h   |  4 ++++
 gdb/breakpoint.c   | 46 +++++++---------------------------------------
 gdb/breakpoint.h   |  5 -----
 gdb/gdbarch.c      | 23 +++++++++++++++++++++++
 gdb/gdbarch.h      |  7 +++++++
 gdb/gdbarch.sh     |  4 ++++
 gdb/infrun.c       |  4 ++--
 9 files changed, 116 insertions(+), 46 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 8451a916dc..7f32df3ec0 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1201,6 +1201,39 @@ aarch64_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_byte op,
   return false;
 }
 
+/* Used for matching BRK instructions for AArch64.  */
+static constexpr uint32_t BRK_INSN_MASK = 0xffe0001f;
+static constexpr uint32_t BRK_INSN_BASE = 0xd4200000;
+
+/* Implementation of gdbarch_program_breakpoint_here_p for aarch64.  */
+
+static bool
+aarch64_program_breakpoint_here_p (gdbarch *gdbarch, CORE_ADDR address)
+{
+  const uint32_t insn_len = 4;
+  gdb_byte target_mem[4];
+
+  /* Enable the automatic memory restoration from breakpoints while
+     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, insn_len) == 0)
+    {
+      uint32_t insn =
+	(uint32_t) extract_unsigned_integer (target_mem, insn_len,
+					     gdbarch_byte_order_for_code (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 ((insn & BRK_INSN_MASK) == BRK_INSN_BASE);
+    }
+
+  return false;
+}
+
 /* 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 +3390,10 @@ 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_program_breakpoint_here_p (gdbarch,
+					 aarch64_program_breakpoint_here_p);
+
   /* 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 92d7153ccb..04955ea847 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -876,6 +876,38 @@ int default_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr)
   return 0;
 }
 
+/*  See arch-utils.h.  */
+
+bool
+default_program_breakpoint_here_p (struct gdbarch *gdbarch,
+				   CORE_ADDR address)
+{
+  int len;
+  const gdb_byte *bpoint = gdbarch_breakpoint_from_pc (gdbarch, &address, &len);
+
+  /* Software breakpoints unsupported?  */
+  if (bpoint == nullptr)
+    return false;
+
+  gdb_byte *target_mem = (gdb_byte *) alloca (len);
+
+  /* Enable the automatic memory restoration from breakpoints while
+     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)
+    {
+      /* Check if this is a breakpoint instruction for this architecture,
+	 including ones used by GDB.  */
+      if (memcmp (target_mem, bpoint, len) == 0)
+	return true;
+    }
+
+  return false;
+}
+
 void
 default_skip_permanent_breakpoint (struct regcache *regcache)
 {
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 3fb9ad317a..43d64b1f4f 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -228,6 +228,10 @@ 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);
 
+/* Default implementation of gdbarch_program_breakpoint_here_p.  */
+extern bool default_program_breakpoint_here_p (struct gdbarch *gdbarch,
+					       CORE_ADDR addr);
+
 /* Do-nothing version of vsyscall_range.  Returns false.  */
 
 extern int default_vsyscall_range (struct gdbarch *gdbarch, struct mem_range *range);
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index a7a378131a..5a9352c26f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -8515,7 +8515,7 @@ mention (struct breakpoint *b)
 }
 \f
 
-static int bp_loc_is_permanent (struct bp_location *loc);
+static bool bp_loc_is_permanent (struct bp_location *loc);
 
 static struct bp_location *
 add_location_to_breakpoint (struct breakpoint *b,
@@ -8581,42 +8581,10 @@ add_location_to_breakpoint (struct breakpoint *b,
 }
 \f
 
-/* See breakpoint.h.  */
-
-int
-program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address)
-{
-  int len;
-  CORE_ADDR addr;
-  const gdb_byte *bpoint;
-  gdb_byte *target_mem;
-
-  addr = address;
-  bpoint = gdbarch_breakpoint_from_pc (gdbarch, &addr, &len);
-
-  /* Software breakpoints unsupported?  */
-  if (bpoint == NULL)
-    return 0;
-
-  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.  */
-  scoped_restore restore_memory
-    = make_scoped_restore_show_memory_breakpoints (0);
+/* Return true if LOC is pointing to a permanent breakpoint,
+   return false otherwise.  */
 
-  if (target_read_memory (address, target_mem, len) == 0
-      && memcmp (target_mem, bpoint, len) == 0)
-    return 1;
-
-  return 0;
-}
-
-/* Return 1 if LOC is pointing to a permanent breakpoint,
-   return 0 otherwise.  */
-
-static int
+static bool
 bp_loc_is_permanent (struct bp_location *loc)
 {
   gdb_assert (loc != NULL);
@@ -8624,14 +8592,14 @@ bp_loc_is_permanent (struct bp_location *loc)
   /* If we have a non-breakpoint-backed catchpoint or a software
      watchpoint, just return 0.  We should not attempt to read from
      the addresses the locations of these breakpoint types point to.
-     program_breakpoint_here_p, below, will attempt to read
+     gdbarch_program_breakpoint_here_p, below, will attempt to read
      memory.  */
   if (!bl_address_is_meaningful (loc))
-    return 0;
+    return false;
 
   scoped_restore_current_pspace_and_thread restore_pspace_thread;
   switch_to_program_space_and_thread (loc->pspace);
-  return program_breakpoint_here_p (loc->gdbarch, loc->address);
+  return gdbarch_program_breakpoint_here_p (loc->gdbarch, loc->address);
 }
 
 /* Build a command list for the dprintf corresponding to the current
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 13d8102c17..347aeb75f3 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1194,11 +1194,6 @@ enum breakpoint_here
 
 /* Prototypes for breakpoint-related functions.  */
 
-/* Return 1 if there's a program/permanent breakpoint planted in
-   memory at ADDRESS, return 0 otherwise.  */
-
-extern int program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address);
-
 extern enum breakpoint_here breakpoint_here_p (const address_space *,
 					       CORE_ADDR);
 
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index cc8569f5c9..d763fc85e6 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_program_breakpoint_here_p_ftype *program_breakpoint_here_p;
   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->program_breakpoint_here_p = default_program_breakpoint_here_p;
   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 program_breakpoint_here_p, 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 */
@@ -1248,6 +1251,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   fprintf_unfiltered (file,
                       "gdbarch_dump: process_record_signal = <%s>\n",
                       host_address_to_string (gdbarch->process_record_signal));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: program_breakpoint_here_p = <%s>\n",
+                      host_address_to_string (gdbarch->program_breakpoint_here_p));
   fprintf_unfiltered (file,
                       "gdbarch_dump: ps_regnum = %s\n",
                       plongest (gdbarch->ps_regnum));
@@ -4928,6 +4934,23 @@ set_gdbarch_insn_is_jump (struct gdbarch *gdbarch,
   gdbarch->insn_is_jump = insn_is_jump;
 }
 
+bool
+gdbarch_program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->program_breakpoint_here_p != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_program_breakpoint_here_p called\n");
+  return gdbarch->program_breakpoint_here_p (gdbarch, address);
+}
+
+void
+set_gdbarch_program_breakpoint_here_p (struct gdbarch *gdbarch,
+                                       gdbarch_program_breakpoint_here_p_ftype program_breakpoint_here_p)
+{
+  gdbarch->program_breakpoint_here_p = program_breakpoint_here_p;
+}
+
 int
 gdbarch_auxv_parse_p (struct gdbarch *gdbarch)
 {
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 9f32ac23ab..800a4e8b16 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 there's a program/permanent breakpoint planted in
+   memory at ADDRESS, return false otherwise. */
+
+typedef bool (gdbarch_program_breakpoint_here_p_ftype) (struct gdbarch *gdbarch, CORE_ADDR address);
+extern bool gdbarch_program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address);
+extern void set_gdbarch_program_breakpoint_here_p (struct gdbarch *gdbarch, gdbarch_program_breakpoint_here_p_ftype *program_breakpoint_here_p);
+
 /* 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 0be3e88bb2..66b54dd700 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 there's a program/permanent breakpoint planted in
+# memory at ADDRESS, return false otherwise.
+m;bool;program_breakpoint_here_p;CORE_ADDR address;address;;default_program_breakpoint_here_p;;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.
diff --git a/gdb/infrun.c b/gdb/infrun.c
index a8636284f1..494324d72b 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6157,8 +6157,8 @@ handle_signal_stop (struct execution_control_state *ecs)
      been removed.  */
   if (random_signal && target_stopped_by_sw_breakpoint ())
     {
-      if (program_breakpoint_here_p (gdbarch,
-				     ecs->event_thread->suspend.stop_pc))
+      if (gdbarch_program_breakpoint_here_p (gdbarch,
+					     ecs->event_thread->suspend.stop_pc))
 	{
 	  struct regcache *regcache;
 	  int decr_pc;
-- 
2.17.1

Re: [PATCH 1/2,v3] [AArch64] Recognize more program breakpoint patterns

[Alan Hayward]

I’m happy with this from AArch64 side.

Alan.

> On 15 Jan 2020, at 11:51, Luis Machado <luis.machado@linaro.org> wrote:
> 
> New in v3:
> 
> - Code cleanups based on reviews.
> 
> New in v2:
> 
> - Fixed misc problems based on reviews.
> - Switched to using gdbarch_program_breakpoint_here_p as opposed to
> gdbarch_insn_is_breakpoint.
> - Fixed matching of brk instructions. Previously the mask was incorrect, which
> was showing up as a few failures in the testsuite. Now it is clean.
> - New testcase (separate patch).
> - Moved program_breakpoint_here () to arch-utils.c and made it the default
> implementation of gdbarch_program_breakpoint_here_p.
> 
> --
> 
> 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_program_breakpoint_here_p)
> 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)
> 
> gdb/ChangeLog:
> 
> 2020-01-15  Luis Machado  <luis.machado@linaro.org>
> 
> 	* aarch64-tdep.c (BRK_INSN_MASK): Define to 0xffe0001f.
> 	(BRK_INSN_MASK): Define to 0xd4200000.
> 	(aarch64_program_breakpoint_here_p): New function.
> 	(aarch64_gdbarch_init): Set gdbarch_program_breakpoint_here_p hook.
> 	* arch-utils.c (default_program_breakpoint_here_p): Moved from
> 	breakpoint.c.
> 	* arch-utils.h (default_program_breakpoint_here_p): Moved from
> 	breakpoint.h
> 	* breakpoint.c (bp_loc_is_permanent): Changed return type to bool and
> 	call gdbarch_program_breakpoint_here_p.
> 	(program_breakpoint_here): Moved to arch-utils.c, renamed to
> 	default_program_breakpoint_here_p, changed return type to bool and
> 	simplified.
> 	* breakpoint.h (program_breakpoint_here): Moved prototype to
> 	arch-utils.h, renamed to default_program_breakpoint_here_p and changed
> 	return type to bool.
> 	* gdbarch.c: Regenerate.
> 	* gdbarch.h: Regenerate.
> 	* gdbarch.sh (program_breakpoint_here_p): New method.
> 	* infrun.c (handle_signal_stop): Call
> 	gdbarch_program_breakpoint_here_p.
> ---
> gdb/aarch64-tdep.c | 37 +++++++++++++++++++++++++++++++++++++
> gdb/arch-utils.c   | 32 ++++++++++++++++++++++++++++++++
> gdb/arch-utils.h   |  4 ++++
> gdb/breakpoint.c   | 46 +++++++---------------------------------------
> gdb/breakpoint.h   |  5 -----
> gdb/gdbarch.c      | 23 +++++++++++++++++++++++
> gdb/gdbarch.h      |  7 +++++++
> gdb/gdbarch.sh     |  4 ++++
> gdb/infrun.c       |  4 ++--
> 9 files changed, 116 insertions(+), 46 deletions(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 8451a916dc..7f32df3ec0 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -1201,6 +1201,39 @@ aarch64_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_byte op,
>   return false;
> }
> 
> +/* Used for matching BRK instructions for AArch64.  */
> +static constexpr uint32_t BRK_INSN_MASK = 0xffe0001f;
> +static constexpr uint32_t BRK_INSN_BASE = 0xd4200000;
> +
> +/* Implementation of gdbarch_program_breakpoint_here_p for aarch64.  */
> +
> +static bool
> +aarch64_program_breakpoint_here_p (gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  const uint32_t insn_len = 4;
> +  gdb_byte target_mem[4];
> +
> +  /* Enable the automatic memory restoration from breakpoints while
> +     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, insn_len) == 0)
> +    {
> +      uint32_t insn =
> +	(uint32_t) extract_unsigned_integer (target_mem, insn_len,
> +					     gdbarch_byte_order_for_code (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 ((insn & BRK_INSN_MASK) == BRK_INSN_BASE);
> +    }
> +
> +  return false;
> +}
> +
> /* 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 +3390,10 @@ 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_program_breakpoint_here_p (gdbarch,
> +					 aarch64_program_breakpoint_here_p);
> +
>   /* 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 92d7153ccb..04955ea847 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -876,6 +876,38 @@ int default_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr)
>   return 0;
> }
> 
> +/*  See arch-utils.h.  */
> +
> +bool
> +default_program_breakpoint_here_p (struct gdbarch *gdbarch,
> +				   CORE_ADDR address)
> +{
> +  int len;
> +  const gdb_byte *bpoint = gdbarch_breakpoint_from_pc (gdbarch, &address, &len);
> +
> +  /* Software breakpoints unsupported?  */
> +  if (bpoint == nullptr)
> +    return false;
> +
> +  gdb_byte *target_mem = (gdb_byte *) alloca (len);
> +
> +  /* Enable the automatic memory restoration from breakpoints while
> +     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)
> +    {
> +      /* Check if this is a breakpoint instruction for this architecture,
> +	 including ones used by GDB.  */
> +      if (memcmp (target_mem, bpoint, len) == 0)
> +	return true;
> +    }
> +
> +  return false;
> +}
> +
> void
> default_skip_permanent_breakpoint (struct regcache *regcache)
> {
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index 3fb9ad317a..43d64b1f4f 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -228,6 +228,10 @@ 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);
> 
> +/* Default implementation of gdbarch_program_breakpoint_here_p.  */
> +extern bool default_program_breakpoint_here_p (struct gdbarch *gdbarch,
> +					       CORE_ADDR addr);
> +
> /* Do-nothing version of vsyscall_range.  Returns false.  */
> 
> extern int default_vsyscall_range (struct gdbarch *gdbarch, struct mem_range *range);
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index a7a378131a..5a9352c26f 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -8515,7 +8515,7 @@ mention (struct breakpoint *b)
> }
> 
> 
> -static int bp_loc_is_permanent (struct bp_location *loc);
> +static bool bp_loc_is_permanent (struct bp_location *loc);
> 
> static struct bp_location *
> add_location_to_breakpoint (struct breakpoint *b,
> @@ -8581,42 +8581,10 @@ add_location_to_breakpoint (struct breakpoint *b,
> }
> 
> 
> -/* See breakpoint.h.  */
> -
> -int
> -program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address)
> -{
> -  int len;
> -  CORE_ADDR addr;
> -  const gdb_byte *bpoint;
> -  gdb_byte *target_mem;
> -
> -  addr = address;
> -  bpoint = gdbarch_breakpoint_from_pc (gdbarch, &addr, &len);
> -
> -  /* Software breakpoints unsupported?  */
> -  if (bpoint == NULL)
> -    return 0;
> -
> -  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.  */
> -  scoped_restore restore_memory
> -    = make_scoped_restore_show_memory_breakpoints (0);
> +/* Return true if LOC is pointing to a permanent breakpoint,
> +   return false otherwise.  */
> 
> -  if (target_read_memory (address, target_mem, len) == 0
> -      && memcmp (target_mem, bpoint, len) == 0)
> -    return 1;
> -
> -  return 0;
> -}
> -
> -/* Return 1 if LOC is pointing to a permanent breakpoint,
> -   return 0 otherwise.  */
> -
> -static int
> +static bool
> bp_loc_is_permanent (struct bp_location *loc)
> {
>   gdb_assert (loc != NULL);
> @@ -8624,14 +8592,14 @@ bp_loc_is_permanent (struct bp_location *loc)
>   /* If we have a non-breakpoint-backed catchpoint or a software
>      watchpoint, just return 0.  We should not attempt to read from
>      the addresses the locations of these breakpoint types point to.
> -     program_breakpoint_here_p, below, will attempt to read
> +     gdbarch_program_breakpoint_here_p, below, will attempt to read
>      memory.  */
>   if (!bl_address_is_meaningful (loc))
> -    return 0;
> +    return false;
> 
>   scoped_restore_current_pspace_and_thread restore_pspace_thread;
>   switch_to_program_space_and_thread (loc->pspace);
> -  return program_breakpoint_here_p (loc->gdbarch, loc->address);
> +  return gdbarch_program_breakpoint_here_p (loc->gdbarch, loc->address);
> }
> 
> /* Build a command list for the dprintf corresponding to the current
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 13d8102c17..347aeb75f3 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -1194,11 +1194,6 @@ enum breakpoint_here
> 
> /* Prototypes for breakpoint-related functions.  */
> 
> -/* Return 1 if there's a program/permanent breakpoint planted in
> -   memory at ADDRESS, return 0 otherwise.  */
> -
> -extern int program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address);
> -
> extern enum breakpoint_here breakpoint_here_p (const address_space *,
> 					       CORE_ADDR);
> 
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index cc8569f5c9..d763fc85e6 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_program_breakpoint_here_p_ftype *program_breakpoint_here_p;
>   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->program_breakpoint_here_p = default_program_breakpoint_here_p;
>   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 program_breakpoint_here_p, 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 */
> @@ -1248,6 +1251,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
>   fprintf_unfiltered (file,
>                       "gdbarch_dump: process_record_signal = <%s>\n",
>                       host_address_to_string (gdbarch->process_record_signal));
> +  fprintf_unfiltered (file,
> +                      "gdbarch_dump: program_breakpoint_here_p = <%s>\n",
> +                      host_address_to_string (gdbarch->program_breakpoint_here_p));
>   fprintf_unfiltered (file,
>                       "gdbarch_dump: ps_regnum = %s\n",
>                       plongest (gdbarch->ps_regnum));
> @@ -4928,6 +4934,23 @@ set_gdbarch_insn_is_jump (struct gdbarch *gdbarch,
>   gdbarch->insn_is_jump = insn_is_jump;
> }
> 
> +bool
> +gdbarch_program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  gdb_assert (gdbarch != NULL);
> +  gdb_assert (gdbarch->program_breakpoint_here_p != NULL);
> +  if (gdbarch_debug >= 2)
> +    fprintf_unfiltered (gdb_stdlog, "gdbarch_program_breakpoint_here_p called\n");
> +  return gdbarch->program_breakpoint_here_p (gdbarch, address);
> +}
> +
> +void
> +set_gdbarch_program_breakpoint_here_p (struct gdbarch *gdbarch,
> +                                       gdbarch_program_breakpoint_here_p_ftype program_breakpoint_here_p)
> +{
> +  gdbarch->program_breakpoint_here_p = program_breakpoint_here_p;
> +}
> +
> int
> gdbarch_auxv_parse_p (struct gdbarch *gdbarch)
> {
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 9f32ac23ab..800a4e8b16 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 there's a program/permanent breakpoint planted in
> +   memory at ADDRESS, return false otherwise. */
> +
> +typedef bool (gdbarch_program_breakpoint_here_p_ftype) (struct gdbarch *gdbarch, CORE_ADDR address);
> +extern bool gdbarch_program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address);
> +extern void set_gdbarch_program_breakpoint_here_p (struct gdbarch *gdbarch, gdbarch_program_breakpoint_here_p_ftype *program_breakpoint_here_p);
> +
> /* 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 0be3e88bb2..66b54dd700 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 there's a program/permanent breakpoint planted in
> +# memory at ADDRESS, return false otherwise.
> +m;bool;program_breakpoint_here_p;CORE_ADDR address;address;;default_program_breakpoint_here_p;;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.
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index a8636284f1..494324d72b 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -6157,8 +6157,8 @@ handle_signal_stop (struct execution_control_state *ecs)
>      been removed.  */
>   if (random_signal && target_stopped_by_sw_breakpoint ())
>     {
> -      if (program_breakpoint_here_p (gdbarch,
> -				     ecs->event_thread->suspend.stop_pc))
> +      if (gdbarch_program_breakpoint_here_p (gdbarch,
> +					     ecs->event_thread->suspend.stop_pc))
> 	{
> 	  struct regcache *regcache;
> 	  int decr_pc;
> -- 
> 2.17.1
> 

Re: [PATCH 1/2,v3] [AArch64] Recognize more program breakpoint patterns

[Luis Machado]

Thanks Alan.

Any other feedback?

On 1/21/20 8:32 AM, Alan Hayward wrote:
> I’m happy with this from AArch64 side.
> 
> Alan.
> 
>> On 15 Jan 2020, at 11:51, Luis Machado <luis.machado@linaro.org> wrote:
>>
>> New in v3:
>>
>> - Code cleanups based on reviews.
>>
>> New in v2:
>>
>> - Fixed misc problems based on reviews.
>> - Switched to using gdbarch_program_breakpoint_here_p as opposed to
>> gdbarch_insn_is_breakpoint.
>> - Fixed matching of brk instructions. Previously the mask was incorrect, which
>> was showing up as a few failures in the testsuite. Now it is clean.
>> - New testcase (separate patch).
>> - Moved program_breakpoint_here () to arch-utils.c and made it the default
>> implementation of gdbarch_program_breakpoint_here_p.
>>
>> --
>>
>> 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_program_breakpoint_here_p)
>> 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)
>>
>> gdb/ChangeLog:
>>
>> 2020-01-15  Luis Machado  <luis.machado@linaro.org>
>>
>> 	* aarch64-tdep.c (BRK_INSN_MASK): Define to 0xffe0001f.
>> 	(BRK_INSN_MASK): Define to 0xd4200000.
>> 	(aarch64_program_breakpoint_here_p): New function.
>> 	(aarch64_gdbarch_init): Set gdbarch_program_breakpoint_here_p hook.
>> 	* arch-utils.c (default_program_breakpoint_here_p): Moved from
>> 	breakpoint.c.
>> 	* arch-utils.h (default_program_breakpoint_here_p): Moved from
>> 	breakpoint.h
>> 	* breakpoint.c (bp_loc_is_permanent): Changed return type to bool and
>> 	call gdbarch_program_breakpoint_here_p.
>> 	(program_breakpoint_here): Moved to arch-utils.c, renamed to
>> 	default_program_breakpoint_here_p, changed return type to bool and
>> 	simplified.
>> 	* breakpoint.h (program_breakpoint_here): Moved prototype to
>> 	arch-utils.h, renamed to default_program_breakpoint_here_p and changed
>> 	return type to bool.
>> 	* gdbarch.c: Regenerate.
>> 	* gdbarch.h: Regenerate.
>> 	* gdbarch.sh (program_breakpoint_here_p): New method.
>> 	* infrun.c (handle_signal_stop): Call
>> 	gdbarch_program_breakpoint_here_p.
>> ---
>> gdb/aarch64-tdep.c | 37 +++++++++++++++++++++++++++++++++++++
>> gdb/arch-utils.c   | 32 ++++++++++++++++++++++++++++++++
>> gdb/arch-utils.h   |  4 ++++
>> gdb/breakpoint.c   | 46 +++++++---------------------------------------
>> gdb/breakpoint.h   |  5 -----
>> gdb/gdbarch.c      | 23 +++++++++++++++++++++++
>> gdb/gdbarch.h      |  7 +++++++
>> gdb/gdbarch.sh     |  4 ++++
>> gdb/infrun.c       |  4 ++--
>> 9 files changed, 116 insertions(+), 46 deletions(-)
>>
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 8451a916dc..7f32df3ec0 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -1201,6 +1201,39 @@ aarch64_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_byte op,
>>    return false;
>> }
>>
>> +/* Used for matching BRK instructions for AArch64.  */
>> +static constexpr uint32_t BRK_INSN_MASK = 0xffe0001f;
>> +static constexpr uint32_t BRK_INSN_BASE = 0xd4200000;
>> +
>> +/* Implementation of gdbarch_program_breakpoint_here_p for aarch64.  */
>> +
>> +static bool
>> +aarch64_program_breakpoint_here_p (gdbarch *gdbarch, CORE_ADDR address)
>> +{
>> +  const uint32_t insn_len = 4;
>> +  gdb_byte target_mem[4];
>> +
>> +  /* Enable the automatic memory restoration from breakpoints while
>> +     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, insn_len) == 0)
>> +    {
>> +      uint32_t insn =
>> +	(uint32_t) extract_unsigned_integer (target_mem, insn_len,
>> +					     gdbarch_byte_order_for_code (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 ((insn & BRK_INSN_MASK) == BRK_INSN_BASE);
>> +    }
>> +
>> +  return false;
>> +}
>> +
>> /* 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 +3390,10 @@ 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_program_breakpoint_here_p (gdbarch,
>> +					 aarch64_program_breakpoint_here_p);
>> +
>>    /* 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 92d7153ccb..04955ea847 100644
>> --- a/gdb/arch-utils.c
>> +++ b/gdb/arch-utils.c
>> @@ -876,6 +876,38 @@ int default_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr)
>>    return 0;
>> }
>>
>> +/*  See arch-utils.h.  */
>> +
>> +bool
>> +default_program_breakpoint_here_p (struct gdbarch *gdbarch,
>> +				   CORE_ADDR address)
>> +{
>> +  int len;
>> +  const gdb_byte *bpoint = gdbarch_breakpoint_from_pc (gdbarch, &address, &len);
>> +
>> +  /* Software breakpoints unsupported?  */
>> +  if (bpoint == nullptr)
>> +    return false;
>> +
>> +  gdb_byte *target_mem = (gdb_byte *) alloca (len);
>> +
>> +  /* Enable the automatic memory restoration from breakpoints while
>> +     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)
>> +    {
>> +      /* Check if this is a breakpoint instruction for this architecture,
>> +	 including ones used by GDB.  */
>> +      if (memcmp (target_mem, bpoint, len) == 0)
>> +	return true;
>> +    }
>> +
>> +  return false;
>> +}
>> +
>> void
>> default_skip_permanent_breakpoint (struct regcache *regcache)
>> {
>> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
>> index 3fb9ad317a..43d64b1f4f 100644
>> --- a/gdb/arch-utils.h
>> +++ b/gdb/arch-utils.h
>> @@ -228,6 +228,10 @@ 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);
>>
>> +/* Default implementation of gdbarch_program_breakpoint_here_p.  */
>> +extern bool default_program_breakpoint_here_p (struct gdbarch *gdbarch,
>> +					       CORE_ADDR addr);
>> +
>> /* Do-nothing version of vsyscall_range.  Returns false.  */
>>
>> extern int default_vsyscall_range (struct gdbarch *gdbarch, struct mem_range *range);
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index a7a378131a..5a9352c26f 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -8515,7 +8515,7 @@ mention (struct breakpoint *b)
>> }
>>
>>
>> -static int bp_loc_is_permanent (struct bp_location *loc);
>> +static bool bp_loc_is_permanent (struct bp_location *loc);
>>
>> static struct bp_location *
>> add_location_to_breakpoint (struct breakpoint *b,
>> @@ -8581,42 +8581,10 @@ add_location_to_breakpoint (struct breakpoint *b,
>> }
>>
>>
>> -/* See breakpoint.h.  */
>> -
>> -int
>> -program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address)
>> -{
>> -  int len;
>> -  CORE_ADDR addr;
>> -  const gdb_byte *bpoint;
>> -  gdb_byte *target_mem;
>> -
>> -  addr = address;
>> -  bpoint = gdbarch_breakpoint_from_pc (gdbarch, &addr, &len);
>> -
>> -  /* Software breakpoints unsupported?  */
>> -  if (bpoint == NULL)
>> -    return 0;
>> -
>> -  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.  */
>> -  scoped_restore restore_memory
>> -    = make_scoped_restore_show_memory_breakpoints (0);
>> +/* Return true if LOC is pointing to a permanent breakpoint,
>> +   return false otherwise.  */
>>
>> -  if (target_read_memory (address, target_mem, len) == 0
>> -      && memcmp (target_mem, bpoint, len) == 0)
>> -    return 1;
>> -
>> -  return 0;
>> -}
>> -
>> -/* Return 1 if LOC is pointing to a permanent breakpoint,
>> -   return 0 otherwise.  */
>> -
>> -static int
>> +static bool
>> bp_loc_is_permanent (struct bp_location *loc)
>> {
>>    gdb_assert (loc != NULL);
>> @@ -8624,14 +8592,14 @@ bp_loc_is_permanent (struct bp_location *loc)
>>    /* If we have a non-breakpoint-backed catchpoint or a software
>>       watchpoint, just return 0.  We should not attempt to read from
>>       the addresses the locations of these breakpoint types point to.
>> -     program_breakpoint_here_p, below, will attempt to read
>> +     gdbarch_program_breakpoint_here_p, below, will attempt to read
>>       memory.  */
>>    if (!bl_address_is_meaningful (loc))
>> -    return 0;
>> +    return false;
>>
>>    scoped_restore_current_pspace_and_thread restore_pspace_thread;
>>    switch_to_program_space_and_thread (loc->pspace);
>> -  return program_breakpoint_here_p (loc->gdbarch, loc->address);
>> +  return gdbarch_program_breakpoint_here_p (loc->gdbarch, loc->address);
>> }
>>
>> /* Build a command list for the dprintf corresponding to the current
>> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
>> index 13d8102c17..347aeb75f3 100644
>> --- a/gdb/breakpoint.h
>> +++ b/gdb/breakpoint.h
>> @@ -1194,11 +1194,6 @@ enum breakpoint_here
>>
>> /* Prototypes for breakpoint-related functions.  */
>>
>> -/* Return 1 if there's a program/permanent breakpoint planted in
>> -   memory at ADDRESS, return 0 otherwise.  */
>> -
>> -extern int program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address);
>> -
>> extern enum breakpoint_here breakpoint_here_p (const address_space *,
>> 					       CORE_ADDR);
>>
>> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
>> index cc8569f5c9..d763fc85e6 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_program_breakpoint_here_p_ftype *program_breakpoint_here_p;
>>    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->program_breakpoint_here_p = default_program_breakpoint_here_p;
>>    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 program_breakpoint_here_p, 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 */
>> @@ -1248,6 +1251,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
>>    fprintf_unfiltered (file,
>>                        "gdbarch_dump: process_record_signal = <%s>\n",
>>                        host_address_to_string (gdbarch->process_record_signal));
>> +  fprintf_unfiltered (file,
>> +                      "gdbarch_dump: program_breakpoint_here_p = <%s>\n",
>> +                      host_address_to_string (gdbarch->program_breakpoint_here_p));
>>    fprintf_unfiltered (file,
>>                        "gdbarch_dump: ps_regnum = %s\n",
>>                        plongest (gdbarch->ps_regnum));
>> @@ -4928,6 +4934,23 @@ set_gdbarch_insn_is_jump (struct gdbarch *gdbarch,
>>    gdbarch->insn_is_jump = insn_is_jump;
>> }
>>
>> +bool
>> +gdbarch_program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address)
>> +{
>> +  gdb_assert (gdbarch != NULL);
>> +  gdb_assert (gdbarch->program_breakpoint_here_p != NULL);
>> +  if (gdbarch_debug >= 2)
>> +    fprintf_unfiltered (gdb_stdlog, "gdbarch_program_breakpoint_here_p called\n");
>> +  return gdbarch->program_breakpoint_here_p (gdbarch, address);
>> +}
>> +
>> +void
>> +set_gdbarch_program_breakpoint_here_p (struct gdbarch *gdbarch,
>> +                                       gdbarch_program_breakpoint_here_p_ftype program_breakpoint_here_p)
>> +{
>> +  gdbarch->program_breakpoint_here_p = program_breakpoint_here_p;
>> +}
>> +
>> int
>> gdbarch_auxv_parse_p (struct gdbarch *gdbarch)
>> {
>> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
>> index 9f32ac23ab..800a4e8b16 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 there's a program/permanent breakpoint planted in
>> +   memory at ADDRESS, return false otherwise. */
>> +
>> +typedef bool (gdbarch_program_breakpoint_here_p_ftype) (struct gdbarch *gdbarch, CORE_ADDR address);
>> +extern bool gdbarch_program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address);
>> +extern void set_gdbarch_program_breakpoint_here_p (struct gdbarch *gdbarch, gdbarch_program_breakpoint_here_p_ftype *program_breakpoint_here_p);
>> +
>> /* 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 0be3e88bb2..66b54dd700 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 there's a program/permanent breakpoint planted in
>> +# memory at ADDRESS, return false otherwise.
>> +m;bool;program_breakpoint_here_p;CORE_ADDR address;address;;default_program_breakpoint_here_p;;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.
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index a8636284f1..494324d72b 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -6157,8 +6157,8 @@ handle_signal_stop (struct execution_control_state *ecs)
>>       been removed.  */
>>    if (random_signal && target_stopped_by_sw_breakpoint ())
>>      {
>> -      if (program_breakpoint_here_p (gdbarch,
>> -				     ecs->event_thread->suspend.stop_pc))
>> +      if (gdbarch_program_breakpoint_here_p (gdbarch,
>> +					     ecs->event_thread->suspend.stop_pc))
>> 	{
>> 	  struct regcache *regcache;
>> 	  int decr_pc;
>> -- 
>> 2.17.1
>>
> 

Re: [PATCH 1/2,v3] [AArch64] Recognize more program breakpoint patterns

[Simon Marchi]

On 2020-01-28 8:49 p.m., Luis Machado wrote:
> Thanks Alan.
> 
> Any other feedback?

This patch LGTM, thanks.

Simon

[PATCH 2/2,v3] [AArch64] Test handling of additional brk instruction patterns

[Luis Machado]

New in v3:

- Minor formatting and code cleanups.
- Added count check to validate number of brk SIGTRAP's.
- Moved count to SIGTRAP check conditional block.

This test exercises the previous patch's code and makes sure GDB can
properly get a SIGTRAP from various brk instruction patterns.

GDB needs to be able to see the program exiting normally. If GDB doesn't
support the additional brk instructions, we will see timeouts.

We bail out with the first timeout since we won't be able to step through
the program breakpoint anyway, so it is no use carrying on.

gdb/testsuite/ChangeLog:

2020-01-15  Luis Machado  <luis.machado@linaro.org>

	* gdb.arch/aarch64-brk-patterns.c: New source file.
	* gdb.arch/aarch64-brk-patterns.exp: New test.
---
 gdb/testsuite/gdb.arch/aarch64-brk-patterns.c | 30 ++++++++
 .../gdb.arch/aarch64-brk-patterns.exp         | 74 +++++++++++++++++++
 2 files changed, 104 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp

diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
new file mode 100644
index 0000000000..ccf9a35a94
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
@@ -0,0 +1,30 @@
+/* This file is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int main(void)
+{
+  /* Dummy instruction just so GDB doesn't stop at the first breakpoint
+     instruction.  */
+  __asm __volatile ("nop\n\t");
+
+  /* Multiple BRK instruction patterns.  */
+  __asm __volatile ("brk %0\n\t" ::"n"(0x0));
+  __asm __volatile ("brk %0\n\t" ::"n"(0x900 + 0xf));
+  __asm __volatile ("brk %0\n\t" ::"n"(0xf000));
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
new file mode 100644
index 0000000000..9a0ec81efa
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
@@ -0,0 +1,74 @@
+# Copyright 2020 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# This file is part of the gdb testsuite.
+
+# Test if GDB stops at various BRK instruction patterns inserted into
+# the code.
+
+if {![is_aarch64_target]} {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile
+if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
+    return -1
+}
+
+if {![runto_main]} {
+    untested "could not run to main"
+    return -1
+}
+
+# Number of expected SIGTRAP's to get.  This needs to be kept in sync
+# with the source file.
+set expected_traps 3
+set keep_going 1
+set count 0
+set old_timeout $timeout
+set timeout 10
+
+while {$keep_going} {
+
+  set test "brk instruction $count causes SIGTRAP"
+
+  # Continue to next program breakpoint instruction.
+  gdb_test_multiple "continue" $test {
+      -re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
+	  pass $test
+
+	  # Insert a breakpoint at the program breakpoint instruction so GDB
+	  # can step over it.
+	  gdb_test "break" \
+	    "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \
+	    "insert breakpoint at brk instruction $count"
+	  incr count
+      }
+      -re "exited normally.*$gdb_prompt $" {
+	  set keep_going 0
+      }
+      timeout {
+	  fail $test
+	  set keep_going 0
+      }
+  }
+}
+
+set timeout $old_timeout
+
+if {$count < $expected_traps} {
+  fail "all brk instructions triggered."
+}
-- 
2.17.1

Re: [PATCH 2/2,v3] [AArch64] Test handling of additional brk instruction patterns

[Alan Hayward]

Minor nit below. Happy for it to be pushed with that one change.

Alan.

> On 15 Jan 2020, at 11:51, Luis Machado <luis.machado@linaro.org> wrote:
> 
> New in v3:
> 
> - Minor formatting and code cleanups.
> - Added count check to validate number of brk SIGTRAP's.
> - Moved count to SIGTRAP check conditional block.
> 
> This test exercises the previous patch's code and makes sure GDB can
> properly get a SIGTRAP from various brk instruction patterns.
> 
> GDB needs to be able to see the program exiting normally. If GDB doesn't
> support the additional brk instructions, we will see timeouts.
> 
> We bail out with the first timeout since we won't be able to step through
> the program breakpoint anyway, so it is no use carrying on.
> 
> gdb/testsuite/ChangeLog:
> 
> 2020-01-15  Luis Machado  <luis.machado@linaro.org>
> 
> 	* gdb.arch/aarch64-brk-patterns.c: New source file.
> 	* gdb.arch/aarch64-brk-patterns.exp: New test.
> ---
> gdb/testsuite/gdb.arch/aarch64-brk-patterns.c | 30 ++++++++
> .../gdb.arch/aarch64-brk-patterns.exp         | 74 +++++++++++++++++++
> 2 files changed, 104 insertions(+)
> create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
> create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
> 
> diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
> new file mode 100644
> index 0000000000..ccf9a35a94
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
> @@ -0,0 +1,30 @@
> +/* This file is part of GDB, the GNU debugger.
> +
> +   Copyright 2020 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +int main(void)

New line between int and main.

> +{
> +  /* Dummy instruction just so GDB doesn't stop at the first breakpoint
> +     instruction.  */
> +  __asm __volatile ("nop\n\t");
> +
> +  /* Multiple BRK instruction patterns.  */
> +  __asm __volatile ("brk %0\n\t" ::"n"(0x0));
> +  __asm __volatile ("brk %0\n\t" ::"n"(0x900 + 0xf));
> +  __asm __volatile ("brk %0\n\t" ::"n"(0xf000));
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
> new file mode 100644
> index 0000000000..9a0ec81efa
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
> @@ -0,0 +1,74 @@
> +# Copyright 2020 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +# This file is part of the gdb testsuite.
> +
> +# Test if GDB stops at various BRK instruction patterns inserted into
> +# the code.
> +
> +if {![is_aarch64_target]} {
> +    verbose "Skipping ${gdb_test_file_name}."
> +    return
> +}
> +
> +standard_testfile
> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
> +    return -1
> +}
> +
> +if {![runto_main]} {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +# Number of expected SIGTRAP's to get.  This needs to be kept in sync
> +# with the source file.
> +set expected_traps 3
> +set keep_going 1
> +set count 0
> +set old_timeout $timeout
> +set timeout 10
> +
> +while {$keep_going} {
> +
> +  set test "brk instruction $count causes SIGTRAP"
> +
> +  # Continue to next program breakpoint instruction.
> +  gdb_test_multiple "continue" $test {
> +      -re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
> +	  pass $test
> +
> +	  # Insert a breakpoint at the program breakpoint instruction so GDB
> +	  # can step over it.
> +	  gdb_test "break" \
> +	    "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \
> +	    "insert breakpoint at brk instruction $count"
> +	  incr count
> +      }
> +      -re "exited normally.*$gdb_prompt $" {
> +	  set keep_going 0
> +      }
> +      timeout {
> +	  fail $test
> +	  set keep_going 0
> +      }
> +  }
> +}
> +
> +set timeout $old_timeout
> +
> +if {$count < $expected_traps} {
> +  fail "all brk instructions triggered."
> +}
> -- 
> 2.17.1
> 

Re: [PATCH 2/2,v3] [AArch64] Test handling of additional brk instruction patterns

[Simon Marchi]

On 2020-01-15 6:51 a.m., Luis Machado wrote:
> New in v3:
> 
> - Minor formatting and code cleanups.
> - Added count check to validate number of brk SIGTRAP's.
> - Moved count to SIGTRAP check conditional block.
> 
> This test exercises the previous patch's code and makes sure GDB can
> properly get a SIGTRAP from various brk instruction patterns.
> 
> GDB needs to be able to see the program exiting normally. If GDB doesn't
> support the additional brk instructions, we will see timeouts.
> 
> We bail out with the first timeout since we won't be able to step through
> the program breakpoint anyway, so it is no use carrying on.
> 
> gdb/testsuite/ChangeLog:
> 
> 2020-01-15  Luis Machado  <luis.machado@linaro.org>
> 
> 	* gdb.arch/aarch64-brk-patterns.c: New source file.
> 	* gdb.arch/aarch64-brk-patterns.exp: New test.
> ---
>  gdb/testsuite/gdb.arch/aarch64-brk-patterns.c | 30 ++++++++
>  .../gdb.arch/aarch64-brk-patterns.exp         | 74 +++++++++++++++++++
>  2 files changed, 104 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
>  create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
> 
> diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
> new file mode 100644
> index 0000000000..ccf9a35a94
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
> @@ -0,0 +1,30 @@
> +/* This file is part of GDB, the GNU debugger.
> +
> +   Copyright 2020 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +int main(void)
> +{
> +  /* Dummy instruction just so GDB doesn't stop at the first breakpoint
> +     instruction.  */
> +  __asm __volatile ("nop\n\t");
> +
> +  /* Multiple BRK instruction patterns.  */
> +  __asm __volatile ("brk %0\n\t" ::"n"(0x0));
> +  __asm __volatile ("brk %0\n\t" ::"n"(0x900 + 0xf));
> +  __asm __volatile ("brk %0\n\t" ::"n"(0xf000));
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
> new file mode 100644
> index 0000000000..9a0ec81efa
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
> @@ -0,0 +1,74 @@
> +# Copyright 2020 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +# This file is part of the gdb testsuite.
> +
> +# Test if GDB stops at various BRK instruction patterns inserted into
> +# the code.
> +
> +if {![is_aarch64_target]} {
> +    verbose "Skipping ${gdb_test_file_name}."
> +    return
> +}
> +
> +standard_testfile
> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
> +    return -1
> +}
> +
> +if {![runto_main]} {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +# Number of expected SIGTRAP's to get.  This needs to be kept in sync
> +# with the source file.
> +set expected_traps 3
> +set keep_going 1
> +set count 0
> +set old_timeout $timeout
> +set timeout 10

Any reason you are changing the timeout?  There is nothing in the test that
looks like it would take time.

If changing the timeout is really necessary, look into using with_timeout_factor.

> +
> +while {$keep_going} {
> +
> +  set test "brk instruction $count causes SIGTRAP"

Instead of setting the test name like that, look into using the special $gdb_test_name
variable, available inside the gdb_test_multiple body.

> +
> +  # Continue to next program breakpoint instruction.
> +  gdb_test_multiple "continue" $test {
> +      -re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
> +	  pass $test
> +
> +	  # Insert a breakpoint at the program breakpoint instruction so GDB
> +	  # can step over it.
> +	  gdb_test "break" \
> +	    "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \
> +	    "insert breakpoint at brk instruction $count"
> +	  incr count
> +      }
> +      -re "exited normally.*$gdb_prompt $" {
> +	  set keep_going 0
> +      }
> +      timeout {
> +	  fail $test
> +	  set keep_going 0
> +      }
> +  }
> +}
> +
> +set timeout $old_timeout
> +
> +if {$count < $expected_traps} {
> +  fail "all brk instructions triggered."
> +}

Use gdb_assert.

Is there any reason why $count would be greater than $expected_taps?  If no,
I would test for "$count == $expected_traps".

Simon

Re: [PATCH 2/2,v3] [AArch64] Test handling of additional brk instruction patterns

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 6410 bytes --]

On 1/29/20 12:18 AM, Simon Marchi wrote:
> On 2020-01-15 6:51 a.m., Luis Machado wrote:
>> New in v3:
>>
>> - Minor formatting and code cleanups.
>> - Added count check to validate number of brk SIGTRAP's.
>> - Moved count to SIGTRAP check conditional block.
>>
>> This test exercises the previous patch's code and makes sure GDB can
>> properly get a SIGTRAP from various brk instruction patterns.
>>
>> GDB needs to be able to see the program exiting normally. If GDB doesn't
>> support the additional brk instructions, we will see timeouts.
>>
>> We bail out with the first timeout since we won't be able to step through
>> the program breakpoint anyway, so it is no use carrying on.
>>
>> gdb/testsuite/ChangeLog:
>>
>> 2020-01-15  Luis Machado  <luis.machado@linaro.org>
>>
>> 	* gdb.arch/aarch64-brk-patterns.c: New source file.
>> 	* gdb.arch/aarch64-brk-patterns.exp: New test.
>> ---
>>   gdb/testsuite/gdb.arch/aarch64-brk-patterns.c | 30 ++++++++
>>   .../gdb.arch/aarch64-brk-patterns.exp         | 74 +++++++++++++++++++
>>   2 files changed, 104 insertions(+)
>>   create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
>>   create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
>>
>> diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
>> new file mode 100644
>> index 0000000000..ccf9a35a94
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
>> @@ -0,0 +1,30 @@
>> +/* This file is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2020 Free Software Foundation, Inc.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +int main(void)
>> +{
>> +  /* Dummy instruction just so GDB doesn't stop at the first breakpoint
>> +     instruction.  */
>> +  __asm __volatile ("nop\n\t");
>> +
>> +  /* Multiple BRK instruction patterns.  */
>> +  __asm __volatile ("brk %0\n\t" ::"n"(0x0));
>> +  __asm __volatile ("brk %0\n\t" ::"n"(0x900 + 0xf));
>> +  __asm __volatile ("brk %0\n\t" ::"n"(0xf000));
>> +
>> +  return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
>> new file mode 100644
>> index 0000000000..9a0ec81efa
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
>> @@ -0,0 +1,74 @@
>> +# Copyright 2020 Free Software Foundation, Inc.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +# This file is part of the gdb testsuite.
>> +
>> +# Test if GDB stops at various BRK instruction patterns inserted into
>> +# the code.
>> +
>> +if {![is_aarch64_target]} {
>> +    verbose "Skipping ${gdb_test_file_name}."
>> +    return
>> +}
>> +
>> +standard_testfile
>> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
>> +    return -1
>> +}
>> +
>> +if {![runto_main]} {
>> +    untested "could not run to main"
>> +    return -1
>> +}
>> +
>> +# Number of expected SIGTRAP's to get.  This needs to be kept in sync
>> +# with the source file.
>> +set expected_traps 3
>> +set keep_going 1
>> +set count 0
>> +set old_timeout $timeout
>> +set timeout 10
> 
> Any reason you are changing the timeout?  There is nothing in the test that
> looks like it would take time.
> 

If GDB doesn't support one of these instructions, it will be caught in 
an infinite loop. The reduced timeout will prevent a long wait time 
until we bail out.

> If changing the timeout is really necessary, look into using with_timeout_factor.
> 

That would raise the timeout even further. We want a reduced one.

It would be nice if we could reduce the timeout with 
with_timeout_factor. I gave it a try but it didn't work.

I think we need adjustments to make it work with a floating point 
number. I'll look into it.

>> +
>> +while {$keep_going} {
>> +
>> +  set test "brk instruction $count causes SIGTRAP"
> 
> Instead of setting the test name like that, look into using the special $gdb_test_name
> variable, available inside the gdb_test_multiple body.
> 

Indeed. I'll tweak this to match the other reviews.

>> +
>> +  # Continue to next program breakpoint instruction.
>> +  gdb_test_multiple "continue" $test {
>> +      -re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
>> +	  pass $test
>> +
>> +	  # Insert a breakpoint at the program breakpoint instruction so GDB
>> +	  # can step over it.
>> +	  gdb_test "break" \
>> +	    "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \
>> +	    "insert breakpoint at brk instruction $count"
>> +	  incr count
>> +      }
>> +      -re "exited normally.*$gdb_prompt $" {
>> +	  set keep_going 0
>> +      }
>> +      timeout {
>> +	  fail $test
>> +	  set keep_going 0
>> +      }
>> +  }
>> +}
>> +
>> +set timeout $old_timeout
>> +
>> +if {$count < $expected_traps} {
>> +  fail "all brk instructions triggered."
>> +}
> 
> Use gdb_assert.
> 

Ditto.

> Is there any reason why $count would be greater than $expected_taps?  If no,
> I would test for "$count == $expected_traps".

Not really. We'd want the exact match. Fixed now.

Thanks for the review!

How does the updated attached patch look?

[-- Attachment #2: 0001-AArch64-Test-handling-of-additional-brk-instruction-.patch --]
[-- Type: text/x-patch, Size: 5449 bytes --]

From 67daaf5b752d6a168452f2d77b50410dd0a78a4f Mon Sep 17 00:00:00 2001
From: Luis Machado <luis.machado@linaro.org>
Date: Mon, 13 Jan 2020 12:31:01 -0300
Subject: [PATCH] [AArch64] Test handling of additional brk instruction
 patterns

New in v5:

- Use gdb_test_name for gdb_test_multiple.
- Use gdb_assert.
- Verify count matches the expected sigtraps exactly.

New in v4:

- Fix formatting nit in gdb/testsuite/gdb.arch/aarch64-brk-patterns.c.

New in v3:

- Minor formatting and code cleanups.
- Added count check to validate number of brk SIGTRAP's.
- Moved count to SIGTRAP check conditional block.

This test exercises the previous patch's code and makes sure GDB can
properly get a SIGTRAP from various brk instruction patterns.

GDB needs to be able to see the program exiting normally. If GDB doesn't
support the additional brk instructions, we will see timeouts.

We bail out with the first timeout since we won't be able to step through
the program breakpoint anyway, so it is no use carrying on.

gdb/testsuite/ChangeLog:

2020-01-29  Luis Machado  <luis.machado@linaro.org>

	* gdb.arch/aarch64-brk-patterns.c: New source file.
	* gdb.arch/aarch64-brk-patterns.exp: New test.
---
 gdb/testsuite/gdb.arch/aarch64-brk-patterns.c | 31 ++++++++
 .../gdb.arch/aarch64-brk-patterns.exp         | 73 +++++++++++++++++++
 2 files changed, 104 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp

diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
new file mode 100644
index 0000000000..920ba8e2cb
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
@@ -0,0 +1,31 @@
+/* This file is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main (void)
+{
+  /* Dummy instruction just so GDB doesn't stop at the first breakpoint
+     instruction.  */
+  __asm __volatile ("nop\n\t");
+
+  /* Multiple BRK instruction patterns.  */
+  __asm __volatile ("brk %0\n\t" ::"n"(0x0));
+  __asm __volatile ("brk %0\n\t" ::"n"(0x900 + 0xf));
+  __asm __volatile ("brk %0\n\t" ::"n"(0xf000));
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
new file mode 100644
index 0000000000..dc95e3f6f5
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
@@ -0,0 +1,73 @@
+# Copyright 2020 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# This file is part of the gdb testsuite.
+
+# Test if GDB stops at various BRK instruction patterns inserted into
+# the code.
+
+if {![is_aarch64_target]} {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile
+if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
+    return -1
+}
+
+if {![runto_main]} {
+    untested "could not run to main"
+    return -1
+}
+
+# Number of expected SIGTRAP's to get.  This needs to be kept in sync
+# with the source file.
+set expected_traps 3
+set keep_going 1
+set count 0
+set oldtimeout $timeout
+set timeout 10
+
+# Make sure we have a lower timeout in case GDB doesn't support a particular
+# instruction.  Such instruction will cause GDB to loop infinitely.
+while {$keep_going} {
+    # Continue to next program breakpoint instruction.
+    gdb_test_multiple "continue" "brk instruction $count causes SIGTRAP" {
+	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
+	    pass $gdb_test_name
+
+	    # Insert a breakpoint at the program breakpoint instruction so
+	    # GDB can step over it.
+	    gdb_test "break" \
+		"Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \
+		"insert breakpoint at brk instruction $count"
+	    incr count
+	}
+	# We've reached the end of the test.
+	-re "exited normally.*$gdb_prompt $" {
+	    set keep_going 0
+	}
+	timeout {
+	    fail $gdb_test_name
+	    set keep_going 0
+	}
+    }
+}
+
+set timeout $oldtimeout
+
+# Verify we stopped at the expected number of SIGTRAP's.
+gdb_assert {$count == $expected_traps} "all brk instructions triggered"
-- 
2.17.1

Re: [PATCH 2/2,v3] [AArch64] Test handling of additional brk instruction patterns

[Simon Marchi]

On 2020-01-29 6:30 a.m., Luis Machado wrote:
>>> +# Number of expected SIGTRAP's to get.  This needs to be kept in sync
>>> +# with the source file.
>>> +set expected_traps 3
>>> +set keep_going 1
>>> +set count 0
>>> +set old_timeout $timeout
>>> +set timeout 10
>>
>> Any reason you are changing the timeout?  There is nothing in the test that
>> looks like it would take time.
>>
> 
> If GDB doesn't support one of these instructions, it will be caught in 
> an infinite loop. The reduced timeout will prevent a long wait time 
> until we bail out.

Ok.  The worry I have with it is that if the target board has raised the timeout
on purpose, because it's testing with a slow target/link/emulator, then this will
cancel it.  In this case I'd just leave the timeout as it is.  Normally, GDB won't
be broken for this test case, so it won't matter.

Moreover, the timeout here on my x86 machine is 10 seconds by default.  I just checked
on an AArch64 box on the compile farm, it's 10 there too.  So in which case was it
useful to set it to 10?

>> If changing the timeout is really necessary, look into using with_timeout_factor.
>>
> 
> That would raise the timeout even further. We want a reduced one.
> 
> It would be nice if we could reduce the timeout with 
> with_timeout_factor. I gave it a try but it didn't work.
> 
> I think we need adjustments to make it work with a floating point 
> number. I'll look into it.

I'm not sure that's desirable for a test case to ever reduce the timeout, for the reason
explained above.

Simon

Re: [PATCH 2/2,v3] [AArch64] Test handling of additional brk instruction patterns

[Luis Machado]

On 1/29/20 11:01 AM, Simon Marchi wrote:
> On 2020-01-29 6:30 a.m., Luis Machado wrote:
>>>> +# Number of expected SIGTRAP's to get.  This needs to be kept in sync
>>>> +# with the source file.
>>>> +set expected_traps 3
>>>> +set keep_going 1
>>>> +set count 0
>>>> +set old_timeout $timeout
>>>> +set timeout 10
>>>
>>> Any reason you are changing the timeout?  There is nothing in the test that
>>> looks like it would take time.
>>>
>>
>> If GDB doesn't support one of these instructions, it will be caught in
>> an infinite loop. The reduced timeout will prevent a long wait time
>> until we bail out.
> 
> Ok.  The worry I have with it is that if the target board has raised the timeout
> on purpose, because it's testing with a slow target/link/emulator, then this will
> cancel it.  In this case I'd just leave the timeout as it is.  Normally, GDB won't
> be broken for this test case, so it won't matter.
> 
> Moreover, the timeout here on my x86 machine is 10 seconds by default.  I just checked
> on an AArch64 box on the compile farm, it's 10 there too.  So in which case was it
> useful to set it to 10?
> 

Interesting. In my mind the default was from 30 to 60. Maybe that has 
changed over the years. If the boards are free to set it, then that is 
more desirable.

I'll let it be then.

Re: [PATCH 2/2,v3] [AArch64] Test handling of additional brk instruction patterns

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 1457 bytes --]

On 1/29/20 11:04 AM, Luis Machado wrote:
> 
> 
> On 1/29/20 11:01 AM, Simon Marchi wrote:
>> On 2020-01-29 6:30 a.m., Luis Machado wrote:
>>>>> +# Number of expected SIGTRAP's to get.  This needs to be kept in sync
>>>>> +# with the source file.
>>>>> +set expected_traps 3
>>>>> +set keep_going 1
>>>>> +set count 0
>>>>> +set old_timeout $timeout
>>>>> +set timeout 10
>>>>
>>>> Any reason you are changing the timeout?  There is nothing in the 
>>>> test that
>>>> looks like it would take time.
>>>>
>>>
>>> If GDB doesn't support one of these instructions, it will be caught in
>>> an infinite loop. The reduced timeout will prevent a long wait time
>>> until we bail out.
>>
>> Ok.  The worry I have with it is that if the target board has raised 
>> the timeout
>> on purpose, because it's testing with a slow target/link/emulator, 
>> then this will
>> cancel it.  In this case I'd just leave the timeout as it is.  
>> Normally, GDB won't
>> be broken for this test case, so it won't matter.
>>
>> Moreover, the timeout here on my x86 machine is 10 seconds by 
>> default.  I just checked
>> on an AArch64 box on the compile farm, it's 10 there too.  So in which 
>> case was it
>> useful to set it to 10?
>>
> 
> Interesting. In my mind the default was from 30 to 60. Maybe that has 
> changed over the years. If the boards are free to set it, then that is 
> more desirable.
> 
> I'll let it be then.

Here's the updated patch.

[-- Attachment #2: 0001-AArch64-Test-handling-of-additional-brk-instruction-.patch --]
[-- Type: text/x-patch, Size: 5382 bytes --]

From a2814e51d2dbec1130cf53b24a3f71041791fe69 Mon Sep 17 00:00:00 2001
From: Luis Machado <luis.machado@linaro.org>
Date: Mon, 13 Jan 2020 12:31:01 -0300
Subject: [PATCH] [AArch64] Test handling of additional brk instruction
 patterns

New in v5:

- Use gdb_test_name for gdb_test_multiple.
- Use gdb_assert.
- Verify count matches the expected sigtraps exactly.

New in v4:

- Fix formatting nit in gdb/testsuite/gdb.arch/aarch64-brk-patterns.c.

New in v3:

- Minor formatting and code cleanups.
- Added count check to validate number of brk SIGTRAP's.
- Moved count to SIGTRAP check conditional block.

This test exercises the previous patch's code and makes sure GDB can
properly get a SIGTRAP from various brk instruction patterns.

GDB needs to be able to see the program exiting normally. If GDB doesn't
support the additional brk instructions, we will see timeouts.

We bail out with the first timeout since we won't be able to step through
the program breakpoint anyway, so it is no use carrying on.

gdb/testsuite/ChangeLog:

2020-01-29  Luis Machado  <luis.machado@linaro.org>

	* gdb.arch/aarch64-brk-patterns.c: New source file.
	* gdb.arch/aarch64-brk-patterns.exp: New test.
---
 gdb/testsuite/gdb.arch/aarch64-brk-patterns.c | 31 +++++++++
 .../gdb.arch/aarch64-brk-patterns.exp         | 69 +++++++++++++++++++
 2 files changed, 100 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp

diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
new file mode 100644
index 0000000000..920ba8e2cb
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c
@@ -0,0 +1,31 @@
+/* This file is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main (void)
+{
+  /* Dummy instruction just so GDB doesn't stop at the first breakpoint
+     instruction.  */
+  __asm __volatile ("nop\n\t");
+
+  /* Multiple BRK instruction patterns.  */
+  __asm __volatile ("brk %0\n\t" ::"n"(0x0));
+  __asm __volatile ("brk %0\n\t" ::"n"(0x900 + 0xf));
+  __asm __volatile ("brk %0\n\t" ::"n"(0xf000));
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
new file mode 100644
index 0000000000..5cb55da219
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
@@ -0,0 +1,69 @@
+# Copyright 2020 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# This file is part of the gdb testsuite.
+
+# Test if GDB stops at various BRK instruction patterns inserted into
+# the code.
+
+if {![is_aarch64_target]} {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile
+if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
+    return -1
+}
+
+if {![runto_main]} {
+    untested "could not run to main"
+    return -1
+}
+
+# Number of expected SIGTRAP's to get.  This needs to be kept in sync
+# with the source file.
+set expected_traps 3
+set keep_going 1
+set count 0
+
+# Make sure we have a lower timeout in case GDB doesn't support a particular
+# instruction.  Such instruction will cause GDB to loop infinitely.
+while {$keep_going} {
+    # Continue to next program breakpoint instruction.
+    gdb_test_multiple "continue" "brk instruction $count causes SIGTRAP" {
+	-re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" {
+	    pass $gdb_test_name
+
+	    # Insert a breakpoint at the program breakpoint instruction so
+	    # GDB can step over it.
+	    gdb_test "break" \
+		"Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \
+		"insert breakpoint at brk instruction $count"
+	    incr count
+	}
+	# We've reached the end of the test.
+	-re "exited normally.*$gdb_prompt $" {
+	    set keep_going 0
+	}
+	timeout {
+	    fail $gdb_test_name
+	    set keep_going 0
+	}
+    }
+}
+
+# Verify we stopped at the expected number of SIGTRAP's.
+gdb_assert {$count == $expected_traps} "all brk instructions triggered"
-- 
2.17.1

Re: [PATCH 2/2,v3] [AArch64] Test handling of additional brk instruction patterns

[Simon Marchi]

On 2020-01-29 9:09 a.m., Luis Machado wrote:
> On 1/29/20 11:04 AM, Luis Machado wrote:
>>
>>
>> On 1/29/20 11:01 AM, Simon Marchi wrote:
>>> On 2020-01-29 6:30 a.m., Luis Machado wrote:
>>>>>> +# Number of expected SIGTRAP's to get.  This needs to be kept in sync
>>>>>> +# with the source file.
>>>>>> +set expected_traps 3
>>>>>> +set keep_going 1
>>>>>> +set count 0
>>>>>> +set old_timeout $timeout
>>>>>> +set timeout 10
>>>>>
>>>>> Any reason you are changing the timeout?  There is nothing in the 
>>>>> test that
>>>>> looks like it would take time.
>>>>>
>>>>
>>>> If GDB doesn't support one of these instructions, it will be caught in
>>>> an infinite loop. The reduced timeout will prevent a long wait time
>>>> until we bail out.
>>>
>>> Ok.  The worry I have with it is that if the target board has raised 
>>> the timeout
>>> on purpose, because it's testing with a slow target/link/emulator, 
>>> then this will
>>> cancel it.  In this case I'd just leave the timeout as it is.  
>>> Normally, GDB won't
>>> be broken for this test case, so it won't matter.
>>>
>>> Moreover, the timeout here on my x86 machine is 10 seconds by 
>>> default.  I just checked
>>> on an AArch64 box on the compile farm, it's 10 there too.  So in which 
>>> case was it
>>> useful to set it to 10?
>>>
>>
>> Interesting. In my mind the default was from 30 to 60. Maybe that has 
>> changed over the years. If the boards are free to set it, then that is 
>> more desirable.
>>
>> I'll let it be then.
> 
> Here's the updated patch.

Thanks, that looks good to me.

Simon

Re: [PATCH 2/2,v3] [AArch64] Test handling of additional brk instruction patterns

[Luis Machado]

On 1/29/20 11:11 AM, Simon Marchi wrote:
> On 2020-01-29 9:09 a.m., Luis Machado wrote:
>> On 1/29/20 11:04 AM, Luis Machado wrote:
>>>
>>>
>>> On 1/29/20 11:01 AM, Simon Marchi wrote:
>>>> On 2020-01-29 6:30 a.m., Luis Machado wrote:
>>>>>>> +# Number of expected SIGTRAP's to get.  This needs to be kept in sync
>>>>>>> +# with the source file.
>>>>>>> +set expected_traps 3
>>>>>>> +set keep_going 1
>>>>>>> +set count 0
>>>>>>> +set old_timeout $timeout
>>>>>>> +set timeout 10
>>>>>>
>>>>>> Any reason you are changing the timeout?  There is nothing in the
>>>>>> test that
>>>>>> looks like it would take time.
>>>>>>
>>>>>
>>>>> If GDB doesn't support one of these instructions, it will be caught in
>>>>> an infinite loop. The reduced timeout will prevent a long wait time
>>>>> until we bail out.
>>>>
>>>> Ok.  The worry I have with it is that if the target board has raised
>>>> the timeout
>>>> on purpose, because it's testing with a slow target/link/emulator,
>>>> then this will
>>>> cancel it.  In this case I'd just leave the timeout as it is.
>>>> Normally, GDB won't
>>>> be broken for this test case, so it won't matter.
>>>>
>>>> Moreover, the timeout here on my x86 machine is 10 seconds by
>>>> default.  I just checked
>>>> on an AArch64 box on the compile farm, it's 10 there too.  So in which
>>>> case was it
>>>> useful to set it to 10?
>>>>
>>>
>>> Interesting. In my mind the default was from 30 to 60. Maybe that has
>>> changed over the years. If the boards are free to set it, then that is
>>> more desirable.
>>>
>>> I'll let it be then.
>>
>> Here's the updated patch.
> 
> Thanks, that looks good to me.
> 
> Simon
> 

Pushed now. Thanks!