* [PATCH] [AArch64] Recognize more program breakpoint patterns
@ 2019-12-23 17:34 Luis Machado
2020-01-09 14:52 ` Alan Hayward
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Luis Machado @ 2019-12-23 17:34 UTC (permalink / raw)
To: gdb-patches
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(-)
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;
}
+bool default_insn_is_breakpoint (struct gdbarch *gdbarch,
+ const gdb_byte *insn,
+ unsigned int insn_size)
+{
+ return false;
+}
+
void
default_skip_permanent_breakpoint (struct regcache *regcache)
{
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 48ff3bb9a1..77ffe8190c 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -227,6 +227,9 @@ extern int default_return_in_first_hidden_param_p (struct gdbarch *,
extern int default_insn_is_call (struct gdbarch *, CORE_ADDR);
extern int default_insn_is_ret (struct gdbarch *, CORE_ADDR);
extern int default_insn_is_jump (struct gdbarch *, CORE_ADDR);
+extern bool default_insn_is_breakpoint (struct gdbarch *gdbarch,
+ const gdb_byte *insn,
+ unsigned int insn_size);
/* Do-nothing version of vsyscall_range. Returns false. */
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 904abda8db..ffb7f7f8be 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -8599,14 +8599,23 @@ program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address)
target_mem = (gdb_byte *) alloca (len);
/* Enable the automatic memory restoration from breakpoints while
- we read the memory. Otherwise we could say about our temporary
- breakpoints they are permanent. */
+ we read the memory. Otherwise we may find temporary breakpoints, ones
+ inserted by GDB, and flag them as permanent breakpoints. */
scoped_restore restore_memory
= make_scoped_restore_show_memory_breakpoints (0);
- if (target_read_memory (address, target_mem, len) == 0
- && memcmp (target_mem, bpoint, len) == 0)
- return 1;
+ if (target_read_memory (address, target_mem, len) == 0)
+ {
+ /* Check if this is a breakpoint instruction for this architecture,
+ including ones used by GDB.
+
+ Some architectures have more than one possible breakpoint
+ instruction, but GDB does not use all of them. We should detect those
+ as well. */
+ if (gdbarch_insn_is_breakpoint (gdbarch, target_mem, len)
+ || memcmp (target_mem, bpoint, len) == 0)
+ return 1;
+ }
return 0;
}
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 59c97da985..9e21ba7eca 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -345,6 +345,7 @@ struct gdbarch
gdbarch_insn_is_call_ftype *insn_is_call;
gdbarch_insn_is_ret_ftype *insn_is_ret;
gdbarch_insn_is_jump_ftype *insn_is_jump;
+ gdbarch_insn_is_breakpoint_ftype *insn_is_breakpoint;
gdbarch_auxv_parse_ftype *auxv_parse;
gdbarch_print_auxv_entry_ftype *print_auxv_entry;
gdbarch_vsyscall_range_ftype *vsyscall_range;
@@ -464,6 +465,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
gdbarch->insn_is_call = default_insn_is_call;
gdbarch->insn_is_ret = default_insn_is_ret;
gdbarch->insn_is_jump = default_insn_is_jump;
+ gdbarch->insn_is_breakpoint = default_insn_is_breakpoint;
gdbarch->print_auxv_entry = default_print_auxv_entry;
gdbarch->vsyscall_range = default_vsyscall_range;
gdbarch->infcall_mmap = default_infcall_mmap;
@@ -708,6 +710,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
/* Skip verify of insn_is_call, invalid_p == 0 */
/* Skip verify of insn_is_ret, invalid_p == 0 */
/* Skip verify of insn_is_jump, invalid_p == 0 */
+ /* Skip verify of insn_is_breakpoint, invalid_p == 0 */
/* Skip verify of auxv_parse, has predicate. */
/* Skip verify of print_auxv_entry, invalid_p == 0 */
/* Skip verify of vsyscall_range, invalid_p == 0 */
@@ -1137,6 +1140,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
fprintf_unfiltered (file,
"gdbarch_dump: inner_than = <%s>\n",
host_address_to_string (gdbarch->inner_than));
+ fprintf_unfiltered (file,
+ "gdbarch_dump: insn_is_breakpoint = <%s>\n",
+ host_address_to_string (gdbarch->insn_is_breakpoint));
fprintf_unfiltered (file,
"gdbarch_dump: insn_is_call = <%s>\n",
host_address_to_string (gdbarch->insn_is_call));
@@ -4928,6 +4934,23 @@ set_gdbarch_insn_is_jump (struct gdbarch *gdbarch,
gdbarch->insn_is_jump = insn_is_jump;
}
+bool
+gdbarch_insn_is_breakpoint (struct gdbarch *gdbarch, const gdb_byte *insn, unsigned int insn_size)
+{
+ gdb_assert (gdbarch != NULL);
+ gdb_assert (gdbarch->insn_is_breakpoint != NULL);
+ if (gdbarch_debug >= 2)
+ fprintf_unfiltered (gdb_stdlog, "gdbarch_insn_is_breakpoint called\n");
+ return gdbarch->insn_is_breakpoint (gdbarch, insn, insn_size);
+}
+
+void
+set_gdbarch_insn_is_breakpoint (struct gdbarch *gdbarch,
+ gdbarch_insn_is_breakpoint_ftype insn_is_breakpoint)
+{
+ gdbarch->insn_is_breakpoint = insn_is_breakpoint;
+}
+
int
gdbarch_auxv_parse_p (struct gdbarch *gdbarch)
{
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 78e05ecdcb..d94950b8f2 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1545,6 +1545,13 @@ typedef int (gdbarch_insn_is_jump_ftype) (struct gdbarch *gdbarch, CORE_ADDR add
extern int gdbarch_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr);
extern void set_gdbarch_insn_is_jump (struct gdbarch *gdbarch, gdbarch_insn_is_jump_ftype *insn_is_jump);
+/* Return true if INSN of size INSN_SIZE acts like a permanent breakpoint and
+ false otherwise. */
+
+typedef bool (gdbarch_insn_is_breakpoint_ftype) (struct gdbarch *gdbarch, const gdb_byte *insn, unsigned int insn_size);
+extern bool gdbarch_insn_is_breakpoint (struct gdbarch *gdbarch, const gdb_byte *insn, unsigned int insn_size);
+extern void set_gdbarch_insn_is_breakpoint (struct gdbarch *gdbarch, gdbarch_insn_is_breakpoint_ftype *insn_is_breakpoint);
+
/* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
Return 0 if *READPTR is already at the end of the buffer.
Return -1 if there is insufficient buffer for a whole entry.
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 331eb39278..dbf31ee8ae 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1152,6 +1152,10 @@ m;int;insn_is_ret;CORE_ADDR addr;addr;;default_insn_is_ret;;0
# Return non-zero if the instruction at ADDR is a jump; zero otherwise.
m;int;insn_is_jump;CORE_ADDR addr;addr;;default_insn_is_jump;;0
+# Return true if INSN of size INSN_SIZE acts like a permanent breakpoint and
+# false otherwise.
+m;bool;insn_is_breakpoint;const gdb_byte *insn, unsigned int insn_size;insn, insn_size;;default_insn_is_breakpoint;;0
+
# Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
# Return 0 if *READPTR is already at the end of the buffer.
# Return -1 if there is insufficient buffer for a whole entry.
--
2.17.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [AArch64] Recognize more program breakpoint patterns
2019-12-23 17:34 [PATCH] [AArch64] Recognize more program breakpoint patterns Luis Machado
@ 2020-01-09 14:52 ` Alan Hayward
2020-01-09 15:45 ` Luis Machado
2020-01-09 15:59 ` Aktemur, Tankut Baris
2020-01-13 17:03 ` [PATCH 1/2] " Luis Machado
2 siblings, 1 reply; 10+ messages in thread
From: Alan Hayward @ 2020-01-09 14:52 UTC (permalink / raw)
To: Luis Machado; +Cc: gdb-patches\@sourceware.org, nd
> On 23 Dec 2019, at 17:34, Luis Machado <luis.machado@linaro.org> wrote:
>
> It was reported to me that program breakpoints (permanent ones inserted into
> the code itself) other than the one GDB uses for AArch64 (0xd4200000) do not
> generate visible stops when continuing, and GDB will continue spinning
> infinitely.
>
> This happens because GDB, upon hitting one of those program breakpoints, thinks
> the SIGTRAP came from a delayed breakpoint hit...
>
> (gdb) x/i $pc
> => 0x4005c0 <problem_function>: brk #0x90f
> (gdb) c
> Continuing.
> infrun: clear_proceed_status_thread (process 14198)
> infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
> infrun: proceed: resuming process 14198
> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
> infrun: infrun_async(1)
> infrun: prepare_to_wait
> infrun: target_wait (-1.0.0, status) =
> infrun: 14198.14198.0 [process 14198],
> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
> infrun: stop_pc = 0x4005c0
> infrun: delayed software breakpoint trap, ignoring
> infrun: no stepping, continue
> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
> infrun: prepare_to_wait
> infrun: target_wait (-1.0.0, status) =
> infrun: 14198.14198.0 [process 14198],
> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
> infrun: stop_pc = 0x4005c0
> infrun: delayed software breakpoint trap, ignoring
> infrun: no stepping, continue
> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
> infrun: prepare_to_wait
> infrun: target_wait (-1.0.0, status) =
> infrun: 14198.14198.0 [process 14198],
> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
> infrun: stop_pc = 0x4005c0
> infrun: delayed software breakpoint trap, ignoring
> infrun: no stepping, continue
> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
> infrun: prepare_to_wait
> infrun: target_wait (-1.0.0, status) =
> infrun: 14198.14198.0 [process 14198],
> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
> infrun: stop_pc = 0x4005c0
> infrun: delayed software breakpoint trap, ignoring
> infrun: no stepping, continue
> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
> infrun: prepare_to_wait
> infrun: target_wait (-1.0.0, status) =
> infrun: 14198.14198.0 [process 14198],
> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
> ...
>
> ... which is not the case.
>
> If the program breakpoint is one GDB recognizes, then it will stop when it
> hits it.
>
> (gdb) x/i $pc
> => 0x4005c0 <problem_function>: brk #0x0
> (gdb) c
> Continuing.
> infrun: clear_proceed_status_thread (process 14193)
> infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
> infrun: proceed: resuming process 14193
> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14193] at 0x4005c0
> infrun: infrun_async(1)
> infrun: prepare_to_wait
> infrun: target_wait (-1.0.0, status) =
> infrun: 14193.14193.0 [process 14193],
> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
> infrun: stop_pc = 0x4005c0
> infrun: random signal (GDB_SIGNAL_TRAP)
> infrun: stop_waiting
> infrun: stop_all_threads
> infrun: stop_all_threads, pass=0, iterations=0
> infrun: process 14193 not executing
> infrun: stop_all_threads, pass=1, iterations=1
> infrun: process 14193 not executing
> infrun: stop_all_threads done
>
> Program received signal SIGTRAP, Trace/breakpoint trap.
> problem_function () at brk_0.c:7
> 7 asm("brk %0\n\t" ::"n"(0x0));
> infrun: infrun_async(0)
>
> Otherwise GDB will keep trying to resume the inferior and will keep
> seeing the SIGTRAP's, without stopping.
>
> To the user it appears GDB has gone into an infinite loop, interruptible only
> by Ctrl-C.
>
> Also, windbg seems to use a different variation of AArch64 breakpoint compared
> to GDB. This causes problems when debugging Windows on ARM binaries, when
> program breakpoints are being used.
>
> The proposed patch creates a new gdbarch method (gdbarch_insn_is_breakpoint)
> that tells GDB whether the underlying instruction is a breakpoint instruction
> or not.
>
> This is more general than only checking for the instruction GDB uses as
> breakpoint.
>
> The existing logic is still preserved for targets that do not implement this
> new gdbarch method.
>
> The end result is like so:
>
> (gdb) x/i $pc
> => 0x4005c0 <problem_function>: brk #0x90f
> (gdb) c
> Continuing.
> infrun: clear_proceed_status_thread (process 16417)
> infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
> infrun: proceed: resuming process 16417
> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 16417] at 0x4005c0
> infrun: infrun_async(1)
> infrun: prepare_to_wait
> infrun: target_wait (-1.0.0, status) =
> infrun: 16417.16417.0 [process 16417],
> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
> infrun: stop_pc = 0x4005c0
> infrun: random signal (GDB_SIGNAL_TRAP)
> infrun: stop_waiting
> infrun: stop_all_threads
> infrun: stop_all_threads, pass=0, iterations=0
> infrun: process 16417 not executing
> infrun: stop_all_threads, pass=1, iterations=1
> infrun: process 16417 not executing
> infrun: stop_all_threads done
>
> Program received signal SIGTRAP, Trace/breakpoint trap.
> problem_function () at brk.c:7
> 7 asm("brk %0\n\t" ::"n"(0x900 + 0xf));
> infrun: infrun_async(0)
>
> Does this change look ok?
>
> gdb/ChangeLog:
>
> 2019-12-23 Luis Machado <luis.machado@linaro.org>
>
> * aarch64-tdep.c (BRK_INSN_MASK): Define to 0xd4200000.
> (aarch64_insn_is_breakpoint): New function.
> (aarch64_gdbarch_init): Set gdbarch_insn_is_breakpoint hook.
> * arch-utils.c (default_insn_is_breakpoint): New function.
> * arch-utils.h (default_insn_is_breakpoint): New prototype.
> * breakpoint.c (program_breakpoint_here): Updated to use
> gdbarch_insn_is_breakpoint.
> Update documentation to clarify behavior.
> * gdbarch.c: Regenerate.
> * gdbarch.h: Regenerate.
> * gdbarch.sh (gdbarch_insn_is_breakpoint): New method.
>
> Change-Id: I96eb27151442f435560a58c87eac48b0f68432bc
> ---
> gdb/aarch64-tdep.c | 25 +++++++++++++++++++++++++
> gdb/arch-utils.c | 7 +++++++
> gdb/arch-utils.h | 3 +++
> gdb/breakpoint.c | 19 ++++++++++++++-----
> gdb/gdbarch.c | 23 +++++++++++++++++++++++
> gdb/gdbarch.h | 7 +++++++
> gdb/gdbarch.sh | 4 ++++
> 7 files changed, 83 insertions(+), 5 deletions(-)
>
Do you have a test case for this? It could go in the gdb.arch/ directory.
It’d be fairly easy to check all the different brk patterns.
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 1d5fb2001d..c69361d4ea 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -1201,6 +1201,28 @@ aarch64_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_byte op,
> return false;
> }
>
> +#define BRK_INSN_MASK 0xd4200000
> +
> +/* Implementation of gdbarch_insn_is_breakpoint for aarch64. */
> +
> +static bool
> +aarch64_insn_is_breakpoint (gdbarch *gdbarch,
> + const gdb_byte *insn,
> + unsigned int insn_size)
> +{
> + gdb_assert (insn != nullptr);
> +
> + uint32_t i;
> +
> + i = (uint32_t) extract_unsigned_integer (insn, insn_size,
> + gdbarch_byte_order (gdbarch));
> +
> + /* Check if INSN is a BRK instruction pattern. There are multiple choices
> + of such instructions with different immediate values. Different OS' may
> + use a different variation, but they have the same outcome. */
> + return (i & BRK_INSN_MASK) == BRK_INSN_MASK;
> +}
> +
> /* When arguments must be pushed onto the stack, they go on in reverse
> order. The code below implements a FILO (stack) to do this. */
>
> @@ -3357,6 +3379,9 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> set_gdbarch_execute_dwarf_cfa_vendor_op (gdbarch,
> aarch64_execute_dwarf_cfa_vendor_op);
>
> + /* Permanent/Program breakpoint handling. */
> + set_gdbarch_insn_is_breakpoint (gdbarch, aarch64_insn_is_breakpoint);
> +
> /* Add some default predicates. */
> frame_unwind_append_unwinder (gdbarch, &aarch64_stub_unwind);
> dwarf2_append_unwinders (gdbarch);
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index a1a003f91f..99c9f281be 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -876,6 +876,13 @@ int default_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr)
> return 0;
> }
Add "/* See arch-utils.h. */" ...
>
> +bool default_insn_is_breakpoint (struct gdbarch *gdbarch,
> + const gdb_byte *insn,
> + unsigned int insn_size)
> +{
> + return false;
I don’t like that this is just returning false, as it’s not really doing what the function name says.
How about if the function did this:
return (memcmp (target_mem, bpoint, len) == 0);
Then remove the memcmp from program_breakpoint_here_p.
You’ll probably have to move the call to gdbarch_breakpoint_from_pc into here too.
> +}
> +
> void
> default_skip_permanent_breakpoint (struct regcache *regcache)
> {
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index 48ff3bb9a1..77ffe8190c 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -227,6 +227,9 @@ extern int default_return_in_first_hidden_param_p (struct gdbarch *,
> extern int default_insn_is_call (struct gdbarch *, CORE_ADDR);
> extern int default_insn_is_ret (struct gdbarch *, CORE_ADDR);
> extern int default_insn_is_jump (struct gdbarch *, CORE_ADDR);
... plus a brief comment here.
> +extern bool default_insn_is_breakpoint (struct gdbarch *gdbarch,
> + const gdb_byte *insn,
> + unsigned int insn_size);
>
> /* Do-nothing version of vsyscall_range. Returns false. */
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 904abda8db..ffb7f7f8be 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -8599,14 +8599,23 @@ program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address)
> target_mem = (gdb_byte *) alloca (len);
>
> /* Enable the automatic memory restoration from breakpoints while
> - we read the memory. Otherwise we could say about our temporary
> - breakpoints they are permanent. */
> + we read the memory. Otherwise we may find temporary breakpoints, ones
> + inserted by GDB, and flag them as permanent breakpoints. */
> scoped_restore restore_memory
> = make_scoped_restore_show_memory_breakpoints (0);
>
> - if (target_read_memory (address, target_mem, len) == 0
> - && memcmp (target_mem, bpoint, len) == 0)
> - return 1;
> + if (target_read_memory (address, target_mem, len) == 0)
> + {
> + /* Check if this is a breakpoint instruction for this architecture,
> + including ones used by GDB.
> +
> + Some architectures have more than one possible breakpoint
> + instruction, but GDB does not use all of them. We should detect those
> + as well. */
> + if (gdbarch_insn_is_breakpoint (gdbarch, target_mem, len)
> + || memcmp (target_mem, bpoint, len) == 0)
> + return 1;
> + }
>
> return 0;
> }
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 59c97da985..9e21ba7eca 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -345,6 +345,7 @@ struct gdbarch
> gdbarch_insn_is_call_ftype *insn_is_call;
> gdbarch_insn_is_ret_ftype *insn_is_ret;
> gdbarch_insn_is_jump_ftype *insn_is_jump;
> + gdbarch_insn_is_breakpoint_ftype *insn_is_breakpoint;
> gdbarch_auxv_parse_ftype *auxv_parse;
> gdbarch_print_auxv_entry_ftype *print_auxv_entry;
> gdbarch_vsyscall_range_ftype *vsyscall_range;
> @@ -464,6 +465,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
> gdbarch->insn_is_call = default_insn_is_call;
> gdbarch->insn_is_ret = default_insn_is_ret;
> gdbarch->insn_is_jump = default_insn_is_jump;
> + gdbarch->insn_is_breakpoint = default_insn_is_breakpoint;
> gdbarch->print_auxv_entry = default_print_auxv_entry;
> gdbarch->vsyscall_range = default_vsyscall_range;
> gdbarch->infcall_mmap = default_infcall_mmap;
> @@ -708,6 +710,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
> /* Skip verify of insn_is_call, invalid_p == 0 */
> /* Skip verify of insn_is_ret, invalid_p == 0 */
> /* Skip verify of insn_is_jump, invalid_p == 0 */
> + /* Skip verify of insn_is_breakpoint, invalid_p == 0 */
> /* Skip verify of auxv_parse, has predicate. */
> /* Skip verify of print_auxv_entry, invalid_p == 0 */
> /* Skip verify of vsyscall_range, invalid_p == 0 */
> @@ -1137,6 +1140,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
> fprintf_unfiltered (file,
> "gdbarch_dump: inner_than = <%s>\n",
> host_address_to_string (gdbarch->inner_than));
> + fprintf_unfiltered (file,
> + "gdbarch_dump: insn_is_breakpoint = <%s>\n",
> + host_address_to_string (gdbarch->insn_is_breakpoint));
> fprintf_unfiltered (file,
> "gdbarch_dump: insn_is_call = <%s>\n",
> host_address_to_string (gdbarch->insn_is_call));
> @@ -4928,6 +4934,23 @@ set_gdbarch_insn_is_jump (struct gdbarch *gdbarch,
> gdbarch->insn_is_jump = insn_is_jump;
> }
>
> +bool
> +gdbarch_insn_is_breakpoint (struct gdbarch *gdbarch, const gdb_byte *insn, unsigned int insn_size)
> +{
> + gdb_assert (gdbarch != NULL);
> + gdb_assert (gdbarch->insn_is_breakpoint != NULL);
> + if (gdbarch_debug >= 2)
> + fprintf_unfiltered (gdb_stdlog, "gdbarch_insn_is_breakpoint called\n");
> + return gdbarch->insn_is_breakpoint (gdbarch, insn, insn_size);
> +}
> +
> +void
> +set_gdbarch_insn_is_breakpoint (struct gdbarch *gdbarch,
> + gdbarch_insn_is_breakpoint_ftype insn_is_breakpoint)
> +{
> + gdbarch->insn_is_breakpoint = insn_is_breakpoint;
> +}
> +
> int
> gdbarch_auxv_parse_p (struct gdbarch *gdbarch)
> {
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 78e05ecdcb..d94950b8f2 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -1545,6 +1545,13 @@ typedef int (gdbarch_insn_is_jump_ftype) (struct gdbarch *gdbarch, CORE_ADDR add
> extern int gdbarch_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr);
> extern void set_gdbarch_insn_is_jump (struct gdbarch *gdbarch, gdbarch_insn_is_jump_ftype *insn_is_jump);
>
> +/* Return true if INSN of size INSN_SIZE acts like a permanent breakpoint and
> + false otherwise. */
> +
> +typedef bool (gdbarch_insn_is_breakpoint_ftype) (struct gdbarch *gdbarch, const gdb_byte *insn, unsigned int insn_size);
> +extern bool gdbarch_insn_is_breakpoint (struct gdbarch *gdbarch, const gdb_byte *insn, unsigned int insn_size);
> +extern void set_gdbarch_insn_is_breakpoint (struct gdbarch *gdbarch, gdbarch_insn_is_breakpoint_ftype *insn_is_breakpoint);
> +
> /* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
> Return 0 if *READPTR is already at the end of the buffer.
> Return -1 if there is insufficient buffer for a whole entry.
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 331eb39278..dbf31ee8ae 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1152,6 +1152,10 @@ m;int;insn_is_ret;CORE_ADDR addr;addr;;default_insn_is_ret;;0
> # Return non-zero if the instruction at ADDR is a jump; zero otherwise.
> m;int;insn_is_jump;CORE_ADDR addr;addr;;default_insn_is_jump;;0
>
> +# Return true if INSN of size INSN_SIZE acts like a permanent breakpoint and
> +# false otherwise.
> +m;bool;insn_is_breakpoint;const gdb_byte *insn, unsigned int insn_size;insn, insn_size;;default_insn_is_breakpoint;;0
> +
> # Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
> # Return 0 if *READPTR is already at the end of the buffer.
> # Return -1 if there is insufficient buffer for a whole entry.
> —
> 2.17.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [AArch64] Recognize more program breakpoint patterns
2020-01-09 14:52 ` Alan Hayward
@ 2020-01-09 15:45 ` Luis Machado
2020-01-09 16:26 ` Alan Hayward
0 siblings, 1 reply; 10+ messages in thread
From: Luis Machado @ 2020-01-09 15:45 UTC (permalink / raw)
To: Alan Hayward; +Cc: gdb-patches\@sourceware.org, nd
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.
>> +}
>> +
>> void
>> default_skip_permanent_breakpoint (struct regcache *regcache)
>> {
>> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
>> index 48ff3bb9a1..77ffe8190c 100644
>> --- a/gdb/arch-utils.h
>> +++ b/gdb/arch-utils.h
>> @@ -227,6 +227,9 @@ extern int default_return_in_first_hidden_param_p (struct gdbarch *,
>> extern int default_insn_is_call (struct gdbarch *, CORE_ADDR);
>> extern int default_insn_is_ret (struct gdbarch *, CORE_ADDR);
>> extern int default_insn_is_jump (struct gdbarch *, CORE_ADDR);
>
> ... plus a brief comment here.
>
> >> +extern bool default_insn_is_breakpoint (struct gdbarch *gdbarch,
>> + const gdb_byte *insn,
>> + unsigned int insn_size);
>>
>> /* Do-nothing version of vsyscall_range. Returns false. */
>>
Fixed as well.
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] [AArch64] Recognize more program breakpoint patterns
2019-12-23 17:34 [PATCH] [AArch64] Recognize more program breakpoint patterns Luis Machado
2020-01-09 14:52 ` Alan Hayward
@ 2020-01-09 15:59 ` Aktemur, Tankut Baris
2020-01-13 17:03 ` [PATCH 1/2] " Luis Machado
2 siblings, 0 replies; 10+ messages in thread
From: Aktemur, Tankut Baris @ 2020-01-09 15:59 UTC (permalink / raw)
To: Luis Machado, gdb-patches
On Monday, December 23, 2019 6:35 PM, Luis Machado 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(-)
>
> 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
I have a general question here. Given that GDB is using C++11, would it
make more sense to have this as
static constexpr uint32_t BRK_INSN_MASK = 0xd4200000;
for more type-safety for the uses of this #define?
> +
> +/* 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));
The declaration and the definition can be combined.
> +
> + /* 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;
> }
>
> +bool default_insn_is_breakpoint (struct gdbarch *gdbarch,
Function name shall start at column 0.
Regards,
-Baris
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [AArch64] Recognize more program breakpoint patterns
2020-01-09 15:45 ` Luis Machado
@ 2020-01-09 16:26 ` Alan Hayward
2020-01-10 14:58 ` Luis Machado
0 siblings, 1 reply; 10+ messages in thread
From: Alan Hayward @ 2020-01-09 16:26 UTC (permalink / raw)
To: Luis Machado; +Cc: gdb-patches\@sourceware.org, nd
> 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.
>
>>> +}
>>> +
>>> void
>>> default_skip_permanent_breakpoint (struct regcache *regcache)
>>> {
>>> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
>>> index 48ff3bb9a1..77ffe8190c 100644
>>> --- a/gdb/arch-utils.h
>>> +++ b/gdb/arch-utils.h
>>> @@ -227,6 +227,9 @@ extern int default_return_in_first_hidden_param_p (struct gdbarch *,
>>> extern int default_insn_is_call (struct gdbarch *, CORE_ADDR);
>>> extern int default_insn_is_ret (struct gdbarch *, CORE_ADDR);
>>> extern int default_insn_is_jump (struct gdbarch *, CORE_ADDR);
>> ... plus a brief comment here.
>> >> +extern bool default_insn_is_breakpoint (struct gdbarch *gdbarch,
>>> + const gdb_byte *insn,
>>> + unsigned int insn_size);
>>>
>>> /* Do-nothing version of vsyscall_range. Returns false. */
>>>
>
> Fixed as well.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [AArch64] Recognize more program breakpoint patterns
2020-01-09 16:26 ` Alan Hayward
@ 2020-01-10 14:58 ` Luis Machado
2020-01-10 16:53 ` Alan Hayward
0 siblings, 1 reply; 10+ messages in thread
From: Luis Machado @ 2020-01-10 14:58 UTC (permalink / raw)
To: Alan Hayward; +Cc: gdb-patches\@sourceware.org, nd
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [AArch64] Recognize more program breakpoint patterns
2020-01-10 14:58 ` Luis Machado
@ 2020-01-10 16:53 ` Alan Hayward
0 siblings, 0 replies; 10+ messages in thread
From: Alan Hayward @ 2020-01-10 16:53 UTC (permalink / raw)
To: Luis Machado; +Cc: gdb-patches\@sourceware.org, nd
> On 10 Jan 2020, at 14:58, Luis Machado <luis.machado@linaro.org> wrote:
>
>
>
> On 1/9/20 1:25 PM, Alan Hayward wrote:
>>> On 9 Jan 2020, at 15:45, Luis Machado <luis.machado@linaro.org> wrote:
>>>
>>> On 1/9/20 11:52 AM, Alan Hayward wrote:
>>>>> On 23 Dec 2019, at 17:34, Luis Machado <luis.machado@linaro.org> wrote:
>>>>>
>>>>> It was reported to me that program breakpoints (permanent ones inserted into
>>>>> the code itself) other than the one GDB uses for AArch64 (0xd4200000) do not
>>>>> generate visible stops when continuing, and GDB will continue spinning
>>>>> infinitely.
>>>>>
>>>>> This happens because GDB, upon hitting one of those program breakpoints, thinks
>>>>> the SIGTRAP came from a delayed breakpoint hit...
>>>>>
>>>>> (gdb) x/i $pc
>>>>> => 0x4005c0 <problem_function>: brk #0x90f
>>>>> (gdb) c
>>>>> Continuing.
>>>>> infrun: clear_proceed_status_thread (process 14198)
>>>>> infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
>>>>> infrun: proceed: resuming process 14198
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
>>>>> infrun: infrun_async(1)
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun: 14198.14198.0 [process 14198],
>>>>> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: stop_pc = 0x4005c0
>>>>> infrun: delayed software breakpoint trap, ignoring
>>>>> infrun: no stepping, continue
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun: 14198.14198.0 [process 14198],
>>>>> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: stop_pc = 0x4005c0
>>>>> infrun: delayed software breakpoint trap, ignoring
>>>>> infrun: no stepping, continue
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun: 14198.14198.0 [process 14198],
>>>>> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: stop_pc = 0x4005c0
>>>>> infrun: delayed software breakpoint trap, ignoring
>>>>> infrun: no stepping, continue
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun: 14198.14198.0 [process 14198],
>>>>> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: stop_pc = 0x4005c0
>>>>> infrun: delayed software breakpoint trap, ignoring
>>>>> infrun: no stepping, continue
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14198] at 0x4005c0
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun: 14198.14198.0 [process 14198],
>>>>> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> ...
>>>>>
>>>>> ... which is not the case.
>>>>>
>>>>> If the program breakpoint is one GDB recognizes, then it will stop when it
>>>>> hits it.
>>>>>
>>>>> (gdb) x/i $pc
>>>>> => 0x4005c0 <problem_function>: brk #0x0
>>>>> (gdb) c
>>>>> Continuing.
>>>>> infrun: clear_proceed_status_thread (process 14193)
>>>>> infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
>>>>> infrun: proceed: resuming process 14193
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 14193] at 0x4005c0
>>>>> infrun: infrun_async(1)
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun: 14193.14193.0 [process 14193],
>>>>> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: stop_pc = 0x4005c0
>>>>> infrun: random signal (GDB_SIGNAL_TRAP)
>>>>> infrun: stop_waiting
>>>>> infrun: stop_all_threads
>>>>> infrun: stop_all_threads, pass=0, iterations=0
>>>>> infrun: process 14193 not executing
>>>>> infrun: stop_all_threads, pass=1, iterations=1
>>>>> infrun: process 14193 not executing
>>>>> infrun: stop_all_threads done
>>>>>
>>>>> Program received signal SIGTRAP, Trace/breakpoint trap.
>>>>> problem_function () at brk_0.c:7
>>>>> 7 asm("brk %0\n\t" ::"n"(0x0));
>>>>> infrun: infrun_async(0)
>>>>>
>>>>> Otherwise GDB will keep trying to resume the inferior and will keep
>>>>> seeing the SIGTRAP's, without stopping.
>>>>>
>>>>> To the user it appears GDB has gone into an infinite loop, interruptible only
>>>>> by Ctrl-C.
>>>>>
>>>>> Also, windbg seems to use a different variation of AArch64 breakpoint compared
>>>>> to GDB. This causes problems when debugging Windows on ARM binaries, when
>>>>> program breakpoints are being used.
>>>>>
>>>>> The proposed patch creates a new gdbarch method (gdbarch_insn_is_breakpoint)
>>>>> that tells GDB whether the underlying instruction is a breakpoint instruction
>>>>> or not.
>>>>>
>>>>> This is more general than only checking for the instruction GDB uses as
>>>>> breakpoint.
>>>>>
>>>>> The existing logic is still preserved for targets that do not implement this
>>>>> new gdbarch method.
>>>>>
>>>>> The end result is like so:
>>>>>
>>>>> (gdb) x/i $pc
>>>>> => 0x4005c0 <problem_function>: brk #0x90f
>>>>> (gdb) c
>>>>> Continuing.
>>>>> infrun: clear_proceed_status_thread (process 16417)
>>>>> infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
>>>>> infrun: proceed: resuming process 16417
>>>>> infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 16417] at 0x4005c0
>>>>> infrun: infrun_async(1)
>>>>> infrun: prepare_to_wait
>>>>> infrun: target_wait (-1.0.0, status) =
>>>>> infrun: 16417.16417.0 [process 16417],
>>>>> infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
>>>>> infrun: stop_pc = 0x4005c0
>>>>> infrun: random signal (GDB_SIGNAL_TRAP)
>>>>> infrun: stop_waiting
>>>>> infrun: stop_all_threads
>>>>> infrun: stop_all_threads, pass=0, iterations=0
>>>>> infrun: process 16417 not executing
>>>>> infrun: stop_all_threads, pass=1, iterations=1
>>>>> infrun: process 16417 not executing
>>>>> infrun: stop_all_threads done
>>>>>
>>>>> Program received signal SIGTRAP, Trace/breakpoint trap.
>>>>> problem_function () at brk.c:7
>>>>> 7 asm("brk %0\n\t" ::"n"(0x900 + 0xf));
>>>>> infrun: infrun_async(0)
>>>>>
>>>>> Does this change look ok?
>>>>>
>>>>> gdb/ChangeLog:
>>>>>
>>>>> 2019-12-23 Luis Machado <luis.machado@linaro.org>
>>>>>
>>>>> * aarch64-tdep.c (BRK_INSN_MASK): Define to 0xd4200000.
>>>>> (aarch64_insn_is_breakpoint): New function.
>>>>> (aarch64_gdbarch_init): Set gdbarch_insn_is_breakpoint hook.
>>>>> * arch-utils.c (default_insn_is_breakpoint): New function.
>>>>> * arch-utils.h (default_insn_is_breakpoint): New prototype.
>>>>> * breakpoint.c (program_breakpoint_here): Updated to use
>>>>> gdbarch_insn_is_breakpoint.
>>>>> Update documentation to clarify behavior.
>>>>> * gdbarch.c: Regenerate.
>>>>> * gdbarch.h: Regenerate.
>>>>> * gdbarch.sh (gdbarch_insn_is_breakpoint): New method.
>>>>>
>>>>> Change-Id: I96eb27151442f435560a58c87eac48b0f68432bc
>>>>> ---
>>>>> gdb/aarch64-tdep.c | 25 +++++++++++++++++++++++++
>>>>> gdb/arch-utils.c | 7 +++++++
>>>>> gdb/arch-utils.h | 3 +++
>>>>> gdb/breakpoint.c | 19 ++++++++++++++-----
>>>>> gdb/gdbarch.c | 23 +++++++++++++++++++++++
>>>>> gdb/gdbarch.h | 7 +++++++
>>>>> gdb/gdbarch.sh | 4 ++++
>>>>> 7 files changed, 83 insertions(+), 5 deletions(-)
>>>>>
>>>> Do you have a test case for this? It could go in the gdb.arch/ directory.
>>>> It’d be fairly easy to check all the different brk patterns.
>>>
>>> I'll work on this. I'm thinking we could auto-generate a number of brk patterns and verify we can continue and hit each of them.
>>>
>>>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>>>> index 1d5fb2001d..c69361d4ea 100644
>>>>> --- a/gdb/aarch64-tdep.c
>>>>> +++ b/gdb/aarch64-tdep.c
>>>>> @@ -1201,6 +1201,28 @@ aarch64_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_byte op,
>>>>> return false;
>>>>> }
>>>>>
>>>>> +#define BRK_INSN_MASK 0xd4200000
>>>>> +
>>>>> +/* Implementation of gdbarch_insn_is_breakpoint for aarch64. */
>>>>> +
>>>>> +static bool
>>>>> +aarch64_insn_is_breakpoint (gdbarch *gdbarch,
>>>>> + const gdb_byte *insn,
>>>>> + unsigned int insn_size)
>>>>> +{
>>>>> + gdb_assert (insn != nullptr);
>>>>> +
>>>>> + uint32_t i;
>>>>> +
>>>>> + i = (uint32_t) extract_unsigned_integer (insn, insn_size,
>>>>> + gdbarch_byte_order (gdbarch));
>>>>> +
>>>>> + /* Check if INSN is a BRK instruction pattern. There are multiple choices
>>>>> + of such instructions with different immediate values. Different OS' may
>>>>> + use a different variation, but they have the same outcome. */
>>>>> + return (i & BRK_INSN_MASK) == BRK_INSN_MASK;
>>>>> +}
>>>>> +
>>>>> /* When arguments must be pushed onto the stack, they go on in reverse
>>>>> order. The code below implements a FILO (stack) to do this. */
>>>>>
>>>>> @@ -3357,6 +3379,9 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>>>> set_gdbarch_execute_dwarf_cfa_vendor_op (gdbarch,
>>>>> aarch64_execute_dwarf_cfa_vendor_op);
>>>>>
>>>>> + /* Permanent/Program breakpoint handling. */
>>>>> + set_gdbarch_insn_is_breakpoint (gdbarch, aarch64_insn_is_breakpoint);
>>>>> +
>>>>> /* Add some default predicates. */
>>>>> frame_unwind_append_unwinder (gdbarch, &aarch64_stub_unwind);
>>>>> dwarf2_append_unwinders (gdbarch);
>>>>> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
>>>>> index a1a003f91f..99c9f281be 100644
>>>>> --- a/gdb/arch-utils.c
>>>>> +++ b/gdb/arch-utils.c
>>>>> @@ -876,6 +876,13 @@ int default_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr)
>>>>> return 0;
>>>>> }
>>>> Add "/* See arch-utils.h. */" ...
>>>
>>> Fixed.
>>>
>>>>>
>>>>> +bool default_insn_is_breakpoint (struct gdbarch *gdbarch,
>>>>> + const gdb_byte *insn,
>>>>> + unsigned int insn_size)
>>>>> +{
>>>>> + return false;
>>>> I don’t like that this is just returning false, as it’s not really doing what the function name says.
>>>> How about if the function did this:
>>>> return (memcmp (target_mem, bpoint, len) == 0);
>>>> Then remove the memcmp from program_breakpoint_here_p.
>>>
>>> I agree it would be cleaner, but ...
>>>
>>>> You’ll probably have to move the call to gdbarch_breakpoint_from_pc into here too.
>>>
>>> ... this depends on calling gdbarch_breakpoint_from_pc to fetch bpoint, and gdbarch_breakpoint_from_pc requires the address information so it can determine the breakpoint kind.
>>>
>>> Passing in the address is a bit out of scope for what the function is supposed to do (verify if a particular instruction is a breakpoint).
>>>
>>> I don't have a strong objection towards passing in the address (or NULL if no address) if others are OK with it.
>> How about replacing program_breakpoint_here_p with
>> gdbarch_program_breakpoint_here_p(struct gdbarch *gdbarch, CORE_ADDR address) ?
>> default_program_breakpoint_here_p would be an exact copy of the existing program_breakpoint_here_p.
>> aarch64_program_breakpoint_here_p would do the copy to memory then the same as aarch64_insn_is_breakpoint.
>
> It sounds interesting at first, but i'm not sure about it. We'll save a memory comparison but will duplicate the code to gdbarch_breakpoint_from_pc to every arch that sets its own gdbarch_program_breakpoint_here_p.
>
> At least the old function dealt with just arch-specific bits, whereas the <arch>_program_breakpoint_here_p implementation will need to do a generic check + generic memory read as well, then do the arch-specific checks.
>
> Thoughts?
It should be rare that any other architecture needs to define their own
gdbarch_program_breakpoint_here_p function.
However, let's rewind a little. My concern was that in the existing code, the common
program_breakpoint_here_p is assuming that the target has a single breakpoint instruction,
which it obtains by calling gdbarch_breakpoint_from_pc. On AArch64, we have multiple patterns
to match. With your patch, program_breakpoint_here_p now checks both the single pattern and
multiple pattern cases, which feels wrong. Instead the common code should just ask the target
code “does this address contain a breakpoint?”
I suggested creating gdbarch_program_breakpoint_here_p as that would reduce the number of
functions called, which helps when trying to follow the code logic.
However, I’m happy for the target_read_memory to remain in common code, then pass that down
to the arch specific versions.
I’m not sure if anything else uses it, but maybe the call to set_gdbarch_breakpoint_kind_from_pc
in aarch64-tdep.c should probably be removed too.
Alan.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] [AArch64] Recognize more program breakpoint patterns
2019-12-23 17:34 [PATCH] [AArch64] Recognize more program breakpoint patterns Luis Machado
2020-01-09 14:52 ` Alan Hayward
2020-01-09 15:59 ` Aktemur, Tankut Baris
@ 2020-01-13 17:03 ` Luis Machado
2020-01-13 17:06 ` [PATCH 2/2] [AArch64] Test handling of additional brk instruction patterns Luis Machado
2020-01-13 17:25 ` [PATCH 1/2] [AArch64] Recognize more program breakpoint patterns Luis Machado
2 siblings, 2 replies; 10+ messages in thread
From: Luis Machado @ 2020-01-13 17:03 UTC (permalink / raw)
To: gdb-patches; +Cc: alan.hayward, tankut.baris.aktemur
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:
2020-01-13 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 and changed return type to bool.
* 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.
Change-Id: I96eb27151442f435560a58c87eac48b0f68432bc
Using gdbarch_program_breakpoint_here_p
---
gdb/aarch64-tdep.c | 39 +++++++++++++++++++++++++++++++++++++++
gdb/arch-utils.c | 37 +++++++++++++++++++++++++++++++++++++
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, 123 insertions(+), 46 deletions(-)
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index da41e22130..6b0ef210b1 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1201,6 +1201,41 @@ 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;
+
+ target_mem = (gdb_byte *) alloca (insn_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, 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 +3392,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 55b115c135..a7364531f9 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -876,6 +876,43 @@ 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;
+ 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 false;
+
+ 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 5b734abf1c..b6ac1f1e98 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 ab9bf1f5f4..277cbf20df 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 a0583a8e65..c45d8e1b46 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] [AArch64] Test handling of additional brk instruction patterns
2020-01-13 17:03 ` [PATCH 1/2] " Luis Machado
@ 2020-01-13 17:06 ` Luis Machado
2020-01-13 17:25 ` [PATCH 1/2] [AArch64] Recognize more program breakpoint patterns Luis Machado
1 sibling, 0 replies; 10+ messages in thread
From: Luis Machado @ 2020-01-13 17:06 UTC (permalink / raw)
To: gdb-patches; +Cc: alan.hayward, tankut.baris.aktemur
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-13 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 | 67 +++++++++++++++++++
2 files changed, 97 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..3532a0df95
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
@@ -0,0 +1,67 @@
+# 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
+}
+
+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"
+ }
+ -re "exited normally.*$gdb_prompt $" {
+ set keep_going 0
+ }
+ timeout {
+ fail $test
+ set keep_going 0
+ }
+ }
+ set count [expr "$count + 1"]
+}
+
+set timeout $old_timeout
--
2.17.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] [AArch64] Recognize more program breakpoint patterns
2020-01-13 17:03 ` [PATCH 1/2] " Luis Machado
2020-01-13 17:06 ` [PATCH 2/2] [AArch64] Test handling of additional brk instruction patterns Luis Machado
@ 2020-01-13 17:25 ` Luis Machado
1 sibling, 0 replies; 10+ messages in thread
From: Luis Machado @ 2020-01-13 17:25 UTC (permalink / raw)
To: gdb-patches; +Cc: alan.hayward, tankut.baris.aktemur
I failed to update the description... I'll send a new version.
On 1/13/20 2:03 PM, Luis Machado 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:
>
> 2020-01-13 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 and changed return type to bool.
> * 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.
>
> Change-Id: I96eb27151442f435560a58c87eac48b0f68432bc
>
> Using gdbarch_program_breakpoint_here_p
> ---
> gdb/aarch64-tdep.c | 39 +++++++++++++++++++++++++++++++++++++++
> gdb/arch-utils.c | 37 +++++++++++++++++++++++++++++++++++++
> 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, 123 insertions(+), 46 deletions(-)
>
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index da41e22130..6b0ef210b1 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -1201,6 +1201,41 @@ 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;
> +
> + target_mem = (gdb_byte *) alloca (insn_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, 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 +3392,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 55b115c135..a7364531f9 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -876,6 +876,43 @@ 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;
> + 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 false;
> +
> + 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 5b734abf1c..b6ac1f1e98 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 ab9bf1f5f4..277cbf20df 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 a0583a8e65..c45d8e1b46 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;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-01-13 17:06 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-23 17:34 [PATCH] [AArch64] Recognize more program breakpoint patterns Luis Machado
2020-01-09 14:52 ` Alan Hayward
2020-01-09 15:45 ` Luis Machado
2020-01-09 16:26 ` Alan Hayward
2020-01-10 14:58 ` Luis Machado
2020-01-10 16:53 ` Alan Hayward
2020-01-09 15:59 ` Aktemur, Tankut Baris
2020-01-13 17:03 ` [PATCH 1/2] " Luis Machado
2020-01-13 17:06 ` [PATCH 2/2] [AArch64] Test handling of additional brk instruction patterns Luis Machado
2020-01-13 17:25 ` [PATCH 1/2] [AArch64] Recognize more program breakpoint patterns Luis Machado
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox