From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15092 invoked by alias); 10 Jan 2020 14:58:55 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 15084 invoked by uid 89); 10 Jan 2020 14:58:55 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=Recognize, arch-specific, archspecific, thinks X-HELO: mail-qt1-f193.google.com Received: from mail-qt1-f193.google.com (HELO mail-qt1-f193.google.com) (209.85.160.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 10 Jan 2020 14:58:53 +0000 Received: by mail-qt1-f193.google.com with SMTP id n15so2130198qtp.5 for ; Fri, 10 Jan 2020 06:58:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=tH30VWjkUKz1FW5/Nbv1FSXEHoS2BTEbtULFsMNjFgw=; b=ULP/a/uYCKxm8kwkcv5ktENogQLLt3fgvj3Tc4xp3kYDytTaVNZA5/Qpkz7xnW0C+i IrBSl3/f7Tsf5LLGdFnEBx2ADxd+ZMHqt3CPArMSxubpJ/jyksDqYIpeNRbSF9bmbZwY ylj+bg1RwljvRqp9zAt1ftn5a7RHDHAlBlZT3x7CKM4AwnuYcYcOo8xFDJ2epgfYEnJi a4OfLwB3MfkLuDTgUN6LnO6ajLspBbWwq6kN8Jq8J5bnUiZ2zcuMCrQeM8GqSBV1DRsz 5HvGG59pBox0HnbB83ua8XyrHcjEnv6VYRlivXHX7FjlQJeTUWETBaHbWZvePGAckUAq egEw== Return-Path: Received: from [192.168.0.185] ([191.249.239.248]) by smtp.gmail.com with ESMTPSA id f19sm956802qkk.69.2020.01.10.06.58.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 10 Jan 2020 06:58:41 -0800 (PST) Subject: Re: [PATCH] [AArch64] Recognize more program breakpoint patterns To: Alan Hayward Cc: "gdb-patches\\@sourceware.org" , nd References: <20191223173432.16955-1-luis.machado@linaro.org> <690B08E1-F6A3-42B3-A788-4101D7A1F04D@arm.com> <9f11b4d9-f851-2900-82d3-2ff3244a8f81@linaro.org> From: Luis Machado Message-ID: Date: Fri, 10 Jan 2020 14:58:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2020-01/txt/msg00253.txt.bz2 On 1/9/20 1:25 PM, Alan Hayward wrote: > > >> On 9 Jan 2020, at 15:45, Luis Machado wrote: >> >> On 1/9/20 11:52 AM, Alan Hayward wrote: >>>> On 23 Dec 2019, at 17:34, 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 : 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 : 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 : 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 >>>> >>>> * 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 _program_breakpoint_here_p implementation will need to do a generic check + generic memory read as well, then do the arch-specific checks. Thoughts?