[00/30] Switch interpreters to use virtual methods

  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
  

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

Any thoughts on this?  Otherwise, I plan merging it by the end of the
week.

Simon
  

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