From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16154 invoked by alias); 18 Jul 2012 15:45:02 -0000 Received: (qmail 16061 invoked by uid 22791); 18 Jul 2012 15:44:58 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,TW_BJ,TW_FN,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 18 Jul 2012 15:44:44 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q6IFiiBg000751 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 18 Jul 2012 11:44:44 -0400 Received: from host2.jankratochvil.net (ovpn-116-30.ams2.redhat.com [10.36.116.30]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q6IFidWW029718 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 18 Jul 2012 11:44:41 -0400 Date: Wed, 18 Jul 2012 15:45:00 -0000 From: Jan Kratochvil To: Sergio Durigan Junior Cc: GDB Patches , Gary Benson Subject: Re: [PATCH] Cleanup `objfile' from probe interface Message-ID: <20120718154436.GA4615@host2.jankratochvil.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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: 2012-07/txt/msg00269.txt.bz2 On Wed, 18 Jul 2012 17:02:20 +0200, Sergio Durigan Junior wrote: > The patch removes several explict references to OBJFILE from many > functions in the current probe API, by internalizing the OBJFILE on > `struct probe'. This also simplified `struct sym_probe_fns'. insert_exception_resume_from_probe still needs this cleanup, doesn't it? > @@ -297,7 +279,7 @@ collect_probes (char *objname, char *provider, char *probe_name, > struct probe *probe; > int ix; > > - if (! objfile->sf || ! objfile->sf->sym_probe_fns) > + if (!objfile->sf || !objfile->sf->sym_probe_fns) > continue; > > if (objname) This is unrelated formatting change but let it be that way... > @@ -334,26 +312,26 @@ collect_probes (char *objname, char *provider, char *probe_name, > return result; > } > > -/* A qsort comparison function for probe_and_objfile_s objects. */ > +/* A qsort comparison function for probe_p objects. */ > > static int > compare_entries (const void *a, const void *b) Rather compare_probes now. There are some more variables called entries/entry which could be probes/probe now but that is more a nitpick. > @@ -440,7 +417,7 @@ gen_ui_out_table_header_info (VEC (probe_and_objfile_s) *probes, > represented by ENTRY. */ represented by PROBE. > > static void > -print_ui_out_info (probe_and_objfile_s *entry) > +print_ui_out_info (struct probe *probe) > { > int ix; > int j = 0; [...] > @@ -118,7 +115,6 @@ struct probe_ops > position in the vector. */ > > void (*gen_info_probes_table_values) (struct probe *probe, > - struct objfile *objfile, > VEC (const_char_ptr) **values); Leftover 'OBJFILE' in its comment: /* Function that will fill VALUES with the values of the extra fields to be printed for PROBE and OBJFILE. If the backend implements > }; > > @@ -157,6 +153,9 @@ struct probe > /* The operations associated with this probe. */ > const struct probe_ops *pops; > > + /* The objfile related to this probe. */ Isn't it rather s/related/containing/? I would also state that if the same probe is contained also in the separate debug objfile this variable contains always the non-separate-debuginfo objfile. > + struct objfile *objfile; > + > /* The name of the probe. */ > const char *name; > [...] > @@ -1159,35 +1158,40 @@ compile_probe_arg (struct internalvar *ivar, struct agent_expr *expr, > { > CORE_ADDR pc = expr->scope; > int sel = (int) (uintptr_t) data; > - struct objfile *objfile; > struct probe *pc_probe; > - int n_probes; > + const struct sym_probe_fns *pc_probe_fns; > + int n_args; > > /* SEL == -1 means "_probe_argc". */ > gdb_assert (sel >= -1); > > - pc_probe = find_probe_by_pc (pc, &objfile); > + pc_probe = find_probe_by_pc (pc); > 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); > + gdb_assert (pc_probe->objfile != NULL); > + gdb_assert (pc_probe->objfile->sf != NULL); > + gdb_assert (pc_probe->objfile->sf->sym_probe_fns != NULL); > + > + pc_probe_fns = pc_probe->objfile->sf->sym_probe_fns; > + > + n_args = pc_probe_fns->sym_get_probe_argument_count (pc_probe); > + > if (sel == -1) > { > value->kind = axs_rvalue; > value->type = builtin_type (expr->gdbarch)->builtin_int; > - ax_const_l (expr, n_probes); > + ax_const_l (expr, n_args); This rename is really a separate cleanup. > return; > } > > gdb_assert (sel >= 0); > - if (sel >= n_probes) > + if (sel >= n_args) > error (_("Invalid probe argument %d -- probe has %d arguments available"), > - sel, n_probes); > + sel, n_args); > > - objfile->sf->sym_probe_fns->sym_compile_to_ax (objfile, pc_probe, > - expr, value, sel); > + pc_probe_fns->sym_compile_to_ax (pc_probe, expr, value, > + sel); This can be on a single line. > } > > OK with those changes, it is pre-approved that way. Thanks, Jan