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

Yao Qi
2011-04-07T02:41:10Z

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 (é½å°§)