Don't send queries to the MI interpreter

Message ID 20170210163650.10334-1-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Feb. 10, 2017, 4:36 p.m. UTC
  We have a little problem in Eclipse CDT related to queries being sent on
the MI channel.  GDB sends queries on the MI stream and waits for an
answer (y or n), but since CDT will never answer, it causes a deadlock.

Note that this is only a problem when using MI as a side-channel
(new-ui) on a dedicated tty.  It doesn't happen if GDB's input/output
streams are pipes, for example.  In that case, the queries are
auto-answered as they should.

The situation can be triggered by modifying a variable value while
replaying.  Roughly, the steps are:

  - Start recording (record-full)
  - Step forward a bit
  - Step backwards a bit
  - Change a variable using the Variables view in the UI

Changing the variable uses the -var-assign command.  Modifying the
memory while replaying causes the following query to be sent:

  ~"Because GDB is in replay mode, writing to memory will make the execution \
    log unusable from this point onward.  Write memory at address \
    0x7fffffffdabc?(y or n) "

In the past, the query would get auto-answered because GDB would
consider the input as non-interactive.  The criterion for this is
whether the input stream is a tty (ISATTY).  Now that we create the MI
channel with new-ui and pass it a tty (/dev/pts/X), GDB considers the
input as interactive, so the query is not auto answered.

I have a hard time defining what should be the right criteria for when
we should or shouldn't query.  I think we'll have to take into account
what is the originating interpreter, CLI or MI.

A special case is when we receive CLI through MI (either
-interpreter-exec console or typing directly CLI commands): it's
possible that it's a human at the other end, typing in a field in the
front-end, which then forwards the commands to GDB.  In that case, we
could want to query, since the user is there to respond.  However, it
would be very fragile, as the front-end should not send any command in
the mean time, not reply anything else than "y" or "n" and not wrap the
user response in -interpreter-exec...  Currently, we never query as a
result of CLI-in-MI commands anyway, because deprecated_query_hook is
set.  So I think we should just keep this behaviour: queries resulting
from CLI-in-MI commands get auto-answered.

Conveniently, when using CLI in MI, the current_interpreter stays MI
(unlike when using MI in CLI), so it allows to differentiate a CLI
command from a CLI-in-MI command easily.

So the criteria I came up with in order to decide whether we should
not auto answer are: the input should be interactive (ISATTY is true)
_and_  the originating interpreter should support queries.

In practice, it means the only time we are going to query is when it's
a CLI command typed in a CLI UI coming from a tty.

Currently, CLI supports queries and MI doesn't.  However, we could
imagine that some day GDB could send the query to the front-end through
MI, so that it displays a nice popup.  That's a whole separate topic
though, since it would require changing the MI paradigm a bit.

gdb/ChangeLog:

	* interps.h (interp::supports_queries): New.
	* cli-interp.h (cli_interp_base::supports_queries): New.
	* cli-interp.c (cli_interp_base::supports_queries): New.
	* utils.c (defaulted_query): Don't query if the current
	interpreter doesn't support queries.
---
 gdb/cli/cli-interp.c | 6 ++++++
 gdb/cli/cli-interp.h | 1 +
 gdb/interps.h        | 4 ++++
 gdb/utils.c          | 5 ++++-
 4 files changed, 15 insertions(+), 1 deletion(-)
  

Comments

Pedro Alves Feb. 10, 2017, 4:48 p.m. UTC | #1
On 02/10/2017 04:36 PM, Simon Marchi wrote:
> We have a little problem in Eclipse CDT related to queries being sent on
> the MI channel.  GDB sends queries on the MI stream and waits for an
> answer (y or n), but since CDT will never answer, it causes a deadlock.
> 
> Note that this is only a problem when using MI as a side-channel
> (new-ui) on a dedicated tty.  It doesn't happen if GDB's input/output
> streams are pipes, for example.  In that case, the queries are
> auto-answered as they should.

I think we could have a testsuite test for this, as the 'new-ui'-related
testcases create a pty for the secondary MI channel ("separate-mi-tty")?

Thanks,
Pedro Alves
  
Simon Marchi Feb. 10, 2017, 4:52 p.m. UTC | #2
On 2017-02-10 11:48, Pedro Alves wrote:
> On 02/10/2017 04:36 PM, Simon Marchi wrote:
>> We have a little problem in Eclipse CDT related to queries being sent 
>> on
>> the MI channel.  GDB sends queries on the MI stream and waits for an
>> answer (y or n), but since CDT will never answer, it causes a 
>> deadlock.
>> 
>> Note that this is only a problem when using MI as a side-channel
>> (new-ui) on a dedicated tty.  It doesn't happen if GDB's input/output
>> streams are pipes, for example.  In that case, the queries are
>> auto-answered as they should.
> 
> I think we could have a testsuite test for this, as the 
> 'new-ui'-related
> testcases create a pty for the secondary MI channel 
> ("separate-mi-tty")?

