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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

Hi Yao,

Thanks for the review.  I'll answer it quickly now, will take a look
deeper later, and re-submit the patch too.

Yao Qi <yao@codesourcery.com> writes:

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

Ok, no problem for me.  I thought that maybe a flag would be easier to
understand, but I don't see any drawbacks in adopting the #define.

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

You are right.

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

You are right.

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

Ok, makes sense.

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

Sorry, I'm not sure I understood your question.  Maybe I'll leave it for
Tom to answer.

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

Makes sense.

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

Ok, I'll take a look at this ASAP.

Thanks for the review again!

Sergio.