RE: [RFC] Add watchpoint hit address function to procfs.c

["Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>]

> > +/*
> > + * Function: proc_watchpoint_address
> > + *
> > + * returns the prstatus.pr_lwp.pr_info.__data.__fault.__addr field.
> > + */
> > +
> > +CORE_ADDR
> > +proc_watchpoint_address (procinfo *pi)
> > +{
> > +  if (!pi->status_valid)
> > +    if (!proc_get_status (pi))
> > +      return 0;	/* FIXME: not a good failure value (but what
> is?) */
> 
> Add a new CORE_ADDR pointer parameter, and use that as
> output. C hange the return type to int to indicate failure/success.
> That is, model the interface of this function from
> to_stopped_data_address's
> interface.

  OK, modified as you suggested.
 
> > +
> > +#ifdef NEW_PROC_API
> > +  return unsigned_pointer_to_address (target_gdbarch,
> > +	   builtin_type (target_gdbarch)->builtin_data_ptr,
> > +	   (gdb_byte *) &pi-
> >prstatus.pr_lwp.pr_info.__data.__fault.__addr);
> > +#else
> > +  return unsigned_pointer_to_address (target_gdbarch,
> > +	   builtin_type (target_gdbarch)->builtin_data_ptr,
> > +	   (gdb_byte *) &pi->prstatus.pr_info.__data.__fault.__addr);
> > +#endif
> 
> Isn't there a macro to access __data.__fault.__addr ?  I'd have
> guessed pi->prstatus.pr_info.sa_addr, similarly to
> how we use pi->prstatus.pr_lwp.pr_info.si_signo already.  That
> is, pr_info is a siginfo_t.  That would probably get rid of
> the NEW_PROC_API usage and worries.
 si_addr is indeed a macro for __data.__fault.__addr
but this doesn't allow to remove the NEW_PROC_API
as there is this extra pr_lwp level for the new api.

> > +}
> > +
> >
> >  #ifndef PIOCSSPCACT	/* The following is not supported on OSF.
*/
> >  /*
> > @@ -5560,6 +5584,45 @@ procfs_stopped_by_watchpoint (void)
> >      }
> >    return 0;
> >  }
> > +/*
> > + * Function procfs_stopped_data_address
> > + * Returns non-zero if we can find the position
> > + * of the triggring watchpoint.
> > + */
> > +
> > +static int
> > +procfs_stopped_data_address (struct target_ops *targ, CORE_ADDR
> *addr)
> > +{
> > +  CORE_ADDR waddr;
> > +  procinfo *pi;
> > +  int watchpoint_hit = 0;
> > +
> > +  pi = find_procinfo_or_die (PIDGET (inferior_ptid), 0);
> > +
> > +  if (!pi)	/* If no process, then not stopped by watchpoint!  */
> > +    return 0;
> 
> Find_procinfo_or_die throws if the procinfo isn't
> found (it's the die part), so this check is dead code.
  Then we should simply call find_procinfo, no?
and also in procfs_stopped_by_watchpoint.
> I wonder if this procinfo is the correct one.  Shouldn't
> this look for the the procinfo for the current thread (the tid
> in inferior_ptid), not the one for the main thread?
  I simply adapted the code from procfs_stopped_by_watchpoint
which also uses (PIDGET (inferior_ptid), 0).

  Should those two be converted into
  (PIDGET (inferior_ptid), TIDGET (inferior_ptid))?
This is what is done below...


  New version of the patch below,
I ran it on a OpenSolaris 2.11 (x86_64 processor)
with RUNTESTFLAGS=gdb*/watch*.exp
It gave 152 PASS for 8 FAIL,
while current CVS gives 127 PASS and 33 FAIL.

  Using TIDGET (inferior_ptid) or 0
had no effect on that subset of the testsuite...

Pierre

PS: I noticed a strange thing for gdb.threads/watchthreads
Each thread is reported twice in `info threads':
(gdb) inf thr
[New Thread 2 (LWP 2)]
[New Thread 3 (LWP 3)]
[New Thread 5 (LWP 5)]
  11 Thread 5 (LWP 5)  0xfffffd7fff2dcf8a in __nanosleep ()
   from /lib/64/libc.so.1
  10 Thread 3 (LWP 3)  0xfffffd7fff2dcf8a in __nanosleep ()
   from /lib/64/libc.so.1
  9 Thread 2 (LWP 2)  0xfffffd7fff2dcf8a in __nanosleep ()
   from /lib/64/libc.so.1
  8 LWP    6          0xfffffd7fff2d4a28 in _thrp_setup ()
   from /lib/64/libc.so.1
* 7 Thread 4 (LWP 4)  0x0000000000401146 in thread_function (arg=0x2)
    at ../../../src/gdb/testsuite/gdb.threads/watchthreads.c:75
  6 LWP    5          0xfffffd7fff2dcf8a in __nanosleep ()
   from /lib/64/libc.so.1
  5 LWP    4          0x0000000000401146 in thread_function (arg=0x2)
    at ../../../src/gdb/testsuite/gdb.threads/watchthreads.c:75
  4 LWP    3          0xfffffd7fff2dcf8a in __nanosleep ()
   from /lib/64/libc.so.1
  3 LWP    2          0xfffffd7fff2dcf8a in __nanosleep ()
   from /lib/64/libc.so.1
  2 Thread 1 (LWP 1)  0xfffffd7fff2de03a in __lwp_create ()
   from /lib/64/libc.so.1
  1 LWP    1          0xfffffd7fff2de03a in __lwp_create ()
   from /lib/64/libc.so.1
Is this expected?



ChangeLog entry unchanged:
 
2010-05-07  Pierre Muller  <muller@ics.u-strasbg.fr>

	* procfs.c (proc_watchpoint_address): New function.
	(procfs_stopped_data_address): New function.
	(procfs_use_watchpoints): Register new watchpoint related
	function.

Index: src/gdb/procfs.c
===================================================================
RCS file: /cvs/src/src/gdb/procfs.c,v
retrieving revision 1.131
diff -u -p -r1.131 procfs.c
--- src/gdb/procfs.c	20 Apr 2010 22:36:35 -0000	1.131
+++ src/gdb/procfs.c	7 May 2010 16:01:59 -0000
@@ -1312,6 +1312,33 @@ proc_what (procinfo *pi)
   return pi->prstatus.pr_what;
 #endif
 }
+/*
+ * Function: proc_watchpoint_address
+ *
+ * returns the prstatus.pr_lwp.pr_info.__data.__fault.__addr field in ADDR.
+ *
+ * si_addr is a macro for __data.__fault.__addr.
+ */
+
+static int
+proc_watchpoint_address (procinfo *pi, CORE_ADDR *addr)
+{
+  if (!pi->status_valid)
+    if (!proc_get_status (pi))
+      return 0;
+
+#ifdef NEW_PROC_API
+  *addr = (CORE_ADDR) unsigned_pointer_to_address (target_gdbarch,
+	    builtin_type (target_gdbarch)->builtin_data_ptr,
+	    (gdb_byte *) &pi->prstatus.pr_lwp.pr_info.si_addr);
+#else
+  *addr = (CORE_ADDR) unsigned_pointer_to_address (target_gdbarch,
+	    builtin_type (target_gdbarch)->builtin_data_ptr,
+	    (gdb_byte *) &pi->prstatus.pr_info.si_addr);
+#endif
+  return 1;
+}
+
 
 #ifndef PIOCSSPCACT	/* The following is not supported on OSF.  */
 /*
@@ -5539,7 +5566,7 @@ procfs_stopped_by_watchpoint (void)
 {
   procinfo *pi;
 
-  pi = find_procinfo_or_die (PIDGET (inferior_ptid), 0);
+  pi = find_procinfo (PIDGET (inferior_ptid), TIDGET (inferior_ptid));
 
   if (!pi)	/* If no process, then not stopped by watchpoint!  */
     return 0;
@@ -5560,6 +5587,29 @@ procfs_stopped_by_watchpoint (void)
     }
   return 0;
 }
+/*
+ * Function procfs_stopped_data_address
+ * Returns non-zero if we can find the position
+ * of the triggring watchpoint.
+ */
+
+static int
+procfs_stopped_data_address (struct target_ops *targ, CORE_ADDR *addr)
+{
+  CORE_ADDR waddr = 0;
+  procinfo *pi;
+  int res;
+
+  pi = find_procinfo (PIDGET (inferior_ptid), TIDGET (inferior_ptid));
+
+  if (!pi)	/* If no process, then not stopped by watchpoint!  */
+    return 0;
+
+  res = proc_watchpoint_address (pi, &waddr);
+  if (addr)
+    *addr = waddr;
+  return res;
+}
 
 static int
 procfs_insert_watchpoint (CORE_ADDR addr, int len, int type)
@@ -5607,6 +5657,7 @@ procfs_use_watchpoints (struct target_op
   t->to_remove_watchpoint = procfs_remove_watchpoint;
   t->to_region_ok_for_hw_watchpoint = procfs_region_ok_for_hw_watchpoint;
   t->to_can_use_hw_breakpoint = procfs_can_use_hw_breakpoint;
+  t->to_stopped_data_address = procfs_stopped_data_address;
 }
 
 /*