From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 302 invoked by alias); 28 May 2003 20:27:37 -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 32649 invoked from network); 28 May 2003 20:27:36 -0000 Received: from unknown (HELO cygnus.equallogic.com) (65.170.102.10) by sources.redhat.com with SMTP; 28 May 2003 20:27:36 -0000 Received: from cygnus.equallogic.com (localhost.localdomain [127.0.0.1]) by cygnus.equallogic.com (8.11.6/8.11.6) with ESMTP id h4SKRYr23983 for ; Wed, 28 May 2003 16:27:34 -0400 Received: from deneb.dev.equallogic.com (deneb.dev.equallogic.com [172.16.1.99]) by cygnus.equallogic.com (8.11.6/8.11.6) with ESMTP id h4SKRXu23974; Wed, 28 May 2003 16:27:33 -0400 Received: from pkoning.dev.equallogic.com.equallogic.com (localhost.localdomain [127.0.0.1]) by deneb.dev.equallogic.com (8.11.6/8.11.6) with ESMTP id h4SKRXr14569; Wed, 28 May 2003 16:27:33 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <16085.7093.776115.863795@pkoning.dev.equallogic.com> Date: Wed, 28 May 2003 20:27:00 -0000 From: Paul Koning To: eliz@elta.co.il Cc: gdb-patches@sources.redhat.com Subject: Re: proposed PATCH: make watchpoints work correctly References: <16084.56661.295275.544414@pkoning.dev.equallogic.com> <1659-Wed28May2003225524+0300-eliz@elta.co.il> X-SW-Source: 2003-05/txt/msg00516.txt.bz2 >>>>> "Eli" == Eli Zaretskii writes: >> Date: Wed, 28 May 2003 12:01:25 -0400 From: Paul Koning >> >> >> If HAVE_NONSTEPPABLE_WATCHPOINT is defined, watchpoints would not >> stop for several reasons. First of all, remote.c would return 0 >> rather than the watch address after the single step past the >> instruction where the watch trap occurred. (Changes to infrun.c, >> remote.c) Second, even after that is fixed, bpstat_stop_status >> wouldn't match the watchpoint because removing the watchpoint (to >> single step) deleted the valchain which is used to do the matching >> (Change to breakpoint.c) >> >> If a watchpoint is defined, but the program stops for some other >> reason -- either a breakpoint, or a break instruction hardcoded in >> the target code -- bpstat_stop_status would encounter the >> watchpoint in its scan for possible reasons. It would take no >> action on it but leave its "stop" and "print" bits set so you >> would see the stop reported as if it were a watchpoint hit. Also, >> a "next" or "step" command would act as "stepi", i.e., stop after >> every instruction. (Changes to breakpoint.c). Eli> (It's been a while since I hacked on watchpoint-related code, so Eli> I apologize in advance for asking possibly dumb questions.) Eli> The above description made me nervous: it almost sounds like the Eli> current watchpoint support is pretty much dysfunctional, as most Eli> of the changes you suggest are not specific neither to remote.c Eli> nor to HAVE_NONSTEPPABLE_WATCHPOINT. So could you please Eli> explain how, given those deficiencies, watchpoints do work for Eli> native targets such as x86, but did not work for your target? I'm not sure. I just built a gdb for x86 on NetBSD, and all I get is software write watchpoints, no hardware watch support seems to be present. I arrived at the patch I sent in two steps. The description I gave combines what I found in those two steps, and I may have made errors in some details. In fact, looking back, there definitely is an error. The original problem I saw with stepping turning into stepi happened *only* for rwatch and awatch, NOT for write watchpoints. The reason is that the existinc code looks like this: /* Come here if it's a watchpoint, or if the break address matches */ bs = bpstat_alloc (b, bs); /* Alloc a bpstat to explain stop */ /* Watchpoints may change this, if not found to have triggered. */ bs->stop = 1; bs->print = 1; sprintf (message, message1, b->number); if (b->type == bp_watchpoint || b->type == bp_hardware_watchpoint) { ... } else if (b->type == bp_read_watchpoint || b->type == bp_access_watchpoint) { CORE_ADDR addr; struct value *v; int found = 0; addr = target_stopped_data_address (); if (addr == 0) continue; The comment says "if it's a watchpoint" -- what that means is: if the entry in the breakpoint list we're currently looking at is a watchpoint. It does not mean "if we stopped because of a watchpoint". The code then allocates a bpstat entry, and sets it to stop and print. Then if it turns out to be a rwatch or awatch, but there is no data address (the stop wasn't a watchpoint stop) the code just leaves the bpstat entry in the bpstat list marked for stop and print... My first fix was to set bs->stop = bs->print = 0 when the "continue" is done. That fixed the stopping after every instruction, but now the stop == 0 status meant that permanent breakpoints would no longer stop. (Regular ones would, but for permanent breakpoints you end up with a single bpstat entry that says stop==0, rather than a null list.) So my new fix is to skip the watchpoint entries in the breakpoint list entirely if the stop isn't a watchpoint stop. That fixes all the problems I've run into. Does that make sense? Clearly I'm not a gdb expert. I've been trying to learn selected pieces in order to fix problems we run into in our product development, and when those look like they are more generally useful I try to pass them along. The purpose of this patch submission is to get input from experts -- not necessarily to claim that the fix I submitted is the best way to solve the problem... paul