This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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



On 1/9/20 1:25 PM, Alan Hayward wrote:




On 9 Jan 2020, at 15:45, Luis Machado <luis.machado@linaro.org> wrote:

On 1/9/20 11:52 AM, Alan Hayward wrote:

On 23 Dec 2019, at 17:34, Luis Machado <luis.machado@linaro.org> wrote:

It was reported to me that program breakpoints (permanent ones inserted into
the code itself) other than the one GDB uses for AArch64 (0xd4200000) do not
generate visible stops when continuing, and GDB will continue spinning
infinitely.

This happens because GDB, upon hitting one of those program breakpoints, thinks
the SIGTRAP came from a delayed breakpoint hit...

(gdb) x/i $pc
=> 0x4005c0 <problem_function>: brk     #0x90f
(gdb) c
Continuing.
infrun: clear_proceed_status_thread (process 14198)
infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
infrun: proceed: resuming process 14198
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
infrun: infrun_async(1)
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   14198.14198.0 [process 14198],
infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: stop_pc = 0x4005c0
infrun: delayed software breakpoint trap, ignoring
infrun: no stepping, continue
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   14198.14198.0 [process 14198],
infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: stop_pc = 0x4005c0
infrun: delayed software breakpoint trap, ignoring
infrun: no stepping, continue
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   14198.14198.0 [process 14198],
infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: stop_pc = 0x4005c0
infrun: delayed software breakpoint trap, ignoring
infrun: no stepping, continue
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   14198.14198.0 [process 14198],
infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: stop_pc = 0x4005c0
infrun: delayed software breakpoint trap, ignoring
infrun: no stepping, continue
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   14198.14198.0 [process 14198],
infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
...

... which is not the case.

If the program breakpoint is one GDB recognizes, then it will stop when it
hits it.

(gdb) x/i $pc
=> 0x4005c0 <problem_function>: brk     #0x0
(gdb) c
Continuing.
infrun: clear_proceed_status_thread (process 14193)
infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
infrun: proceed: resuming process 14193
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14193] at 0x4005c0
infrun: infrun_async(1)
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   14193.14193.0 [process 14193],
infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: stop_pc = 0x4005c0
infrun: random signal (GDB_SIGNAL_TRAP)
infrun: stop_waiting
infrun: stop_all_threads
infrun: stop_all_threads, pass=0, iterations=0
infrun:   process 14193 not executing
infrun: stop_all_threads, pass=1, iterations=1
infrun:   process 14193 not executing
infrun: stop_all_threads done

Program received signal SIGTRAP, Trace/breakpoint trap.
problem_function () at brk_0.c:7
7        asm("brk %0\n\t" ::"n"(0x0));
infrun: infrun_async(0)

Otherwise GDB will keep trying to resume the inferior and will keep
seeing the SIGTRAP's, without stopping.

To the user it appears GDB has gone into an infinite loop, interruptible only
by Ctrl-C.

Also, windbg seems to use a different variation of AArch64 breakpoint compared
to GDB. This causes problems when debugging Windows on ARM binaries, when
program breakpoints are being used.

The proposed patch creates a new gdbarch method (gdbarch_insn_is_breakpoint)
that tells GDB whether the underlying instruction is a breakpoint instruction
or not.

This is more general than only checking for the instruction GDB uses as
breakpoint.

The existing logic is still preserved for targets that do not implement this
new gdbarch method.

The end result is like so:

(gdb) x/i $pc
=> 0x4005c0 <problem_function>: brk     #0x90f
(gdb) c
Continuing.
infrun: clear_proceed_status_thread (process 16417)
infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
infrun: proceed: resuming process 16417
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 16417] at 0x4005c0
infrun: infrun_async(1)
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   16417.16417.0 [process 16417],
infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: stop_pc = 0x4005c0
infrun: random signal (GDB_SIGNAL_TRAP)
infrun: stop_waiting
infrun: stop_all_threads
infrun: stop_all_threads, pass=0, iterations=0
infrun:   process 16417 not executing
infrun: stop_all_threads, pass=1, iterations=1
infrun:   process 16417 not executing
infrun: stop_all_threads done

Program received signal SIGTRAP, Trace/breakpoint trap.
problem_function () at brk.c:7
7        asm("brk %0\n\t" ::"n"(0x900 + 0xf));
infrun: infrun_async(0)

Does this change look ok?

gdb/ChangeLog:

