From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12730 invoked by alias); 28 Oct 2013 16:05:44 -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 12703 invoked by uid 89); 28 Oct 2013 16:05:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 28 Oct 2013 16:05:43 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r9SG5gY4013329 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 28 Oct 2013 12:05:42 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r9SG5eIs010371; Mon, 28 Oct 2013 12:05:41 -0400 Message-ID: <526E8B54.8040104@redhat.com> Date: Mon, 28 Oct 2013 16:05:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Tom Tromey CC: gdb-patches@sourceware.org Subject: Re: [PATCH v4 3/9] add target method delegation References: <1382464769-2465-1-git-send-email-tromey@redhat.com> <1382464769-2465-4-git-send-email-tromey@redhat.com> In-Reply-To: <1382464769-2465-4-git-send-email-tromey@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-10/txt/msg00864.txt.bz2 On 10/22/2013 06:59 PM, Tom Tromey wrote: > This patch replaces some code in the record targets with target method > delegation. > > Right now there are two latent problems in the record target. > > First, record-full.c stores pointers to many target methods when the > record target is pushed. Then it later delegates some calls via > these. This is wrong because it violates the target stack contract. > In particular it is ok to unpush a target at any stratum, but > record-full does not keep track of this, so it could potentially call > into an unpushed target. > > Second, RECORD_IS_USED and some other spots look at > current_target.to_stratum to determine whether a record target is in > use. This is bad because arch_stratum is greater than record_stratum. > > To fix the first problem, this patch introduces a handful of > target_delegate_* functions, which forward calls further down the > target stack. > > To fix the second problem, this patch adds find_target_at to determine > whether a target appears at a given stratum. This may seem like > overkill somehow, but I have a subsequent patch series (see archer.git > tromey/multi-target) that uses it more heavily. Hmm, I think this is trying to do too much at once. Could you please split out the patch for the second problem? I think it'll be a small one. Then changes like these: > #define target_stopped_by_watchpoint() \ > - ((*current_target.to_stopped_by_watchpoint) (¤t_target)) > + (target_delegate_stopped_by_watchpoint (¤t_target)) > + > +/* Delegate "target_stopped_by_watchpoint" to a target beneath SELF. */ > + > +extern int target_delegate_stopped_by_watchpoint (struct target_ops *self); switch from using the inheritance scheme to the run-time walk mechanism, but leave update_current_target still doing the INHERIT/de_fault business. What's the plan for the existing target methods that currently already do a similar beneath lookup? I'd like it that there's be at least a plan, so we don't end up with yet another way of doing things, two incomplete transitions, and no clear direction. > + gdb_assert_not_reached (_("reached end of target stack during delegation")); > +} This appears in several places. Whenever I see the same string repeated over and over, I tend to think it'd be good to add a utility helper function: static void assert_end_of_stack_not_reached_or_something (void) { gdb_assert_not_reached (_("reached end of target stack during delegation")); } Some of the delegation methods have that assert, while others don't have anything at the tail end. What's the story there? > + > +int > +target_delegate_insert_breakpoint (struct target_ops *self, > + struct gdbarch *gdbarch, > + struct bp_target_info *bp_tgt) > +{ > + struct target_ops *t; > + > + for (t = self->beneath; t != NULL; t = t->beneath) > + { > + if (t->to_insert_breakpoint) if (t->to_insert_breakpoint != NULL) -- Pedro Alves