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

[Yao Qi <yao at codesourcery dot com>]

On 04/04/2011 11:08 AM, Sergio Durigan Junior wrote:

Code looks pretty good!  Thanks.  Some small cents....

> +struct stap_evaluation_info
> +{
....
....
> +
> +  /* Flag to indicate if we are compiling an agent expression.  */
> +  int compiling_p;
> +
> +  /* If the above flag is true (one), this field will contain the
> +     pointer to the agent expression.  */
> +  struct agent_expr *aexpr;

Field `compiling_p' looks redundant to me.  We can use field `aexpr'
directly.  Maybe, we can create a macro

#define COMPILING_AGENT_EXPR_P(eval_info) (eval_info->aexpr != NULL)

> +
> +  /* The value we are modifying (for agent expression).  */
> +  struct axs_value *avalue;
> +};

> +/* Helper function which is responsible for freeing the space allocated to
> +   hold information about a probe's arguments.  */
> +
> +static void
> +stap_free_args_info (void *args_info_ptr)
> +{
> +  struct stap_args_info *a = (struct stap_args_info *) args_info_ptr;
> +  int i;
> +
> +  for (i = 0; i < STAP_MAX_ARGS; i++)
> +    {
> +      xfree (a->arg->arg_str);

                   ^^^^
I guess it should be `a->arg[i].arg_str.

> +    }
> +
> +  xfree (a->arg);
> +  xfree (a);
> +}


> +static struct value *
> +stap_evaluate_single_operand (struct stap_evaluation_info *eval_info)
> +{
...
...
> +	    }
> +	  else if (*eval_info->exp_buf == '$')
> +	    {
> +	      int number;
> +
> +	      /* Last case.  We are dealing with an integer constant, so
> +		 we must read it and then apply the necessary operation,
> +		 either `-' or `~'.  */
> +	      ++eval_info->exp_buf;
> +	      number = strtol (eval_info->exp_buf,
> +			       &eval_info->exp_buf, 0);
> +
> +	      if (!eval_info->compiling_p)
> +		res
> +		  = value_from_longest (builtin_type (gdbarch)->builtin_int,
> +					number);
> +
> +	      if (eval_info->compiling_p)
> +		ax_const_l (eval_info->aexpr, number);

We can use if/else to replace these two if statements.


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

Comment here is good, but `10' is still like a `magic number'.  We may
use STAP_MAX_ARGS directly here.

> +
> +  pc_probe = find_probe_by_pc (pc, &objfile);

I don't understand this part.  We are looking for probe by matching
frame's PC here, but address of stap_probe is the address where the
probe is inserted.  So, probably, we can't find any probe here, is that
correct?

> +  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)
> +    return value_from_longest (builtin_type (arch)->builtin_int,
n_probes);
> +
> +  gdb_assert (sel >= 0);

This check is redundant, because of another check in several lines
before `gdb_assert (sel >= 0 && sel <= 10);'.  We can remove it.

This function looks quite similar to `stap_safe_evaluate_at_pc', some
code in these two functions are duplicated.  We can merge them together.

-- 
Yao (éå)