2019-12-23  Luis Machado  <luis.machado@linaro.org>

	* aarch64-tdep.c (BRK_INSN_MASK): Define to 0xd4200000.
	(aarch64_insn_is_breakpoint): New function.
	(aarch64_gdbarch_init): Set gdbarch_insn_is_breakpoint hook.
	* arch-utils.c (default_insn_is_breakpoint): New function.
	* arch-utils.h (default_insn_is_breakpoint): New prototype.
	* breakpoint.c (program_breakpoint_here): Updated to use
	  gdbarch_insn_is_breakpoint.
	Update documentation to clarify behavior.
	* gdbarch.c: Regenerate.
	* gdbarch.h: Regenerate.
	* gdbarch.sh (gdbarch_insn_is_breakpoint): New method.

Change-Id: I96eb27151442f435560a58c87eac48b0f68432bc
---
gdb/aarch64-tdep.c | 25 +++++++++++++++++++++++++
gdb/arch-utils.c   |  7 +++++++
gdb/arch-utils.h   |  3 +++
gdb/breakpoint.c   | 19 ++++++++++++++-----
gdb/gdbarch.c      | 23 +++++++++++++++++++++++
gdb/gdbarch.h      |  7 +++++++
gdb/gdbarch.sh     |  4 ++++
7 files changed, 83 insertions(+), 5 deletions(-)


Do you have a test case for this? It could go in the gdb.arch/ directory.
It’d be fairly easy to check all the different brk patterns.


I'll work on this. I'm thinking we could auto-generate a number of brk patterns and verify we can continue and hit each of them.


diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 1d5fb2001d..c69361d4ea 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1201,6 +1201,28 @@ aarch64_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_byte op,
   return false;
}

+#define BRK_INSN_MASK 0xd4200000
+
+/* Implementation of gdbarch_insn_is_breakpoint for aarch64.  */
+
+static bool
+aarch64_insn_is_breakpoint (gdbarch *gdbarch,
+			    const gdb_byte *insn,
+			    unsigned int insn_size)
+{
+  gdb_assert (insn != nullptr);
+
+  uint32_t i;
+
+  i = (uint32_t) extract_unsigned_integer (insn, insn_size,
+					   gdbarch_byte_order (gdbarch));
+
+  /* Check if INSN is a BRK instruction pattern.  There are multiple choices
+     of such instructions with different immediate values.  Different OS' may
+     use a different variation, but they have the same outcome.  */
+  return (i & BRK_INSN_MASK) == BRK_INSN_MASK;
+}
+
/* When arguments must be pushed onto the stack, they go on in reverse
    order.  The code below implements a FILO (stack) to do this.  */

@@ -3357,6 +3379,9 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_execute_dwarf_cfa_vendor_op (gdbarch,
					   aarch64_execute_dwarf_cfa_vendor_op);

+  /* Permanent/Program breakpoint handling.  */
+  set_gdbarch_insn_is_breakpoint (gdbarch, aarch64_insn_is_breakpoint);
+
   /* Add some default predicates.  */
   frame_unwind_append_unwinder (gdbarch, &aarch64_stub_unwind);
   dwarf2_append_unwinders (gdbarch);
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index a1a003f91f..99c9f281be 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -876,6 +876,13 @@ int default_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr)
   return 0;
}

Add "/* See arch-utils.h.  */" ...


Fixed.



+bool default_insn_is_breakpoint (struct gdbarch *gdbarch,
+				 const gdb_byte  *insn,
+				 unsigned int insn_size)
+{
+  return false;

I don’t like that this is just returning false, as it’s not really doing what the function name says.
How about if the function did this:
   return (memcmp (target_mem, bpoint, len) == 0);
Then remove the memcmp from program_breakpoint_here_p.


I agree it would be cleaner, but ...


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


... this depends on calling gdbarch_breakpoint_from_pc to fetch bpoint, and gdbarch_breakpoint_from_pc requires the address information so it can determine the breakpoint kind.

Passing in the address is a bit out of scope for what the function is supposed to do (verify if a particular instruction is a breakpoint).

I don't have a strong objection towards passing in the address (or NULL if no address) if others are OK with it.


How about replacing program_breakpoint_here_p with
gdbarch_program_breakpoint_here_p(struct gdbarch *gdbarch, CORE_ADDR address) ?

default_program_breakpoint_here_p would be an exact copy of the existing program_breakpoint_here_p.

aarch64_program_breakpoint_here_p would do the copy to memory then the same as aarch64_insn_is_breakpoint.
It sounds interesting at first, but i'm not sure about it. We'll save a memory comparison but will duplicate the code to gdbarch_breakpoint_from_pc to every arch that sets its own gdbarch_program_breakpoint_here_p.
At least the old function dealt with just arch-specific bits, whereas the <arch>_program_breakpoint_here_p implementation will need to do a generic check + generic memory read as well, then do the arch-specific checks. 

Thoughts?
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]