From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11021 invoked by alias); 12 Jun 2004 09:42:53 -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 11005 invoked from network); 12 Jun 2004 09:42:52 -0000 Received: from unknown (HELO aragorn.inter.net.il) (192.114.186.23) by sourceware.org with SMTP; 12 Jun 2004 09:42:52 -0000 Received: from zaretski ([80.230.141.54]) by aragorn.inter.net.il (MOS 3.4.6-GR) with ESMTP id DBN08308; Sat, 12 Jun 2004 12:42:37 +0300 (IDT) Date: Sat, 12 Jun 2004 09:42:00 -0000 From: "Eli Zaretskii" To: Jeff Johnston Message-Id: <9743-Sat12Jun2004123939+0300-eliz@gnu.org> CC: gdb-patches@sources.redhat.com In-reply-to: <40CA252E.8050109@redhat.com> (message from Jeff Johnston on Fri, 11 Jun 2004 17:33:34 -0400) Subject: Re: [RFC]: x86 threaded watchpoint support [2/3] Reply-to: Eli Zaretskii References: <40CA252E.8050109@redhat.com> X-SW-Source: 2004-06/txt/msg00285.txt.bz2 > Date: Fri, 11 Jun 2004 17:33:34 -0400 > From: Jeff Johnston > > The most major change is that a check has been added for a > hardware_watchpoint to ensure that the stopped data address matches > the watchpoint address. I don't necessarily object to this change, but could you first explain why is this needed, while it was never needed before? (I have my guess for the answer, but I'd like to hear yours.) I have also a minor comment about the change itself, see below. > @@ -2683,45 +2688,100 @@ bpstat_stop_status (CORE_ADDR bp_addr, p > if (b->type == bp_watchpoint || > b->type == bp_hardware_watchpoint) > { > - char *message = xstrprintf ("Error evaluating expression for watchpoint %d\n", > + CORE_ADDR addr; > + struct value *v; > + int found = 0; > + > + /* If we have a hardware watchpoint, ensure that the address > + being watched caused the trap event. */ > + if (b->type == bp_hardware_watchpoint) > + { > + addr = target_stopped_data_address (); > + if (addr == 0) > + { > + /* Don't stop. */ > + bs->print_it = print_it_noop; > + bs->stop = 0; > + continue; > + } > + for (v = b->val_chain; v; v = v->next) It looks to me that this change makes the bp_hardware_watchpoint case exactly identical to bp_read_watchpoint and bp_access_watchpoint, is that right? If so, why not add bp_hardware_watchpoint to the if clause that handles read and access watchpoints, and leave only bp_watchpoint alone with the current code?