From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 279553857729 for ; Tue, 30 May 2023 19:09:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 279553857729 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [10.0.0.170] (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 6AFDD1E0D4; Tue, 30 May 2023 15:09:39 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1685473779; bh=BbFrrBO4UHkqTcyv5XlBm5Jex2UAsKc/EOyyJj6lON8=; h=Date:Subject:To:References:From:In-Reply-To:From; b=nf6qaYFlcI1QvmxivRxaEvxoZVtixe4+Djzwlg2QMofXOoUT1sMUWkREGSd8/MprG PbrrNDs64F5d5cVDIlTi+nAY14QoDQC4s/05Gu/HB6q1NFLXHg8LBYyC2m0XrWZmkK XmiTnkOPfAkq51a+dAslJoC1Kr0k9sre8uvsJsUc= Message-ID: <29e6b424-77dc-ad44-ab8c-633b8a4046f3@simark.ca> Date: Tue, 30 May 2023 15:09:38 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.2 Subject: Re: [PATCH 00/30] Switch interpreters to use virtual methods Content-Language: fr To: Simon Marchi , gdb-patches@sourceware.org References: <20230502205011.132151-1-simon.marchi@efficios.com> <48eb5ee9-53e1-f1df-b424-d08342c7e857@simark.ca> From: Simon Marchi In-Reply-To: <48eb5ee9-53e1-f1df-b424-d08342c7e857@simark.ca> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 5/23/23 08:43, Simon Marchi via Gdb-patches wrote: > On 5/2/23 16:49, Simon Marchi via Gdb-patches wrote: >> Interpreters currently get notified of events through observers, usually >> doing this pattern: >> >> SWITCH_THRU_ALL_UIS () >> { >> struct mi_interp *mi = as_mi_interp (top_level_interpreter ()); >> >> if (mi == NULL) >> continue; >> >> ... >> } >> >> The example here is for MI interpreters, but the CLI does the same. >> >> This series adds virtual methods to struct interp such that interpreters >> get notified of things happening through virtual method calls instead. >> >> The original reason for looking at that area was to fix some unstable >> ordering between a breakpoint stop message and a message output in an >> observer, related to the amd-dbgapi target. Pedro suggested this design >> change, which solves my ordering problem indirectly, and thought it was >> a good idea as well. The result looks much more like idiomatic C++ than >> the original. In particular, I like that each method implementation, in >> mi-interp.c and cli-interp.c, only has to worry about the current >> ("this") interpreter, removing all those scattered SWITCH_THRU_ALL_UIS >> calls. It also removes the dynamic_casts hidden in as_mi_interp and >> as_cli_interp_base. >> >> The testsuite passes fine for me. One thing I was wondering about is >> that the MI interpreter currently has this: >> >> static struct mi_interp * >> find_mi_interp (void) >> { >> struct mi_interp *mi; >> >> mi = as_mi_interp (top_level_interpreter ()); >> if (mi != NULL) >> return mi; >> >> mi = as_mi_interp (command_interp ()); >> if (mi != NULL) >> return mi; >> >> return NULL; >> } >> >> So, find_mi_interp sometimes returns the command_interp, if it's an MI >> interpreter. In my series, however, I only ever notify the top level >> interpreter, using this templated function: >> >> /* Helper interps_notify_* functions. Call METHOD on the top-level interpreter >> of all UIs. */ >> >> template >> void >> interps_notify (void (interp::*method) (Args...), Args... args) >> { >> SWITCH_THRU_ALL_UIS () >> { >> interp *tli = top_level_interpreter (); >> if (tli != nullptr) >> (tli->*method) (args...); >> } >> } >> >> I was wondering if I had to notify the command_interpreter at some >> point. Butwever I was not able to find a behavior change related to >> this, caused by my series. command_interpreter is set (temporarily) by >> interp_exec, so in order to have an MI interpreter as the command >> interpreter, you'd have to use an `interpreter-exec mi ...` command in >> the CLI. >> >> find_mi_interp is only used in a few observers observers that react to >> target wait events (like mi_on_signal_received). When I do, for >> instance: >> >> (gdb) interpreter-exec mi -exec-continue >> >> ... then the command_interpreter is only set for the duration of the >> -exec-continue command, which does not include consuming and handling >> the target event. Even with a synchronous target, the "wait" part is >> done outside the continue command, after coming back to the event loop, >> since 0b333c5e7d6c: >> >> @@ -3094,15 +3102,11 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) >> >> discard_cleanups (old_chain); >> >> - /* Wait for it to stop (if not standalone) >> - and in any case decode why it stopped, and act accordingly. */ >> - /* Do this only if we are not using the event loop, or if the target >> - does not support asynchronous execution. */ >> + /* Tell the event loop to wait for it to stop. If the target >> + supports asynchronous execution, it'll do this from within >> + target_resume. */ >> if (!target_can_async_p ()) >> - { >> - wait_for_inferior (); >> - normal_stop (); >> - } >> + mark_async_event_handler (infrun_async_inferior_event_token); >> } >> >> This means that all the observers using find_mi_interp are called when >> we are back at the event loop, after command_interpreter has been >> reset. And that would explain why my change looks good, despite never >> notifying the command_interpreter. >> >> Other than that, the first patch is a bug fix, fixing a problem I >> noticed along the way. > > Any thoughts on this? Otherwise, I plan merging it by the end of the > week. I pushed this series, except patch 2 ("gdb/mi: make current_token a field of mi_interp") which conflicts with master in a non-trivial way. However, it is not really necessary for the rest of the series. Simon