Re: [PATCH 4/6] Implement support for SystemTap probes

[Yao Qi <yao at codesourcery dot com>]

On 04/08/2011 08:37 PM, Sergio Durigan Junior wrote:
> +static void
> +stap_compile_to_ax_1 (struct objfile *objfile,
> +		      const struct stap_probe *probe,
> +		      struct agent_expr *expr,
> +		      struct axs_value *value,
> +		      int n)
> +{
> +  struct stap_evaluation_info eval_info;
> +  struct gdbarch *gdbarch = expr->gdbarch;
> +  char *s = (char *) probe->parsed_args->arg[n].arg_str;
> +
> +  /* Filling necessary information for evaluation function.  */
> +  eval_info.saved_expr = s;
> +  eval_info.exp_buf = s;
> +  eval_info.gdbarch = expr->gdbarch;
> +  eval_info.frame = NULL;
> +  eval_info.bitness = probe->parsed_args->arg[n].bitness;
> +  /* We are compiling to an agent expression.  */
> +  eval_info.aexpr = expr;
> +  eval_info.avalue = value;
> +
> +  /* We can always use this kind.  */
> +  value->kind = axs_rvalue;
> +
> +  /* Figuring out the correct type for this axs_value.  */
> +  switch (eval_info.bitness)
> +    {
> +    case STAP_ARG_BITNESS_UNDEFINED:
> +      if (gdbarch_addr_bit (gdbarch) == 32)
> +	value->type = builtin_type (gdbarch)->builtin_uint32;
> +      else
> +	value->type = builtin_type (gdbarch)->builtin_uint64;
> +      break;

Do we have to consider the case when gdbarch_addr_bit returns 16?

> +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 objfile *objfile;
> +  const struct stap_probe *pc_probe;
> +  int n_probes;
> +
> +  /* SEL==10 means "_probe_argc".  */
> +  gdb_assert (sel >= 0 && sel <= 10);

`10' can be replaced by STAP_MAX_ARGS, as you fixed in compute_probe_arg.

> +
> +  pc_probe = find_probe_by_pc (pc, &objfile);
> +  if (pc_probe == NULL)
> +    error (_("No SystemTap probe at PC %s"), core_addr_to_string (pc));
> +
> +  n_probes
> +    = objfile->sf->sym_probe_fns->sym_get_probe_argument_count (objfile,
> +								pc_probe);
> +  if (sel == 10)
> +    {
> +      value->kind = axs_rvalue;
> +      value->type = builtin_type (expr->gdbarch)->builtin_int;
> +      ax_const_l (expr, n_probes);
> +      return;
> +    }
> +
> +  gdb_assert (sel >= 0);

It is a redundant check.

The whole patch looks pretty nice to me, and I have no other comments.

-- 
Yao (éå)