[PATCH] [AArch64] Recognize more program breakpoint patterns
Luis Machado
luis.machado@linaro.org
Fri Jan 10 14:58:00 GMT 2020
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?
More information about the Gdb-patches
mailing list