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

[Simon Marchi <simark@simark.ca>]

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