From: Yao Qi <yao@codesourcery.com>
To: Sergio Durigan Junior <sergiodj@redhat.com>
Cc: gdb-patches@sourceware.org, Tom Tromey <tromey@redhat.com>
Subject: Re: [PATCH 4/6] Implement support for SystemTap probes
Date: Thu, 07 Apr 2011 02:41:00 -0000 [thread overview]
Message-ID: <4D9D243A.3090505@codesourcery.com> (raw)
In-Reply-To: <m3k4fapvb8.fsf@redhat.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 (é½å°§)
next prev parent reply other threads:[~2011-04-07 2:41 UTC|newest]
Thread overview: 83+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-04 3:09 Sergio Durigan Junior
2011-04-04 19:06 ` Eli Zaretskii
2011-04-06 20:20 ` Tom Tromey
2011-04-06 20:52 ` Sergio Durigan Junior
2011-04-07 2:41 ` Yao Qi [this message]
2011-04-07 3:32 ` Sergio Durigan Junior
2011-04-07 17:04 ` Tom Tromey
2011-04-11 3:21 ` Yao Qi
2011-04-08 12:38 ` Sergio Durigan Junior
2011-04-11 3:52 ` Yao Qi
2011-08-12 15:45 ` Jan Kratochvil
2011-08-12 17:22 ` Frank Ch. Eigler
2011-08-12 21:33 ` Sergio Durigan Junior
2011-04-19 16:42 ` Jan Kratochvil
2012-05-07 19:36 ` Jan Kratochvil
2012-05-07 19:54 ` Sergio Durigan Junior
2012-05-07 19:58 ` Jan Kratochvil
2012-05-07 20:26 ` Sergio Durigan Junior
2012-05-07 20:38 ` Jan Kratochvil
2012-05-08 1:36 ` Sergio Durigan Junior
2011-10-26 21:08 [PATCH] Implement new `info core mappings' command Sergio Durigan Junior
2011-10-26 21:25 ` Sergio Durigan Junior
2011-10-27 7:30 ` Eli Zaretskii
2011-10-27 18:09 ` Sergio Durigan Junior
2011-10-29 19:48 ` Eli Zaretskii
2011-10-31 0:34 ` Jan Kratochvil
2011-10-31 7:00 ` Sergio Durigan Junior
2011-10-31 8:13 ` Jan Kratochvil
2011-10-31 12:57 ` Pedro Alves
2011-11-01 11:54 ` [patch] `info proc ' completion [Re: [PATCH] Implement new `info core mappings' command] Jan Kratochvil
2011-11-01 16:23 ` Eli Zaretskii
2011-11-03 14:12 ` [patch] `info proc *' help fix [Re: [patch] `info proc ' completion] Jan Kratochvil
2011-11-03 16:57 ` Eli Zaretskii
2011-11-03 17:07 ` Jan Kratochvil
2011-11-03 18:08 ` Eli Zaretskii
2011-11-03 18:25 ` Jan Kratochvil
2011-11-02 18:30 ` [patch] `info proc ' completion [Re: [PATCH] Implement new `info core mappings' command] Pedro Alves
2011-11-02 18:48 ` [commit] " Jan Kratochvil
2011-11-03 20:01 ` [PATCH] Implement new `info core mappings' command Sergio Durigan Junior
2011-11-04 10:38 ` Eli Zaretskii
2011-11-04 16:27 ` Jan Kratochvil
2011-11-08 1:49 ` Sergio Durigan Junior
2011-11-08 21:47 ` Jan Kratochvil
2011-11-09 20:32 ` Jan Kratochvil
2011-11-16 4:10 ` Sergio Durigan Junior
2011-11-21 16:15 ` Sergio Durigan Junior
2011-11-23 16:32 ` [rfc] Options for "info mappings" etc. (Re: [PATCH] Implement new `info core mappings' command) Ulrich Weigand
2011-11-23 23:37 ` Sergio Durigan Junior
2011-12-01 19:51 ` Ulrich Weigand
2011-12-05 12:59 ` Pedro Alves
2011-12-05 15:02 ` Ulrich Weigand
2011-12-06 16:01 ` Pedro Alves
2011-12-06 17:19 ` Ulrich Weigand
2011-12-07 16:29 ` Pedro Alves
2011-12-07 17:24 ` Pedro Alves
2011-12-07 20:14 ` Ulrich Weigand
2011-12-09 13:28 ` Pedro Alves
2011-12-09 14:10 ` Pedro Alves
2011-12-20 23:08 ` Ulrich Weigand
2011-12-21 22:36 ` Jan Kratochvil
2011-12-22 16:15 ` Ulrich Weigand
2012-01-05 16:02 ` Pedro Alves
2012-01-05 18:03 ` Ulrich Weigand
2012-01-05 18:20 ` Pedro Alves
2012-01-05 19:54 ` Ulrich Weigand
2012-01-06 6:41 ` Joel Brobecker
2012-01-06 12:29 ` Pedro Alves
2012-01-06 12:27 ` Pedro Alves
2012-01-09 15:44 ` Ulrich Weigand
2012-01-11 16:38 ` Pedro Alves
2012-01-11 18:32 ` Ulrich Weigand
2012-01-05 18:37 ` Mark Kettenis
2012-01-05 19:35 ` Ulrich Weigand
2012-04-06 3:28 [PATCH 0/4 v2] Implement support for SystemTap probes on userspace Sergio Durigan Junior
2012-04-06 3:32 ` [PATCH 1/4 v2] Refactor internal variable mechanism Sergio Durigan Junior
2012-04-06 3:36 ` [PATCH 2/4 v2] Implement new features needed for handling SystemTap probes Sergio Durigan Junior
2012-04-11 19:06 ` Jan Kratochvil
2012-04-11 22:14 ` Sergio Durigan Junior
2012-04-11 23:33 ` Jan Kratochvil
2012-04-06 3:37 ` [PATCH 4/4 v2] Documentation and testsuite changes Sergio Durigan Junior
2012-04-06 9:27 ` Eli Zaretskii
2012-04-09 21:37 ` Sergio Durigan Junior
2012-04-06 4:11 ` [PATCH 3/4 v2] Use longjmp and exception probes when available Sergio Durigan Junior
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4D9D243A.3090505@codesourcery.com \
--to=yao@codesourcery.com \
--cc=gdb-patches@sourceware.org \
--cc=sergiodj@redhat.com \
--cc=tromey@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).