From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30167 invoked by alias); 9 Jun 2011 00:00:03 -0000 Received: (qmail 30052 invoked by uid 22791); 9 Jun 2011 00:00:01 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 08 Jun 2011 23:59:46 +0000 Received: (qmail 19772 invoked from network); 8 Jun 2011 23:59:45 -0000 Received: from unknown (HELO scottsdale.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 8 Jun 2011 23:59:45 -0000 From: Pedro Alves To: "Philippe Waroquiers" Subject: Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) Date: Thu, 09 Jun 2011 00:00:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-8-generic; KDE/4.6.2; x86_64; ; ) Cc: gdb-patches@sourceware.org, yao@codesourcery.com References: <201106011534.50994.pedro@codesourcery.com> <1B06384E85DD4533B1F1C59A5EE8383D@soleil> In-Reply-To: <1B06384E85DD4533B1F1C59A5EE8383D@soleil> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201106090059.42380.pedro@codesourcery.com> X-IsSubscribed: yes 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: 2011-06/txt/msg00117.txt.bz2 On Wednesday 08 June 2011 23:55:11, Philippe Waroquiers wrote: > I think there is still something wrong with the DR registers, at least > with gdbserver. Yes. Many thanks for noticing this. > I suspect the problem might be in the following piece of code: > static void > update_inferior (struct i386_debug_reg_state *inf_state, > struct i386_debug_reg_state *new_state) > { > int i; > > ALL_DEBUG_REGISTERS (i) > { > if (new_state->dr_mirror[i] != inf_state->dr_mirror[i] > || (new_state->dr_ref_count[i] != 0 > && inf_state->dr_ref_count[i] == 0)) > { > > The dr_mirror is the address being watched. > But if address being watched is 0x0, then a 'busy' register > watching 0x0 and a non-busy register will have equal dr_mirror. > Then the || condition is bizarre as the ref.count will be updated > only if the current inf_state ref.count is 0. Not the ref.count. The address to watch, DR[0-3]. > It looks to me it would be clearer (and correct?) to always also > enter in the block updating really the inferior > if the dr_ref_counts are != (similarly to the dr_mirror). The point was that we need to set the DR[0-3] register when activating the watchpoint. If going from refcount 1 to 2, say, then the address hasn't changed, and, we already know it contains the correct address (set when we went from 0 to 1). The problem is actually that I forgot one important line of code when porting the changes from gdb to gdbserver. Index: src/gdb/gdbserver/i386-low.c =================================================================== --- src.orig/gdb/gdbserver/i386-low.c 2011-06-01 15:19:13.000000000 +0100 +++ src/gdb/gdbserver/i386-low.c 2011-06-09 00:36:18.008128016 +0100 @@ -458,6 +458,8 @@ update_inferior (struct i386_debug_reg_s inf_state->dr_control_mirror = new_state->dr_control_mirror; i386_dr_low_set_control (inf_state); } + + *inf_state = *new_state; } /* Insert a watchpoint to watch a memory region which starts at That is, I missed the "commit" from the mirror of the mirror to the regular process mirror. It's present on the gdb side of the changes as: static void update_inferior (struct i386_debug_reg_state *state) { ... dr_mirror = *state; } Sorry about that. :-P I'll take a better look at this tomorrow and check if I missed anything else. I'll try to add yet some other test that catches this. > Maybe it would be possible to add some asserts in the handling > of the DR ? E.g. if the ref.count > 0, then the register must be 'active' > and if ref.count == 0, then register must be 'inactive' ? Sounds like a good idea. > Finally, a question about the below comment from a new test. > If I understood correctly, the 'single (low level)' should rather be 'single (high level)' ? > > +# Regression test for a bug where the i386 watchpoints support backend > +# would leave some debug registers occupied even if not enough debug > +# registers were available to cover a single (low level) watchpoint. In the context of that comment, a high level watchpoint would be a watchpoint on the GDB side (e.g., watch $expression creates one high level watchpoint, with possibly many low level watchpoints, or watchpoint locations). This expression may require watching more than one memory region. Each of those regions will end up as a watchpoint location, and a "low level" watchpoint inserted on the target for each of them (a Z2 packet for each). The bug happens when not enough debug registers are available to cover a single Z2 packet / low level watchpoint. So the comment was correct if you think of high and low level watchpoints like I was thinking. Maybe you were thinking of a high level watchpoint as what the target sees? -- Pedro Alves