From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17976 invoked by alias); 13 May 2013 14:16:15 -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 17967 invoked by uid 89); 13 May 2013 14:16:14 -0000 X-Spam-SWARE-Status: No, score=-5.4 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RP_MATCHES_RCVD autolearn=ham version=3.3.1 Received: from e24smtp05.br.ibm.com (HELO e24smtp05.br.ibm.com) (32.104.18.26) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Mon, 13 May 2013 14:16:14 +0000 Received: from /spool/local by e24smtp05.br.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 13 May 2013 11:16:10 -0300 Received: from d24dlp01.br.ibm.com (9.18.248.204) by e24smtp05.br.ibm.com (10.172.0.141) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Mon, 13 May 2013 11:16:09 -0300 Received: from d24relay02.br.ibm.com (d24relay02.br.ibm.com [9.13.184.26]) by d24dlp01.br.ibm.com (Postfix) with ESMTP id DE545352005F for ; Mon, 13 May 2013 10:16:08 -0400 (EDT) Received: from d24av02.br.ibm.com (d24av02.br.ibm.com [9.8.31.93]) by d24relay02.br.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r4DEF1sm24903908 for ; Mon, 13 May 2013 11:15:01 -0300 Received: from d24av02.br.ibm.com (loopback [127.0.0.1]) by d24av02.br.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r4DEG8FT004495 for ; Mon, 13 May 2013 11:16:08 -0300 Received: from [9.8.0.182] ([9.8.0.182]) by d24av02.br.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id r4DEG73D003450; Mon, 13 May 2013 11:16:07 -0300 Message-ID: <5190F585.2060105@linux.vnet.ibm.com> Date: Mon, 13 May 2013 14:16:00 -0000 From: Edjunior Barbosa Machado User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: lgustavo@codesourcery.com 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> <51909160.2070302@codesourcery.com> In-Reply-To: <51909160.2070302@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-TM-AS-MML: No X-Content-Scanned: Fidelis XPS MAILER x-cbid: 13051314-2362-0000-0000-00000A92E218 X-SW-Source: 2013-05/txt/msg00434.txt.bz2 On 05/13/2013 04:08 AM, Luis Machado wrote: > 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. I tried to highlight that the if() below was focused on embedded processors ("ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE"). >> 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? Yes, this new ptrace interface is now a common interface that deals with DAC and DABR-based processors. >> 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. Agree with you. I believe all these functions and structures prefixed by booke_* are quite confusing now, and they might be renamed to something more significant, since this interface is no longer related only to embedded processors now. I'll work on a separate patch for this. Thank you for the review, Luis. -- Edjunior