From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31500 invoked by alias); 13 Jan 2002 23:21:45 -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 31455 invoked from network); 13 Jan 2002 23:21:41 -0000 Received: from unknown (HELO factorix.sdv.fr) (194.206.196.2) by sources.redhat.com with SMTP; 13 Jan 2002 23:21:41 -0000 Received: from ordimaison (ip-76-192.evc.net [212.95.76.192]) by factorix.sdv.fr (8.11.4/8.11.4) with SMTP id g0DNFdR07908 for ; Mon, 14 Jan 2002 00:15:39 +0100 Message-Id: <3.0.6.32.20020114003326.0088d460@ics.u-strasbg.fr> X-Sender: muller@ics.u-strasbg.fr X-Mailer: QUALCOMM Windows Eudora Light Version 3.0.6 (32) Date: Sun, 13 Jan 2002 15:21:00 -0000 To: gdb-patches@sources.redhat.com From: muller@cerbere.u-strasbg.fr Subject: Re: [RFA 2] Debug register support in win32-nat.c In-Reply-To: <20020113183913.GD2647@redhat.com> References: <4.2.0.58.20020108102529.021d6ac8@ics.u-strasbg.fr> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" X-SW-Source: 2002-01/txt/msg00367.txt.bz2 At 13:39 13/01/02 -0500, Christopher Faylor wrote: >On Tue, Jan 08, 2002 at 10:26:12AM +0100, Pierre Muller wrote: >>At 10:20 08/01/2002 , vous avez ?crit: >>> This is a follow up of my first >>>proposal for win32 debug register support >>>(which enables hardware watchpoints, >>>I never tested the hardware breakpoints, as they don't have >>>much advantages over normal breakpoints on i386 processors). >> >>Sorry, I forgot to add the link >>http://sources.redhat.com/ml/gdb-patches/2001-11/msg00537.html >>and all the follow-ups. > >I applied this patch but it doesn't seem to build. I get a: >libgdb.a(win32-nat.o): In function `child_mourn_inferior': >/cygnus/src/uberbaum/gdb/win32-nat.c:1398: undefined reference to `_i386_cleanup_dregs' Did I forget to incorporate the change in config/i386/cygwin.mh ? No, I rechecked the reference patch and it is included. Did you rerun configure in gdb build directory? This is necessary as the nm.h link is changed by my patch to cygwin.mh, but might be missed by the Makefile itself. Run ../../src/gdb/configure in build/gdb if src and build dirs are at same dir level should suppress that error. >I assume that this is related to your other patch. > >In the meantime, I noticed a couple of things: > >- ChangeLog needs to be wrapped to 80 columns. > >- ChangeLog wording needs more verbs and more description. For instance: > (debug_registers_changed): Non zero whenever the debug registers where changed and > need to be written to inferior. > You need to mention that this is a new variable: > (debug_registers_changed): New variable. Reflects when debug registers are changed and > need to be written to inferior. > >- In do_initial_child_stuff, I'd prefer that you either use sizeof to > derive the size of the dr array for zeroing or use a defined constant, > rather than just a raw "7". OK, so I should rewrite it to + for (i = 0; i < sizeof (dr) / sizeof (dr[0]); i++) + dr[i] = 0; Is that correct? (Please excuse me to still ask so basic C questions, but I never used C outside GDB code itself...) >- I'm wondering if your implementation is thread safe? You're storing > debug registers in a global array and copying them into a structure > as needed. Couldn't they just be stored in the per-thread structure? > You could add a debug_registers_used value to the structure, if necessary. The basic idea of my implementation is that watchpoints should be triggered by an expression that is modifed by any thread, and thus the same value should be written to all threads. This means that: - we only need one copy of the debug registers. - we need to write their value to all threads (including newly created ones, which is the major difference between this patch respective to the first patch). - this of course assumes that the debuggee does not itself change its debug registers, in that case the patch will not work correctly, but I think that this is a reasonable limitation. I agree that the linux implementation does not set the debug registers for all threads but this means that if a watched expression is modified by another thread than the current thread at the time of setting the watchpoint will not be caught and that is much worse... If Christopher is able to build and test my patch after a rerun of the configure scriptand if he agrees with my remarks above, I will be happy to resubmit a third patch taking his remarks into account. Thanks for reviewing my patch, Christopher.