From patchwork Tue May 2 20:49:40 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 55731 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E002D3857346 for ; Tue, 2 May 2023 20:50:41 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E002D3857346 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1683060641; bh=yskoenAUk7CeJEFzPnE4grlP8FYA+7MuKEN69g8g2oI=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=Tf7YeP8fM9Kq6E1WXn2AY0eudvHbTuO/aJ8Ok6pXlq8tuBeIuemIM6HyzbH+JbmMh ucmn7QourlqhhycgldykiDDK+IPxjaADYHVHsBxpzlOSB9IkunNhpvl9Al6RKx3895 YRAeetzIt8sdBy51RD+gnxe/amPXltdd8js5JD3k= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 1CFED3858D1E for ; Tue, 2 May 2023 20:50:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1CFED3858D1E Received: from smarchi-efficios.internal.efficios.com (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (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 ESMTPSA id A6FAA1E0D4; Tue, 2 May 2023 16:50:13 -0400 (EDT) To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH 00/30] Switch interpreters to use virtual methods Date: Tue, 2 May 2023 16:49:40 -0400 Message-Id: <20230502205011.132151-1-simon.marchi@efficios.com> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 X-Spam-Status: No, score=-3491.3 required=5.0 tests=BAYES_00, KAM_DMARC_NONE, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_SOFTFAIL, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org 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: , X-Patchwork-Original-From: Simon Marchi via Gdb-patches From: Simon Marchi Reply-To: Simon Marchi Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" 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. Simon Marchi (30): gdb/mi: fix ^running record with multiple MI interpreters gdb/mi: make current_token a field of mi_interp gdb: add interp::on_signal_received method gdb: add interp::on_normal_stop method gdb: add interp::on_signal_exited method gdb: add interp::on_exited method gdb: add interp::on_no_history method gdb: add interp::on_sync_execution_done method gdb: add interp::on_command_error method gdb: add interp::on_user_selected_context_changed method gdb: add interp::on_new_thread method gdb: add interp::on_thread_exited method gdb: add interp::on_inferior_added method gdb: add interp::on_inferior_appeared method gdb: add interp::on_inferior_disappeared method gdb: add interp::on_inferior_removed method gdb: add interp::on_record_changed method gdb: add interp::on_target_resumed method gdb: add interp::on_solib_loaded method gdb: add interp::on_solib_unloaded method gdb: add interp::on_about_to_proceed method gdb: add interp::on_traceframe_changed method gdb: add interp::on_tsv_created method gdb: add interp::on_tsv_deleted method gdb: add interp::on_tsv_modified method gdb: add interp::on_breakpoint_created method gdb: add interp::on_breakpoint_deleted method gdb: add interp::on_breakpoint_modified method gdb: add interp::on_param_changed method gdb: add interp::on_memory_changed method gdb/breakpoint.c | 70 +- gdb/breakpoint.h | 5 + gdb/cli/cli-interp.c | 149 +-- gdb/cli/cli-interp.h | 9 + gdb/cli/cli-setshow.c | 13 +- gdb/corefile.c | 13 +- gdb/inferior.c | 49 +- gdb/infrun.c | 61 +- gdb/infrun.h | 11 + gdb/interps.c | 217 +++++ gdb/interps.h | 197 ++++ gdb/main.c | 2 +- gdb/mi/mi-cmd-break.c | 2 +- gdb/mi/mi-interp.c | 898 ++++++------------- gdb/mi/mi-interp.h | 39 + gdb/mi/mi-main.c | 33 +- gdb/mi/mi-main.h | 5 - gdb/observable.c | 11 - gdb/observable.h | 50 -- gdb/record-btrace.c | 3 +- gdb/record-full.c | 3 +- gdb/record.c | 3 +- gdb/remote.c | 5 +- gdb/solib.c | 24 +- gdb/source.c | 5 +- gdb/stack.c | 8 +- gdb/testsuite/gdb.mi/run-with-two-mi-uis.c | 7 + gdb/testsuite/gdb.mi/run-with-two-mi-uis.exp | 67 ++ gdb/testsuite/lib/mi-support.exp | 26 +- gdb/thread.c | 44 +- gdb/tracepoint.c | 17 +- 31 files changed, 1109 insertions(+), 937 deletions(-) create mode 100644 gdb/testsuite/gdb.mi/run-with-two-mi-uis.c create mode 100644 gdb/testsuite/gdb.mi/run-with-two-mi-uis.exp