From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 34568 invoked by alias); 11 Mar 2015 09:33:41 -0000 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 Received: (qmail 34558 invoked by uid 89); 11 Mar 2015 09:33:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_NEUTRAL autolearn=ham version=3.3.2 X-HELO: sasl.smtp.pobox.com Received: from pb-sasl1.int.icgroup.com (HELO sasl.smtp.pobox.com) (208.72.237.25) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 11 Mar 2015 09:33:39 +0000 Received: from sasl.smtp.pobox.com (unknown [127.0.0.1]) by pb-sasl1.pobox.com (Postfix) with ESMTP id B4EBE3A2D5; Wed, 11 Mar 2015 05:33:36 -0400 (EDT) Received: from pb-sasl1.int.icgroup.com (unknown [127.0.0.1]) by pb-sasl1.pobox.com (Postfix) with ESMTP id AD2FB3A2D4; Wed, 11 Mar 2015 05:33:36 -0400 (EDT) Received: from rusty (unknown [88.160.190.192]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by pb-sasl1.pobox.com (Postfix) with ESMTPSA id CFEAF3A2D3; Wed, 11 Mar 2015 05:33:35 -0400 (EDT) From: Andy Wingo To: Doug Evans Cc: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCH v3] Add Guile frame unwinder interface References: <87oao7wi66.fsf@igalia.com> <87zj7msbly.fsf@igalia.com> <54FDBF21.4090400@redhat.com> <87pp8iq9wc.fsf@igalia.com> <87r3sxp6lj.fsf_-_@igalia.com> <21759.11877.588480.923080@ruffy2.mtv.corp.google.com> Date: Wed, 11 Mar 2015 09:33:00 -0000 In-Reply-To: <21759.11877.588480.923080@ruffy2.mtv.corp.google.com> (Doug Evans's message of "Tue, 10 Mar 2015 10:48:21 -0700") Message-ID: <87lhj3naj7.fsf@igalia.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.4 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Pobox-Relay-ID: B10312B4-C7D1-11E4-943B-96E29252DF99-02397024!pb-sasl1.pobox.com X-IsSubscribed: yes X-SW-Source: 2015-03/txt/msg00272.txt.bz2 Hi :) [-asmundak, as he probably doesn't care about Guile API things] A couple quick replies. ACK to all the other things. On Tue 10 Mar 2015 18:48, Doug Evans writes: > > +@deffn {Scheme Procedure} make-frame-unwinder name procedure @ > > + @r{[}#:priority priority@r{]} @r{[}#:enabled? boolean@r{]} @ > > + @r{[}#:objfile objfile@r{]} @r{[}#:progspace progspace@r{]} > > Instead of both #:objfile and #:progspace I'd rather just provide > say, #:locus or #:scope or some such. Let's do #:scope then. Note that I copied this infrastructure from the frame filter patch, which should also be ready for review :) I'll apply corresponding changes there and update both patches. > > + add-frame-unwinder! > > + remove-frame-unwinder! > > As a user who can't remember names very well, consistent naming > is welcome. There is a (relatively) consistent naming thus far: > register-breakpoint!, delete-breakpoint!, register-parameter!, > register-command!. > > -> register-frame-unwinder! and delete-frame-unwinder! > > But I can quibble over the names too :-). > "delete-breakpoint!" doesn't really delete it so much as remove it > from gdb's tables. So I could go with remove-frame-unwinder!, > add remove-breakpoint! and deprecate delete-breakpoint!. > > Plus it's nice to have names that go together: If we're going to have > register-foo, the intuitive opposite is unregister-foo. > But I'm ok with register-foo and remove-foo. How about register-frame-unwinder! and unregister-frame-unwinder!. It matches the frame-unwinder-registered? predicate. "Add" and "remove" already raised the question of "to what?", and in that regard "register" might be a bit clearer. > Now's a good a time as any to lay down how things get deprecated in > the Guile API. We could have a guile/lib/gdb/deprecated.scm file that > defined register-foo! (and others) or some such. As for when to delete > them, I'm ok with anything <= 5 years. Sure. Or, we leave the interfaces where they are and just issue deprecation warnings when they are used. > > > + enable-frame-unwinder! > > + disable-frame-unwinder! > > Similarly, there is set-breakpoint-enabled!. > -> set-frame-unwinder-enabled! Uf, that's terrible :) In Guile the convention is mostly set-TYPE-FIELD!, and here TYPE is "frame-unwinder" and FIELD is "enabled?", so the usual production is "set-frame-unwinder-enabled?!". Really. It's ugly but it does avoid the ambiguous reading of "set-frame-unwinder-enabled!" as "set the frame unwinder to be enabled". Actually it's so ugly that historically set-foo-enabled?! sometimes gets replaced by more verby forms (cf in Guile's NEWS, `set-batch-mode?!' replaced by `ensure-batch-mode!'). There is also the issue that without a good story yet on how to control enabled/disabled status from the console, a common thing a user might want to do would be to say "%&^%*&%@@@! It's not working, so let's try again with all the unwinders disabled." It's much easier to do: (gdb) guile (for-each disable-frame-unwinder! (all-frame-unwinders)) than to (gdb) guile (for-each (lambda (uw) (set-frame-unwinder-enabled! uw #f)) (all-frame-unwinders)) (Should enabling / disabling unwinders invalidate the frame cache, I wonder?) What's the right way out of here? :) I can change, it doesn't matter much, but I wanted to argue a bit for the status quo. What about adding a set-frame-unwinder-enabled! setter for consistency and also keeping enable-frame-unwinder! / disable-frame-unwinder!. I don't know, none of the options are perfect :) > > +/* (set-ephemeral-frame-id! ephemeral-frame stack-address > > + [code-address [special-address]]) > > + > > + Set the frame ID on this ephemeral frame. */ > > + > > +static SCM > > +uwscm_set_ephemeral_frame_id_x (SCM ephemeral_frame, SCM sp, SCM ip, > > + SCM special) > > I'm not sold on exporting the term "special" into the API. > It conveys no information to the reader. "How special?" "Special in what way?" > Internally we can call it whatever we like, but we should have a > better name in the published API. > I realize we don't want to pick an architecture-specific name, > but as data for the discussion, is this only used by ia64? It's "special" all through the internal API, of course. However it does appear to only be called by ia64-hpux-tdep.c and ia64-tdep.c. We could simply not support "special" in the Guile and/or Python APIs, for now at least. I hope to live a long life and, when I die, to look back in satisfaction that I never worked on an IA64 system :-)) Thanks for the review, will update this and the frame filter patch. Andy