Re: [RFA]: Fix for watchpoint regressions

Re: [RFA]: Fix for watchpoint regressions

[Ulrich Weigand]

Jeff Johnston wrote:

>Things worked in the past for ia64 because the code near the bottom of the
loop
>in bpstat_stop_status would check whether a hardware watchpoint value had
>changed.  With Ulrich's original patch in place, this never happens
because we
>never get to that piece of code in the loop.  With the revised patch, for
the
>original scenario that Ulrich was fixing, we will not get to the
watchpoint
>checking code, but for platforms that do not have
HAVE_CONTINUABLE_WATCHPOINT,
>we will.  We won't yet be able to handle the breakpoint insn in code
scenario
>for ia64 that Ulrich was fixing, but that is a separate fix.

Note that my patch wasn't the first in the sequence; I only tried to fix
s390 breakage caused by the original patch (by Orjan Friberg) ...

In the past, we'd indeed always check for hardware watchpoints by comparing
all memory locations for changes.  It was not my patch that changed this,
but Orjan's http://sources.redhat.com/ml/gdb-patches/2004-04/msg00521.html.
That patch was the one that tried to fix the breakpoint in code scenario.

This caused breakage on s390 because that patch unconditionally used
target_stopped_data_address; but s390 is unable to provide that info.
My change was to simply use STOPPED_BY_WATCHPOINT instead to provide
that single bit of information actually required.


But anyway, as far as s390 is concerned your patch should make no
difference, so I have no objections to it ...


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand
  Linux for S/390 Design & Development
  IBM Deutschland Entwicklung GmbH, Schoenaicher Str. 220, 71032 Boeblingen
  Phone: +49-7031/16-3727   ---   Email: Ulrich.Weigand@de.ibm.com

[RFA]: Fix for watchpoint regressions

[Jeff Johnston]