Right, I didn't think of making a test, I'll work on it.  I'll try to 
find a query that's easier to trigger than the 
modify-memory-while-replaying one though.
  
Pedro Alves Feb. 10, 2017, 5:12 p.m. UTC | #3
On 02/10/2017 04:52 PM, Simon Marchi wrote:
> On 2017-02-10 11:48, Pedro Alves wrote:
>> On 02/10/2017 04:36 PM, Simon Marchi wrote:
>>> We have a little problem in Eclipse CDT related to queries being sent on
>>> the MI channel.  GDB sends queries on the MI stream and waits for an
>>> answer (y or n), but since CDT will never answer, it causes a deadlock.
>>>
>>> Note that this is only a problem when using MI as a side-channel
>>> (new-ui) on a dedicated tty.  It doesn't happen if GDB's input/output
>>> streams are pipes, for example.  In that case, the queries are
>>> auto-answered as they should.
>>
>> I think we could have a testsuite test for this, as the 'new-ui'-related
>> testcases create a pty for the secondary MI channel ("separate-mi-tty")?
> 
> Right, I didn't think of making a test, I'll work on it.  I'll try to
> find a query that's easier to trigger than the
> modify-memory-while-replaying one though.

I've run into these queries deep down in the record layer
in the past, and wondered about getting rid of them.

Particularly this one:

	  if (!yquery (_("Do you want to auto delete previous execution "
			"log entries when record/replay buffer becomes "
			"full (record full stop-at-limit)?")))
	    error (_("Process record: stopped by user."));

In order to query, the inferior is already temporarily stopped.
So why not just unconditionally error, and lets user tweak
"set record stop-at-limit" and continue if they want.

In this case, instead of:

	      query (_("Because GDB is in replay mode, changing the "
		       "value of a register will make the execution "
		       "log unusable from this point onward.  "
		       "Change all registers?"));

we'd error here with something like:

 Cannot change the value of a register in replay mode, it would
 make the execution log unusable from this point onward.  See whatnot.

and then if users really want to change memory, they would use
something like a "record discard" or "record commit" or some such
command that discards history leaving the inferior state as it
is right now (not sure there's a command that does that already.)
In scripts and maybe frontends, you'd use the existing
"set record memory-query off", renamed to
"set record allow-memory-write on" or some such.

Thanks,
Pedro Alves
  
Pedro Alves Feb. 10, 2017, 5:44 p.m. UTC | #4
On 02/10/2017 05:12 PM, Pedro Alves wrote:

> I've run into these queries deep down in the record layer
> in the past, and wondered about getting rid of them.
> 
> Particularly this one:
> 
> 	  if (!yquery (_("Do you want to auto delete previous execution "
> 			"log entries when record/replay buffer becomes "
> 			"full (record full stop-at-limit)?")))
> 	    error (_("Process record: stopped by user."));
> 
> In order to query, the inferior is already temporarily stopped.
> So why not just unconditionally error, and lets user tweak
> "set record stop-at-limit" and continue if they want.

Let me expand on this a bit.

I have a local unfinished branch somewhere that attempts to address
the "pagination/queries in the primary CLI prevent MI async
notifications (like *stopped) on the secondary MI channel" issue.
I think there's a PR about it, but I can't find it now.  It's the
one that led to Eclipse disabling pagination.

The issue is that pagination is handled by starting a new nested
event loop deep inside the print that triggered the pagination,
and while a secondary event loop is running, no target events
are processed at all (see gdb_readline_wrapper, the
target_async(0) call).  So GDB will only notice that the inferior
stopped for a trap when the pagination/query is unblocked.

So the idea to address this was to continue processing target
events in the background and send them to MI, even while the
CLI is stopped in the pagination.  Meanwhile, if some target event
ends up producing _more_ output _while_ the CLI is already
waiting for the user to request another page of output, we'd need
to buffer the output somewhere temporarily, in order
to later dump it on the screen when whatever we were
outputting that paginated is fully flushed/output.

This suggested actually always buffering asynchronous output
and dumping it on the screen at a safe point.  I.e., output
generated inside handle_inferior_event would always go to
a buffer, and then dumped on the screen after processing the event,
when safe.

And it is at this point that I thought that it is odd to
query and ask for user input deep down inside the record layer,
while handling some asynchronous execution event -- i.e.,
deep down inside handle_inferior_event.  If we're buffering
output, when the user won't see the query anyway.
Hmm, now that I think of it, the "Do you want to auto delete
previous execution" query wouldn't be converted to an error,
but instead to a something like TARGET_WAITKIND_NO_HISTORY,
I suppose.

OK, I found the branch.  Pushed here now:

  https://github.com/palves/gdb/commits/palves/console-pagination

Thanks,
Pedro Alves
  
Pedro Alves Feb. 10, 2017, 6:07 p.m. UTC | #5
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. 

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.

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.

Thanks,
Pedro Alves
  
Pedro Alves Feb. 10, 2017, 6:36 p.m. UTC | #6
On 02/10/2017 06: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.

So with with all the rationale I've thrown out, and idea
that we shouldn't see a query anyway, and considering that
real MI commands shouldn't really query anyway (nobody sees
the query output), it may be actually impossible to find
a way to this with MI on the secondary channel, that will
remain stable going forward.  Hmm.

