From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16620 invoked by alias); 13 May 2013 07:08:40 -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 16594 invoked by uid 89); 13 May 2013 07:08:36 -0000 X-Spam-SWARE-Status: No, score=-4.7 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL autolearn=ham version=3.3.1 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Mon, 13 May 2013 07:08:35 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1Ubms3-0001pg-PK from Luis_Gustavo@mentor.com ; Mon, 13 May 2013 00:08:31 -0700 Received: from NA1-MAIL.mgc.mentorg.com ([147.34.98.181]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Mon, 13 May 2013 00:08:32 -0700 Received: from [172.30.9.163] ([172.30.9.163]) by NA1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.3959); Mon, 13 May 2013 00:08:29 -0700 Message-ID: <51909160.2070302@codesourcery.com> Date: Mon, 13 May 2013 07:08:00 -0000 From: Luis Machado Reply-To: lgustavo@codesourcery.com User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: Edjunior Barbosa Machado CC: gdb-patches@sourceware.org, Ulrich Weigand Subject: Re: [PATCH] Fix hardware watchpoints on PowerPC servers References: <1368426484-32623-1-git-send-email-emachado@linux.vnet.ibm.com> In-Reply-To: <1368426484-32623-1-git-send-email-emachado@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-05/txt/msg00423.txt.bz2 Hi, As a general thought, the generic ptrace interface for powerpc hardware debugging resources was limited to the BOOK E processors. Since it is no longer the case, using the BOOK E naming throughout the code looks confusing now. On 05/13/2013 08:28 AM, Edjunior Barbosa Machado wrote: > gdb/ChangeLog > 2013-05-12 Edjunior Machado > > * ppc-linux-nat.c (ppc_linux_region_ok_for_hw_watchpoint): Check if the > region is ok for a hardware watchpoint using the new ptrace interface > on Power servers. > > --- > gdb/ppc-linux-nat.c | 19 +++++++++++-------- > 1 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c > index 280dcbe..1ff00a6 100644 > --- a/gdb/ppc-linux-nat.c > +++ b/gdb/ppc-linux-nat.c > @@ -1503,16 +1503,19 @@ ppc_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len) > to determine the hardcoded watchable region for watchpoints. */ > if (have_ptrace_booke_interface ()) > { > - /* DAC-based processors (i.e., embedded processors), like the PowerPC 440 > - have ranged watchpoints and can watch any access within an arbitrary > - memory region. This is useful to watch arrays and structs, for > - instance. It takes two hardware watchpoints though. */ > + /* Embedded DAC-based processors, like the PowerPC 440 have ranged > + watchpoints and can watch any access within an arbitrary memory > + region. This is useful to watch arrays and structs, for instance. It > + takes two hardware watchpoints though. */ Any special reason this comment was tweaked? It does not seem to add more substantial information. > if (len > 1 > - && booke_debug_info.features & PPC_DEBUG_FEATURE_DATA_BP_RANGE) > + && booke_debug_info.features & PPC_DEBUG_FEATURE_DATA_BP_RANGE > + && ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE) This bit, though correct, looks confusing now. We are dealing with a structure named booke_debug_info, but we are checking "ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE" to make sure we are really dealing with a BOOK-E processor now. I think people will eventually scratch their heads when they get to this point. We should probably rename the structure to something more generic now that this is no longer BOOK E-specific and make it clear that we are dealing with either BOOK E or BOOK S processors (maybe even explicitly mentioning IBM's POWER processors). Are we also handling 64-bit DABR-based PowerPC processors like the 970? > return 2; > - else if (booke_debug_info.data_bp_alignment > - && (addr + len > (addr & ~(booke_debug_info.data_bp_alignment - 1)) > - + booke_debug_info.data_bp_alignment)) > + /* Server processors provide one hardware watchpoint and addr+len should > + fall in the watchable region provided by the ptrace interface. */ > + if (booke_debug_info.data_bp_alignment > + && (addr + len > (addr & ~(booke_debug_info.data_bp_alignment - 1)) > + + booke_debug_info.data_bp_alignment)) Similarly, we're dealing with a server processor in this chunk, but it is not clear due to the naming. While going through this code, I wonder if we should extract these alignment checks and put them inside functions with more meaningful names. As is, they can get confusing. It doesn't need to be in this patch though. Thanks! Luis