From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21840 invoked by alias); 4 Dec 2004 13:36:30 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 21691 invoked from network); 4 Dec 2004 13:36:11 -0000 Received: from unknown (HELO sibelius.xs4all.nl) (82.92.89.47) by sourceware.org with SMTP; 4 Dec 2004 13:36:11 -0000 Received: from elgar.sibelius.xs4all.nl (elgar.sibelius.xs4all.nl [192.168.0.2]) by sibelius.xs4all.nl (8.13.0/8.13.0) with ESMTP id iB4Da77O021899; Sat, 4 Dec 2004 14:36:08 +0100 (CET) Received: from elgar.sibelius.xs4all.nl (localhost [127.0.0.1]) by elgar.sibelius.xs4all.nl (8.12.6p3/8.12.6) with ESMTP id iB4Da7tI000852; Sat, 4 Dec 2004 14:36:07 +0100 (CET) (envelope-from kettenis@elgar.sibelius.xs4all.nl) Received: (from kettenis@localhost) by elgar.sibelius.xs4all.nl (8.12.6p3/8.12.6/Submit) id iB4Da3RB000849; Sat, 4 Dec 2004 14:36:03 +0100 (CET) Date: Sat, 04 Dec 2004 14:07:00 -0000 Message-Id: <200412041336.iB4Da3RB000849@elgar.sibelius.xs4all.nl> From: Mark Kettenis To: eliz@gnu.org CC: gdb-patches@sources.redhat.com In-reply-to: <01c4d9fe$Blat.v2.2.2$30abc2e0@zahav.net.il> (eliz@gnu.org) Subject: Re: [COMMIT] Hardware watchpoints for new inf-ttrace.c module References: <200412032224.iB3MOqUr021181@elgar.sibelius.xs4all.nl> <01c4d9fe$Blat.v2.2.2$30abc2e0@zahav.net.il> X-SW-Source: 2004-12/txt/msg00108.txt.bz2 Date: Sat, 04 Dec 2004 14:37:40 +0200 From: "Eli Zaretskii" > Date: Fri, 3 Dec 2004 23:24:52 +0100 (CET) > From: Mark Kettenis > > Virtually all of the HP-UX-specific handling is moved within this > module, so I expect that whenever we switch, quite a few ugly hacks > can go. It would be good to have a short description of this machinery in gdbint.texinfo. Right now, it describes only the x86 way of implementing hardware-assisted watchpoints. I can give that a try. In principle, the HP-UX way of implementing watchpoints is pretty generic, and could work on any system that allows GDB to fiddle with memory page protetections. As such, I think this would indeed be good information to have in the internals manual. (Note that this code is just a clean re-implementation of code that's already present in infttrace.c.) > +/* Add the page at address ADDR for process PID to the dictionary. */ > [...] > +/* Insert the page at address ADDR of process PID to the dictionary. */ > + > +static void > +inf_ttrace_insert_page (pid_t pid, CORE_ADDR addr) > +{ > + struct inf_ttrace_page *page; > + > + page = inf_ttrace_get_page (pid, addr); > + if (!page) > + page = inf_ttrace_add_page (pid, addr); > + > + page->refcount++; > +} Hmmm... wouldn't it be better to have just one function that adds an address's page to the dictionary? Or do you see a situation where a call to inf_ttrace_add_page will not be immediately followed by incrementing page->refcount? I generally find it undesirable to have two or more functions whose names and purpose comments are synonyms ("add page" and "insert page"). It is confusing for a programmer who needs to use the functionality, and usually forces to read the code to understand how to DTRT. Yes, it is somewhat confusing, although I don't really see how I can avoid having two functions without duplicating code. Perhaps I can rename the functions or tweak the comments a bit to make this clearer. Thanks for the heads-up. > +static int > +inf_ttrace_can_use_hw_breakpoint (int type, int len, int ot) > +{ > + return (type == bp_hardware_watchpoint); > +} I was going to ask why not try to support rwatch and awatch, but then I realized that you cannot implement target_stopped_data_address, and that in turn made it clear that gdbint.texinfo is inaccurate when it describes the watchpoint-related primitives. I will fix the manual shortly. Didn't realize that. I might be able to implement target_stopped_data_address though, although there are some 32x64-bit cross-debugging issues here. I haven't really looked into it yet though, since my primary goal is to implement everything that the current HP-UX native GDB supports. > +static int > +inf_ttrace_region_size_ok_for_hw_watchpoint (int len) > +{ > + return 1; > +} Hmmm... I have a minor issue with this. Inserting a watchpoint might mean that you need to add a page to the dictionary, which in turn could fail because there's not enough memory available to GDB. You add a page by using xmalloc, so if there isn't enough memory, GDB will exit. Isn't it better to fail the watchpoint insertion in that case rather than abort the entire session? I realize that inf_ttrace_region_size_ok_for_hw_watchpoint is probably not the place where such a situation should be handled, but perhaps inf_ttrace_add_page or inf_ttrace_insert_watchpoint should be modified to do that? Don't think this is worth it. The generic breakpoint machinery needs bits of memory too. It's probably just as likely that it will fail as well. Anyway, thanks for the comments. Mark