Re: [PATCH 00/30] Switch interpreters to use virtual methods

[Simon Marchi <simark@simark.ca>]

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