Re: gdb requires watchpoints to fire after the write

[Pedro Alves <palves@redhat.com>]

On 08/30/2018 09:03 AM, Joel Brobecker wrote:

> 
>> Pedro> We could most probably streamline all of this and come up with a better
>> Pedro> design with some thought.  See also the comment in mips-tdep.c:
>>
>> Pedro>   /* FIXME: cagney/2003-08-29: The macros target_have_steppable_watchpoint,
>> Pedro>      HAVE_NONSTEPPABLE_WATCHPOINT, and target_have_continuable_watchpoint
>> Pedro>      need to all be folded into the target vector.  Since they are
>> Pedro>      being used as guards for target_stopped_by_watchpoint, why not have
>> Pedro>      target_stopped_by_watchpoint return the type of watchpoint that the code
>> Pedro>      is sitting on?  */
>> Pedro>   set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);
>>
>> I'm curious about why this should be in the target rather than in the
>> gdbarch.  It seems like a property of the ISA.

Yeah, I think it could be both, actually.  For example, we could have a
default for the ISA (a gdbarch property) that could be overridden by
a target method.  Making it a property of the target, or the watchpoint
stop itself, would make it possible for example to support trapping
before the memory access is complete on emulators/simulators/record-replay,
even if the real hardware doesn't support it, which could be exposed
to the user as a new kind of watchpoint.

As for the current implementation of target_have_steppable_watchpoint,
the only target that uses it is IA-64, and if we look at the code, we see:

  /* Override watchpoint routines.  */

  /* The IA-64 architecture can step over a watch point (without
     triggering it again) if the "dd" (data debug fault disable) bit
     in the processor status word is set.

     This PSR bit is set in
     ia64_linux_nat_target::stopped_by_watchpoint when the code there
     has determined that a hardware watchpoint has indeed been hit.
     The CPU will then be able to execute one instruction without
     triggering a watchpoint.  */
  bool have_steppable_watchpoint () { return 1; }

and as the comment says, PSR is set here (a callee of
stopped_by_watchpoint):

 bool
 ia64_linux_nat_target::stopped_data_address (CORE_ADDR *addr_p)
 {
 ....
   regcache_cooked_read_unsigned (regcache, IA64_PSR_REGNUM, &psr);
   psr |= IA64_PSR_DD;	/* Set the dd bit - this will disable the watchpoint
                            for the next instruction.  */
   regcache_cooked_write_unsigned (regcache, IA64_PSR_REGNUM, psr);
 
   *addr_p = (CORE_ADDR) siginfo.si_addr;
 ...
   return true;
 }

So we can see that it's not that watchpoints are always steppable by
nature.  We have to explicitly poke a register to prepare for the magic step.
This is really an optimization as far as I can tell -- it seems to me that we could
remove this and instead make IA-64 set gdbarch_have_nonsteppable_watchpoint.
Then GDB would do the longer remove-watchpoint,single-step,reinsert-watchpoint
dance on IA-64 too.  It seems a bit of a hack to do this DD bit frobbing from within
target_stopped_by_watchpoint.  It could be that we set/clear the DD bit incorrectly
in some corner cases involving the thread stopping between trapping for
the watchpoint and then the single-step being interrupted.  Not sure, haven't
given it much thought or even actually ever played with an IA-64 machine.

If we ever port gdbserver to IA-64, and made target_have_steppable_watchpoint
a gdbarch property, we'd be baking in a target/agent feature into GDB's
awareness of the ISA.  I.e., we'd be assuming all IA-64 stubs would behave
the same.

It's with all that in mind that I mentioned that maybe with some thought we
could come up with a different way to implement this.  Dunno, maybe
with a new "step over watchpoint" remote protocol request.  But I wouldn't
bother with it, especially given only IA-64 seems to care all these years.

>>
>> Is it possible for gdbserver to do the single-step itself, avoiding a
>> round trip?  That was the only rationale I could think of.
> 
> It might not be gdbserver itself, which I don't think should try
> to remain as minimalistic as possible in terms of this kind of
> "intelligence", but maybe some other stubs?  For instance, a stub
> might be designed to be usable against another kind of debugger
> which might be expecting a certain type of behavior forcing
> the stub to have to do the single-step itself. (?)
> 
> My perspective is that, if we don't have a concrete situation where
> this functionality should be a property of the target, and we find
> that it simplifies the code or avoids confusion to remove that target
> property, then let's. The obvious question then becomes - what to do
> with ia64-linux? Is this platform still in use and relevant enough
> for us to invest energy into redesigning the watchpoint support on
> this platform a bit?

We could remove target_have_steppable_watchpoint, and then we'd be
left with a gdbarch setting for "watchpoint triggers either before or
after access".
I don't know whether there are people who still care about IA-64.

Thanks,
Pedro Alves