So how about using the hunk shown above, which should handle
your case as well, and while at it write a test that
uses CLI in the secondary UI instead, making sure a query
is auto-answered in that UI?

Thanks,
Pedro Alves
  
Simon Marchi Feb. 10, 2017, 7:02 p.m. UTC | #7
On 17-02-10 12:44 PM, Pedro Alves wrote:
> And it is at this point that I thought that it is odd to
> query and ask for user input deep down inside the record layer,
> while handling some asynchronous execution event -- i.e.,
> deep down inside handle_inferior_event.  If we're buffering
> output, when the user won't see the query anyway.
> Hmm, now that I think of it, the "Do you want to auto delete
> previous execution" query wouldn't be converted to an error,
> but instead to a something like TARGET_WAITKIND_NO_HISTORY,
> I suppose.

And TARGET_WAITKIND_NO_HISTORY would cause a user-visible stop?

(I'm processing the rest and will reply to your latest message)
  
Simon Marchi Feb. 10, 2017, 7:05 p.m. UTC | #8
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.
  
Simon Marchi Feb. 10, 2017, 7:07 p.m. UTC | #9
On 17-02-10 01:36 PM, Pedro Alves wrote:
> On 02/10/2017 06: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.
> 
> So with with all the rationale I've thrown out, and idea
> that we shouldn't see a query anyway, and considering that
> real MI commands shouldn't really query anyway (nobody sees
> the query output), it may be actually impossible to find
> a way to this with MI on the secondary channel, that will
> remain stable going forward.  Hmm.

I'm not sure I understand this part:

> it may be actually impossible to find
> a way to this with MI on the secondary channel, that will
> remain stable going forward.


> So how about using the hunk shown above, which should handle
> your case as well, and while at it write a test that
> uses CLI in the secondary UI instead, making sure a query
> is auto-answered in that UI?
> 
> Thanks,
> Pedro Alves
> 

That's fine with me.
  
Pedro Alves Feb. 10, 2017, 7:08 p.m. UTC | #10
On 02/10/2017 07:02 PM, Simon Marchi wrote:
> On 17-02-10 12:44 PM, Pedro Alves wrote:
>> And it is at this point that I thought that it is odd to
>> query and ask for user input deep down inside the record layer,
>> while handling some asynchronous execution event -- i.e.,
>> deep down inside handle_inferior_event.  If we're buffering
>> output, when the user won't see the query anyway.
>> Hmm, now that I think of it, the "Do you want to auto delete
>> previous execution" query wouldn't be converted to an error,
>> but instead to a something like TARGET_WAITKIND_NO_HISTORY,
>> I suppose.
> 
> And TARGET_WAITKIND_NO_HISTORY would cause a user-visible stop?

Yes.  TARGET_WAITKIND_NO_HISTORY already exists.  It causes a
user-visible stop with "No more reverse-execution history".

This case is similar, except it's more like
"No more SPACE in reverse-execution history".

> (I'm processing the rest and will reply to your latest message)

Thanks,
Pedro Alves
  
Pedro Alves Feb. 10, 2017, 7:19 p.m. UTC | #11
On 02/10/2017 07:05 PM, Simon Marchi wrote:
> On 17-02-10 01:07 PM, Pedro Alves wrote:

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

I had it working with a GIL/BKL-style lock, in the original
"new-console" prototype that was later rewritten into what
is "new-ui" today.  I.e,. even though we'd have multiple
threads, only one thread really runs at a time.  The idea
was that we'd start with a big lock, and then over time break
down the lock into more finer-grained locks.

Here:

https://github.com/palves/gdb/commits/palves/console-extra

- A thread per UI.  See the "Start a thread for each UI" patch.
- Per-UI readline (with a giant readline hack)
- Per-UI nurses/TUI instance (it really works!) :-)

The trouble is that this then trips on another nasty problem:

All ptrace calls targeting a process must be issued
from the thread that first attached to that inferior process.
The kernel rejects the ptrace call otherwise eith EIO/EINVAL
or some such, I don't recall which.  So on that branch, with
native debugging, if you start the inferior on UI #1, and then
try to read memory from UI #2, it fails...  If you instead
try the same against gdbserver, it all works, because in that
case gdbserver handles the ptrace calls, not gdb, so all
ptrace calls come from the same thread in that case.
So for native debugging, we'd need to marshall all ptrace
requests through the same thread...

