[v2] Restore terminal state in mi_thread_exit (PR gdb/17627)

  When a thread exits, the terminal is left in mode "terminal_is_ours"
while the target executes. This patch fixes that.

From my understanding, a function calling target_terminal_ours expects
that the terminal could be in any state at the moment it is called.
Therefore, it should be its reponsibility to put back the terminal in
whatever state it was before being called.

I find that this fits quite well the cleanup model, so I implemented a
cleanup for that.

New in v2:

* Coding style fixes.
* Use make_cleanup_dtor instead of make_cleanup.

gdb/ChangeLog:

	PR gdb/17627
	* target.c (cleanup_restore_target_terminal): New function.
	(make_cleanup_restore_target_terminal): New function.
	* target.h (make_cleanup_restore_target_terminal): New
	declaration.
	* mi/mi-interp.c (mi_thread_exit): Use the new cleanup.

Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>
---
 gdb/mi/mi-interp.c |  4 ++++
 gdb/target.c       | 34 ++++++++++++++++++++++++++++++++++
 gdb/target.h       |  4 ++++
 3 files changed, 42 insertions(+)
  

On Tue, Dec 2, 2014 at 5:10 PM, Simon Marchi <simon.marchi@ericsson.com> wrote:
> When a thread exits, the terminal is left in mode "terminal_is_ours"
> while the target executes. This patch fixes that.
>
> From my understanding, a function calling target_terminal_ours expects
> that the terminal could be in any state at the moment it is called.
> Therefore, it should be its reponsibility to put back the terminal in
> whatever state it was before being called.

It seems to me that the other observer callbacks defined in
mi-interp.c are also affected by this issue, that is, the issue of
having to restore the original terminal state after altering it.  So I
wonder if it would make sense to shift this responsibility to the
observer module itself (i.e. generic_observer_notify()), so that all
observers implicitly restore the original terminal state when they
return.  That way this kind of pattern wouldn't have to be duplicated
for each individual observer.

