From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id khIxBQYyHGBiBgAAWB0awg (envelope-from ) for ; Thu, 04 Feb 2021 12:42:30 -0500 Received: by simark.ca (Postfix, from userid 112) id 070CB1EFCB; Thu, 4 Feb 2021 12:42:30 -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.0 required=5.0 tests=MAILING_LIST_MULTI 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 9EA7C1EF7A for ; Thu, 4 Feb 2021 12:42:28 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 1510F393C878; Thu, 4 Feb 2021 17:42:28 +0000 (GMT) Received: from mail-wr1-f43.google.com (mail-wr1-f43.google.com [209.85.221.43]) by sourceware.org (Postfix) with ESMTPS id A45663854817 for ; Thu, 4 Feb 2021 17:42:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A45663854817 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=alves.ped@gmail.com Received: by mail-wr1-f43.google.com with SMTP id z6so4504995wrq.10 for ; Thu, 04 Feb 2021 09:42:25 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=6UyTpfxHdyx3poFJXZ3zJAVGr9adMylQGJbexYgJNS4=; b=OqB/sNTJdeSX3lhojtzgz9phZ1Kl1m9q6KmfaXkQxowDC84EmKUrs2lNFuhQsXqJXh wzQDm7iNcxggTaJXVzkujFcDHZn74NJNJ36CKUeEv7TnHG5si6NIloUUEckJBPJY9liH Hf8fg4auJl18hQYrpeNPLZmrNsVlEan9/8y8vDUjgpkr5PjmJt9wP0/sE4yo+2htIFc2 HJZYZTsRk5lvYeRVb6xSFCpUfpjOvvJVuER3X6rh147+sHRmXwMfMEwSPKHv26lXh+TY f/ScIKJAyXih74Q1XVUoKvazsnYvdD7jna0ggABS/JhZmWILlCsZPB+o9ZQqrDHm0CwV vTrQ== X-Gm-Message-State: AOAM532wISeQSCCp4QPNT/cBTABTGARaqiv7N+hWHG1ixEMjAXiJ+6/o ZYB2xxslh3wV3AjZ7e71ZH5mkbkLHGOe5Q== X-Google-Smtp-Source: ABdhPJyq2YT1tNYp6z0m5uFf2xpqW+kwnyfta2wTsW2lY7XEZti8ykA+tIBlC0V77GfooZsNanNr5g== X-Received: by 2002:a5d:4d0b:: with SMTP id z11mr517849wrt.388.1612460544145; Thu, 04 Feb 2021 09:42:24 -0800 (PST) Received: from ?IPv6:2001:8a0:f91f:e900:642f:5bb0:6cba:b1bd? ([2001:8a0:f91f:e900:642f:5bb0:6cba:b1bd]) by smtp.gmail.com with ESMTPSA id j15sm9141455wrw.9.2021.02.04.09.42.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 04 Feb 2021 09:42:23 -0800 (PST) Subject: Re: [PATCH 1/4] gdb: make async event handlers clear themselves From: Pedro Alves To: Simon Marchi , gdb-patches@sourceware.org References: <20201130165251.830482-1-simon.marchi@efficios.com> <20201130165251.830482-2-simon.marchi@efficios.com> Message-ID: <0fe68d72-6530-cb97-a40a-e68a8f230702@palves.net> Date: Thu, 4 Feb 2021 17:42:06 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: <20201130165251.830482-2-simon.marchi@efficios.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: , Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 30/11/20 16:52, Simon Marchi via Gdb-patches wrote: > 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. (Watch out for spaces/tabs in the ChangeLog.) I like this. OK.