From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 59753 invoked by alias); 11 Sep 2019 17:11:08 -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 59735 invoked by uid 89); 11 Sep 2019 17:11:07 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-7.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=dream, sk:iterate, multi-target, multitarget X-HELO: gateway34.websitewelcome.com Received: from gateway34.websitewelcome.com (HELO gateway34.websitewelcome.com) (192.185.148.194) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 11 Sep 2019 17:11:05 +0000 Received: from cm13.websitewelcome.com (cm13.websitewelcome.com [100.42.49.6]) by gateway34.websitewelcome.com (Postfix) with ESMTP id 8D3DE76FF35 for ; Wed, 11 Sep 2019 12:11:03 -0500 (CDT) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id 868ziGijZ3Qi0868zitGTx; Wed, 11 Sep 2019 12:11:03 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=4JOOHwaJTpQBLLabL6YSj9CxWnZ7zepUpj/1ysajq/0=; b=f5P7nhRGngAS7hcN5dfG5OuSfm 5PysvXQnr8AJIvBaAmyWFDv+N8Z5IBTbMbJ71nDO+QF55xFf8Jue0LPAzevoluyTw5juWIeJg9HYI 7srikkTLOAMHBVzn2dvcynlfW; Received: from 71-218-73-27.hlrn.qwest.net ([71.218.73.27]:41086 helo=murgatroyd) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.92) (envelope-from ) id 1i868z-002d9n-3e; Wed, 11 Sep 2019 12:11:01 -0500 From: Tom Tromey To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 17/23] Multi-target support References: <20190906232807.6191-1-palves@redhat.com> <20190906232807.6191-18-palves@redhat.com> Date: Wed, 11 Sep 2019 17:11:00 -0000 In-Reply-To: <20190906232807.6191-18-palves@redhat.com> (Pedro Alves's message of "Sat, 7 Sep 2019 00:28:01 +0100") Message-ID: <87zhja4v2z.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2019-09/txt/msg00201.txt.bz2 >>>>> "Pedro" == Pedro Alves writes: Pedro> This commit adds multi-target support to GDB. What this means is that Pedro> with this commit, GDB can now be connected to different targets at the Pedro> same time. E.g., you can debug a live native process and a core dump Pedro> at the same time, connect to multiple gdbservers, etc. Awesome! I read through the patch, but I can't really claim to understand how all the parts interrelate. However your overview made sense to me, and I don't have any conceptual concerns; just the usual sort of thing that there are lurking bugs. I guess my main fear is that this will introduce regressions... but on the other hand, I think it's important direction for gdb, so I'd rather err on the side of moving forward with it. Pedro> To fix this, Pedro> this commit renames gdbserver's target_ops to process_stratum_target. Pedro> I think this makes sense. There's no concept of target stack in Pedro> gdbserver, and gdbserver's target_ops really implements a Pedro> process_stratum-like target. Makes sense. Sometimes I dream about re-merging the target stacks, further shrinking the size of the gdbserver fork. Pedro> - you can only resume more that one target at the same time if all Pedro> targets support asynchronous debugging, and support non-stop mode. Pedro> It should be possible to support mixed all-stop + non-stop Pedro> backends, but that is left for another time. This means that Pedro> currently in order to do multi-target with gdbserver you need to Pedro> issue "maint set target-non-stop on". I would like to make that Pedro> mode be the default, but we're not there yet. Note that I'm Pedro> talking about how the target backend works, only. User-visible Pedro> all-stop mode works just fine. Pedro> - as explained above, connecting to different remote servers at the Pedro> same time is likely to produce bad results if they don't support the Pedro> exact set of RSP features. Are these limitations something that can be noticed when making the remote connection, or do they require the user to be careful? What happens if the user forgets or just doesn't know? Pedro> - thread_info *tp = find_thread_ptid (task_info->ptid); Pedro> + thread_info *tp = find_thread_ptid (inf->process_target (), task_info->ptid); There are a few spots in the patch that could use the overload that accepts an inferior, but instead call the process_target method directly. Pedro> +/* Calls target_commit_resume on all targets. */ Pedro> + Pedro> +static void Pedro> +commit_resume_all_targets () Pedro> +{ Pedro> + scoped_restore_current_thread restore_thread; Pedro> + Pedro> + for (inferior *inf : all_non_exited_inferiors ()) Pedro> + if (inf->has_execution ()) Pedro> + { Pedro> + switch_to_inferior_no_thread (inf); Pedro> + target_commit_resume (); Pedro> + } IIUC this can cause multiple calls to a given target's commit_resume method. That seems fine (assuming you audited these implementations) but I suppose it would be good to document this somewhere. Alternatively I guess gdb would need some kind of iterator that ensures it only visits each target once. Pedro> - event_ptid = wait_one (&ws); Pedro> + wait_one_event event = wait_one (); Pedro> + target_waitstatus &ws = event.ws; Pedro> + ptid_t &event_ptid = event.ptid; I was wondering if these need to be references. It seemed like maybe they could be const references but I couldn't immediately tell. Pedro> - For all-stop targets, we only step INFERIOR_PTID and continue others. */ Pedro> + For all-stop targets, we only step INFERIOR_PTID and continue others. */ This looks like extra indentation was added. Pedro> struct regcache * Pedro> get_thread_regcache_for_ptid (ptid_t ptid) Pedro> { Pedro> - return get_thread_regcache (ptid); Pedro> + /* This function doesn't take a process_stratum_target parameter Pedro> + because it's a common/ routine implemented by both gdb and Pedro> + gdbserver. It always refers to a ptid of the current target. */ It's "gdbsupport/" now. Pedro> /* The status of the stub support for the various vCont actions. */ Pedro> vCont_action_support supports_vCont; Pedro> + /* Whether vCont support was probed already. */ Pedro> + bool supports_vCont_probed; If it's the case that this is only a temporary measure, then I think this comment should mention that. Pedro> @@ -6601,6 +6622,8 @@ remote_target::commit_resume () Pedro> } Pedro> vcont_builder.flush (); Pedro> + Pedro> + target_async (1); Pedro> } I didn't understand this. Like, is it always ok to do this? Pedro> /* Callback for iterate_over_inferiors. Gets rid of the given Pedro> inferior. */ Pedro> -static int Pedro> -dispose_inferior (struct inferior *inf, void *args) Pedro> +static void Pedro> +dispose_inferior (inferior *inf) Pedro> { Pedro> /* Not all killed inferiors can, or will ever be, removed from the Pedro> inferior list. Killed inferiors clearly don't need to be killed Pedro> again, so, we're done. */ Pedro> if (inf->pid == 0) Pedro> - return 0; Pedro> + return; I think the comments here (both the intro comment and the one in the function) need to be updated, since it seems that just a single inferior is handled here now, and this is no longer a callback for iterate_over_inferiors. I didn't understand this change. Why did it used to iterate but now does not? Pedro> target_pass_ctrlc (void) Pedro> { Pedro> - current_top_target ()->pass_ctrlc (); Pedro> + /* Pass the Ctrl-C to the first inferior that has a thread Pedro> + running. */ Pedro> + for (inferior *inf : all_inferiors ()) Pedro> + { [...] Pedro> + return; I didn't understand this. It seemed to me that a C-c should maybe be sent to all running inferiors? I don't actually know this area. Maybe that's obvious now :) Pedro> /* See target.h. */ Pedro> @@ -3987,10 +4047,8 @@ set_write_memory_permission (const char *args, int from_tty, Pedro> } Pedro> void Pedro> -initialize_targets (void) Pedro> +_initialize_target (void) You might as well remove the 'void' when touching the line. Pedro> - explicit all_matching_threads_iterator (ptid_t filter_ptid); Pedro> + explicit all_matching_threads_iterator (process_stratum_target *filter_target, Pedro> + ptid_t filter_ptid); I guess this could drop the "explicit". Pedro> - explicit all_matching_threads_range (ptid_t filter_ptid) Pedro> - : m_filter_ptid (filter_ptid) Pedro> + explicit all_matching_threads_range (process_stratum_target *filter_target, Pedro> + ptid_t filter_ptid) Here too. Pedro> - explicit all_non_exited_threads_range (ptid_t filter_ptid) Pedro> - : m_filter_ptid (filter_ptid) Pedro> + explicit all_non_exited_threads_range (process_stratum_target *filter_target, Pedro> + ptid_t filter_ptid) Pedro> + : m_filter_target (filter_target), m_filter_ptid (filter_ptid) And here. Tom