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

Pedro Alves
2009-04-20T19:07:20Z

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