Thanks,
Pedro Alves
  
Pedro Alves Feb. 10, 2017, 7:23 p.m. UTC | #12
On 02/10/2017 07:07 PM, Simon Marchi wrote:
> I'm not sure I understand this part:
> 
>> > it may be actually impossible to find
>> > a way to this with MI on the secondary channel, that will
>> > remain stable going forward.

If we write a test today that relies on the output of a query
that arguably shouldn't exist, then when we finally remove
the query call, we must remove the test as well.

Thanks,
Pedro Alves
  
Simon Marchi Feb. 10, 2017, 7:26 p.m. UTC | #13
On 17-02-10 02:19 PM, Pedro Alves wrote:
> On 02/10/2017 07:05 PM, Simon Marchi wrote:
>> On 17-02-10 01:07 PM, Pedro Alves wrote:
> 
>>> 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.
> 
> I had it working with a GIL/BKL-style lock, in the original
> "new-console" prototype that was later rewritten into what
> is "new-ui" today.  I.e,. even though we'd have multiple
> threads, only one thread really runs at a time.  The idea
> was that we'd start with a big lock, and then over time break
> down the lock into more finer-grained locks.
> 
> Here:
> 
> https://github.com/palves/gdb/commits/palves/console-extra
> 
> - A thread per UI.  See the "Start a thread for each UI" patch.
> - Per-UI readline (with a giant readline hack)
> - Per-UI nurses/TUI instance (it really works!) :-)

And which thread handles inferior events?

> The trouble is that this then trips on another nasty problem:
> 
> All ptrace calls targeting a process must be issued
> from the thread that first attached to that inferior process.
> The kernel rejects the ptrace call otherwise eith EIO/EINVAL
> or some such, I don't recall which.  So on that branch, with
> native debugging, if you start the inferior on UI #1, and then
> try to read memory from UI #2, it fails...  If you instead
> try the same against gdbserver, it all works, because in that
> case gdbserver handles the ptrace calls, not gdb, so all
> ptrace calls come from the same thread in that case.
> So for native debugging, we'd need to marshall all ptrace
> requests through the same thread...

A PtraceService...  it starts to look just like CDT! :)
  
Pedro Alves Feb. 10, 2017, 7:29 p.m. UTC | #14
On 02/10/2017 07:05 PM, Simon Marchi wrote:
> 
> 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.

I assume you mean that on UI #2 you do the same.

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

Yes, I think so.  The branch disabled it too, see filtering_enabled.

Thanks,
Pedro Alves
  
Pedro Alves Feb. 10, 2017, 7:32 p.m. UTC | #15
On 02/10/2017 07:26 PM, Simon Marchi wrote:

> And which thread handles inferior events?

The main thread, IIRC.  If you only have one UI,
then everything runs on the main thread like it runs today.
If you have one extra UI, that UI gets its own thread, and
on and on.  Baby steps.  :-)

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 8712c75f39..709bfbbce3 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -356,6 +356,12 @@  cli_interp_base::supports_command_editing ()
   return true;
 }
 
+bool
+cli_interp_base::supports_queries ()
+{
+  return true;
+}
+
 static struct gdb_exception
 safe_execute_command (struct ui_out *command_uiout, char *command, int from_tty)
 {
diff --git a/gdb/cli/cli-interp.h b/gdb/cli/cli-interp.h
index de9da83347..f1e966fe77 100644
--- a/gdb/cli/cli-interp.h
+++ b/gdb/cli/cli-interp.h
@@ -31,6 +31,7 @@  public:
   void set_logging (ui_file_up logfile, bool logging_redirect) override;
   void pre_command_loop () override;
   bool supports_command_editing () override;
+  bool supports_queries () override;
 };
 
 /* Make the output ui_file to use when logging is enabled.
diff --git a/gdb/interps.h b/gdb/interps.h
index 1b9580c118..d328163027 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -76,6 +76,10 @@  public:
   virtual bool supports_command_editing ()
   { return false; }
 
+  /* Returns true if this interpreter supports queries.  */
+  virtual bool supports_queries ()
+  { return false; }
+
   /* This is the name in "-i=" and "set interpreter".  */
   const char *name;
 
diff --git a/gdb/utils.c b/gdb/utils.c
index 3dc2f03d61..ba1db8a45b 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1170,12 +1170,15 @@  defaulted_query (const char *ctlstr, const char defchar, va_list args)
   if (!confirm || server_command)
     return def_value;
 
+  struct interp *interp = current_interpreter ();
+
   /* If input isn't coming from the user directly, just say what
      question we're asking, and then answer the default automatically.  This
      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)
+      || !interp->supports_queries ())
     {
       old_chain = make_cleanup_restore_target_terminal ();