From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11850 invoked by alias); 12 Mar 2012 23:15:41 -0000 Received: (qmail 11833 invoked by uid 22791); 12 Mar 2012 23:15:39 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,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; Mon, 12 Mar 2012 23:15:20 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q2CNFJxW005007 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 12 Mar 2012 19:15:20 -0400 Received: from host2.jankratochvil.net (ovpn-116-16.ams2.redhat.com [10.36.116.16]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q2CNFF9N006239 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 12 Mar 2012 19:15:18 -0400 Date: Mon, 12 Mar 2012 23:15:00 -0000 From: Jan Kratochvil To: Tom Tromey Cc: Sergio Durigan Junior , gdb-patches@sourceware.org Subject: Re: [PATCH 2/3] Implement new features needed for handling SystemTap probes Message-ID: <20120312231514.GA10376@host2.jankratochvil.net> References: <20120310192054.GA27097@host2.jankratochvil.net> <87obs1o8ry.fsf@fleche.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87obs1o8ry.fsf@fleche.redhat.com> 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-03/txt/msg00421.txt.bz2 On Mon, 12 Mar 2012 21:37:37 +0100, Tom Tromey wrote: > >>>>> "Jan" == Jan Kratochvil writes: [...] > Jan> I believe there was intended some abstraction, more talked about it > Jan> elsewhere, breakpoint.c should only include some (nonexistent) > Jan> "probe.h", not "stap-probe.h". > > What did you have in mind? > > In the abstract, my concern about adding abstraction is that we aren't > sure it would be good for anything. Then we can wind up with an > over-engineered solution. > > But, this is an abstract concern; perhaps your idea is not susceptible > to this. I find important that user interface gets established in a way needing no changes in the future, as discussed with Eli. Including probe.h vs. stap-probe.h also seems more easier for readability of the generic GDB code. So called code encapsulation - not to complicate unrelated code with the specific stap backend details visibility. It would also make it more easy to implement breakpoints on UST addresses; although I do not have ust.h available here to examine that possibility more. > Jan> Here you leak PROV_PAT, PROBE_PAT and OBJ_PAT. > > No, compile_rx_or_error makes a cleanup. Yes but there is: discard_cleanups (cleanup); which discards those cleanups. IMO that discard_cleanups was meant for RESULT but not for PROV_PAT, PROBE_PAT and OBJ_PAT - or I really still miss it? There should be two cleanup trackers: cleanup = make_cleanup (VEC_cleanup (stap_entry), &result); cleanup_temps = compile_rx_or_error (&prov_pat, provider, _("Invalid provider regexp")); compile_rx_or_error (&probe_pat, probe, _("Invalid probe regexp")); compile_rx_or_error (&obj_pat, objname, _("Invalid object file regexp")); [...] do_cleanups (cleanup_temps); discard_cleanups (cleanup); > Jan> Moreover I would still more see to drop [patch 1/3], call just > Jan> compute_probe_arg which returns lazy lval_computed struct value * > Jan> which provides new struct lval_funcs member which can return > Jan> `struct expression *' and generic code can call gen_expr on its > Jan> own. There is no need for special > Jan> internalvar_funcs-> compile_to_ax member. [...] > Putting this into lval_funcs seems very roundabout to me. First, it > means computing a value in the middle of compiling to AX. But, when > compiling to AX we are not generally computing values. + > Simply asking the internalvar for an expression is simpler all around. lval_computed value is not a real value, it is just a way how to implement the needed functionality on top of existing API, without needing making that API more rich. I do not mind if you have considered the lval_computed way. > We could change the new method to return an expression; this doesn't > seem vital to me since it isn't used anywhere else, but I find it a > valid aesthetic choice. That's already detail, I was objecting more the whole [patch 1/3]. Thanks, Jan