From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id qP33EWkjxV+3LAAAWB0awg (envelope-from ) for ; Mon, 30 Nov 2020 11:52:57 -0500 Received: by simark.ca (Postfix, from userid 112) id 46E7E1F0AA; Mon, 30 Nov 2020 11:52:57 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 153881F0AA for ; Mon, 30 Nov 2020 11:52:56 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9FEA13898023; Mon, 30 Nov 2020 16:52:55 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 9FEA13898023 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1606755175; bh=cXx/0Il7kVnwOkaCPIm8vDt1u8Wo6Xt3rq7oIuvaeFc=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=BGZr8fehfQskvCpTtkW3llhyZZuFRs8HxM2yUWY9tX0+YNqwXLEZ7orZPIO8Bj20S h9HRkjpNAJjyLmwW3UrB7WgxudUYEg0ZA7BNJfbq1bW1tiFaqw4DQVzkDzlEUZHS6d hXaUmhrzXdcdVvcEwq+IaJ7xU4jmUdK2lngLT5XE= Received: from barracuda.ebox.ca (barracuda.ebox.ca [96.127.255.19]) by sourceware.org (Postfix) with ESMTPS id 1C0983896C27 for ; Mon, 30 Nov 2020 16:52:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1C0983896C27 X-ASG-Debug-ID: 1606755172-0c856e6cd6295f40001-fS2M51 Received: from smtp.ebox.ca (smtp.ebox.ca [96.127.255.82]) by barracuda.ebox.ca with ESMTP id Gdxenkd5UU6VntF2 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 30 Nov 2020 11:52:52 -0500 (EST) X-Barracuda-Envelope-From: simon.marchi@efficios.com X-Barracuda-RBL-Trusted-Forwarder: 96.127.255.82 Received: from epycamd.internal.efficios.com (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) by smtp.ebox.ca (Postfix) with ESMTP id 31F34441B21; Mon, 30 Nov 2020 11:52:52 -0500 (EST) X-Barracuda-RBL-IP: 192.222.181.218 X-Barracuda-Effective-Source-IP: 192-222-181-218.qc.cable.ebox.net[192.222.181.218] X-Barracuda-Apparent-Source-IP: 192.222.181.218 To: gdb-patches@sourceware.org Subject: [PATCH 1/4] gdb: make async event handlers clear themselves Date: Mon, 30 Nov 2020 11:52:48 -0500 X-ASG-Orig-Subj: [PATCH 1/4] gdb: make async event handlers clear themselves Message-Id: <20201130165251.830482-2-simon.marchi@efficios.com> X-Mailer: git-send-email 2.29.2 In-Reply-To: <20201130165251.830482-1-simon.marchi@efficios.com> References: <20201130165251.830482-1-simon.marchi@efficios.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Barracuda-Connect: smtp.ebox.ca[96.127.255.82] X-Barracuda-Start-Time: 1606755172 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://96.127.255.19:443/cgi-mod/mark.cgi X-Virus-Scanned: by bsmtpd at ebox.ca X-Barracuda-Scan-Msg-Size: 7006 X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 0.50 X-Barracuda-Spam-Status: No, SCORE=0.50 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=8.0 tests=BSF_RULE7568M X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.86230 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 0.50 BSF_RULE7568M Custom Rule 7568M X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Cc: Simon Marchi Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" The `ready` flag of async event handlers is cleared by the async event handler system right before invoking the associated callback, in check_async_event_handlers. This is not ideal with how the infrun subsystem consumes events: all targets' async event handler callbacks essentially just invoke `inferior_event_handler`, which eventually calls `fetch_inferior_event` and `do_target_wait`. `do_target_wait` picks an inferior at random, and thus a target at random (it could be the target whose `ready` flag was cleared, or not), and pulls one event from it. So it's possible that: - the async event handler for a target A is called - we end up consuming an event for target B - all threads of target B are stopped, target_async(0) is called on it, so its async event handler is cleared (e.g. record_btrace_target::async) As a result, target A still has events to report while its async event handler is left unmarked, so these events are not consumed. To counter this, at the end of their async event handler callbacks, targets check if they still have something to report and re-mark their async event handler (e.g. remote_async_inferior_event_handler). The linux_nat target does not suffer from this because it doesn't use an async event handler at the moment. It only uses a pipe registered with the event loop. It is written to in the SIGCHLD handler (and in other spots that want to get target wait method called) and read from in the target's wait method. So if linux_nat happened to be target A in the example above, the pipe would just stay readable, and the event loop would wake up again, until linux_nat's wait method is finally called and consumes the contents of the pipe. I think it would be nicer if targets using async_event_handler worked in a similar way, where the flag would stay set until the target's wait method is actually called. As a first step towards that, this patch moves the responsibility of clearing the ready flags of async event handlers to the invoked callback. All async event handler callbacks are modified to clear their ready flag before doing anything else. So in practice, nothing changes with this patch. It's only the responsibility of clearing the flag that is shifted toward the callee. gdb/ChangeLog: * async-event.h (async_event_handler_func): Add documentation. * async-event.c (check_async_event_handlers): Don't clear async_event_handler ready flag. * infrun.c (infrun_async_inferior_event_handler): Clear ready flag. * record-btrace.c (record_btrace_handle_async_inferior_event): Likewise. * record-full.c (record_full_async_inferior_event_handler): Likewise. * remote-notif.c (remote_async_get_pending_events_handler): Likewise. * remote.c (remote_async_inferior_event_handler): Likewise. Change-Id: I179ef8e99580eae642d332846fd13664dbddc0c1 --- gdb/async-event.c | 1 - gdb/async-event.h | 9 +++++++++ gdb/infrun.c | 1 + gdb/record-btrace.c | 1 + gdb/record-full.c | 1 + gdb/remote-notif.c | 4 +++- gdb/remote.c | 5 +++-- 7 files changed, 18 insertions(+), 4 deletions(-) diff --git a/gdb/async-event.c b/gdb/async-event.c index 4228dfb09e6..eb967b568c5 100644 --- a/gdb/async-event.c +++ b/gdb/async-event.c @@ -322,7 +322,6 @@ check_async_event_handlers () { if (async_handler_ptr->ready) { - async_handler_ptr->ready = 0; event_loop_debug_printf ("invoking async event handler `%s`", async_handler_ptr->name); (*async_handler_ptr->proc) (async_handler_ptr->client_data); diff --git a/gdb/async-event.h b/gdb/async-event.h index 8f279d63d63..10b9ae85112 100644 --- a/gdb/async-event.h +++ b/gdb/async-event.h @@ -24,6 +24,15 @@ struct async_signal_handler; struct async_event_handler; typedef void (sig_handler_func) (gdb_client_data); + +/* Type of async event handler callbacks. + + DATA is the client data originally passed to create_async_event_handler. + + The callback is called when the async event handler is marked. The callback + is responsible for clearing the async event handler if it no longer needs + to be called. */ + typedef void (async_event_handler_func) (gdb_client_data); extern struct async_signal_handler * diff --git a/gdb/infrun.c b/gdb/infrun.c index 2e5e837452d..eee96816e3b 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -9158,6 +9158,7 @@ static const struct internalvar_funcs siginfo_funcs = static void infrun_async_inferior_event_handler (gdb_client_data data) { + clear_async_event_handler (infrun_async_inferior_event_token); inferior_event_handler (INF_REG_EVENT); } diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c index dece9b60778..d5338c74aed 100644 --- a/gdb/record-btrace.c +++ b/gdb/record-btrace.c @@ -326,6 +326,7 @@ record_btrace_auto_disable (void) static void record_btrace_handle_async_inferior_event (gdb_client_data data) { + clear_async_event_handler (record_btrace_async_inferior_event_handler); inferior_event_handler (INF_REG_EVENT); } diff --git a/gdb/record-full.c b/gdb/record-full.c index 3b5e6fee7cd..410028aed60 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -904,6 +904,7 @@ static struct async_event_handler *record_full_async_inferior_event_token; static void record_full_async_inferior_event_handler (gdb_client_data data) { + clear_async_event_handler (record_full_async_inferior_event_token); inferior_event_handler (INF_REG_EVENT); } diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c index f18bc8678e3..3b81fd55557 100644 --- a/gdb/remote-notif.c +++ b/gdb/remote-notif.c @@ -108,8 +108,10 @@ remote_notif_process (struct remote_notif_state *state, static void remote_async_get_pending_events_handler (gdb_client_data data) { + remote_notif_state *notif_state = (remote_notif_state *) data; + clear_async_event_handler (notif_state->get_pending_events_token); gdb_assert (target_is_non_stop_p ()); - remote_notif_process ((struct remote_notif_state *) data, NULL); + remote_notif_process (notif_state, NULL); } /* Remote notification handler. Parse BUF, queue notification and diff --git a/gdb/remote.c b/gdb/remote.c index 71f814efb36..23c1bab0b27 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -14174,10 +14174,11 @@ remote_async_serial_handler (struct serial *scb, void *context) static void remote_async_inferior_event_handler (gdb_client_data data) { - inferior_event_handler (INF_REG_EVENT); - remote_target *remote = (remote_target *) data; remote_state *rs = remote->get_remote_state (); + clear_async_event_handler (rs->remote_async_inferior_event_token); + + inferior_event_handler (INF_REG_EVENT); /* inferior_event_handler may have consumed an event pending on the infrun side without calling target_wait on the REMOTE target, or -- 2.29.2