Re: [Patch 1/2] Fix aftermath of 'infrun.c support for MIPS hardware watchpoints.'

[Pedro Alves <pedro at codesourcery dot com>]

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  <ddaney@caviumnetworks.com>
> 
>         * 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