>
> I find that this fits quite well the cleanup model, so I implemented a
> cleanup for that.
>
> New in v2:
>
> * Coding style fixes.
> * Use make_cleanup_dtor instead of make_cleanup.
>
> gdb/ChangeLog:
>
>         PR gdb/17627
>         * target.c (cleanup_restore_target_terminal): New function.
>         (make_cleanup_restore_target_terminal): New function.
>         * target.h (make_cleanup_restore_target_terminal): New
>         declaration.
>         * mi/mi-interp.c (mi_thread_exit): Use the new cleanup.
>
> Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>
> ---
>  gdb/mi/mi-interp.c |  4 ++++
>  gdb/target.c       | 34 ++++++++++++++++++++++++++++++++++
>  gdb/target.h       |  4 ++++
>  3 files changed, 42 insertions(+)
>
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index df2b558..60f0666 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -386,6 +386,7 @@ mi_thread_exit (struct thread_info *t, int silent)
>  {
>    struct mi_interp *mi;
>    struct inferior *inf;
> +  struct cleanup *old_chain;
>
>    if (silent)
>      return;
> @@ -393,11 +394,14 @@ mi_thread_exit (struct thread_info *t, int silent)
>    inf = find_inferior_pid (ptid_get_pid (t->ptid));
>
>    mi = top_level_interpreter_data ();
> +  old_chain = make_cleanup_restore_target_terminal ();
>    target_terminal_ours ();
>    fprintf_unfiltered (mi->event_channel,
>                       "thread-exited,id=\"%d\",group-id=\"i%d\"",
>                       t->num, inf->num);
>    gdb_flush (mi->event_channel);
> +
> +  do_cleanups (old_chain);
>  }
>
>  /* Emit notification on changing the state of record.  */
> diff --git a/gdb/target.c b/gdb/target.c
> index ab5f2b9..7161e62 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -528,6 +528,40 @@ target_supports_terminal_ours (void)
>    return 0;
>  }
>
> +/* Restore the terminal to its previous state (helper for
> +   make_cleanup_restore_target_terminal). */
> +
> +static void
> +cleanup_restore_target_terminal (void *arg)
> +{
> +  enum terminal_state *previous_state = arg;
> +
> +  switch (*previous_state)
> +    {
> +    case terminal_is_ours:
> +      target_terminal_ours ();
> +      break;
> +    case terminal_is_ours_for_output:
> +      target_terminal_ours_for_output ();
> +      break;
> +    case terminal_is_inferior:
> +      target_terminal_inferior ();
> +      break;
> +    }
> +}
> +
> +/* See target.h. */
> +
> +struct cleanup *
> +make_cleanup_restore_target_terminal (void)
> +{
> +  enum terminal_state *ts = xmalloc (sizeof (*ts));
> +
> +  *ts = terminal_state;
> +
> +  return make_cleanup_dtor (cleanup_restore_target_terminal, ts, xfree);
> +}
> +
>  static void
>  tcomplain (void)
>  {
> diff --git a/gdb/target.h b/gdb/target.h
> index d363b61..eb3220e 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -1413,6 +1413,10 @@ extern void target_terminal_ours (void);
>
>  extern int target_supports_terminal_ours (void);
>
> +/* Make a cleanup that restores the state of the terminal to the current
> +   value.  */
> +extern struct cleanup *make_cleanup_restore_target_terminal (void);
> +
>  /* Print useful information about our terminal status, if such a thing
>     exists.  */
>
> --
> 2.1.3
>
  

On 2014-12-02 07:08 PM, Patrick Palka wrote:
> On Tue, Dec 2, 2014 at 5:10 PM, Simon Marchi <simon.marchi@ericsson.com> wrote:
>> When a thread exits, the terminal is left in mode "terminal_is_ours"
>> while the target executes. This patch fixes that.
>>
>> From my understanding, a function calling target_terminal_ours expects
>> that the terminal could be in any state at the moment it is called.
>> Therefore, it should be its reponsibility to put back the terminal in
>> whatever state it was before being called.
> 
> It seems to me that the other observer callbacks defined in
> mi-interp.c are also affected by this issue, that is, the issue of
> having to restore the original terminal state after altering it.

Probably, although this one is the only one for which I saw the problem happening.
For the other handlers that call target_terminal_ours, my guess is that it happens
that the terminal gets reset somewhere else in the program, for unrelated reasons.

Like I said in the patch log, if the function explicitly calls target_terminal_ours,
it's because it expects that the terminal could not be ours at the beginning of its
execution. Thus, it makes no sense to call target_terminal_ours without resetting it.

For correctness, indeed, I think that the same should be applied to the few observers
in that file that call target_terminal_ours.

> So I
> wonder if it would make sense to shift this responsibility to the
> observer module itself (i.e. generic_observer_notify()), so that all
> observers implicitly restore the original terminal state when they
> return.  That way this kind of pattern wouldn't have to be duplicated
> for each individual observer.

I wouldn't put that responsibility in the observer module itself. It's a pretty
generic piece of code (not tied to GDB business logic) and should stay that way
I think.

Also, I think that for clarity it's better to leave that responsibility of changing
the terminal mode to the functions that know that something is going to be printed
(which are not necessarily the functions that actually print the things). Moving that
responsibility to some code that has nothing to do with printing (e.g. observer, or
the caller of observer_notify_*) would make things more confusing. Basically, separation
of concerns.
  

On 12/02/2014 10:10 PM, Simon Marchi wrote:
> When a thread exits, the terminal is left in mode "terminal_is_ours"
> while the target executes. This patch fixes that.

Right.  The reason we can then type stuff is that the input fd is left
registered in the event loop.  target_terminal_inferior is what
removes it.

> 
> From my understanding, a function calling target_terminal_ours expects
> that the terminal could be in any state at the moment it is called.

Right.  The target_terminal_* functions are idempotent.

> Therefore, it should be its reponsibility to put back the terminal in
> whatever state it was before being called.

That doesn't follow though.  We have a ton of places that call
target_terminal_our that don't need to put back the terminal in
whatever state is was before.  The reason is that usually we'll
be printing after the inferior stopped for some event.  If we end
up re-resuming it, we'll call target_terminal_inferior then.

But in the thread exit case, there's nothing to be re-resumed,
so we need to care about it "manually".  If you take a backtrace
at the point the thread exit event is printed, we should be
still deep within the target backend code.

Please update the commit log to clarify this.

>  extern int target_supports_terminal_ours (void);
>  
> +/* Make a cleanup that restores the state of the terminal to the current
> +   value.  */

Should be "to the current state", I think.

Otherwise OK.

Thanks,
Pedro Alves
  

On 12/03/2014 02:29 PM, Simon Marchi wrote:
> On 2014-12-02 07:08 PM, Patrick Palka wrote:

>> So I
>> wonder if it would make sense to shift this responsibility to the
>> observer module itself (i.e. generic_observer_notify()), so that all
>> observers implicitly restore the original terminal state when they
>> return.  That way this kind of pattern wouldn't have to be duplicated
>> for each individual observer.
> 
> I wouldn't put that responsibility in the observer module itself. It's a pretty
> generic piece of code (not tied to GDB business logic) and should stay that way
> I think.

Agreed.  An observer could end up resuming the target for instance, or
it could be that the normal_stop observer ends up responsible for calling
target_terminal_ours if nothing else called it before.  In both
those cases it'd be wrong to revert the terminal to the previous state.

> Also, I think that for clarity it's better to leave that responsibility of changing
> the terminal mode to the functions that know that something is going to be printed
> (which are not necessarily the functions that actually print the things). Moving that
> responsibility to some code that has nothing to do with printing (e.g. observer, or
> the caller of observer_notify_*) would make things more confusing. Basically, separation
> of concerns.

*nod*

Thanks,
Pedro Alves
  

> That doesn't follow though.  We have a ton of places that call
> target_terminal_our that don't need to put back the terminal in
> whatever state is was before.  The reason is that usually we'll
> be printing after the inferior stopped for some event.  If we end
> up re-resuming it, we'll call target_terminal_inferior then.

I am not sure I understand that part. The thread exit is noticed because
of a breakpoint being hit (a breakpoint in libthread-db). In all-stop, I
would expect that when the thread-exit breakpoint is hit, we stop all
threads, then process the event (display the message). After that, we
would need to resume the remaining threads, so the application continues
executing. If so, we would call target_terminal_inferior there.

But, I also remember you mentioning that these thread-db breakpoints are
not handled the same way as regular breakpoints. If I inspect the
target-stack when debugging a threaded program:

    The current target stack is:
      - multi-thread (multi-threaded child process.)
      - native (Native process)
      - exec (Local exec file)
      - None (None)

I would guess that the layer responsible for the all-stop behavior (stop
all the threads when one is stopped) is the native process one. The
thread-db breakpoint handling, however, is done in the multi-thread layer,
so the native target is not reached for handling these. So when the
message is printed, the other threads are actually still running.

Is this last explanation correct?

> But in the thread exit case, there's nothing to be re-resumed,
> so we need to care about it "manually".  If you take a backtrace
> at the point the thread exit event is printed, we should be
> still deep within the target backend code.
> 
> Please update the commit log to clarify this.

I would write this:

We need to manually restore the terminal setting in this particular observer.
In the case of the other observers that call target_terminal_ours, gdb will end
up resuming the inferior later in the execution and call
target_terminal_inferior. In the case of the thread exit event, we still need
to call target_terminal_ours to be able to print something, but there is nothing
that gdb will need to resume after that. We therefore need to call
target_terminal_inferior ourselves.

is this correct?

>>  extern int target_supports_terminal_ours (void);
>>  
>> +/* Make a cleanup that restores the state of the terminal to the current
>> +   value.  */
> 
> Should be "to the current state", I think.

Done.

Thanks, I will push if the clarification above is OK.

Simon
  

On 12/05/2014 04:12 PM, Simon Marchi wrote:
>> That doesn't follow though.  We have a ton of places that call
>> target_terminal_our that don't need to put back the terminal in
>> whatever state is was before.  The reason is that usually we'll
>> be printing after the inferior stopped for some event.  If we end
>> up re-resuming it, we'll call target_terminal_inferior then.
> 
> I am not sure I understand that part. The thread exit is noticed because
> of a breakpoint being hit (a breakpoint in libthread-db).

Nope, it's not.  When that breakpoint is hit, the only thing we do
is mark the thread as "dying", in linux-thread-db.c:detach_thread.

The thread exit is noticed when the kernel reports an exit status
to the waitpid calls in linux-nat.c (WIFEXITED/WIFSIGNALED)
(and few other corner cases in that same file).  Look for exit_lwp.
E.g.:

(top-gdb) bt
#0  delete_thread (ptid=...) at /home/pedro/gdb/mygit/src/gdb/thread.c:371
#1  0x00000000004b76ee in exit_lwp (lp=0x1682000) at /home/pedro/gdb/mygit/src/gdb/linux-nat.c:895
#2  0x00000000004bbb20 in linux_nat_filter_event (lwpid=19001, status=0, new_pending_p=0x7fffffffcf50) at /home/pedro/gdb/mygit/src/gdb/linux-nat.c:2847
#3  0x00000000004bc6d8 in linux_nat_wait_1 (ops=0xddf5e0, ptid=..., ourstatus=0x7fffffffd300, target_options=1) at /home/pedro/gdb/mygit/src/gdb/linux-nat.c:3136
#4  0x00000000004bd583 in linux_nat_wait (ops=0xddf5e0, ptid=..., ourstatus=0x7fffffffd300, target_options=1) at /home/pedro/gdb/mygit/src/gdb/linux-nat.c:3505
#5  0x00000000004c597b in thread_db_wait (ops=0xd4f0c0 <thread_db_ops>, ptid=..., ourstatus=0x7fffffffd300, options=1)
    at /home/pedro/gdb/mygit/src/gdb/linux-thread-db.c:1532
#6  0x000000000065c037 in delegate_wait (self=0xd4f0c0 <thread_db_ops>, arg1=..., arg2=0x7fffffffd300, arg3=1) at /home/pedro/gdb/mygit/src/gdb/target-delegates.c:116
#7  0x000000000066c465 in target_wait (ptid=..., status=0x7fffffffd300, options=1) at /home/pedro/gdb/mygit/src/gdb/target.c:2175
#8  0x00000000006165ff in fetch_inferior_event (client_data=0x0) at /home/pedro/gdb/mygit/src/gdb/infrun.c:3241
#9  0x0000000000639d57 in inferior_event_handler (event_type=INF_REG_EVENT, client_data=0x0) at /home/pedro/gdb/mygit/src/gdb/inf-loop.c:57

> In all-stop, I
> would expect that when the thread-exit breakpoint is hit, we stop all
> threads, then process the event (display the message). After that, we
> would need to resume the remaining threads, so the application continues
> executing. If so, we would call target_terminal_inferior there.

Nope, thread exits are actually handled inside linux-nat.c, without
stopping all threads.

(I've got a patch to stop native gdb from using the create and death
event breakpoints, like gdbserver, btw.  Not necessary when we have
PTRACE_EVENT_CLONE.)

> 
> But, I also remember you mentioning that these thread-db breakpoints are
> not handled the same way as regular breakpoints. If I inspect the
> target-stack when debugging a threaded program:
> 
>     The current target stack is:
>       - multi-thread (multi-threaded child process.)
>       - native (Native process)
>       - exec (Local exec file)
>       - None (None)
> 
> I would guess that the layer responsible for the all-stop behavior (stop
> all the threads when one is stopped) is the native process one.

Right.

> The
> thread-db breakpoint handling, however, is done in the multi-thread layer,
> so the native target is not reached for handling these. So when the
> message is printed, the other threads are actually still running.
> 
> Is this last explanation correct?
> 
>> But in the thread exit case, there's nothing to be re-resumed,
>> so we need to care about it "manually".  If you take a backtrace
>> at the point the thread exit event is printed, we should be
>> still deep within the target backend code.
>>
>> Please update the commit log to clarify this.
> 
> I would write this:
> 
> We need to manually restore the terminal setting in this particular observer.
> In the case of the other observers that call target_terminal_ours, gdb will end

"other MI observers"?

> up resuming the inferior later in the execution and call
> target_terminal_inferior. In the case of the thread exit event, we still need
> to call target_terminal_ours to be able to print something, but there is nothing
> that gdb will need to resume after that. We therefore need to call
> target_terminal_inferior ourselves.
> 
> is this correct?

Yes.

Thanks,
Pedro Alves