[-- Attachment #1: Type: text/plain, Size: 470 bytes --]

The attached patch fixes the recent watchpoint regressions in ia64.  Tested on 
ia64 and x86.

Ok to commit?

-- Jeff J.

2004-06-18  Jeff Johnston  <jjohnstn@redhat.com>

         * infrun.c (handle_inferior_event): Initialize stopped_by_watchpoint
         to -1.
         * breakpoint.c (bpstat_stop_status): Move check for ignoring
         untriggered watchpoints to a separate if clause.  Update function
         comment regarding STOPPED_BY_WATCHPOINT argument.

[-- Attachment #2: watchpoint-fix.patch --]
[-- Type: text/plain, Size: 3113 bytes --]

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.174
diff -u -p -r1.174 breakpoint.c
--- breakpoint.c	7 Jun 2004 17:58:32 -0000	1.174
+++ breakpoint.c	18 Jun 2004 22:47:01 -0000
@@ -2559,8 +2559,9 @@ which its expression is valid.\n");     
 }
 
 /* Get a bpstat associated with having just stopped at address
-   BP_ADDR in thread PTID.  STOPPED_BY_WATCHPOINT is true if the
-   target thinks we stopped due to a hardware watchpoint.  */
+   BP_ADDR in thread PTID.  STOPPED_BY_WATCHPOINT is 1 if the
+   target thinks we stopped due to a hardware watchpoint, 0 if we
+   know we did not trigger a hardware watchpoint, and -1 if we do not know.  */
 
 /* Determine whether we stopped at a breakpoint, etc, or whether we
    don't understand this stop.  Result is a chain of bpstat's such that:
@@ -2593,15 +2594,10 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
     if (!breakpoint_enabled (b) && b->enable_state != bp_permanent)
       continue;
 
-    /* Hardware watchpoints are treated as non-existent if the reason we 
-       stopped wasn't a hardware watchpoint (we didn't stop on some data 
-       address).  Otherwise gdb won't stop on a break instruction in the code
-       (not from a breakpoint) when a hardware watchpoint has been defined.  */
     if (b->type != bp_watchpoint
-	&& !((b->type == bp_hardware_watchpoint
-	      || b->type == bp_read_watchpoint
-	      || b->type == bp_access_watchpoint)
-	     && stopped_by_watchpoint)
+	&& b->type != bp_hardware_watchpoint
+	&& b->type != bp_read_watchpoint
+	&& b->type != bp_access_watchpoint
 	&& b->type != bp_hardware_breakpoint
 	&& b->type != bp_catch_fork
 	&& b->type != bp_catch_vfork
@@ -2617,6 +2613,18 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
 	  continue;
       }
 
+    /* Continuable hardware watchpoints are treated as non-existent if the 
+       reason we stopped wasn't a hardware watchpoint (we didn't stop on 
+       some data address).  Otherwise gdb won't stop on a break instruction 
+       in the code (not from a breakpoint) when a hardware watchpoint has 
+       been defined.  */
+
+    if ((b->type == bp_hardware_watchpoint
+	 || b->type == bp_read_watchpoint
+	 || b->type == bp_access_watchpoint)
+	&& !stopped_by_watchpoint)
+      continue;
+
     if (b->type == bp_hardware_breakpoint)
       {
 	if (b->loc->address != bp_addr)
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.166
diff -u -p -r1.166 infrun.c
--- infrun.c	11 Jun 2004 23:39:51 -0000	1.166
+++ infrun.c	18 Jun 2004 22:47:02 -0000
@@ -1252,7 +1252,7 @@ handle_inferior_event (struct execution_
      defined in the file "config/pa/nm-hppah.h", accesses the variable
      indirectly.  Mutter something rude about the HP merge.  */
   int sw_single_step_trap_p = 0;
-  int stopped_by_watchpoint = 0;
+  int stopped_by_watchpoint = -1;  /* Mark as unknown.  */
 
   /* Cache the last pid/waitstatus. */
   target_last_wait_ptid = ecs->ptid;

Re: [RFA]: Fix for watchpoint regressions

[Eli Zaretskii]

> Date: Mon, 21 Jun 2004 11:40:31 -0400
> From: Jeff Johnston <jjohnstn@redhat.com>
> 
> 2004-06-18  Jeff Johnston  <jjohnstn@redhat.com>
> 
>          * infrun.c (handle_inferior_event): Initialize stopped_by_watchpoint
>          to -1.
>          * breakpoint.c (bpstat_stop_status): Move check for ignoring
>          untriggered watchpoints to a separate if clause.  Update function
>          comment regarding STOPPED_BY_WATCHPOINT argument.

Won't this break the cases that caused the addition of testing that
stopped_by_watchpoint is non-zero in bpstat_stop_status?

Re: [RFA]: Fix for watchpoint regressions

[Jeff Johnston]

Eli Zaretskii wrote:
>>Date: Mon, 21 Jun 2004 11:40:31 -0400
>>From: Jeff Johnston <jjohnstn@redhat.com>
>>
>>2004-06-18  Jeff Johnston  <jjohnstn@redhat.com>
>>
>>         * infrun.c (handle_inferior_event): Initialize stopped_by_watchpoint
>>         to -1.
>>         * breakpoint.c (bpstat_stop_status): Move check for ignoring
>>         untriggered watchpoints to a separate if clause.  Update function
>>         comment regarding STOPPED_BY_WATCHPOINT argument.
> 
> 
> Won't this break the cases that caused the addition of testing that
> stopped_by_watchpoint is non-zero in bpstat_stop_status?
>

I took care to try to ensure that the problem the original patch was fixing was 
still being addressed.

The stopped_by_watchpoint flag was only being set by handle_inferior_event if 
HAVE_CONTINUABLE_WATCHPOINT is true.  In the cases where the flag should be 
tested, it will be set correctly to false (0) or true (1).  The problem is that 
there is third state (don't know) which is due to the fact that the flag is not 
set in certain cases and you don't really know if you are stopped by a 
watchpoint or not.  This occurs, for example, when HAVE_NONSTEPPABLE_WATCHPOINT 
or HAVE_STEPPABLE_WATCHPOINT is true as in the case of ia64.  The flags indicate 
that the watchpoint has triggered prior to the actual change or value.  The 
handle_inferior_event function steps through the watchpoint change.  At that 
point, the STOPPED_BY_WATCHPOINT test doesn't return true for ia64, were it to 
be tested.  My original attempt at this was to try and set the flag regardless 
of the HAVE_CONTINUABLE_WATCHPOINT.  It did not work because 
STOPPED_BY_WATCHPOINT is not true after the step.

Things worked in the past for ia64 because the code near the bottom of the loop 
in bpstat_stop_status would check whether a hardware watchpoint value had 
changed.  With Ulrich's original patch in place, this never happens because we 
never get to that piece of code in the loop.  With the revised patch, for the 
original scenario that Ulrich was fixing, we will not get to the watchpoint 
checking code, but for platforms that do not have HAVE_CONTINUABLE_WATCHPOINT, 
we will.  We won't yet be able to handle the breakpoint insn in code scenario 
for ia64 that Ulrich was fixing, but that is a separate fix.

I am looking at making another patch which would mark a watchpoint for the 
stepping cases so we can tell if a particular watchpoint triggered or not prior 
to the step and thus allow gdb to avoid having to randomly test all value 
changes.  This would help to eliminate the cases where we might report n 
different watchpoint changes for a line of code where the code in question may 
only be responsible for one of the value changes.  This would also allow the 
breakpoint insn in code scenario to work on ia64.

-- Jeff J.

Re: [RFA]: Fix for watchpoint regressions

[Eli Zaretskii]

> Date: Mon, 21 Jun 2004 15:57:52 -0400
> From: Jeff Johnston <jjohnstn@redhat.com>
> >>
> >>2004-06-18  Jeff Johnston  <jjohnstn@redhat.com>
> >>
> >>         * infrun.c (handle_inferior_event): Initialize stopped_by_watchpoint
> >>         to -1.
> >>         * breakpoint.c (bpstat_stop_status): Move check for ignoring
> >>         untriggered watchpoints to a separate if clause.  Update function
> >>         comment regarding STOPPED_BY_WATCHPOINT argument.
> > 
> > 
> > Won't this break the cases that caused the addition of testing that
> > stopped_by_watchpoint is non-zero in bpstat_stop_status?
> >
> 
> I took care to try to ensure that the problem the original patch was fixing was 
> still being addressed.

Thanks for taking time to explain the situation and the fix.

Your patch is approved.

Re: [RFA]: Fix for watchpoint regressions

[Jeff Johnston]

Eli Zaretskii wrote:
>>Date: Mon, 21 Jun 2004 15:57:52 -0400
>>From: Jeff Johnston <jjohnstn@redhat.com>
>>
>>>>2004-06-18  Jeff Johnston  <jjohnstn@redhat.com>
>>>>
>>>>        * infrun.c (handle_inferior_event): Initialize stopped_by_watchpoint
>>>>        to -1.
>>>>        * breakpoint.c (bpstat_stop_status): Move check for ignoring
>>>>        untriggered watchpoints to a separate if clause.  Update function
>>>>        comment regarding STOPPED_BY_WATCHPOINT argument.
>>>
>>>
>>>Won't this break the cases that caused the addition of testing that
>>>stopped_by_watchpoint is non-zero in bpstat_stop_status?
>>>
>>
>>I took care to try to ensure that the problem the original patch was fixing was 
>>still being addressed.
> 
> 
> Thanks for taking time to explain the situation and the fix.
>

No problem. :)

> Your patch is approved.
> 

Thanks.  Patch applied.

-- Jeff J.