From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29149 invoked by alias); 17 Nov 2009 00:12:06 -0000 Received: (qmail 29136 invoked by uid 22791); 17 Nov 2009 00:12:03 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 17 Nov 2009 00:10:59 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 345072BAB45; Mon, 16 Nov 2009 19:10:57 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 3nOZLsumMDAd; Mon, 16 Nov 2009 19:10:57 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 1A8D72BAB44; Mon, 16 Nov 2009 19:10:56 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 9E1EDF5905; Mon, 16 Nov 2009 16:10:56 -0800 (PST) Date: Tue, 17 Nov 2009 00:12:00 -0000 From: Joel Brobecker To: Jan Kratochvil Cc: gdb-patches@sourceware.org Subject: Re: [patch 3/4] Fix hw watchpoints #2: reordered / simultaneously hit Message-ID: <20091117001056.GE4557@adacore.com> References: <20091116034156.GD22701@host0.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: <20091116034156.GD22701@host0.dyn.jankratochvil.net> User-Agent: Mutt/1.5.18 (2008-05-17) 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: 2009-11/txt/msg00377.txt.bz2 Jan, > If you hit two watchpoints at the same time in the default all-stop mode = GDB > does not cope well with the pending watchpoint hit in non-current LWP. > Moreover when you change the watchpoints setup during such stop. Can you be more specific as to what happens in this case? > gdb/ > 2009-11-16 Jan Kratochvil >=20 > Fix reordered watchpoints triggered in other threads during all-stop. > * i386-nat.c (i386_stopped_by_watchpoint): Call > i386_stopped_data_address through the target vector. > * linux-nat.c (save_sigtrap, linux_nat_stopped_data_address): New. > (stop_wait_callback, linux_nat_filter_event): Call save_sigtrap. > (linux_nat_add_target): Install linux_nat_stopped_data_address. > * linux-nat.h (struct lwp_info): New fields watchpoint_hit_set and > watchpoint_hit. Well, I agree now that this stopped_by_watchpoint/stopped_data_address can get you confused... It took me a LONG time to understand how the patch is working... Some comments below. > gdb/testsuite/ > 2009-11-16 Jan Kratochvil >=20 > * gdb.base/watchthreads-reorder.exp, gdb.base/watchthreads-reorde= r.c: > New. Pre-approved, with just a few minor comments. > + /* WATCHPOINT_HIT_SET is non-zero if this LWP stopped with a trap and = a data > + watchpoint has been found as triggered. In such case WATCHPOINT_HIT > + contains data address of the triggered data watchpoint. */ > + unsigned watchpoint_hit_set : 1; I think we should avoid bitfields unless we can show that they make a difference in terms of memory usage. I don't think that this structure is critical. Do you think that a name like "watchpoint_hit" or "watchpoint_hit_p" might be simpler? int watchpoint_hit; for instance? > + CORE_ADDR watchpoint_hit; I'm being a little bit on the picky side (again! Sorry...), but I think it's better to use names that are consistent with the associated target methods. We may want to change these names later if we decide to go ahead with your proposed interface cleanup (merging the two watchpoint target routines into one), but for now, I would personally prefer: CORE_ADDR stopped_data_address; > + /* linux_nat_stopped_data_address is not even installed in this case. = */ > + if (linux_ops->to_stopped_data_address =3D=3D NULL) > + return; I think the comment is a little obscure and that it should be placed after the if. Or, if you want to place it before the if, I'd make the commend conditional. Suggestions: if (linux_ops->to_stopped_data_address =3D=3D NULL) /* This platform does not seem to support watchpoints. */ return; Or: /* Nothing to do if this platform does not seem to support watchpoints. = */ if (linux_ops->to_stopped_data_address =3D=3D NULL) return; > +/* Wrap target_stopped_data_address where the GNU/Linux native target ma= y be > + directed by the watchpoint/debug register number. Base the reported = value > + on the triggered data address instead. During inferior stop the assi= gnment > + of watchpoint/debug registers may change making the register number s= pecific > + trigger info stale. > + > + See the comment at update_watchpoint for the trigger lifecycle. */ Part of this comment, I think, would be easier to understand if it was placed in the description of save_sigtrap(). (the part that explains that the data might change, etc, and thus you have to save it). For this function, I'd just stick to a general description, explaining that you base the results on the info cached in the lwp struct, and refer them to save_sigtrap. > + /* Just prevent compiler `warning: =1B$B!F=1B(BunusedX_rwatch=1B$B!G= =1B(B defined but not used'. */ I see some weird codes in that comment... > +# two watchpoint being hit at the same time, without reordering them dur= ing the ^^^^^^^^^^ watchpoints > + if ![runto_main] { > + gdb_suppress_tests > + } Please avoid the use of gdb_suppress_tests. If you can't run to main, then there is no point in continuing. Just abort via return. > + # Use "rwatch" as "watch" would report the watchpoint changed just b= ased on its > + # read memory value during a stop by unrelated event. We are intere= sted to not > + # to lose the hardware watchpoint trigger. "we are interested in not losing" --=20 Joel