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

[Pedro Alves <pedro at codesourcery dot com>]

On Monday 10 May 2010 08:16:08, Pierre Muller wrote:
> 
>   I tried to take all your remarks into account and
> decreased further the size of the patch.

Thanks.  Now we can focus on the real interesting
part of the patch. :-)

>   I also checked the testsuite by 
> running with RUNTESTFLAGS=gdb*/watch*.exp
> with both CVS GDB and with patch applied.
> (I used my modified gdb.exp to avoid problems
> with the 'DOS-like' newlines..)

See here for a workaround that requires no testsuite changes:
 <http://sourceware.org/ml/gdb-patches/2010-04/msg00544.html>

>   So there are no regressions.

Thanks.

> Remaining failures are:
> pierre@fpcOpenSolaris:~/gdbcvs/build64/gdb/testsuite$ grep FAIL gdbv4.sum
> FAIL: gdb.base/watch-read.exp: read watchpoint triggers when value doesn't change, trapping reads and writes
> FAIL: gdb.base/watch-read.exp: only read watchpoint triggers when value doesn't change

Odd.  I don't suppose a procfs client is supposed to
handle overlapping watchpoints instead of having the kernel
handle them?  The test explicitly sets a write watchpoint watching
the same memory as a read watchpoint, and expects reads to still
be trapped.  Maybe the write request overwrote the read
request?  Maybe this is a kernel bug?

> KFAIL: gdb.threads/watchthreads2.exp: gdb can drop watchpoints in
> multithreaded
> app (PRMS: gdb/10116)
> FAIL: gdb.threads/watchthreads.exp: threaded watch loop
> FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit
> FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[1] hit
> FAIL: gdb.threads/watchthreads.exp: watchpoint on args[0] hit in thread
> FAIL: gdb.threads/watchthreads.exp: watchpoint on args[1] hit in thread
> FAIL: gdb.threads/watchthreads.exp: combination of threaded watchpoints = 30
> 
>   I also checked without adding TIDGET(inferior_ptid), leaving 0 instead,
> for both find_procinfo_or_die calls.
> This had no effect on the testsuite output (same 8 failures).

As I said, if you have this test failing, then it may be that
you haven't tested this change at all.  What you could do instead,
is to try running the test manually, and check whether a
watchpoints work in the non-main threads, before and after patch.
The theory was that they weren't working correctly before the patch,
because target_stopped_by_watchpoint would be returning false for
non-main threads.

> 2010-05-10  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	* procfs.c (proc_watchpoint_address): New function.
> 	(procfs_stopped_by_watchpoint): Change second parameter of
> 	find_procinfo_or_die call to TIDGET (INFERIOR_PTID).
> 	Remove useless check after find_procinfo_or_die call.
> 	(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	10 May 2010 06:24:55 -0000
> @@ -1313,6 +1313,34 @@ proc_what (procinfo *pi)
>  #endif
>  }
>  
> +/*
> + * Function: proc_watchpoint_address
> + *
> + *   This function is only called when PI is stopped by a watchpoint.
> + *   Assuming OS supports it, write to *ADDR the data address which
> + * triggered it and return 1.
> + * Return 0 if it is not possible to know the address.
> + */
> +
> +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;
> +}

target_gdbarch looks to be a bit of a stretch, though close
enough probably.  The alternative is to just assign the pointer
to *addr directly, with an uintptr_t cast.  IIRC, last we talked
about this, we concluded that any addresses we see here are always
below the address range that would need sign extension on signed
addresses targets (like mips-irix).  Still, in any case,
if we're going to use an extraction method, please use
gdbarch_pointer_to_address instead of unsigned_pointer_to_address
directly, exactly so that archs that have signed pointers use
signed_pointer_to_address automatically. 

Hmm.  If there's a procfs target that doesn't report
this, but instead leaves the si_addr field as 0, then perhaps we
should revert to the hackish interface of claiming no support
for the stopped data address if si_addr is 0.  Joel, perhaps you could
give this patch a quick test on mips-irix or AIX?  (before
or after it is committed, as you prefer.)  Not sure if mips-irix even
supports watchpoints, but AIX seems to?  Even testing manually, if an OS
doesn't report the stopped data address, then this will completely
break watchpoints (I don't know if they're supported presently).  If
it does report it, then this should make read watchpoints actually
work.

>    return 0;
>  }
> +/*

Still missing empty line between functions.

> + * Function procfs_stopped_data_address
> + * 
> + * Returns 1 if we the OS knows the position of the triggered

("position" is vague, IMO; we say "to that address" not "to that position",
just below.)

> + * watchpoint.  Sets *ADDR to that address.
> + * Returns 0 if OS cannot report that address.
> + * This function is only called if procfs_stopped_by_watchpoint
> + * returned 1, thus no further checks are done.
> + * The function also assumes that ADDR is not NULL.
> + */

(BTW, I'm thinking of reformating all comments in procfs.c to
follow the coding conventions, so we avoid the tentation
of proliferating this other format in procfs.c.  Joel, you're
the most likely to have non-FSF contributed patches to procfs.c.
If that change would make it harder for you to merge patches
upstream, just say so, and I'll forget doing the reformating.)

Otherwise, looks fine.  Please commit when you've
addressed the comments above, and Joel as commented.

-- 
Pedro Alves