From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21895 invoked by alias); 29 Dec 2009 06:12:55 -0000 Received: (qmail 21871 invoked by uid 22791); 29 Dec 2009 06:12:51 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 29 Dec 2009 06:12:45 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 259B52BAB8B; Tue, 29 Dec 2009 01:12:43 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id ntx+BkW7KJcu; Tue, 29 Dec 2009 01:12:43 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id DD6BF2BAB4E; Tue, 29 Dec 2009 01:12:40 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 6837DF5937; Tue, 29 Dec 2009 07:11:41 +0100 (CET) Date: Tue, 29 Dec 2009 06:12:00 -0000 From: Joel Brobecker To: Thiago Jung Bauermann Cc: gdb-patches@sourceware.org, Luis Machado , Matt Tyrlik Subject: Re: [PATCH 1/4] Support the new BookE ptrace interface Message-ID: <20091229061141.GA2788@adacore.com> References: <200912232230.56227.bauerman@br.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200912232230.56227.bauerman@br.ibm.com> User-Agent: Mutt/1.5.20 (2009-06-14) 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 X-SW-Source: 2009-12/txt/msg00413.txt.bz2 > 2009-12-23 Sergio Durigan Junior > Thiago Jung Bauermann > > * ppc-linux-nat.c (PTRACE_GET_DEBUGREG): Update comment. > (PPC_PTRACE_GETWDBGINFO, PPC_PTRACE_SETHWDEBUG, PPC_PTRACE_DELHWDEBUG, > ppc_debug_info, PPC_DEBUG_FEATURE_INSN_BP_RANGE, > PPC_DEBUG_FEATURE_INSN_BP_MASK, PPC_DEBUG_FEATURE_DATA_BP_RANGE, > PPC_DEBUG_FEATURE_DATA_BP_MASK, ppc_hw_breakpoint, > PPC_BREAKPOINT_TRIGGER_EXECUTE, PPC_BREAKPOINT_TRIGGER READ, > PPC_BREAKPOINT_TRIGGER_WRITE, PPC_BREAKPOINT_TRIGGER_RW, > PPC_BREAKPOINT_MODE_EXACT PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE, > PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE, PPC_BREAKPOINT_MODE_MASK, > PPC_BREAKPOINT_CONDITION_NONE, PPC_BREAKPOINT_CONDITION_AND, > PPC_BREAKPOINT_CONDITION_EXACT, PPC_BREAKPOINT_CONDITION_OR, > PPC_BREAKPOINT_CONDITION_AND_OR, PPC_BREAKPOINT_CONDITION_BE_ALL, > PPC_BREAKPOINT_CONDITION_BE_SHIFT, PPC_BREAKPOINT_CONDITION_BE): > Define, in case doesn't provide it. > (have_ptrace_new_debug_booke): New flag. > (ppc_linux_check_watch_resources): Renamed to ... > (ppc_linux_can_use_hw_breakpoint): ... this. Handle BookE processors. > (booke_debug_info): New variable. > (max_slots_number): Ditto. > (struct hw_break_tuple): New struct. > (struct thread_points): Ditto. > (ppc_threads): New variable. > (PPC_DEBUG_CURRENT_VERSION): New define. > (ppc_linux_region_ok_for_hw_watchpoint): Handle BookE processors. > (booke_cmp_hw_point): New function. > (booke_find_thread_points_by_tid): Ditto. > (booke_insert_point): Ditto. > (booke_remove_point): Ditto. > (ppc_linux_insert_hw_breakpoint): Ditto. > (ppc_linux_remove_hw_breakpoint): Ditto. > (HW_WATCH_RW_TRIGGER): New define. > (ppc_linux_insert_watchpoint): Handle BookE processors. > (ppc_linux_remove_watchpoint): Ditto. > (ppc_linux_new_thread): Ditto. > (ppc_linux_stopped_data_address): Ditto. > (ppc_linux_watchpoint_addr_within_range): Ditto. > with and without the new kernel interface. > (ppc_linux_read_description): Query the target to know if it > supports the new kernel BookE interface. > (_initialize_ppc_linux_nat): Initialize to_insert_hw_breakpoint and > to_remove_hw_breakpoint fields of the target operations struct. Overall, this looks OK to me. A few general, non-blocking comments: (1) That's a lot of definitions to add at the beginning of the file, and I was going to suggest that we move them to a separate include. But I see that we've already started doing that with other definitions, so this is fine. (2) Again, non-blocking, but I'm left wondering if the "old" case (when we don't have the new interface available), could be made a special case of the new one. For instance, you maintain two sets of globals, one used for booke, and one used when non-booke. Would it be possible to maintain just one set that works for booke, and then infer the non-booke ones if necessary? Or said another way, in the non-booke case, we can still maintain the same data structures as in the booke case, and we can then convert that data to whatever we need in the non-booke case. The only time I think this would be needed is during insertion/deletion. I think that this might simplify a bit some of the code, avoid maintaining two exclusive sets of globals, and this might also help with the transition away from the "old" interface when we decide not to support it anymore. Has Ulrich had a chance to look at these patches? As I said, they look OK to me, and are pre-approved pending the little nits I mentioned below. But this is a rather large change, and it would be nice if someone more knowledgeable with the ppc than me also had a look too. The comments above can be discussed as followup patches if you like. > +/* > + * features will have bits indication whether there is support for: > + */ Small nit: We avoid the star ('*') at the beginning of the line (and the sentence should start with a capital letter): /* Features will have bits indication whether there is support for: */ The sentence itself does not sound too English to me, but I'm OK with it anyways. I think we get the idea :). (there are a few more instances of the extra stars in that same section). > +struct hw_break_tuple { > + long slot; > + struct ppc_hw_breakpoint *hw_break; > +}; Another small formatting nit: struct hw_break_tuple { long slot; struct ppc_hw_breakpoint *hw_break; }; > + if (!have_ptrace_new_debug_booke) > + { > + ptid_t ptid; > + int tid; > + > + /* We need to know whether ptrace supports PTRACE_SET_DEBUGREG and whether > + the target has DABR. If either answer is no, the ptrace call will > + return -1. Fail in that case. */ > + tid = TIDGET (ptid); > + if (tid == 0) > + tid = PIDGET (ptid); > + > + if (ptrace (PTRACE_SET_DEBUGREG, tid, 0, 0) == -1) > + return 0; I'm ok with the implementation as is, but I'm wondering if this is the type of data that we'd want to cache. Maybe this makes things more complicated than it is worth? > +/* This function can be used to retrieve a thread_points by > + the TID of the related process/thread. If nothing has been > + found, it returns NULL. */ Can you document the behavior when ALLOC_NEW is non-zero if nothing has been found? > + if ((slot = ptrace (PPC_PTRACE_SETHWDEBUG, tid, 0, p)) < 0) Hmmm, Let's avoid assignments inside conditions. It's harder to read, IMO. slot = ptrace (PPC_PTRACE_SETHWDEBUG, tid, 0, p); if (slot < 0) > + perror_with_name (_("\n\ > +Please review your watchpoints/breakpoints and make sure the mask, ranges \n\ > +and addresses are within acceptable limits.")); Is this something that we could catch earlier, when the *point is created by the user? > + perror_with_name (_("\n\ > +Could not delete the breakpoint/watchpoint.")); All the other calls to perror_with_name do not insert a newline at the beginning of the error string. The previous one was rather long, so I thought it made sense. But this one, it seems more consistent to not have it. WDYT? > + struct ppc_hw_breakpoint p; > + > + p.version = PPC_DEBUG_CURRENT_VERSION; > + p.trigger_type = PPC_BREAKPOINT_TRIGGER_EXECUTE; > + p.addr_mode = PPC_BREAKPOINT_MODE_EXACT; > + p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE; > + p.addr = (uint64_t) bp_tgt->placed_address; > + p.addr2 = 0; > + p.condition_value = 0; > + > + booke_remove_point (&p, TIDGET (ptid)); It seemed a bit strange that you're using the entire ppc_hw_breakpoint as a key to your hw_breaks_structure. But thinking more about it, I don't know if you have many options or not. At first, I thought it would be a simple matter of remembering the slot where the point was inserted, but then you'd need a map between the breakpoint and the slot. The current interface is too awkward for that (we only get passed the bp_target_info). I have the same problem on VxWorks, where created a map to be able to store some target-specific private date alongside each breakpoint. Oh well... > +#define HW_WATCH_RW_TRIGGER(t, rw) \ > + if (rw == hw_read) \ > + t = PPC_BREAKPOINT_TRIGGER_READ; \ > + else if (rw == hw_write) \ > + t = PPC_BREAKPOINT_TRIGGER_WRITE; \ > + else \ > + t = PPC_BREAKPOINT_TRIGGER_READ | PPC_BREAKPOINT_TRIGGER_WRITE; Can we use a function instead. I don't mind macros when it's a litteral constant, or a simple arithmetic expression, but this is really complex enough that a function would be better (particularly during debugging). > + if (have_ptrace_new_debug_booke) > + { > + if (len <= 4) > + ALL_LWPS (lp, ptid) What happens if len > 4? Is that even possible? Right now, it looks like we're returning zero as if the watchpoint has been inserted, even though it hasn't... Same for ppc_linux_remove_watchpoint. -- Joel