From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18853 invoked by alias); 20 Apr 2009 19:07:20 -0000 Received: (qmail 18833 invoked by uid 22791); 20 Apr 2009 19:07:18 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 20 Apr 2009 19:07:13 +0000 Received: (qmail 31258 invoked from network); 20 Apr 2009 19:07:11 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 20 Apr 2009 19:07:11 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [Patch 1/2] Fix aftermath of 'infrun.c support for MIPS hardware watchpoints.' Date: Mon, 20 Apr 2009 19:07:00 -0000 User-Agent: KMail/1.9.10 Cc: David Daney References: <49D9AECB.7040302@gmail.com> <200904171443.26794.pedro@codesourcery.com> <49ECBF74.8020306@caviumnetworks.com> In-Reply-To: <49ECBF74.8020306@caviumnetworks.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200904202007.37943.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: 2009-04/txt/msg00534.txt.bz2 On Monday 20 April 2009 19:31:16, David Daney wrote: > This patch had a small problem: > > http://sourceware.org/ml/gdb-patches/2009-04/msg00245.html > > I was calling registers_changed() to clear the register cache, but then > immediately calling read_pc() which caused it to be reloaded. After the > single step to move past the watchpoint, the old cached register values > were used instead of the current values. > > The fix: Move the registers_changed() call after read_pc(). > > Tested on x86_64-unknown-linux-gnu and mips64-unknown-linux-gnu with no > regressions. > > OK to commit? I see. There's a call to prepare_to_wait just below, that usually calls registers_changed, but ... surprise, surprise, ... static void prepare_to_wait (struct execution_control_state *ecs) { if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: prepare_to_wait\n"); if (infwait_state == infwait_normal_state) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ { overlay_cache_invalid = 1; /* We have to invalidate the registers BEFORE calling target_wait because they can be loaded from the target while in target_wait. This makes remote debugging a bit more efficient for those targets that provide critical registers as part of their normal status mechanism. */ registers_changed (); ^^^^^^^^^^^^^^^^^^^^ waiton_ptid = pid_to_ptid (-1); } /* This is the old end of the while loop. Let everybody know we want to wait for the inferior some more and get called again soon. */ ecs->wait_some_more = 1; } ... it's skipped this time, because infwait_state != infwait_normal_state. This just makes no sense, the register cache should *always* be flushed if we resume the target. Also, wait_for_inferior only calls registers_changed once, on entry to the loop, instead of calling it always before calling target_wait, which would be much simpler... I'm fairly sure Ulrich was cleaning this up ... ah, here: http://sourceware.org/ml/gdb-patches/2008-12/msg00120.html Ulrich, maybe we should apply the pieces of that series we're happy with? In the mean time, your patch is OK. I'd just move the registers_changed call to *after* target_resume, because the target_resume implementation could trigger a register cache refetch, thus reintroducing the problem (e.g., it doesn't happen on mips *today*, but see e.g., i386-linux-nat.c:i386_linux_resume). I'd put it right after the prepare_to_wait call. > 2009-04-20 David Daney > > * infrun.c (handle_inferior_event): Move registers_changed call down. > > Index: infrun.c > =================================================================== > RCS file: /cvs/src/src/gdb/infrun.c,v > retrieving revision 1.368 > diff -u -p -r1.368 infrun.c > --- infrun.c 14 Apr 2009 00:59:47 -0000 1.368 > +++ infrun.c 20 Apr 2009 18:14:35 -0000 > @@ -2842,9 +2842,9 @@ targets should add new threads to the th > > if (!HAVE_STEPPABLE_WATCHPOINT) > remove_breakpoints (); > - registers_changed (); > /* Single step */ > hw_step = maybe_software_singlestep (current_gdbarch, read_pc ()); > + registers_changed (); > target_resume (ecs->ptid, hw_step, TARGET_SIGNAL_0); > waiton_ptid = ecs->ptid; > if (HAVE_STEPPABLE_WATCHPOINT) -- Pedro Alves