From patchwork Thu Oct 15 08:14:48 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yao Qi X-Patchwork-Id: 9159 Received: (qmail 23989 invoked by alias); 15 Oct 2015 08:14:58 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 22212 invoked by uid 89); 15 Oct 2015 08:14:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f49.google.com Received: from mail-pa0-f49.google.com (HELO mail-pa0-f49.google.com) (209.85.220.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 15 Oct 2015 08:14:54 +0000 Received: by payp3 with SMTP id p3so32898049pay.1 for ; Thu, 15 Oct 2015 01:14:53 -0700 (PDT) X-Received: by 10.66.132.37 with SMTP id or5mr8549687pab.5.1444896893097; Thu, 15 Oct 2015 01:14:53 -0700 (PDT) Received: from E107787-LIN (gcc2-power8.osuosl.org. [140.211.9.43]) by smtp.gmail.com with ESMTPSA id iu2sm11414566pbd.16.2015.10.15.01.14.50 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Thu, 15 Oct 2015 01:14:52 -0700 (PDT) From: Yao Qi To: Pedro Alves Cc: Yao Qi , gdb-patches@sourceware.org Subject: Re: [PATCH] aarch64 multi-arch part 6: HW breakpoint on unaligned address In-Reply-To: <561D4008.90009@redhat.com> (Pedro Alves's message of "Tue, 13 Oct 2015 18:31:52 +0100") References: <1444731060-16237-1-git-send-email-yao.qi@linaro.org> <561CE5D2.8030505@redhat.com> <861tcy6b84.fsf@gmail.com> <561D4008.90009@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) Date: Thu, 15 Oct 2015 09:14:48 +0100 Message-ID: <86fv1c4kg7.fsf@gmail.com> MIME-Version: 1.0 X-IsSubscribed: yes Pedro Alves writes: >>>> aarch64_point_is_aligned (int is_watchpoint, CORE_ADDR addr, int len) >>>> { >>>> - unsigned int alignment = is_watchpoint ? AARCH64_HWP_ALIGNMENT >>>> - : AARCH64_HBP_ALIGNMENT; >>>> + unsigned int alignment = 0; >>>> + >>>> + if (is_watchpoint) >>>> + alignment = AARCH64_HWP_ALIGNMENT; >>>> + else >>>> + { >>>> + /* Set alignment to 2 only if the current process is 32-bit, >>>> + since thumb instruction can be 2-byte aligned. Otherwise, set >>>> + alignment to AARCH64_HBP_ALIGNMENT. */ >>>> + alignment = 2; >>> >>> Is some other code doing what the comment says? I'm not seeing >>> any obvious 32-bit check. >> >> No, I don't do the 32-bit check here. Ideally, we should set alignment >> to 2 only when the process is 32-bit, and still use 4 as alignment >> otherwise. However, I don't find an easy way to do the 32-bit check >> here, because this code is used by both GDB and GDBserver. We can do >> the 32-bit check in GDB and GDBserver respectively, and pass the result >> to nat/aarch64-linux-hw-point.c, but I don't like putting information down >> multiple levels like this. > > At least the comment should be updated. It's quite misleading as is. In order to do 32-bit check in nat/aarch64-linux-hw-point.c, I add a new regcache interface regcache_register_size which is defined in both GDB and GDBserver. It has two arguments, regcache and number, which looks more reasonable than register_size, IMO. With regcache_register_size in place, we can check 32-bit like this, struct regcache *regcache = get_thread_regcache_for_ptid (current_lwp_ptid ()); /* Set alignment to 2 only if the current process is 32-bit, since thumb instruction can be 2-byte aligned. Otherwise, set alignment to AARCH64_HBP_ALIGNMENT. */ if (regcache_register_size (regcache, 0) == 8) alignment = AARCH64_HBP_ALIGNMENT; else alignment = 2; on the other hand, a lot of register_size calls in GDB and GDBserver can be replaced by regcache_register_size. This can be done separately. Here is the patch V2, regression tested on aarch64-linux. diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c index c9f439f..4d3d55a 100644 --- a/gdb/aarch64-linux-nat.c +++ b/gdb/aarch64-linux-nat.c @@ -608,11 +608,13 @@ aarch64_linux_insert_hw_breakpoint (struct target_ops *self, { int ret; CORE_ADDR addr = bp_tgt->placed_address = bp_tgt->reqstd_address; - const int len = 4; + int len; const enum target_hw_bp_type type = hw_execute; struct aarch64_debug_reg_state *state = aarch64_get_debug_reg_state (ptid_get_pid (inferior_ptid)); + gdbarch_breakpoint_from_pc (gdbarch, &addr, &len); + if (show_debug_regs) fprintf_unfiltered (gdb_stdlog, @@ -640,11 +642,13 @@ aarch64_linux_remove_hw_breakpoint (struct target_ops *self, { int ret; CORE_ADDR addr = bp_tgt->placed_address; - const int len = 4; + int len = 4; const enum target_hw_bp_type type = hw_execute; struct aarch64_debug_reg_state *state = aarch64_get_debug_reg_state (ptid_get_pid (inferior_ptid)); + gdbarch_breakpoint_from_pc (gdbarch, &addr, &len); + if (show_debug_regs) fprintf_unfiltered (gdb_stdlog, "remove_hw_breakpoint on entry (addr=0x%08lx, len=%d))\n", diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h index a922316..c470603 100644 --- a/gdb/common/common-regcache.h +++ b/gdb/common/common-regcache.h @@ -28,6 +28,11 @@ extern struct regcache *get_thread_regcache_for_ptid (ptid_t ptid); +/* Return the size of register numbered N in REGCACHE. This function + must be provided by the client. */ + +extern int regcache_register_size (const struct regcache *regcache, int n); + /* Read the PC register. This function must be provided by the client. */ diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c index 9cefdda..780c5e3 100644 --- a/gdb/gdbserver/linux-aarch64-low.c +++ b/gdb/gdbserver/linux-aarch64-low.c @@ -315,9 +315,17 @@ aarch64_insert_point (enum raw_bkpt_type type, CORE_ADDR addr, ret = -1; } else - ret = - aarch64_handle_breakpoint (targ_type, addr, len, 1 /* is_insert */, - state); + { + if (len == 3) + { + /* LEN is 3 means the breakpoint is set on a 32-bit thumb + instruction. Set it to 2 to correctly encode length bit + mask in hardware/watchpoint control register. */ + len = 2; + } + ret = aarch64_handle_breakpoint (targ_type, addr, len, + 1 /* is_insert */, state); + } if (show_debug_regs) aarch64_show_debug_reg_state (state, "insert_point", addr, len, @@ -353,9 +361,17 @@ aarch64_remove_point (enum raw_bkpt_type type, CORE_ADDR addr, aarch64_handle_watchpoint (targ_type, addr, len, 0 /* is_insert */, state); else - ret = - aarch64_handle_breakpoint (targ_type, addr, len, 0 /* is_insert */, - state); + { + if (len == 3) + { + /* LEN is 3 means the breakpoint is set on a 32-bit thumb + instruction. Set it to 2 to correctly encode length bit + mask in hardware/watchpoint control register. */ + len = 2; + } + ret = aarch64_handle_breakpoint (targ_type, addr, len, + 0 /* is_insert */, state); + } if (show_debug_regs) aarch64_show_debug_reg_state (state, "remove_point", addr, len, diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c index f79063c..e11b173 100644 --- a/gdb/gdbserver/regcache.c +++ b/gdb/gdbserver/regcache.c @@ -316,6 +316,14 @@ register_size (const struct target_desc *tdesc, int n) return tdesc->reg_defs[n].size / 8; } +/* See common/common-regcache.h. */ + +int +regcache_register_size (const struct regcache *regcache, int n) +{ + return register_size (regcache->tdesc, n); +} + static unsigned char * register_data (struct regcache *regcache, int n, int fetch) { diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c index bca6ec1..1a5fa6a 100644 --- a/gdb/nat/aarch64-linux-hw-point.c +++ b/gdb/nat/aarch64-linux-hw-point.c @@ -18,6 +18,7 @@ #include "common-defs.h" #include "break-common.h" +#include "common-regcache.h" #include "nat/linux-nat.h" #include "aarch64-linux-hw-point.h" @@ -112,8 +113,23 @@ aarch64_point_encode_ctrl_reg (enum target_hw_bp_type type, int len) static int aarch64_point_is_aligned (int is_watchpoint, CORE_ADDR addr, int len) { - unsigned int alignment = is_watchpoint ? AARCH64_HWP_ALIGNMENT - : AARCH64_HBP_ALIGNMENT; + unsigned int alignment = 0; + + if (is_watchpoint) + alignment = AARCH64_HWP_ALIGNMENT; + else + { + struct regcache *regcache + = get_thread_regcache_for_ptid (current_lwp_ptid ()); + + /* Set alignment to 2 only if the current process is 32-bit, + since thumb instruction can be 2-byte aligned. Otherwise, set + alignment to AARCH64_HBP_ALIGNMENT. */ + if (regcache_register_size (regcache, 0) == 8) + alignment = AARCH64_HBP_ALIGNMENT; + else + alignment = 2; + } if (addr & (alignment - 1)) return 0; @@ -445,7 +461,7 @@ aarch64_handle_breakpoint (enum target_hw_bp_type type, CORE_ADDR addr, struct aarch64_debug_reg_state *state) { /* The hardware breakpoint on AArch64 should always be 4-byte - aligned. */ + aligned, but on AArch32, it can be 2-byte aligned. */ if (!aarch64_point_is_aligned (0 /* is_watchpoint */ , addr, len)) return -1; diff --git a/gdb/regcache.c b/gdb/regcache.c index 74d883a..5ee31fb 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -179,6 +179,14 @@ register_size (struct gdbarch *gdbarch, int regnum) return size; } +/* See common/common-regcache.h. */ + +int +regcache_register_size (const struct regcache *regcache, int n) +{ + return register_size (get_regcache_arch (regcache), n); +} + /* The register cache for storing raw register values. */ struct regcache