Re: [PATCH 2/9] Move `compute_probe_arg' and `compile_probe_arg' to probe.c

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Friday, September 26 2014, Jose E. Marchesi wrote:

> This patch moves the `compute_probe_arg' and `compile_probe_arg' functions
> from stap-probe.c to probe.c.  The rationale is that it is reasonable to
> assume that all backends will provide the `$_probe_argN' convenience
> variables, and that the user must be placed on the PC of the probe when
> requesting that information.  The value and type of the argument can still be
> determined by the probe backend via the `pops->evaluate_probe_argument' and
> `pops->compile_to_ax' handlers.
>
> Note that a test in gdb.base/stap-probe.exp had to be adjusted because the "No
> SystemTap probe at PC" messages are now "No probe at PC".

Hi Jose,

Thanks for this patch.

Just a nit on the ChangeLog entry.

> gdb:
>
> 2014-09-26  Jose E. Marchesi  <jose.marchesi@oracle.com>
>
> 	* probe.c (compute_probe_arg): Function moved from the probe
> 	backends.
> 	(compile_probe_arg): Likewise.
> 	(probe_funcs): Likewise.
>
> 	* stap-probe.c (compute_probe_arg): Deleted.
> 	(compile_probe_arg): Likewise.
> 	(probe_funcs): Likewise.

First, ISTR that putting newlines between each file's entry is not used
anymore on GDB, so please write them all together.

I would also like that you mention things differently, to make it
explicit that we are just moving code here.  I think something like:


	* probe.c (compute_probe_arg): Moved from stap-probe.c.
	...
	* stap-probe.c (compute_probe_arg): Moved to probe.c.
	...

Other than that, the patch is OK and can be applied.

Thanks,

> gdb/testsuite:
>
> 2014-09-26  Jose E. Marchesi  <jose.marchesi@oracle.com>
>
> 	* gdb.base/stap-probe.exp (stap_test): Remove "SystemTap" from
> 	expected message when trying to access $_probe_* convenience
> 	variables while not on a probe.
> ---
>  gdb/ChangeLog                         |   11 ++++
>  gdb/probe.c                           |  111 +++++++++++++++++++++++++++++++++
>  gdb/stap-probe.c                      |  109 --------------------------------
>  gdb/testsuite/ChangeLog               |    6 ++
>  gdb/testsuite/gdb.base/stap-probe.exp |    2 +-
>  5 files changed, 129 insertions(+), 110 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 7cc4f00..5ec0d85 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,14 @@
> +2014-09-26  Jose E. Marchesi  <jose.marchesi@oracle.com>
> +
> +	* probe.c (compute_probe_arg): Function moved from the probe
> +	backends.
> +	(compile_probe_arg): Likewise.
> +	(probe_funcs): Likewise.
> +
> +	* stap-probe.c (compute_probe_arg): Deleted.
> +	(compile_probe_arg): Likewise.
> +	(probe_funcs): Likewise.
> +
>  2014-09-25  Jose E. Marchesi  <jose.marchesi@oracle.com>
>  
>  	* probe.c (print_ui_out_not_applicables): New function.
> diff --git a/gdb/probe.c b/gdb/probe.c
> index 5458372..d218f00 100644
> --- a/gdb/probe.c
> +++ b/gdb/probe.c
> @@ -31,6 +31,9 @@
>  #include "gdb_regex.h"
>  #include "frame.h"
>  #include "arch-utils.h"
> +#include "value.h"
> +#include "ax.h"
> +#include "ax-gdb.h"
>  #include <ctype.h>
>  
>  typedef struct bound_probe bound_probe_s;
> @@ -822,6 +825,87 @@ will show information about all types of probes."),
>    return &info_probes_cmdlist;
>  }
>  
> +\f
> +
> +/* This is called to compute the value of one of the $_probe_arg*
> +   convenience variables.  */
> +
> +static struct value *
> +compute_probe_arg (struct gdbarch *arch, struct internalvar *ivar,
> +		   void *data)
> +{
> +  struct frame_info *frame = get_selected_frame (_("No frame selected"));
> +  CORE_ADDR pc = get_frame_pc (frame);
> +  int sel = (int) (uintptr_t) data;
> +  struct bound_probe pc_probe;
> +  const struct sym_probe_fns *pc_probe_fns;
> +  unsigned n_args;
> +
> +  /* SEL == -1 means "_probe_argc".  */
> +  gdb_assert (sel >= -1);
> +
> +  pc_probe = find_probe_by_pc (pc);
> +  if (pc_probe.probe == NULL)
> +    error (_("No probe at PC %s"), core_addr_to_string (pc));
> +
> +  n_args = get_probe_argument_count (pc_probe.probe, frame);
> +  if (sel == -1)
> +    return value_from_longest (builtin_type (arch)->builtin_int, n_args);
> +
> +  if (sel >= n_args)
> +    error (_("Invalid probe argument %d -- probe has %u arguments available"),
> +	   sel, n_args);
> +
> +  return evaluate_probe_argument (pc_probe.probe, sel, frame);
> +}
> +
> +/* This is called to compile one of the $_probe_arg* convenience
> +   variables into an agent expression.  */
> +
> +static void
> +compile_probe_arg (struct internalvar *ivar, struct agent_expr *expr,
> +		   struct axs_value *value, void *data)
> +{
> +  CORE_ADDR pc = expr->scope;
> +  int sel = (int) (uintptr_t) data;
> +  struct bound_probe pc_probe;
> +  const struct sym_probe_fns *pc_probe_fns;
> +  int n_args;
> +  struct frame_info *frame = get_selected_frame (NULL);
> +
> +  /* SEL == -1 means "_probe_argc".  */
> +  gdb_assert (sel >= -1);
> +
> +  pc_probe = find_probe_by_pc (pc);
> +  if (pc_probe.probe == NULL)
> +    error (_("No probe at PC %s"), core_addr_to_string (pc));
> +
> +  n_args = get_probe_argument_count (pc_probe.probe, frame);
> +
> +  if (sel == -1)
> +    {
> +      value->kind = axs_rvalue;
> +      value->type = builtin_type (expr->gdbarch)->builtin_int;
> +      ax_const_l (expr, n_args);
> +      return;
> +    }
> +
> +  gdb_assert (sel >= 0);
> +  if (sel >= n_args)
> +    error (_("Invalid probe argument %d -- probe has %d arguments available"),
> +	   sel, n_args);
> +
> +  pc_probe.probe->pops->compile_to_ax (pc_probe.probe, expr, value, sel);
> +}
> +
> +static const struct internalvar_funcs probe_funcs =
> +{
> +  compute_probe_arg,
> +  compile_probe_arg,
> +  NULL
> +};
> +
> +
>  VEC (probe_ops_cp) *all_probe_ops;
>  
>  void _initialize_probe (void);
> @@ -831,6 +915,33 @@ _initialize_probe (void)
>  {
>    VEC_safe_push (probe_ops_cp, all_probe_ops, &probe_ops_any);
>  
> +  create_internalvar_type_lazy ("_probe_argc", &probe_funcs,
> +				(void *) (uintptr_t) -1);
> +  create_internalvar_type_lazy ("_probe_arg0", &probe_funcs,
> +				(void *) (uintptr_t) 0);
> +  create_internalvar_type_lazy ("_probe_arg1", &probe_funcs,
> +				(void *) (uintptr_t) 1);
> +  create_internalvar_type_lazy ("_probe_arg2", &probe_funcs,
> +				(void *) (uintptr_t) 2);
> +  create_internalvar_type_lazy ("_probe_arg3", &probe_funcs,
> +				(void *) (uintptr_t) 3);
> +  create_internalvar_type_lazy ("_probe_arg4", &probe_funcs,
> +				(void *) (uintptr_t) 4);
> +  create_internalvar_type_lazy ("_probe_arg5", &probe_funcs,
> +				(void *) (uintptr_t) 5);
> +  create_internalvar_type_lazy ("_probe_arg6", &probe_funcs,
> +				(void *) (uintptr_t) 6);
> +  create_internalvar_type_lazy ("_probe_arg7", &probe_funcs,
> +				(void *) (uintptr_t) 7);
> +  create_internalvar_type_lazy ("_probe_arg8", &probe_funcs,
> +				(void *) (uintptr_t) 8);
> +  create_internalvar_type_lazy ("_probe_arg9", &probe_funcs,
> +				(void *) (uintptr_t) 9);
> +  create_internalvar_type_lazy ("_probe_arg10", &probe_funcs,
> +				(void *) (uintptr_t) 10);
> +  create_internalvar_type_lazy ("_probe_arg11", &probe_funcs,
> +				(void *) (uintptr_t) 11);
> +
>    add_cmd ("all", class_info, info_probes_command,
>  	   _("\
>  Show information about all type of probes."),
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index 23202d7..17a73fc 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -1366,79 +1366,6 @@ stap_probe_destroy (struct probe *probe_generic)
>  
>  \f
>  
> -/* This is called to compute the value of one of the $_probe_arg*
> -   convenience variables.  */
> -
> -static struct value *
> -compute_probe_arg (struct gdbarch *arch, struct internalvar *ivar,
> -		   void *data)
> -{
> -  struct frame_info *frame = get_selected_frame (_("No frame selected"));
> -  CORE_ADDR pc = get_frame_pc (frame);
> -  int sel = (int) (uintptr_t) data;
> -  struct bound_probe pc_probe;
> -  const struct sym_probe_fns *pc_probe_fns;
> -  unsigned n_args;
> -
> -  /* SEL == -1 means "_probe_argc".  */
> -  gdb_assert (sel >= -1);
> -
> -  pc_probe = find_probe_by_pc (pc);
> -  if (pc_probe.probe == NULL)
> -    error (_("No SystemTap probe at PC %s"), core_addr_to_string (pc));
> -
> -  n_args = get_probe_argument_count (pc_probe.probe, frame);
> -  if (sel == -1)
> -    return value_from_longest (builtin_type (arch)->builtin_int, n_args);
> -
> -  if (sel >= n_args)
> -    error (_("Invalid probe argument %d -- probe has %u arguments available"),
> -	   sel, n_args);
> -
> -  return evaluate_probe_argument (pc_probe.probe, sel, frame);
> -}
> -
> -/* This is called to compile one of the $_probe_arg* convenience
> -   variables into an agent expression.  */
> -
> -static void
> -compile_probe_arg (struct internalvar *ivar, struct agent_expr *expr,
> -		   struct axs_value *value, void *data)
> -{
> -  CORE_ADDR pc = expr->scope;
> -  int sel = (int) (uintptr_t) data;
> -  struct bound_probe pc_probe;
> -  const struct sym_probe_fns *pc_probe_fns;
> -  int n_args;
> -  struct frame_info *frame = get_selected_frame (NULL);
> -
> -  /* SEL == -1 means "_probe_argc".  */
> -  gdb_assert (sel >= -1);
> -
> -  pc_probe = find_probe_by_pc (pc);
> -  if (pc_probe.probe == NULL)
> -    error (_("No SystemTap probe at PC %s"), core_addr_to_string (pc));
> -
> -  n_args = get_probe_argument_count (pc_probe.probe, frame);
> -
> -  if (sel == -1)
> -    {
> -      value->kind = axs_rvalue;
> -      value->type = builtin_type (expr->gdbarch)->builtin_int;
> -      ax_const_l (expr, n_args);
> -      return;
> -    }
> -
> -  gdb_assert (sel >= 0);
> -  if (sel >= n_args)
> -    error (_("Invalid probe argument %d -- probe has %d arguments available"),
> -	   sel, n_args);
> -
> -  pc_probe.probe->pops->compile_to_ax (pc_probe.probe, expr, value, sel);
> -}
> -
> -\f
> -
>  /* Set or clear a SystemTap semaphore.  ADDRESS is the semaphore's
>     address.  SET is zero if the semaphore should be cleared, or one
>     if it should be set.  This is a helper function for `stap_semaphore_down'
> @@ -1515,15 +1442,6 @@ stap_clear_semaphore (struct probe *probe_generic, struct objfile *objfile,
>    stap_modify_semaphore (addr, 0, gdbarch);
>  }
>  
> -/* Implementation of `$_probe_arg*' set of variables.  */
> -
> -static const struct internalvar_funcs probe_funcs =
> -{
> -  compute_probe_arg,
> -  compile_probe_arg,
> -  NULL
> -};
> -
>  /* Helper function that parses the information contained in a
>     SystemTap's probe.  Basically, the information consists in:
>  
> @@ -1784,33 +1702,6 @@ _initialize_stap_probe (void)
>  			     show_stapexpressiondebug,
>  			     &setdebuglist, &showdebuglist);
>  
> -  create_internalvar_type_lazy ("_probe_argc", &probe_funcs,
> -				(void *) (uintptr_t) -1);
> -  create_internalvar_type_lazy ("_probe_arg0", &probe_funcs,
> -				(void *) (uintptr_t) 0);
> -  create_internalvar_type_lazy ("_probe_arg1", &probe_funcs,
> -				(void *) (uintptr_t) 1);
> -  create_internalvar_type_lazy ("_probe_arg2", &probe_funcs,
> -				(void *) (uintptr_t) 2);
> -  create_internalvar_type_lazy ("_probe_arg3", &probe_funcs,
> -				(void *) (uintptr_t) 3);
> -  create_internalvar_type_lazy ("_probe_arg4", &probe_funcs,
> -				(void *) (uintptr_t) 4);
> -  create_internalvar_type_lazy ("_probe_arg5", &probe_funcs,
> -				(void *) (uintptr_t) 5);
> -  create_internalvar_type_lazy ("_probe_arg6", &probe_funcs,
> -				(void *) (uintptr_t) 6);
> -  create_internalvar_type_lazy ("_probe_arg7", &probe_funcs,
> -				(void *) (uintptr_t) 7);
> -  create_internalvar_type_lazy ("_probe_arg8", &probe_funcs,
> -				(void *) (uintptr_t) 8);
> -  create_internalvar_type_lazy ("_probe_arg9", &probe_funcs,
> -				(void *) (uintptr_t) 9);
> -  create_internalvar_type_lazy ("_probe_arg10", &probe_funcs,
> -				(void *) (uintptr_t) 10);
> -  create_internalvar_type_lazy ("_probe_arg11", &probe_funcs,
> -				(void *) (uintptr_t) 11);
> -
>    add_cmd ("stap", class_info, info_probes_stap_command,
>  	   _("\
>  Show information about SystemTap static probes.\n\
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index 539a983..c93d7cf 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,9 @@
> +2014-09-26  Jose E. Marchesi  <jose.marchesi@oracle.com>
> +
> +	* gdb.base/stap-probe.exp (stap_test): Remove "SystemTap" from
> +	expected message when trying to access $_probe_* convenience
> +	variables while not on a probe.
> +
>  2014-09-22  Pedro Alves  <palves@redhat.com>
>  
>  	* gdb.threads/break-while-running.exp: New file.
> diff --git a/gdb/testsuite/gdb.base/stap-probe.exp b/gdb/testsuite/gdb.base/stap-probe.exp
> index 7710bc3..9f71d1d 100644
> --- a/gdb/testsuite/gdb.base/stap-probe.exp
> +++ b/gdb/testsuite/gdb.base/stap-probe.exp
> @@ -30,7 +30,7 @@ proc stap_test {exec_name {arg ""}} {
>  	return -1
>      }
>  
> -    gdb_test "print \$_probe_argc" "No SystemTap probe at PC $hex" \
> +    gdb_test "print \$_probe_argc" "No probe at PC $hex" \
>  	"check argument not at probe point"
>  
>      gdb_test "info probes stap" \
> -- 
> 1.7.10.4

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/