gdb requires watchpoints to fire after the write

gdb requires watchpoints to fire after the write

[Tim Newsome]

The RISC-V Debug Spec suggests that memory write triggers fire before the
access actually takes place. This allows a user to see what the value in
memory is before it's overwritten. This does not play nice with gdb's
watchpoint expectation, however. When a watchpoint is hit, gdb instantly
reads the memory that it fired on, and if it hasn't changed it silently
lets execution resume. Since on RISC-V the watchpoint fired before the
write actually took place, gdb will always miss the first write because
memory won't have changed yet.

Does that sound right? How should I fix this? With hardware breakpoint
functionality being generally all over the map, I can't imagine this hasn't
come up before.

Thank you,
Tim

Re: gdb requires watchpoints to fire after the write

[Simon Marchi]

On 2018-08-28 18:08, Tim Newsome wrote:
> The RISC-V Debug Spec suggests that memory write triggers fire before 
> the
> access actually takes place. This allows a user to see what the value 
> in
> memory is before it's overwritten. This does not play nice with gdb's
> watchpoint expectation, however. When a watchpoint is hit, gdb 
> instantly
> reads the memory that it fired on, and if it hasn't changed it silently
> lets execution resume. Since on RISC-V the watchpoint fired before the
> write actually took place, gdb will always miss the first write because
> memory won't have changed yet.
> 
> Does that sound right? How should I fix this? With hardware breakpoint
> functionality being generally all over the map, I can't imagine this 
> hasn't
> come up before.

I don't have experience with many different architectures, but as far as 
I know, the expectation of the GDB is that the watchpoint is reported 
after the write.  Otherwise it wouldn't need to save the value of the 
watched expression.  That's also how software watchpoints seem to work.

The easiest way to deal with this would be to match GDB's expectation.  
But if you really prefer the behavior of reporting the watchpoint before 
the event, I suppose it's always possible to teach GDB about this, but 
it's a less trivial task.  Especially that when you GDB evaluates 
whether the watch expression has changed value, it would need to 
consider the not-yet-written value in memory.

I'm also curious to know if other architectures work in this way (report 
the event before the write actually take place).

Simon

Re: gdb requires watchpoints to fire after the write

[Joel Brobecker]

> I don't have experience with many different architectures, but as far as I
> know, the expectation of the GDB is that the watchpoint is reported after
> the write.  Otherwise it wouldn't need to save the value of the watched
> expression.  That's also how software watchpoints seem to work.
> 
> The easiest way to deal with this would be to match GDB's expectation.  But
> if you really prefer the behavior of reporting the watchpoint before the
> event, I suppose it's always possible to teach GDB about this, but it's a
> less trivial task.  Especially that when you GDB evaluates whether the watch
> expression has changed value, it would need to consider the not-yet-written
> value in memory.
> 
> I'm also curious to know if other architectures work in this way (report the
> event before the write actually take place).

I seem to remember some architectures having different behaviors,
and so we have a couple of entry points in GDB. For architecture-specific
settings, we have gdbarch_have_nonsteppable_watchpoint. For target-specific
settings, you would use target_have_steppable_watchpoint. (IIRC)

-- 
Joel

Re: gdb requires watchpoints to fire after the write

[Pedro Alves]

On 08/29/2018 04:47 PM, Joel Brobecker wrote:
>> I don't have experience with many different architectures, but as far as I
>> know, the expectation of the GDB is that the watchpoint is reported after
>> the write.  Otherwise it wouldn't need to save the value of the watched
>> expression.  That's also how software watchpoints seem to work.
>>
>> The easiest way to deal with this would be to match GDB's expectation.  But
>> if you really prefer the behavior of reporting the watchpoint before the
>> event, I suppose it's always possible to teach GDB about this, but it's a
>> less trivial task.  Especially that when you GDB evaluates whether the watch
>> expression has changed value, it would need to consider the not-yet-written
>> value in memory.
>>
>> I'm also curious to know if other architectures work in this way (report the
>> event before the write actually take place).
> 
> I seem to remember some architectures having different behaviors,
> and so we have a couple of entry points in GDB. For architecture-specific
> settings, we have gdbarch_have_nonsteppable_watchpoint. For target-specific
> settings, you would use target_have_steppable_watchpoint. (IIRC)

Yes, the key bit is this here in infrun.c:

  /* If necessary, step over this watchpoint.  We'll be back to display
     it in a moment.  */
  if (stopped_by_watchpoint
      && (target_have_steppable_watchpoint
	  || gdbarch_have_nonsteppable_watchpoint (gdbarch)))
    {
      /* At this point, we are stopped at an instruction which has
         attempted to write to a piece of memory under control of
         a watchpoint.  The instruction hasn't actually executed
         yet.  If we were to evaluate the watchpoint expression
         now, we would get the old value, and therefore no change
         would seem to have occurred.

         In order to make watchpoints work `right', we really need
         to complete the memory write, and then evaluate the
         watchpoint expression.  We do this by single-stepping the
	 target.

	 It may not be necessary to disable the watchpoint to step over
	 it.  For example, the PA can (with some kernel cooperation)
	 single step over a watchpoint without disabling the watchpoint.

	 It is far more common to need to disable a watchpoint to step
	 the inferior over it.  If we have non-steppable watchpoints,
	 we must disable the current watchpoint; it's simplest to
	 disable all watchpoints.

	 Any breakpoint at PC must also be stepped over -- if there's
	 one, it will have already triggered before the watchpoint
	 triggered, and we either already reported it to the user, or
	 it didn't cause a stop and we called keep_going.  In either
	 case, if there was a breakpoint at PC, we must be trying to
	 step past it.  */
      ecs->event_thread->stepping_over_watchpoint = 1;
      keep_going (ecs);
      return;
    }

