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

Yao Qi
2011-04-11T03:52:06Z

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