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

[Pedro Alves <pedro@codesourcery.com>]

On Saturday 08 May 2010 17:46:11, Pierre Muller wrote:
> > > +    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.

Thanks.


> > >  }
> > > +/*
> > > + * Function procfs_stopped_data_address
> > > + * Returns non-zero if we can find the position
> > > + * of the triggring watchpoint.
> > > + */
> > > +

Typo, "triggring".

> > > +static int
> > > +procfs_stopped_data_address (struct target_ops *targ, CORE_ADDR
> > *addr)
> > > +{
> > > +  CORE_ADDR waddr;
> > > +  procinfo *pi;
> > > +  int watchpoint_hit = 0;

Left over unused variable.

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

Well, we can never reach here without finding a procinfo, unless we have a
weird bug somewhere.  This function is only ever called if
to_stopped_by_watchpoint returned true.  If that one returned true, a
procinfo must be on the list..  And when to_stopped_by_watchpoint
is called, the thread that reported the watchpoint must be on
the list, an hence so its procinfo...

I'd prefer to simply remove these checks; they really add nothing, and only
add to the mess that is procfs.c... IMO.

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

I'd think so, but I didn't go check solaris's man
pages to check which lwp reports the watchpoint hit.  Did you?

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

Sounds good, but the numbers alone aren't the full story: are any of
those 8 FAILs regressions?

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

If the "watchthreads*" are passing cleanly, it sounds good.
If they're not passing neither before/after patch, you haven't
really tested the change.  All other watchpoints tests in the
testsuite are single-threaded, IIRC.

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

Yes.

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

Please remove all mentions of "__data.__fault.__addr".  It's an
internal detail you're not really supposed to be exposing
anywhere.  All accesses to siginfo_t fields like those go
through macros like si_addr, si_FOO, everywhere, in all
OSs.  Actually, the whole comment doesn't add much.  It is
just repeating what the code does, like this:

  /* Increment i by one.  */
  i++;

It should instead describe what the function does from
a high level perspective.  E.g., something like:

  Assuming PI is stopped at a watchpoint, and the OS supports
  it, write to *ADDR the data address which triggered it and return 1.
  Returns 0 if the 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;
> +}
> +
>  
>  #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;
>  }
> +/*

Space between functions.

> + * Function procfs_stopped_data_address
> + * Returns non-zero if we can find the position
> + * of the triggring watchpoint.

Typo, "triggring".


> + */
> +
> +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;
>  }
>  
>  /*


-- 
Pedro Alves