Thanks,
Pedro Alves

Re: gdb requires watchpoints to fire after the write

[Simon Marchi]

On 2018-08-29 11:47, Joel Brobecker wrote:
>> I don't have experience with many different architectures, but as far 
>> as I
>> know, the expectation of the GDB is that the watchpoint is reported 
>> after
>> the write.  Otherwise it wouldn't need to save the value of the 
>> watched
>> expression.  That's also how software watchpoints seem to work.
>> 
>> The easiest way to deal with this would be to match GDB's expectation. 
>>  But
>> if you really prefer the behavior of reporting the watchpoint before 
>> the
>> event, I suppose it's always possible to teach GDB about this, but 
>> it's a
>> less trivial task.  Especially that when you GDB evaluates whether the 
>> watch
>> expression has changed value, it would need to consider the 
>> not-yet-written
>> value in memory.
>> 
>> I'm also curious to know if other architectures work in this way 
>> (report the
>> event before the write actually take place).
> 
> I seem to remember some architectures having different behaviors,
> and so we have a couple of entry points in GDB. For 
> architecture-specific
> settings, we have gdbarch_have_nonsteppable_watchpoint. For 
> target-specific
> settings, you would use target_have_steppable_watchpoint. (IIRC)

Indeed, the comment at infrun.c:5805 seems to hint that some (or all?) 
targets/arches do work like that?  And the fix is that GDB does a single 
step to execute the instruction that modifies the memory, and then 
evaluates the expression.  I hadn't thought about that.

See: 
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/infrun.c;h=7731ccda68343b0118b9806615ff45b9f4d56c63;hb=HEAD#l5805

I'm just confused by this condition:

   if (stopped_by_watchpoint
       && (target_have_steppable_watchpoint
	  || gdbarch_have_nonsteppable_watchpoint (gdbarch)))

I don't understand why we check for target_have_steppable_watchpoint OR 
gdbarch_have_nonsteppable_watchpoint, they seem to mean opposite things.

Simon

Re: gdb requires watchpoints to fire after the write

[Pedro Alves]

On 08/29/2018 05:02 PM, Simon Marchi wrote:

> I'm just confused by this condition:
> 
>   if (stopped_by_watchpoint
>       && (target_have_steppable_watchpoint
>       || gdbarch_have_nonsteppable_watchpoint (gdbarch)))
> 
> I don't understand why we check for target_have_steppable_watchpoint OR gdbarch_have_nonsteppable_watchpoint, they seem to mean opposite things.

Yeah, it's confusing.

GDB's current "model" is that there are three "kinds" of watchpoints,
wrt to when they trigger and how you can move past them.

Those are: continuable, steppable, and non-steppable.

Continuable watchpoints are like x86's -- those trigger after
the memory access's side effects are fully committed to memory.
I.e., they trap with the PC pointing at the next instruction
already.  Continuing past such a watchpoint is doable
by just normally continuing, hence the name.

Both steppable and nonsteppable watchpoints trap before
the memory access.  I.e, the PC points at the instruction that
is accessing the memory.  So GDB needs to single-step once past
the current instruction in order to make the access effective
and check whether the instruction's side effects change the
watched expression.

Now, in order to step past that instruction, depending on 
architecture, you can have two situations:

- steppable watchpoints: you can single-step with the watchpoint still
  armed, and the watchpoint won't trigger again.

- non-steppable watchpoints: if you try to single-step with the watchpoint
  still armed, you'd trap the watchpoint again and the thread wouldn't
  make any progress.  So GDB needs to temporarily remove the watchpoint
  in order to step past it.

So that's why we have all of target_have_continuable_watchpoint,
target_have_steppable_watchpoint and gdbarch_have_nonsteppable_watchpoint.

Now, the main oddity is that from the definitions above,
we can tell that "continuable" is the same as "!steppable && !nonsteppable",
which makes target_continuable_watchpoint redundant.

Some targets do set "have_continuable_watchpoint" (like x86
in x86-nat.h), but it doesn't seem like target_have_continuable_watchpoint
is checked anywhere nowadays.

