From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 102340 invoked by alias); 23 Oct 2018 11:11:09 -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 99053 invoked by uid 89); 23 Oct 2018 11:11:08 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=beneath, teach 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; Tue, 23 Oct 2018 11:11:06 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 778DBC04F4CD; Tue, 23 Oct 2018 11:11:05 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id C6859608F2; Tue, 23 Oct 2018 11:11:04 +0000 (UTC) Subject: Re: [PATCH 1/3] Pass inferior to terminal_save_inferior To: Simon Marchi References: <20181016033835.17594-1-simon.marchi@polymtl.ca> <3cd4ddb6-b791-858e-58c9-721b6f7177af@redhat.com> <7a8c48b3033d53f0c6031843b6424ba7@polymtl.ca> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <048ffbfd-e9b8-d223-f59d-d0b7dc139f04@redhat.com> Date: Tue, 23 Oct 2018 11:11:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <7a8c48b3033d53f0c6031843b6424ba7@polymtl.ca> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2018-10/txt/msg00502.txt.bz2 On 10/23/2018 10:00 AM, Simon Marchi wrote: > On 2018-10-22 16:54, Pedro Alves wrote: >> On 10/16/2018 04:38 AM, Simon Marchi wrote: >>> Instead of relying on the current inferior, pass an inferior pointer to >>> the target implementing terminal_save_inferior.  There should be no >>> change in behavior. >>> >> >> Your original patch 3/3 ended up not using the inferior pointer.  :-) >> >> I suppose that this doesn't hurt, like the equivalent target_detach >> change. > > Well, the original 3/3 patch doesn't change the implementation of terminal_save_inferior, it changes the inferior_exit observer, so it's not really related to this change. Oh, in that case it'd be clearer to not bundle it in the same series then. > >>> I added documentation to terminal_save_inferior, as I understand it >>> (maybe I understood it wrong, so please take a look). >> >> That looks good.  (please recall to update commit log.) >> >>> diff --git a/gdb/target.c b/gdb/target.c >>> index 2d98954b54ac..93d16b90f179 100644 >>> --- a/gdb/target.c >>> +++ b/gdb/target.c >>> @@ -511,10 +511,7 @@ target_terminal_is_ours_kind (target_terminal_state desired_state) >>>    ALL_INFERIORS (inf) >>>      { >>>        if (inf->terminal_state == target_terminal_state::is_inferior) >>> -    { >>> -      set_current_inferior (inf); >>> -      current_top_target ()->terminal_save_inferior (); >>> -    } >>> +    current_top_target ()->terminal_save_inferior (inf); >>>      } >> With multi-target, current_top_target() will depend on >> the current inferior, so I'll need to put that >> set_current_inferior call back. > > Can't you access the inferior's target stack directly instead of changing the current inferior? I can for the call to the top target, but then the problem is the beneath() calls in all the target-delegates.c delegating implementations. E.g. here: void -target_ops::terminal_save_inferior () +target_ops::terminal_save_inferior (inferior *arg0) { - this->beneath ()->terminal_save_inferior (); + this->beneath ()->terminal_save_inferior (arg0); ^^^^^^^^^ } because beneath() looks at the target stack of the current inferior. It would need to look for the target beneath in the target stack of the arg0 inferior instead. Otherwise you start the top target call in inferior B, and then cross the the beneath target of inferior A (the current inferior). Whoops. At some point in the branch I made target_ops::beneath take an optional inferior pointer, but when I stumbled on this issue in target-delegates.c I ended up reverting it, as it wasn't easy to fix. I think that we could maybe teach make-target-delegates to automatically emit void target_ops::method (inferior *arg0) { this->beneath (arg0)->method (arg0); } and: void target_ops::method (thread_info *arg0) { this->beneath (arg0->inf)->method (arg0); } iff the method's first parameter is an inferior or thread_info pointer. But that was just an idea, I never toyed with it, because it would be a detour. So I gave up on the inferior parameter to target beneath, and thought I'd better focus instead of getting the multi-target basics in first, even if that means we need to swap current inferior/thread here and there, as usual. Thanks, Pedro Alves