Re: Hardware watchpoints

[Eli Zaretskii <eliz@gnu.org>]

As promised, diffs for all the changes that avoid inserting/removing
watchpoints for lazy values are attached below.

Now for the next problem: hardware watchpoints still don't work, even
after these changes, for struct members that are bitfields.  As far as
I could see, the reason for this is that value_primitive_field needs
to fetch the value of the field in order to unpack it into an int, and
that causes the lazy flag of the struct itself to be reset.

It seems that relying on the lazy flag is too fragile: any code that
needs to fetch the value of the struct, for some purpose, will break
the ability to watch members of that struct.

Perhaps we need a special flag for this, which will be set for structs
and arrays only if they are not the watched expression itself.


	* breakpoint.c (insert_breakpoints): Fetch the value of the
	expression we need to watch, to make sure its LAZY flag is off.
	Don't insert watchpoints for values whose LAZY flag is set.
	(remove_breakpoint): Don't remove a watchpoint if the value's LAZY
	flag is set (since we didn't insert them in the first place).
	(bpstat_stop_status): Don't check values whose LAZY flag is set,
	since we don't watch them.


*** gdb/breakpoi.c~4	Sun Aug 22 19:03:14 1999
--- gdb/breakpoi.c	Wed Oct 27 19:37:04 1999
*************** insert_breakpoints ()
*** 779,784 ****
--- 779,786 ----
  	      /* Evaluate the expression and cut the chain of values
  		 produced off from the value chain.  */
  	      v = evaluate_expression (b->exp);
+ 	      if (VALUE_LAZY (v))
+ 		value_fetch_lazy (v);
  	      value_release_to_mark (mark);
  	    
  	      b->val_chain = v;
*************** insert_breakpoints ()
*** 788,794 ****
  	      for ( ; v; v=v->next)
  		{
  		  /* If it's a memory location, then we must watch it.  */
! 		  if (v->lval == lval_memory)
  		    {
  		      int addr, len, type;
  		    
--- 790,796 ----
  	      for ( ; v; v=v->next)
  		{
  		  /* If it's a memory location, then we must watch it.  */
! 		  if (v->lval == lval_memory && !v->lazy)
  		    {
  		      int addr, len, type;
  		    
*************** remove_breakpoint (b, is)
*** 1127,1133 ****
  	{
  	  /* For each memory reference remove the watchpoint
  	     at that address.  */
! 	  if (v->lval == lval_memory)
  	    {
  	      int addr, len, type;
  	      
--- 1129,1135 ----
  	{
  	  /* For each memory reference remove the watchpoint
  	     at that address.  */
! 	  if (v->lval == lval_memory && !v->lazy)
  	    {
  	      int addr, len, type;
  	      
*************** bpstat_stop_status (pc, not_a_breakpoint
*** 2199,2205 ****
  	    }
            for (v = b->val_chain; v; v = v->next)
              {
!               if (v->lval == lval_memory)
                  {
                    CORE_ADDR vaddr;
  
--- 2201,2207 ----
  	    }
            for (v = b->val_chain; v; v = v->next)
              {
!               if (v->lval == lval_memory && !v->lazy)
                  {
                    CORE_ADDR vaddr;
  
*************** can_use_hardware_watchpoint (v)
*** 4486,4492 ****
       hardware watchpoint for the constant expression.  */
    for ( ; v; v = v->next)
      {
!       if (v->lval == lval_memory)
  	{
  	  int vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
  	  int len = TYPE_LENGTH (VALUE_TYPE (v));
--- 4488,4494 ----
       hardware watchpoint for the constant expression.  */
    for ( ; v; v = v->next)
      {
!       if (v->lval == lval_memory && !v->lazy)
  	{
  	  int vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
  	  int len = TYPE_LENGTH (VALUE_TYPE (v));
From hjl@valinux.com Wed Oct 27 13:43:00 1999
From: hjl@valinux.com (H.J. Lu)
To: gdb-patches@sourceware.cygnus.com
Cc: slouken@devolution.com, gdb@sourceware.cygnus.com (GDB)
Subject: A patch for shared library support
Date: Wed, 27 Oct 1999 13:43:00 -0000
Message-id: <19991027204300.7816E3FC1@valinux.com>
X-SW-Source: 1999-q4/msg00175.html
Content-length: 3401

Hi,

I finally get annoyed enough to port Sam's patch to gdb 4.18 in CVS.
When you set a break point in shared library, gdb will crash when you
restart the program. This patch seems to work for me.


-- 
H.J. Lu (hjl@gnu.org)
--
Wed Mar 17 19:49:22 1999  Sam Lantinga (slouken@devolution.com)

	Added function check_solib_consistency() to reload list of
	shared objects when they are added or deleted. This fixed
	crashing when the program being debugged unloaded a dynamic
	library and added a new library afterwards.

	* solib.h (CHECK_SOLIB_CONSISTENCY): New.
	(check_solib_consistency): New prototype.

	* solib.c (check_solib_consistency): Defined.

	* infrun.c (handle_inferior_event): Before calling SOLIB_ADD (),
	call CHECK_SOLIB_CONSISTENCY () if defined.

Index: solib.h
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/solib.h,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 solib.h
--- solib.h	1999/09/09 00:38:38	1.1.1.1
+++ solib.h	1999/10/27 20:35:54
@@ -186,6 +186,11 @@ solib_create_inferior_hook PARAMS ((void
 extern char *
   solib_address PARAMS ((CORE_ADDR));	/* solib.c */
 
+/* Check shared library consistency */
+
+#define CHECK_SOLIB_CONSISTENCY()	check_solib_consistency()
+extern void check_solib_consistency PARAMS ((void));
+
 /* If ADDR lies in a shared library, return its name.  */
 
 #define PC_SOLIB(addr)	solib_address (addr)
Index: solib.c
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/solib.c,v
retrieving revision 1.1.1.2
diff -u -p -r1.1.1.2 solib.c
--- solib.c	1999/10/19 16:16:27	1.1.1.2
+++ solib.c	1999/10/27 20:29:03
@@ -950,6 +954,38 @@ open_symbol_file_object (arg)
   return 1;
 }
 #endif /* SVR4_SHARED_LIBS */
+
+/*
+  
+GLOBAL FUNCTION
+
+	check_solib_consistency -- check solib list consistency
+
+SYNOPSIS
+
+	void check_solib_consistency (void)
+
+DESCRIPTION
+
+	This module is called whenever we hit a dynamic linker breakpoint
+	and allows us to check the consistency of our shared object list.
+	Without this, dynamic unlinking of objects could crash us.
+ */
+
+void
+check_solib_consistency (void)
+{
+#ifdef SVR4_SHARED_LIBS
+
+  if ( debug_base ) {
+    read_memory (debug_base, (char *) &debug_copy, sizeof (struct r_debug));
+    /* If the shared object state is consistent, we can reload our list */
+    if ( debug_copy.r_state == RT_CONSISTENT )
+      clear_solib();
+  }
+
+#endif	/* SVR4_SHARED_LIBS */
+}
 
 /*
 
Index: infrun.c
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/infrun.c,v
retrieving revision 1.1.1.2
diff -u -p -r1.1.1.2 infrun.c
--- infrun.c	1999/10/19 16:16:22	1.1.1.2
+++ infrun.c	1999/10/27 20:30:20
@@ -1479,6 +1479,9 @@ handle_inferior_event (struct execution_
 		/* Switch terminal for any messages produced by
 		   breakpoint_re_set.  */
 		target_terminal_ours_for_output ();
+#ifdef CHECK_SOLIB_CONSISTENCY
+		CHECK_SOLIB_CONSISTENCY();
+#endif
 		SOLIB_ADD (NULL, 0, NULL);
 		target_terminal_inferior ();
 	      }
@@ -2409,6 +2412,9 @@ handle_inferior_event (struct execution_
 		/* Switch terminal for any messages produced by
 		   breakpoint_re_set.  */
 		target_terminal_ours_for_output ();
+#ifdef CHECK_SOLIB_CONSISTENCY
+		CHECK_SOLIB_CONSISTENCY();
+#endif
 		SOLIB_ADD (NULL, 0, NULL);
 		target_terminal_inferior ();
 	      }
From jimb@cygnus.com Wed Oct 27 23:44:00 1999
From: Jim Blandy <jimb@cygnus.com>
To: Eli Zaretskii <eliz@gnu.org>
Cc: gdb@sourceware.cygnus.com
Subject: Re: Hardware watchpoints
Date: Wed, 27 Oct 1999 23:44:00 -0000
Message-id: <npk8o8m1ch.fsf@zwingli.cygnus.com>
References: <np3dv6k64w.fsf@zwingli.cygnus.com> <19991019235249.917DC1B494@ocean.lucon.org> <199910201401.KAA28719@mescaline.gnu.org> <npvh82htxn.fsf@zwingli.cygnus.com> <199910221200.IAA24556@mescaline.gnu.org> <npn1tbnr5f.fsf@zwingli.cygnus.com> <199910231048.GAA31392@mescaline.gnu.org> <npaep8o97e.fsf@zwingli.cygnus.com> <npu2ndmzyh.fsf@zwingli.cygnus.com> <199910272007.QAA04553@mescaline.gnu.org>
X-SW-Source: 1999-q4/msg00176.html
Content-length: 1309

> Now for the next problem: hardware watchpoints still don't work, even
> after these changes, for struct members that are bitfields.  As far as
> I could see, the reason for this is that value_primitive_field needs
> to fetch the value of the field in order to unpack it into an int, and
> that causes the lazy flag of the struct itself to be reset.
> 
> It seems that relying on the lazy flag is too fragile: any code that
> needs to fetch the value of the struct, for some purpose, will break
> the ability to watch members of that struct.
> 
> Perhaps we need a special flag for this, which will be set for structs
> and arrays only if they are not the watched expression itself.

I agree that using the value chain is fragile.  It's my impression
that it was never intended for anything more than freeing old values.
Nobody ever promised that it would have any specific semantics.

Instead of adding yet another special flag, we could change
value_primitive_field, or whoever extracts bitfields, to create a new
value object, also on the chain, containing just the words from the
original value that hold the bitfield, and then let that smaller value
become unlazy.  That should yield exactly the words we need to set
hardware watchpoints on.

I'm not sure what type that intermediate value should have.
From aj@suse.de Thu Oct 28 01:31:00 1999
From: Andreas Jaeger <aj@suse.de>
To: gdb@sourceware.cygnus.com
Subject: Patch for warning in gdb/linux-thread.c
Date: Thu, 28 Oct 1999 01:31:00 -0000
Message-id: <u8ogdjx4xt.fsf@gromit.rhein-neckar.de>
X-SW-Source: 1999-q4/msg00177.html
Content-length: 5354

The current cvs version of gdb produces a number of harmless warnings
in linuxthreads.  I've fixed the following warnings with the appended
patch.

../../cvs/gdb/gdb/linux-thread.c:165: warning: missing initializer
../../cvs/gdb/gdb/linux-thread.c:165: warning: (near initialization for `linuxthreads_sig_restart.print')
../../cvs/gdb/gdb/linux-thread.c:168: warning: missing initializer
../../cvs/gdb/gdb/linux-thread.c:168: warning: (near initialization for `linuxthreads_sig_cancel.print')
../../cvs/gdb/gdb/linux-thread.c:171: warning: missing initializer
../../cvs/gdb/gdb/linux-thread.c:171: warning: (near initialization for `linuxthreads_sig_debug.print')
../../cvs/gdb/gdb/linux-thread.c: In function `linuxthreads_find_trap':
../../cvs/gdb/gdb/linux-thread.c:338: warning: suggest explicit braces to avoid ambiguous `else'
../../cvs/gdb/gdb/linux-thread.c: In function `resume_thread':
../../cvs/gdb/gdb/linux-thread.c:653: warning: suggest explicit braces to avoid ambiguous `else'
../../cvs/gdb/gdb/linux-thread.c: In function `stop_thread':
../../cvs/gdb/gdb/linux-thread.c:681: warning: suggest explicit braces to avoid ambiguous `else'
../../cvs/gdb/gdb/linux-thread.c: In function `linuxthreads_wait':
../../cvs/gdb/gdb/linux-thread.c:1286: warning: suggest explicit braces to avoid ambiguous `else'
../../cvs/gdb/gdb/linux-thread.c:1366: warning: suggest explicit braces to avoid ambiguous `else'

Andreas

1999-10-28  Andreas Jaeger  <aj@suse.de>

	* linux-thread.c: Add missing initializers to avoid gcc warnings.
	(resume_thread): Add braces as  recommended by gcc -Wparentheses.
	(stop_thread): Likewise.
	(linuxthreads_wait): Likewise.
	(linuxthreads_find_trap): Likewise.

--- gdb/linux-thread.c.~1~	Thu Sep  9 01:59:18 1999
+++ gdb/linux-thread.c	Thu Oct 28 10:28:53 1999
@@ -161,13 +161,13 @@ struct linuxthreads_signal {
 };
 
 struct linuxthreads_signal linuxthreads_sig_restart = {
-  "__pthread_sig_restart", 1, 0, 0, 0
+  "__pthread_sig_restart", 1, 0, 0, 0, 0
 };
 struct linuxthreads_signal linuxthreads_sig_cancel = {
-  "__pthread_sig_cancel", 1, 0, 0, 0
+  "__pthread_sig_cancel", 1, 0, 0, 0, 0
 };
 struct linuxthreads_signal linuxthreads_sig_debug = {
-  "__pthread_sig_debug", 0, 0, 0, 0
+  "__pthread_sig_debug", 0, 0, 0, 0, 0
 };
 
 /* A table of breakpoint locations, one per PID.  */
@@ -336,10 +336,12 @@ linuxthreads_find_trap (pid, stop)
       else if (WSTOPSIG(status) != SIGSTOP)
 	wstatus[last++] = status;
       else if (stop)
-	if (found_trap)
-	  break;
-	else
-	  found_stop = 1;
+	{
+	  if (found_trap)
+	    break;
+	  else
+	    found_stop = 1;
+	}
     }
 
   /* Resend any other signals we noticed to the thread, to be received
@@ -651,10 +653,12 @@ resume_thread (pid)
   if (pid != inferior_pid
       && in_thread_list (pid)
       && linuxthreads_thread_alive (pid))
-    if (pid == linuxthreads_step_pid)
-      child_resume (pid, 1, linuxthreads_step_signo);
-    else
-      child_resume (pid, 0, TARGET_SIGNAL_0);
+    {
+      if (pid == linuxthreads_step_pid)
+	child_resume (pid, 1, linuxthreads_step_signo);
+      else
+	child_resume (pid, 0, TARGET_SIGNAL_0);
+    }
 }
 
 /* Detach a thread */
@@ -679,21 +683,23 @@ stop_thread (pid)
     int pid;
 {
   if (pid != inferior_pid)
-    if (in_thread_list (pid))
-      kill (pid, SIGSTOP);
-    else if (ptrace (PT_ATTACH, pid, (PTRACE_ARG3_TYPE) 0, 0) == 0)
-      {
-	if (!linuxthreads_attach_pending)
-	  printf_unfiltered ("[New %s]\n", target_pid_to_str (pid));
-	add_thread (pid);
-	if (linuxthreads_sig_debug.signal)
-	  /* After a new thread in glibc 2.1 signals gdb its existence,
-	     it suspends itself and wait for linuxthreads_sig_restart,
-	     now we can wake up it. */
-	  kill (pid, linuxthreads_sig_restart.signal);
-      }
-    else
-      perror_with_name ("ptrace in stop_thread");
+    {
+      if (in_thread_list (pid))
+	kill (pid, SIGSTOP);
+      else if (ptrace (PT_ATTACH, pid, (PTRACE_ARG3_TYPE) 0, 0) == 0)
+	{
+	  if (!linuxthreads_attach_pending)
+	    printf_unfiltered ("[New %s]\n", target_pid_to_str (pid));
+	  add_thread (pid);
+	  if (linuxthreads_sig_debug.signal)
+	    /* After a new thread in glibc 2.1 signals gdb its existence,
+	       it suspends itself and wait for linuxthreads_sig_restart,
+	       now we can wake up it. */
+	    kill (pid, linuxthreads_sig_restart.signal);
+	}
+      else
+	perror_with_name ("ptrace in stop_thread");
+    }
 }
 
 /* Wait for a thread */
@@ -1284,10 +1290,12 @@ linuxthreads_wait (pid, ourstatus)
 	      if (rpid > 0)
 		break;
 	      if (rpid < 0)
-		if (errno == EINTR)
-		  continue;
-		else if (save_errno != 0)
-		  break;
+		{
+		  if (errno == EINTR)
+		    continue;
+		  else if (save_errno != 0)
+		    break;
+		}
 
 	      sigsuspend(&omask);
 	    }
@@ -1364,10 +1372,12 @@ linuxthreads_wait (pid, ourstatus)
 	    {
 	      /* Skip SIGSTOP signals.  */
 	      if (!linuxthreads_pending_status (rpid))
-		if (linuxthreads_step_pid == rpid)
-		  child_resume (rpid, 1, linuxthreads_step_signo);
-		else
-		  child_resume (rpid, 0, TARGET_SIGNAL_0);
+		{
+		  if (linuxthreads_step_pid == rpid)
+		    child_resume (rpid, 1, linuxthreads_step_signo);
+		  else
+		    child_resume (rpid, 0, TARGET_SIGNAL_0);
+		}
 	      continue;
 	    }
 

-- 
 Andreas Jaeger   
  SuSE Labs aj@suse.de	
   private aj@arthur.rhein-neckar.de
From bryce@albatross.co.nz Thu Oct 28 03:05:00 1999
From: Bryce McKinlay <bryce@albatross.co.nz>
To: "H.J. Lu" <hjl@valinux.com>
Cc: slouken@devolution.com, GDB <gdb@sourceware.cygnus.com>
Subject: Re: A patch for shared library support
Date: Thu, 28 Oct 1999 03:05:00 -0000
Message-id: <3818203B.28D094A5@albatross.co.nz>
References: <19991027204300.7816E3FC1@valinux.com>
X-SW-Source: 1999-q4/msg00178.html
Content-length: 393

Great! Thanks - this bug has been annoying me too, and the patch seems to have
fixed it. I've noticed that Redhat 6.1's gdb suffers from this as well.

regards

  [ bryce ]


"H.J. Lu" wrote:

> I finally get annoyed enough to port Sam's patch to gdb 4.18 in CVS.
> When you set a break point in shared library, gdb will crash when you
> restart the program. This patch seems to work for me.