From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8815 invoked by alias); 7 Apr 2011 02:41:10 -0000 Received: (qmail 8768 invoked by uid 22791); 7 Apr 2011 02:41:08 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,TW_BJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 07 Apr 2011 02:41:04 +0000 Received: (qmail 7085 invoked from network); 7 Apr 2011 02:41:03 -0000 Received: from unknown (HELO ?192.168.0.102?) (yao@127.0.0.2) by mail.codesourcery.com with ESMTPA; 7 Apr 2011 02:41:03 -0000 Message-ID: <4D9D243A.3090505@codesourcery.com> Date: Thu, 07 Apr 2011 02:41:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.14) Gecko/20110223 Lightning/1.0b2 Thunderbird/3.1.8 MIME-Version: 1.0 To: Sergio Durigan Junior CC: gdb-patches@sourceware.org, Tom Tromey Subject: Re: [PATCH 4/6] Implement support for SystemTap probes References: In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-04/txt/msg00102.txt.bz2 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 (齐尧)