From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31805 invoked by alias); 14 Jun 2004 21:40:22 -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 31789 invoked from network); 14 Jun 2004 21:40:21 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org with SMTP; 14 Jun 2004 21:40:21 -0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.12.10/8.12.10) with ESMTP id i5ELeLi5013617 for ; Mon, 14 Jun 2004 17:40:21 -0400 Received: from pobox.toronto.redhat.com (pobox.toronto.redhat.com [172.16.14.4]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with ESMTP id i5ELeK009867; Mon, 14 Jun 2004 17:40:20 -0400 Received: from touchme.toronto.redhat.com (IDENT:postfix@touchme.toronto.redhat.com [172.16.14.9]) by pobox.toronto.redhat.com (8.12.8/8.12.8) with ESMTP id i5ELeKtn005691; Mon, 14 Jun 2004 17:40:20 -0400 Received: from redhat.com (toocool.toronto.redhat.com [172.16.14.72]) by touchme.toronto.redhat.com (Postfix) with ESMTP id 35824800326; Mon, 14 Jun 2004 17:40:20 -0400 (EDT) Message-ID: <40CE1B44.4070708@redhat.com> Date: Mon, 14 Jun 2004 21:40:00 -0000 From: Jeff Johnston User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 MIME-Version: 1.0 To: Eli Zaretskii Cc: gdb-patches@sources.redhat.com Subject: Re: [RFC]: x86 threaded watchpoint support [2/3] References: <40CA252E.8050109@redhat.com> <9743-Sat12Jun2004123939+0300-eliz@gnu.org> In-Reply-To: <9743-Sat12Jun2004123939+0300-eliz@gnu.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2004-06/txt/msg00327.txt.bz2 Eli Zaretskii wrote: >>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 since found out that certain platforms do not actually know the stopped data address. Thus, the original code was needed to support such platforms. as you had to figure out which values changed manually. This always applies to software watchpoints. The change is needed because with the threading model, you can have multiple events to process. So, if you check your watchpoint values, all of them may have changed but you end up reporting an invalid thread location. For example, I was getting watchpoints changing at the same time of a new thread event (it couldn't discern). The reported location was __nptl_create_event (not very useful). An end-user would have to do a info thread to find out the location for each thread and determine where the real culprit was. If you have a lot of threads, you can see this being very frustrating. In the case of x86, we do know the stopped data address so we shouldn't bother looking at the changed value of a hardware watchpoint that doesn't match. With the fix, it is possible to see the proper location of where a single watchpoint location was modified. Very useful. > 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? > I definitely could have combined the code more efficiently for the two if clauses after making my changes. I have talked to Andrew and he suggests that the target_stopped_data_address code should be modified to add a status code instead of the current 0 addr return value. The address of 0 could be a valid watchpoint or could indicate no watchpoint triggered or failure or the lower level code "doesn't know" so the current interface isn't robust enough. I will be performing another version of the change to add this and that should handle platforms such as S390 but allow platforms like the x86 to work properly. I will also combine the two if clauses better this time :) -- Jeff J.