Re: [PATCH] Don't send queries to the MI interpreter

[Simon Marchi <simon.marchi@ericsson.com>]

On 17-02-10 01:07 PM, Pedro Alves wrote:
> On 02/10/2017 05:44 PM, Pedro Alves wrote:
> 
>> OK, I found the branch.  Pushed here now:
>>
>>   https://github.com/palves/gdb/commits/palves/console-pagination
> 
> And re-reading the branch again, I noticed this hunk:
> 
> @@ -1255,7 +1258,9 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args)
>       way, important error messages don't get lost when talking to GDB
>       over a pipe.  */
>    if (current_ui->instream != current_ui->stdin_stream
> -      || !input_interactive_p (current_ui))
> +      || !input_interactive_p (current_ui)
> +      /* We can't handle nested queries.  */
> +      || current_ui != main_ui)
>      {
>        old_chain = make_cleanup_restore_target_terminal ();
>  
> @@ -2045,36 +2050,48 @@ begin_line (void)
>      }
>  }
> 
> This is similar to your patch, but it handles something your
> version doesn't, I think.  That is the case of handling the secondary
> channel being a CLI interpreter, not an MI one, and that UI
> having been started on a terminal.
> 
> Until we put each UI/interpreter on its own thread, with its
> own event loop, we can't handle multiple UIs querying
> simultaneously.  Picture this situation:
> 
> Do this first on UI #1:
> 
>  (gdb) handle SIGINT
>  SIGINT is used by the debugger.
>  Are you sure you want to change it? (y or n) 
> 
> And then this on UI #2:
> 
>  (gdb) handle SIGTRAP
>  SIGTRAP is used by the debugger.
>  Are you sure you want to change it? (y or n) 
> 
> Now answer "yes" on UI #1.  What happens?
> 
> GDB incorrectly changes SIGTRAP, not SIGINT, and
> probably gets the UIs messed up. 

Indeed, it crashes too!

(gdb) handle SIGINT
SIGINT is used by the debugger.
Are you sure you want to change it? (y or n) y
y
readline: readline_callback_read_char() called with no handler!
[1]    15931 abort (core dumped)  ./gdb -nx test -ex "new-ui console /dev/pts/27" -ex "start" -ex "b main"

> Why?
> 
> Because we'd have this in pseudo-backtrace:
> 
> #0 gdb_readline_wrapper
> #1 defaulted_query                 // for UI #2
> #2 handle_command
> #3 execute_command ("handle SIGTRAP" ....
> #4 stdin_event_handler             // input on UI #2
> #5 gdb_do_one_event
> #7 gdb_readline_wrapper
> #8 defaulted_query                 // for UI #1
> #9 handle_command
> #10 execute_command ("handle SIGINT" ....
> #11 stdin_event_handler            // input on UI #1
> #12 gdb_do_one_event
> #13 gdb_readline_wrapper
> 
> So answering "yes", returns the read input to the
> query in frame #1...
> 
> So that hunk addresses this by only allowing queries on
> the main UI, similarly to how we only enable readline
> on the main UI.

The idea of only allowing queries on the main UI was also brought up in
discussions here.  I initially thought it was a bit too arbitrary, and
that we would want some on secondary UIs.  However, realistically, the only
use case of new-ui we know about so far is CLI as the main UI and MI as a
secondary UI.  Plus, since there won't be readline on secondary CLI UIs, they
will already be "dumbed" down, so I think it's fine to not have queries as well...
So I think your suggestion is good, at least until we discover that people use
multiple CLI UIs at the same time.

Note that we can trigger the same bug you mentioned higher with pagination.  For
example, I have a huge list of breakpoints and I do "info break" on UI #1 then on
UI #2.  Pressing enter on UI #1 unblocks UI #2, and the following enter crashes
GDB.  So should we also say that pagination is only allowed on the main UI for the
same reasons?

> So I think that to support multiple queries like that
> the simplest / most natural would be to make each
> UI above run on its own thread, so that each would have
> its own independent stack/frames.

Indeed.  That represents a tremendous amount of work I imagine,
putting the proper locking mechanisms in place...  And if you
are holding a lock while the query is issued, it would still
block some other things.