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

[Luis Machado <luis.machado@linaro.org>]

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.