The target_have_steppable_watchpoint property is only set by ia64 GNU/Linux
nowadays:

  /* 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; }

There's of course also the oddity that target_have_continuable_watchpoint
and target_have_steppable_watchpoint are target methods, while 
gdbarch_have_nonsteppable_watchpoint is a gdbarch method...

We could most probably streamline all of this and come up with a better
design with some thought.  See also the comment in mips-tdep.c:

  /* FIXME: cagney/2003-08-29: The macros target_have_steppable_watchpoint,
     HAVE_NONSTEPPABLE_WATCHPOINT, and target_have_continuable_watchpoint
     need to all be folded into the target vector.  Since they are
     being used as guards for target_stopped_by_watchpoint, why not have
     target_stopped_by_watchpoint return the type of watchpoint that the code
     is sitting on?  */
  set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);

Thanks,
Pedro Alves

Re: gdb requires watchpoints to fire after the write

[Tim Newsome]

Thanks everybody. That's really helpful.

Tim

On Wed, Aug 29, 2018 at 10:29 AM, Pedro Alves <palves@redhat.com> wrote:

> On 08/29/2018 05:02 PM, Simon Marchi wrote:
>
> > I'm just confused by this condition:
> >
> >   if (stopped_by_watchpoint
> >       && (target_have_steppable_watchpoint
> >       || gdbarch_have_nonsteppable_watchpoint (gdbarch)))
> >
> > I don't understand why we check for target_have_steppable_watchpoint OR
> gdbarch_have_nonsteppable_watchpoint, they seem to mean opposite things.
>
> Yeah, it's confusing.
>
> GDB's current "model" is that there are three "kinds" of watchpoints,
> wrt to when they trigger and how you can move past them.
>
> Those are: continuable, steppable, and non-steppable.
>
> Continuable watchpoints are like x86's -- those trigger after
> the memory access's side effects are fully committed to memory.
> I.e., they trap with the PC pointing at the next instruction
> already.  Continuing past such a watchpoint is doable
> by just normally continuing, hence the name.
>
> Both steppable and nonsteppable watchpoints trap before
> the memory access.  I.e, the PC points at the instruction that
> is accessing the memory.  So GDB needs to single-step once past
> the current instruction in order to make the access effective
> and check whether the instruction's side effects change the
> watched expression.
>
> Now, in order to step past that instruction, depending on
> architecture, you can have two situations:
>
> - steppable watchpoints: you can single-step with the watchpoint still
>   armed, and the watchpoint won't trigger again.
>
> - non-steppable watchpoints: if you try to single-step with the watchpoint
>   still armed, you'd trap the watchpoint again and the thread wouldn't
>   make any progress.  So GDB needs to temporarily remove the watchpoint
>   in order to step past it.
>
> So that's why we have all of target_have_continuable_watchpoint,
> target_have_steppable_watchpoint and gdbarch_have_nonsteppable_watchpoint.
>
> Now, the main oddity is that from the definitions above,
> we can tell that "continuable" is the same as "!steppable &&
> !nonsteppable",
> which makes target_continuable_watchpoint redundant.
>
> Some targets do set "have_continuable_watchpoint" (like x86
> in x86-nat.h), but it doesn't seem like target_have_continuable_watchpoint
> is checked anywhere nowadays.
>
> The target_have_steppable_watchpoint property is only set by ia64
> GNU/Linux
> nowadays:
>
>   /* 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; }
>
> There's of course also the oddity that target_have_continuable_watchpoint
> and target_have_steppable_watchpoint are target methods, while
> gdbarch_have_nonsteppable_watchpoint is a gdbarch method...
>
> We could most probably streamline all of this and come up with a better
> design with some thought.  See also the comment in mips-tdep.c:
>
>   /* FIXME: cagney/2003-08-29: The macros target_have_steppable_
> watchpoint,
>      HAVE_NONSTEPPABLE_WATCHPOINT, and target_have_continuable_watchpoint
>      need to all be folded into the target vector.  Since they are
>      being used as guards for target_stopped_by_watchpoint, why not have
>      target_stopped_by_watchpoint return the type of watchpoint that the
> code
>      is sitting on?  */
>   set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);
>
> Thanks,
> Pedro Alves
>

Re: gdb requires watchpoints to fire after the write

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Yeah, it's confusing.

[... great explanation ... ]

This would be great as comments in gdbarch.sh and target.h.
None of these things have comments currently.

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.

Is it possible for gdbserver to do the single-step itself, avoiding a
round trip?  That was the only rationale I could think of.

Tom

Re: gdb requires watchpoints to fire after the write

[Joel Brobecker]

> Pedro> Yeah, it's confusing.
> 
> [... great explanation ... ]
> 
> This would be great as comments in gdbarch.sh and target.h.
> None of these things have comments currently.

Agreed. I was going to send a patch doing so when I read Pedro's
message, and then read this. I'm happy to do the patch, but don't
want to start if Pedro is already on it.

> 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.
> 
> 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?

-- 
Joel

Re: gdb requires watchpoints to fire after the write

[Pedro Alves]

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

Re: gdb requires watchpoints to fire after the write

[Pedro Alves]

On 08/29/2018 09:57 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Yeah, it's confusing.
> 
> [... great explanation ... ]
> 
> This would be great as comments in gdbarch.sh and target.h.
> None of these things have comments currently.

I've just sent a mini series for that:

https://sourceware.org/ml/gdb-patches/2018-08/msg00864.html

Let me know what you think.

Thanks,
Pedro Alves