public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 00/30] Switch interpreters to use virtual methods
@ 2023-05-02 20:49 Simon Marchi
  2023-05-02 20:49 ` [PATCH 01/30] gdb/mi: fix ^running record with multiple MI interpreters Simon Marchi
                   ` (31 more replies)
  0 siblings, 32 replies; 39+ messages in thread
From: Simon Marchi @ 2023-05-02 20:49 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

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 <typename ...Args>
    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

-- 
2.40.1


^ permalink raw reply	[flat|nested] 39+ messages in thread
end of thread, other threads:[~2023-05-30 19:09 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-02 20:49 [PATCH 00/30] Switch interpreters to use virtual methods Simon Marchi
2023-05-02 20:49 ` [PATCH 01/30] gdb/mi: fix ^running record with multiple MI interpreters Simon Marchi
2023-05-03 14:42   ` Alexandra Petlanova Hajkova
2023-05-03 15:02     ` Simon Marchi
2023-05-29 14:53   ` Simon Marchi
2023-05-02 20:49 ` [PATCH 02/30] gdb/mi: make current_token a field of mi_interp Simon Marchi
2023-05-04 12:31   ` Alexandra Petlanova Hajkova
2023-05-02 20:49 ` [PATCH 03/30] gdb: add interp::on_signal_received method Simon Marchi
2023-05-04 14:38   ` Alexandra Petlanova Hajkova
2023-05-02 20:49 ` [PATCH 04/30] gdb: add interp::on_normal_stop method Simon Marchi
2023-05-02 20:49 ` [PATCH 05/30] gdb: add interp::on_signal_exited method Simon Marchi
2023-05-02 20:49 ` [PATCH 06/30] gdb: add interp::on_exited method Simon Marchi
2023-05-02 20:49 ` [PATCH 07/30] gdb: add interp::on_no_history method Simon Marchi
2023-05-02 20:49 ` [PATCH 08/30] gdb: add interp::on_sync_execution_done method Simon Marchi
2023-05-02 20:49 ` [PATCH 09/30] gdb: add interp::on_command_error method Simon Marchi
2023-05-02 20:49 ` [PATCH 10/30] gdb: add interp::on_user_selected_context_changed method Simon Marchi
2023-05-02 20:49 ` [PATCH 11/30] gdb: add interp::on_new_thread method Simon Marchi
2023-05-02 20:49 ` [PATCH 12/30] gdb: add interp::on_thread_exited method Simon Marchi
2023-05-02 20:49 ` [PATCH 13/30] gdb: add interp::on_inferior_added method Simon Marchi
2023-05-02 20:49 ` [PATCH 14/30] gdb: add interp::on_inferior_appeared method Simon Marchi
2023-05-02 20:49 ` [PATCH 15/30] gdb: add interp::on_inferior_disappeared method Simon Marchi
2023-05-02 20:49 ` [PATCH 16/30] gdb: add interp::on_inferior_removed method Simon Marchi
2023-05-02 20:49 ` [PATCH 17/30] gdb: add interp::on_record_changed method Simon Marchi
2023-05-02 20:49 ` [PATCH 18/30] gdb: add interp::on_target_resumed method Simon Marchi
2023-05-02 20:49 ` [PATCH 19/30] gdb: add interp::on_solib_loaded method Simon Marchi
2023-05-02 20:50 ` [PATCH 20/30] gdb: add interp::on_solib_unloaded method Simon Marchi
2023-05-02 20:50 ` [PATCH 21/30] gdb: add interp::on_about_to_proceed method Simon Marchi
2023-05-02 20:50 ` [PATCH 22/30] gdb: add interp::on_traceframe_changed method Simon Marchi
2023-05-02 20:50 ` [PATCH 23/30] gdb: add interp::on_tsv_created method Simon Marchi
2023-05-02 20:50 ` [PATCH 24/30] gdb: add interp::on_tsv_deleted method Simon Marchi
2023-05-02 20:50 ` [PATCH 25/30] gdb: add interp::on_tsv_modified method Simon Marchi
2023-05-02 20:50 ` [PATCH 26/30] gdb: add interp::on_breakpoint_created method Simon Marchi
2023-05-02 20:50 ` [PATCH 27/30] gdb: add interp::on_breakpoint_deleted method Simon Marchi
2023-05-02 20:50 ` [PATCH 28/30] gdb: add interp::on_breakpoint_modified method Simon Marchi
2023-05-02 20:50 ` [PATCH 29/30] gdb: add interp::on_param_changed method Simon Marchi
2023-05-02 20:50 ` [PATCH 30/30] gdb: add interp::on_memory_changed method Simon Marchi
2023-05-02 20:50 ` [PATCH 00/30] Make interpreters use virtual methods (instead of observers) Simon Marchi
2023-05-23 12:43 ` [PATCH 00/30] Switch interpreters to use virtual methods Simon Marchi
2023-05-30 19:09   ` Simon Marchi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).