From patchwork Mon Dec 30 18:30:42 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 37129 Received: (qmail 107560 invoked by alias); 30 Dec 2019 18:30:52 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 107549 invoked by uid 89); 30 Dec 2019 18:30:52 -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, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=Connection, H*Ad:U*palves, nonstop, Sep X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-1.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (205.139.110.61) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 30 Dec 2019 18:30:49 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1577730648; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=rioSG3ERwbWlfxyC/TJ8qiNPyatLv4Oe6Vn5nWO76pY=; b=OnfWMrS8NzSVxPR4/6Y4Nu+hPTAHkPEGHmjghw0xn/edJPKudul5v+jmzjVldLJ4bTOmtJ pq65esnqpxz4O6jkrHEaWePZa/naBsUOabNie+Cq9iGNe6yuBb0t4q4sgodgXbfeNtCbzE U2Uqv/L3XQ6/89oqKIcQshs1jwUrc3U= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-148-IYZuB9hsPqGlVmR0FpdjKg-1; Mon, 30 Dec 2019 13:30:45 -0500 Received: by mail-ed1-f71.google.com with SMTP id w9so10945596eds.4 for ; Mon, 30 Dec 2019 10:30:45 -0800 (PST) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id o15sm5380680edj.3.2019.12.30.10.30.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 30 Dec 2019 10:30:43 -0800 (PST) Subject: Re: [PATCH v2 23/24] Require always-non-stop for multi-target resumptions To: Tom Tromey References: <20191017225026.30496-1-palves@redhat.com> <20191017225026.30496-24-palves@redhat.com> <877e4j4plh.fsf@tromey.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <20fe7d0b-d6c3-5eb4-9b44-b9cd86f0e576@redhat.com> Date: Mon, 30 Dec 2019 18:30:42 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <877e4j4plh.fsf@tromey.com> X-Mimecast-Spam-Score: 0 Hello, On 11/1/19 2:51 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves writes: > > Pedro> Currently, we can only support resuming multiple targets at the same > Pedro> time if all targets are in non-stop mode (or user-visible all-stop > Pedro> mode with target backend in non-stop mode). > > I don't understand this, but I would like to. > > I would have thought that target-async would be enough here. As in, the > necessary part is that target events be integrated with the event loop, > so that multiple event sources can be selected on at once. > > Why is non-stop needed instead? The main reason is the remote target -- in all-stop mode, the remote protocol is synchronous, irrespective of target-async. I.e., as soon as you resume the remote target in all-stop (with e.g., vCont), you can't send any packet to the remote until it reports a stop. The non-stop variant of the protocol makes vCont be asynchronous instead. A vCont results in an immediate "OK" reply, and then stops are reported via an asynchronous mechanism (%Stop). So if the remote backend is in non-stop mode, we can talk to it even if some thread is running. This makes things simpler for the common code. For example, breakpoint re-set resets every breakpoint and location, and that results in reading memory from all targets. If a remote all-stop target is running, then that memory read will fail, because we can't talk to the remote all-stop target until it fully stops. See . Another place that needs more work is where we call stop_all_threads, which needs to be taught to only explicitly pause all threads of a target one by one if the target is in non-stop mode. I ran into things like these when I last tried to make it work, which made me punt until the main chunks of multi-target are in. How about this alternative comment? +/* Check that all the targets we're about to resume are in non-stop + mode. Ideally, we'd only care whether all targets support + target-async, but we're not there yet. E.g., stop_all_threads + doesn't know how to handle all-stop targets. Also, the remote + protocol in all-stop mode is synchronous, irrespective of + target-async, which means that things like a breakpoint re-set + triggered by one target would try to read memory from all targets + and fail. */ + +static void +check_multi_target_resumption (process_stratum_target *resume_target) From a33b584acec4af5d11dc35ec78afed0bf25742ff Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 30 Oct 2019 16:51:04 +0000 Subject: [PATCH] Require always-non-stop for multi-target resumptions Currently, we can only support resuming multiple targets at the same time if all targets are in non-stop mode (or user-visible all-stop mode with target backend in non-stop mode). This patch makes GDB error out if the user tries to resume more than one target at the same time and one of the resumed targets isn't in non-stop mode: (gdb) info inferiors Num Description Connection Executable 1 process 15303 1 (native) a.out * 2 process 15286 2 (extended-remote :9999) a.out (gdb) set schedule-multiple on (gdb) c Continuing. Connection 2 (extended-remote :9999) does not support multi-target resumption. This is here later in the series instead of in the main multi-target patch because it depends the previous patch, which added process_stratum_target::connection_string(). gdb/ChangeLog: yyyy-mm-dd Pedro Alves * infrun.c: Include "target-connection.h". (check_multi_target_resumption): New. (proceed): Call it. * target-connection.c (make_target_connection_string): Make static. * target-connection.h (make_target_connection_string): Declare. --- gdb/infrun.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++ gdb/target-connection.c | 7 ++---- gdb/target-connection.h | 8 +++++++ 3 files changed, 68 insertions(+), 5 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index 688f9e029a..ce2de256df 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -28,6 +28,7 @@ #include "gdbcore.h" #include "gdbcmd.h" #include "target.h" +#include "target-connection.h" #include "gdbthread.h" #include "annotate.h" #include "symfile.h" @@ -2881,6 +2882,61 @@ commit_resume_all_targets () } } +/* Check that all the targets we're about to resume are in non-stop + mode. Ideally, we'd only care whether all targets support + target-async, but we're not there yet. E.g., stop_all_threads + doesn't know how to handle all-stop targets. Also, the remote + protocol in all-stop mode is synchronous, irrespective of + target-async, which means that things like a breakpoint re-set + triggered by one target would try to read memory from all targets + and fail. */ + +static void +check_multi_target_resumption (process_stratum_target *resume_target) +{ + if (!non_stop && resume_target == nullptr) + { + scoped_restore_current_thread restore_thread; + + /* This is used to track whether we're resuming more than one + target. */ + process_stratum_target *first_connection = nullptr; + + /* The first inferior we see with a target that does not work in + always-non-stop mode. */ + inferior *first_not_non_stop = nullptr; + + for (inferior *inf : all_non_exited_inferiors (resume_target)) + { + switch_to_inferior_no_thread (inf); + + if (!target_has_execution) + continue; + + process_stratum_target *proc_target + = current_inferior ()->process_target(); + + if (!target_is_non_stop_p ()) + first_not_non_stop = inf; + + if (first_connection == nullptr) + first_connection = proc_target; + else if (first_connection != proc_target + && first_not_non_stop != nullptr) + { + switch_to_inferior_no_thread (first_not_non_stop); + + proc_target = current_inferior ()->process_target(); + + error (_("Connection %d (%s) does not support " + "multi-target resumption."), + proc_target->connection_number, + make_target_connection_string (proc_target).c_str ()); + } + } + } +} + /* Basic routine for continuing the program in various fashions. ADDR is the address to resume at, or -1 for resume where stopped. @@ -2931,6 +2987,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) process_stratum_target *resume_target = user_visible_resume_target (resume_ptid); + check_multi_target_resumption (resume_target); + if (addr == (CORE_ADDR) -1) { if (pc == cur_thr->suspend.stop_pc diff --git a/gdb/target-connection.c b/gdb/target-connection.c index a079f218bd..dd0300a82b 100644 --- a/gdb/target-connection.c +++ b/gdb/target-connection.c @@ -53,12 +53,9 @@ connection_list_remove (process_stratum_target *t) t->connection_number = 0; } -/* Make a target connection string for T. This is usually T's - shortname, but it includes the result of - process_stratum_target::connection_string() too if T supports - it. */ +/* See target-connection.h. */ -static std::string +std::string make_target_connection_string (process_stratum_target *t) { if (t->connection_string () != NULL) diff --git a/gdb/target-connection.h b/gdb/target-connection.h index 0e2dc128d8..0b31924b99 100644 --- a/gdb/target-connection.h +++ b/gdb/target-connection.h @@ -20,6 +20,8 @@ #ifndef TARGET_CONNECTION_H #define TARGET_CONNECTION_H +#include + struct process_stratum_target; /* Add a process target to the connection list, if not already @@ -29,4 +31,10 @@ void connection_list_add (process_stratum_target *t); /* Remove a process target from the connection list. */ void connection_list_remove (process_stratum_target *t); +/* Make a target connection string for T. This is usually T's + shortname, but it includes the result of + process_stratum_target::connection_string() too if T supports + it. */ +std::string make_target_connection_string (process_stratum_target *t); + #endif /* TARGET_CONNECTION_H */