From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16071 invoked by alias); 3 May 2011 06:05:57 -0000 Received: (qmail 16059 invoked by uid 22791); 3 May 2011 06:05:55 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from fencepost.gnu.org (HELO fencepost.gnu.org) (140.186.70.10) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 03 May 2011 06:05:41 +0000 Received: from eliz by fencepost.gnu.org with local (Exim 4.71) (envelope-from ) id 1QH8jn-0001Wm-64; Tue, 03 May 2011 02:05:35 -0400 Date: Tue, 03 May 2011 06:05:00 -0000 Message-Id: From: Eli Zaretskii To: Thiago Jung Bauermann CC: uweigand@de.ibm.com, gdb-patches@sourceware.org In-reply-to: <1304398546.2245.80.camel@hactar> (message from Thiago Jung Bauermann on Tue, 03 May 2011 01:55:46 -0300) Subject: Re: [RFA 2/3] Demote to sw watchpoint only in update_watchpoint Reply-to: Eli Zaretskii References: <201104291726.p3THQVaC029608@d06av02.portsmouth.uk.ibm.com> <1304398546.2245.80.camel@hactar> X-IsSubscribed: yes 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: 2011-05/txt/msg00045.txt.bz2 > From: Thiago Jung Bauermann > Cc: gdb-patches ml > Date: Tue, 03 May 2011 01:55:46 -0300 > > I hardcoded bp_hardware_watchpoint in an attempt to make read and access > watchpoints also count as hardware watchpoints when determining the > number of used and available debug resources (otherwise only watchpoints > of the same type as the one being created are taken into account when > counting how many debug resources are used). > > I just realised it doesn't work, and to fix it I'd have to slightly > change the meaning of the other_type_used argument to be the number of > other types of watchpoints, instead of just one or zero indicated > whether there is any watchpoint of another type being used. > > I didn't make the change because I don't know if there's any target > which would break. I couldn't find anywhere what other_type_used > actually means... > > On the other hand, the current code is already broken in that regard, > and creating several read and access watchpoints on today's GDB will > confuse it and allow creating more hardware watchpoints (read, access or > regular) than possible. Thus, this patch doesn't make the situation any > worse. So now I just use b->type. Sorry, but I object. This kind of changes just adds kludges upon kludges on what is a fundamentally broken design to begin with. It was originally designed and implemented for a single platform (which we no longer support, btw), and doesn't scale well, or not at all, to other platforms, not even to x86. The problem with the meaning of the other_type_used argument is the telltale sign: it only made sense for that single platform for which hardware watchpoints were originally implemented in GDB. It no longer makes any sense with today's platforms, and actually cannot be used at all with most of them (at least those I'm familiar with), for a very simple reason: a single `int' parameter doesn't provide a way to report enough information about the consumption of related resources. It doesn't matter if that parameter is a boolean or not; it simply isn't up to this job. Much more information is needed about the related resources to decide whether another watchpoint can or cannot be set. The fundamental design problem here is this: there's no way GDB application level can know whether the target can accommodate an additional hardware watchpoint of a given type and attributes. Only the target itself can tell that accurately, because the information needed to answer that question is extremely target-dependent. OTOH, it is bad UI design to let the user set a hardware watchpoint, then demote it to a software watchpoint (or even refuse to insert a watchpoint at all) at "resume" time. So on the one hand, we _want_ the GDB application level to know if a hardware watchpoint is possible, to tell the user it cannot, and OTOH there's a problem knowing this at that level. We should resolve this conflict in the most direct way: introduce a method, to be implemented by each target, that will answer these questions. It should accept the exact spec of the new watchpoint, and it should have access to the watchpoints and breakpoints already set, including their full specs. With that information in hand, a target can reliably produce a definitive response, at least in the vast majority of cases, when the corresponding resources are under GDB control. To avoid breaking too many targets, the default implementation should be what we do now. This will make the result no worse than the current code. Let's solve this problem once and for all. Any other way of treating this problem will only sweep it under the carpet for a few more months or years, until some other advanced platform comes out that needs more kludgeying. The result will be (already is) code that is hard to maintain and that is in real danger of breaking platforms other than the one which needs the patch. When will it stop? > The real fix IMHO would be to make the -tdep code manage the creation > and deletion of bp_locations, so that it could always make an informed > decision about what can and cannot be done with the available hardware > debug resources. Exactly! > But that's out of the scope of this patch series. I say, let's stop postponing this to some other patch series. I've been to that movie enough to know that it's a polite form of saying "never". Sorry for being so blunt, but I think this problem is well past the time we should have solved it for good.