[RFC]: x86 threaded watchpoint support [2/3]

[RFC]: x86 threaded watchpoint support [2/3]

[Jeff Johnston]

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

This is the 2nd part of getting x86 threaded watchpoints to work.  It involves 
some fixes to breakpoint.c.

The most major change is that a check has been added for a hardware_watchpoint 
to ensure that the stopped data address matches the watchpoint address.  I do 
not know if there are platforms that have hardware_watchpoints but cannot tell 
what the data address is.  If this problem exists, I can easily add a flag to 
determine if the stopped address can be used or not.  Alternatively, the default 
stopped_data_address for such a platform could return a special value that the 
comparison check could treat as "don't know".

Comments?  Ok?

-- Jeff J.

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

         * breakpoint.c (print_it_typical): Do not issue a warning message
         if we find a bp_thread_event and there are other items on the
         stop list.
         (bpstat_stop_status): When dealing with a hardware_watchpoint,
         check if the stopped data address matches the watched address
         for the current breakpoint in the list.  When dealing with a
         read or access watchpoint, if the address does not match, remember
         to turn off the print for the newly added bpstat.


[-- Attachment #2: x86-watchpoint.patch2 --]
[-- Type: text/plain, Size: 5430 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	11 Jun 2004 21:22:42 -0000
@@ -2120,8 +2120,13 @@ print_it_typical (bpstat bs)
       break;
 
     case bp_thread_event:
-      /* Not sure how we will get here. 
-	 GDB should not stop for these breakpoints.  */
+      /* We can only get here legitimately if something further on the bs 
+	 list has caused the stop status to be noisy.  A valid example
+	 of this is a new thread event and a software watchpoint have
+	 both occurred.  */
+      if (bs->next)
+        return PRINT_UNKNOWN;
+	          
       printf_filtered ("Thread Event Breakpoint: gdb should not stop!\n");
       return PRINT_NOTHING;
       break;
@@ -2683,45 +2688,100 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
     if (b->type == bp_watchpoint ||
 	b->type == bp_hardware_watchpoint)
       {
-	char *message = xstrprintf ("Error evaluating expression for watchpoint %d\n",
+	CORE_ADDR addr;
+	struct value *v;
+	int found = 0;
+
+	/* If we have a hardware watchpoint, ensure that the address
+	   being watched caused the trap event.  */
+	if (b->type == bp_hardware_watchpoint)
+	  {
+	    addr = target_stopped_data_address ();
+	    if (addr == 0)
+	      {
+	        /* Don't stop.  */
+	        bs->print_it = print_it_noop;
+	        bs->stop = 0;
+	        continue;
+	      }
+	    for (v = b->val_chain; v; v = v->next)
+	      {
+	        if (VALUE_LVAL (v) == lval_memory
+	            && ! VALUE_LAZY (v))
+	          {
+		    struct type *vtype = check_typedef (VALUE_TYPE (v));
+
+		    if (v == b->val_chain
+		        || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
+			&& TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
+		      {
+		        CORE_ADDR vaddr;
+
+		        vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
+		        /* Exact match not required.  Within range is
+                           sufficient.  */
+		        if (addr >= vaddr &&
+			    addr < vaddr + TYPE_LENGTH (VALUE_TYPE (v)))
+		          found = 1;
+		      }
+	          }
+	      }
+	  }
+	else /* For a non-hardware watchpoint we need to test values.  */
+	  found = 1;
+	
+	if (found)
+	  {
+	    char *message = xstrprintf ("Error evaluating expression for watchpoint %d\n",
 				    b->number);
-	struct cleanup *cleanups = make_cleanup (xfree, message);
-	int e = catch_errors (watchpoint_check, bs, message, 
-			      RETURN_MASK_ALL);
-	do_cleanups (cleanups);
-	switch (e)
+	    struct cleanup *cleanups = make_cleanup (xfree, message);
+	    int e = catch_errors (watchpoint_check, bs, message, 
+				  RETURN_MASK_ALL);
+	    do_cleanups (cleanups);
+	    switch (e)
+	      {
+	      case WP_DELETED:
+		/* We've already printed what needs to be printed.  */
+		/* Actually this is superfluous, because by the time we
+		   call print_it_typical() the wp will be already deleted,
+		   and the function will return immediately. */
+		bs->print_it = print_it_done;
+		/* Stop.  */
+		break;
+	      case WP_VALUE_CHANGED:
+		/* Stop.  */
+		++(b->hit_count);
+		break;
+	      case WP_VALUE_NOT_CHANGED:
+		/* Don't stop.  */
+		bs->print_it = print_it_noop;
+		bs->stop = 0;
+		continue;
+	      default:
+		/* Can't happen.  */
+		/* FALLTHROUGH */
+	      case 0:
+		/* Error from catch_errors.  */
+		printf_filtered ("Watchpoint %d deleted.\n", b->number);
+		if (b->related_breakpoint)
+		  b->related_breakpoint->disposition = disp_del_at_next_stop;
+		b->disposition = disp_del_at_next_stop;
+		/* We've already printed what needs to be printed.  */
+		bs->print_it = print_it_done;
+		
+		/* Stop.  */
+		break;
+	      }
+	  }
+	else	/* found == 0 */
 	  {
-	  case WP_DELETED:
-	    /* We've already printed what needs to be printed.  */
-	    /* Actually this is superfluous, because by the time we
-               call print_it_typical() the wp will be already deleted,
-               and the function will return immediately. */
-	    bs->print_it = print_it_done;
-	    /* Stop.  */
-	    break;
-	  case WP_VALUE_CHANGED:
-	    /* Stop.  */
-	    ++(b->hit_count);
-	    break;
-	  case WP_VALUE_NOT_CHANGED:
-	    /* Don't stop.  */
+	    /* This is a case where some watchpoint(s) triggered,
+	       but not at the address of this watchpoint (FOUND
+	       was left zero).  So don't print anything for this
+	       watchpoint.  */
 	    bs->print_it = print_it_noop;
 	    bs->stop = 0;
-	    continue;
-	  default:
-	    /* Can't happen.  */
-	    /* FALLTHROUGH */
-	  case 0:
-	    /* Error from catch_errors.  */
-	    printf_filtered ("Watchpoint %d deleted.\n", b->number);
-	    if (b->related_breakpoint)
-	      b->related_breakpoint->disposition = disp_del_at_next_stop;
-	    b->disposition = disp_del_at_next_stop;
-	    /* We've already printed what needs to be printed.  */
-	    bs->print_it = print_it_done;
-
-	    /* Stop.  */
-	    break;
+            continue;
 	  }
       }
     else if (b->type == bp_read_watchpoint || 
@@ -2733,7 +2793,12 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
 
 	addr = target_stopped_data_address ();
 	if (addr == 0)
-	  continue;
+	  {
+	    /* Don't stop.  */
+	    bs->print_it = print_it_noop;
+	    bs->stop = 0;
+	    continue;
+	  }
 	for (v = b->val_chain; v; v = v->next)
 	  {
 	    if (VALUE_LVAL (v) == lval_memory

Re: [RFC]: x86 threaded watchpoint support [2/3]

[Eli Zaretskii]

> Date: Fri, 11 Jun 2004 17:33:34 -0400
> From: Jeff Johnston <jjohnstn@redhat.com>
> 
> The most major change is that a check has been added for a
> hardware_watchpoint to ensure that the stopped data address matches
> the watchpoint address.

I don't necessarily object to this change, but could you first explain
why is this needed, while it was never needed before?  (I have my
guess for the answer, but I'd like to hear yours.)

I have also a minor comment about the change itself, see below.

> @@ -2683,45 +2688,100 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
>      if (b->type == bp_watchpoint ||
>  	b->type == bp_hardware_watchpoint)
>        {
> -	char *message = xstrprintf ("Error evaluating expression for watchpoint %d\n",
> +	CORE_ADDR addr;
> +	struct value *v;
> +	int found = 0;
> +
> +	/* If we have a hardware watchpoint, ensure that the address
> +	   being watched caused the trap event.  */
> +	if (b->type == bp_hardware_watchpoint)
> +	  {
> +	    addr = target_stopped_data_address ();
> +	    if (addr == 0)
> +	      {
> +	        /* Don't stop.  */
> +	        bs->print_it = print_it_noop;
> +	        bs->stop = 0;
> +	        continue;
> +	      }
> +	    for (v = b->val_chain; v; v = v->next)

It looks to me that this change makes the bp_hardware_watchpoint case
exactly identical to bp_read_watchpoint and bp_access_watchpoint, is
that right?  If so, why not add bp_hardware_watchpoint to the if
clause that handles read and access watchpoints, and leave only
bp_watchpoint alone with the current code?

Re: [RFC]: x86 threaded watchpoint support [2/3]

[Jeff Johnston]

Eli Zaretskii wrote:
>>Date: Fri, 11 Jun 2004 17:33:34 -0400
>>From: Jeff Johnston <jjohnstn@redhat.com>
>>
>>The most major change is that a check has been added for a
>>hardware_watchpoint to ensure that the stopped data address matches
>>the watchpoint address.
> 
> 
> I don't necessarily object to this change, but could you first explain
> why is this needed, while it was never needed before?  (I have my
> guess for the answer, but I'd like to hear yours.)
> 

I have since found out that certain platforms do not actually know the stopped 
data address.  Thus, the original code was needed to support such platforms.  as 
you had to figure out which values changed manually.  This always applies to 
software watchpoints.

The change is needed because with the threading model, you can have multiple 
events to process.  So, if you check your watchpoint values, all of them may 
have changed but you end up reporting an invalid thread location.  For example, 
I was getting watchpoints changing at the same time of a new thread event (it 
couldn't discern).  The reported location was __nptl_create_event (not very 
useful).  An end-user would have to do a info thread to find out the location 
for each thread and determine where the real culprit was.  If you have a lot of 
threads, you can see this being very frustrating.

In the case of x86, we do know the stopped data address so we shouldn't bother 
looking at the changed value of a hardware watchpoint that doesn't match.  With 
the fix, it is possible to see the proper location of where a single watchpoint 
location was modified.  Very useful.

> I have also a minor comment about the change itself, see below.
> 
> 
>>@@ -2683,45 +2688,100 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
>>     if (b->type == bp_watchpoint ||
>> 	b->type == bp_hardware_watchpoint)
>>       {
>>-	char *message = xstrprintf ("Error evaluating expression for watchpoint %d\n",
>>+	CORE_ADDR addr;
>>+	struct value *v;
>>+	int found = 0;
>>+
>>+	/* If we have a hardware watchpoint, ensure that the address
>>+	   being watched caused the trap event.  */
>>+	if (b->type == bp_hardware_watchpoint)
>>+	  {
>>+	    addr = target_stopped_data_address ();
>>+	    if (addr == 0)
>>+	      {
>>+	        /* Don't stop.  */
>>+	        bs->print_it = print_it_noop;
>>+	        bs->stop = 0;
>>+	        continue;
>>+	      }
>>+	    for (v = b->val_chain; v; v = v->next)
> 
> 
> It looks to me that this change makes the bp_hardware_watchpoint case
> exactly identical to bp_read_watchpoint and bp_access_watchpoint, is
> that right?  If so, why not add bp_hardware_watchpoint to the if
> clause that handles read and access watchpoints, and leave only
> bp_watchpoint alone with the current code?
> 

I definitely could have combined the code more efficiently for the two if 
clauses after making my changes.

I have talked to Andrew and he suggests that the target_stopped_data_address 
code should be modified to add a status code instead of the current 0 addr 
return value.  The address of 0 could be a valid watchpoint or could indicate no 
watchpoint triggered or failure or the lower level code "doesn't know" so the 
current interface isn't robust enough. I will be performing another version of 
the change to add this and that should handle platforms such as S390 but allow 
platforms like the x86 to work properly.  I will also combine the two if clauses 
better this time :)

-- Jeff J.

Re: [RFC]: x86 threaded watchpoint support [2/3]

[Eli Zaretskii]

> Date: Mon, 14 Jun 2004 17:40:20 -0400
> From: Jeff Johnston <jjohnstn@redhat.com>
> 
> The change is needed because with the threading model, you can have multiple 
> events to process.  So, if you check your watchpoint values, all of them may 
> have changed but you end up reporting an invalid thread location.  For example, 
> I was getting watchpoints changing at the same time of a new thread event (it 
> couldn't discern).  The reported location was __nptl_create_event (not very 
> useful).

Does this mean that when a watchpoint breaks, it stops only the thread
that hit the watchpoint, while other threads continue to run?

If all threads stop, then there could not be multiple events, unless
we are talking about a machine with more than one CPU.

Or am I missing something?

> I have talked to Andrew and he suggests that the target_stopped_data_address 
> code should be modified to add a status code instead of the current 0 addr 
> return value.

That plan is okay with me, but I suggest to run the design by us
before you start coding, in case there are platforms out there that
need some adjustments in what you intend to do.

Thanks.

Re: [RFC]: x86 threaded watchpoint support [2/3]

[Daniel Jacobowitz]

On Tue, Jun 15, 2004 at 07:19:17AM +0200, Eli Zaretskii wrote:
> > Date: Mon, 14 Jun 2004 17:40:20 -0400
> > From: Jeff Johnston <jjohnstn@redhat.com>
> > 
> > The change is needed because with the threading model, you can have multiple 
> > events to process.  So, if you check your watchpoint values, all of them may 
> > have changed but you end up reporting an invalid thread location.  For example, 
> > I was getting watchpoints changing at the same time of a new thread event (it 
> > couldn't discern).  The reported location was __nptl_create_event (not very 
> > useful).
> 
> Does this mean that when a watchpoint breaks, it stops only the thread
> that hit the watchpoint, while other threads continue to run?
> 
> If all threads stop, then there could not be multiple events, unless
> we are talking about a machine with more than one CPU.
> 
> Or am I missing something?

It's the same problem we already have, just more likely to trigger. 
The kernel does not stop threads for us; we do it ourselves after we
see the first thread trap.  Even if the kernel did it, there would be a
substantial window for other threads to run.

So any time that we see an event from one thread, there can be an
arbitrary set of events from other threads.  In a heavily threaded
application, having extra events at the thread creation breakpoint is
typical.

-- 
Daniel Jacobowitz

Re: [RFC]: x86 threaded watchpoint support [2/3]

[Ulrich Weigand]

Jeff Johnston wrote:

>The most major change is that a check has been added for a
>hardware_watchpoint to ensure that the stopped data address matches
>the watchpoint address. I do not know if there are platforms that
>have hardware_watchpoints but cannot tell what the data address is.

This is the case on s390/s390x, and apparently also some other
platforms.  See the discussion at and around:
http://sources.redhat.com/ml/gdb-patches/2004-05/msg00123.html


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

Re: [RFC]: x86 threaded watchpoint support [2/3]

[Ulrich Weigand]

Jeff Johnston wrote:

>The change is needed because with the threading model, you can have
>multiple events to process. So, if you check your watchpoint values,
>all of them may have changed but you end up reporting an invalid thread
>location. For example, I was getting watchpoints changing at the same
>time of a new thread event (it couldn't discern).

Isn't it enough to know whether you got the SIGTRAP because of a (any)
hardware watchpoint, or for some other reason (e.g. new thread event)?

*This* information is available on s390, and as of this patch:
http://sources.redhat.com/ml/gdb-patches/2004-05/msg00290.html
it is actually used to prevent bpstat_stop_status to recognize
a watchpoint event unless the kernel says it was a watchpoint ...

Do you have more details about n what situation is this still not
enough?


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

Re: [RFC]: x86 threaded watchpoint support [2/3]

[jjohnstn]

On Tue, 15 Jun 2004, Ulrich Weigand wrote:

> 
> 
> 
> 
> Jeff Johnston wrote:
> 
> >The change is needed because with the threading model, you can have
> >multiple events to process. So, if you check your watchpoint values,
> >all of them may have changed but you end up reporting an invalid thread
> >location. For example, I was getting watchpoints changing at the same
> >time of a new thread event (it couldn't discern).
> 
> Isn't it enough to know whether you got the SIGTRAP because of a (any)
> hardware watchpoint, or for some other reason (e.g. new thread event)?
> 
> *This* information is available on s390, and as of this patch:
> http://sources.redhat.com/ml/gdb-patches/2004-05/msg00290.html
> it is actually used to prevent bpstat_stop_status to recognize
> a watchpoint event unless the kernel says it was a watchpoint ...
>

Actually, I believe this patch needs to be reworked.  It is causing a 
failure for ia64 watchpoints.  There are a number of problems to be looked 
at.  First of all, the stopped_by_watchpoint flag only gets set if 
HAVE_CONTINUABLE_WATCHPOINT is true.  Thus, any platform that doesn't 
define this macro or have the low level target 
to_have_continuable_watchpoint value set, will never set the 
stopped_by_watchpoint flag.  The logic in 
bpstat_stop_status ignores all hardware watchpoints if the flag is not set 
so we never do the value comparison.

Another problem is that some watchpoints are stepped (i.e. the signal 
occurs before the value is actually changed).  When we step over the 
watchpoint, again the flag won't get set because we have actually trapped 
due to a step operation, not a watchpoint.

 > Do you have more details about n what situation is this still 
not > enough?
> 

Knowing at least one watchpoint triggered is not enough detail. Gdb then 
has to check all values for change.  We may not stop all threads until 
multiple watchpoints change.  Now, imagine you have 1000 threads.  Even 
if we get one thread correct, the end-user has to do an info threads and search 
the other 999 threads to figure out where the other values got changed.  I 
personally use a watchpoint when I know a value has changed but can't 
figure out why it changed or there is some particular transition of value 
I am interested in.  In some cases, gdb won't be able to tie the 
thread to the watchpoint but where we can do so, we definitely should.

-- Jeff J.


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

Re: [RFC]: x86 threaded watchpoint support [2/3]

[Eli Zaretskii]

> Date: Wed, 16 Jun 2004 17:39:28 -0400 (EDT)
> From: jjohnstn <jjohnstn@redhat.com>
> 
> Actually, I believe this patch needs to be reworked.  It is causing a 
> failure for ia64 watchpoints.  There are a number of problems to be looked 
> at.  First of all, the stopped_by_watchpoint flag only gets set if 
> HAVE_CONTINUABLE_WATCHPOINT is true.  Thus, any platform that doesn't 
> define this macro or have the low level target 
> to_have_continuable_watchpoint value set, will never set the 
> stopped_by_watchpoint flag.  The logic in 
> bpstat_stop_status ignores all hardware watchpoints if the flag is not set 
> so we never do the value comparison.
> 
> Another problem is that some watchpoints are stepped (i.e. the signal 
> occurs before the value is actually changed).  When we step over the 
> watchpoint, again the flag won't get set because we have actually trapped 
> due to a step operation, not a watchpoint.

Can you suggest a patch to take care of these problems?

> Knowing at least one watchpoint triggered is not enough detail. Gdb then 
> has to check all values for change.

If all the platform does is tell us that _some_ watchpoint breaks,
what else can we do?

> We may not stop all threads until multiple watchpoints change.

This should produce several more SIGTRAPs, one each for every
watchpoint that triggers in between.  Can we somehow find out there
are several more SIGTRAPs pending, and what threads have them pending?

If not, I again don't see how can GDB do better in this situation.
Sounds like some help from the kernel is in order.

Re: [RFC]: x86 threaded watchpoint support [2/3]

[Jeff Johnston]

Eli Zaretskii wrote:
>>Date: Wed, 16 Jun 2004 17:39:28 -0400 (EDT)
>>From: jjohnstn <jjohnstn@redhat.com>
>>
>>Actually, I believe this patch needs to be reworked.  It is causing a 
>>failure for ia64 watchpoints.  There are a number of problems to be looked 
>>at.  First of all, the stopped_by_watchpoint flag only gets set if 
>>HAVE_CONTINUABLE_WATCHPOINT is true.  Thus, any platform that doesn't 
>>define this macro or have the low level target 
>>to_have_continuable_watchpoint value set, will never set the 
>>stopped_by_watchpoint flag.  The logic in 
>>bpstat_stop_status ignores all hardware watchpoints if the flag is not set 
>>so we never do the value comparison.
>>
>>Another problem is that some watchpoints are stepped (i.e. the signal 
>>occurs before the value is actually changed).  When we step over the 
>>watchpoint, again the flag won't get set because we have actually trapped 
>>due to a step operation, not a watchpoint.
> 
> 
> Can you suggest a patch to take care of these problems?
>

I intend to.  I just wanted to point out that patch is causing a number of 
problems people are seeing.  I have to first respond to a myriad of comments to 
other patches of mine.

> 
>>Knowing at least one watchpoint triggered is not enough detail. Gdb then 
>>has to check all values for change.
> 
> 
> If all the platform does is tell us that _some_ watchpoint breaks,
> what else can we do?
>

You cut out my later sentence which translated to: "if gdb can get the 
information, it should do so, otherwise it will have to do its best".

> 
>>We may not stop all threads until multiple watchpoints change.
> 
> 
> This should produce several more SIGTRAPs, one each for every
> watchpoint that triggers in between.  Can we somehow find out there
> are several more SIGTRAPs pending, and what threads have them pending?
> 
> If not, I again don't see how can GDB do better in this situation.
> Sounds like some help from the kernel is in order.
>

In some cases, the best gdb will be able to do is check all watchpoints and 
print out all values that have changed.  Yes, some kernel changes could help in 
these situations.

-- Jeff J.