From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16891 invoked by alias); 12 Mar 2012 20:37:56 -0000 Received: (qmail 16882 invoked by uid 22791); 12 Mar 2012 20:37:55 -0000 X-SWARE-Spam-Status: No, hits=-6.9 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 20:37:39 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q2CKbdjn013231 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 12 Mar 2012 16:37:39 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q2CKbb7a017612 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 12 Mar 2012 16:37:38 -0400 From: Tom Tromey To: Jan Kratochvil Cc: Sergio Durigan Junior , gdb-patches@sourceware.org Subject: Re: [PATCH 2/3] Implement new features needed for handling SystemTap probes References: <20120310192054.GA27097@host2.jankratochvil.net> Date: Mon, 12 Mar 2012 20:37:00 -0000 In-Reply-To: <20120310192054.GA27097@host2.jankratochvil.net> (Jan Kratochvil's message of "Sat, 10 Mar 2012 20:20:54 +0100") Message-ID: <87obs1o8ry.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.94 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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/msg00415.txt.bz2 >>>>> "Jan" == Jan Kratochvil writes: Thanks for the thorough and detailed review. I have a few comments. 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. >> + /* Swallow errors. */ Jan> Could here be a reason why it is valid? >> + target_write_memory (address, bytes, TYPE_LENGTH (type)); Jan> And here again, why not to check errors? I think we should warn. Originally I thought there was just nothing sensible to do if we got an error, but warning is better than silently ignoring it. Jan> Here you leak PROV_PAT, PROBE_PAT and OBJ_PAT. No, compile_rx_or_error makes a cleanup. >> + return result; >> +} >> + >> +/* A qsort comparison function for stap_entry objects. */ >> + >> +static int >> +compare_entries (const void *a, const void *b) >> +{ >> + const stap_entry *ea = a; >> + const stap_entry *eb = b; >> + int v; >> + >> + v = strcmp (ea->probe->provider, eb->probe->provider); Jan> handle_probe can create NULL probe->provider or NULL probe->name etc. But Jan> other code like this one will crash on it. Jan> I believe such non-valid probes rather should be dropped during parsing. I agree. 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. This is a relic of an earlier implementation, which had two different code paths. 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. The probe argument might not even be valid at the time at which AX compilation is done. And, this means that we must always return a special computed value for all probe arguments, just to make compilation work. All these gyrations provide no extra benefit, though. Simply asking the internalvar for an expression is simpler all around. 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. Tom