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

[Pedro Alves <pedro@codesourcery.com>]

On Monday 26 April 2010 15:17:33, Pierre Muller wrote:
>   The following patch adds support for
> to_stopped_data_address field of target_ops,
> using
> prstatus.pr_lwp.pr_info.__data.__fault.__addr field.
> (or prstatus.pr_info.__data.__fault.__addr if NEW_PROC_API
> is not defined) I found that in the OpenSolaris sources.

> 
>   It works for my OpenSolaris 2.11.
> 
>   The main problem is that I have no ideas if this works for other
> targets using procfs.c source.
> 
>   How should this be modified to avoid problem if other targets
> do not have/set these fields?
> 
>   Comments welcome.
> 
> Pierre Muller
> Pascal language support maintainer for GDB
> 
> 2010-04-26  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: procfs.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/procfs.c,v
> retrieving revision 1.131
> diff -u -p -r1.131 procfs.c
> --- procfs.c	20 Apr 2010 22:36:35 -0000	1.131
> +++ procfs.c	26 Apr 2010 14:05:29 -0000
> @@ -1312,6 +1312,30 @@ 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.
> + */
> +
> +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.

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

> +}
> +
>  
>  #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.
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?

> +
> +  if (proc_flags (pi) & (PR_STOPPED | PR_ISTOP))
> +    {
> +      if (proc_why (pi) == PR_FAULTED)
> +	{
> +#ifdef FLTWATCH
> +	  if (proc_what (pi) == FLTWATCH)
> +	    watchpoint_hit = 1;
> +#endif
> +#ifdef FLTKWATCH
> +	  if (proc_what (pi) == FLTKWATCH)
> +	    watchpoint_hit = 1;
> +#endif
> +	}
> +    }
> +  if (!watchpoint_hit)
> +    return 0;

You can assume that this target_ops method is only
ever called if a watchpoint is hit in the first
place (if target_stopped_by_watchpoint returned
true), so remove all this (duplicated) code.

> +  waddr = proc_watchpoint_address (pi);
> +  if (addr)
> +    *addr = waddr;
> +  return (waddr != 0);
> +}
>  
>  static int
>  procfs_insert_watchpoint (CORE_ADDR addr, int len, int type)
> @@ -5607,6 +5670,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