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.

gdb/ChangeLog:

2014-11-25  Simon Marchi  <simon.marchi@ericsson.com>

	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       | 30 ++++++++++++++++++++++++++++++
 gdb/target.h       |  4 ++++
 3 files changed, 38 insertions(+)
  

On Tuesday, November 25 2014, Simon Marchi 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.
>
> I find that this fits quite well the cleanup model, so I implemented a
> cleanup for that.

Thanks for the patch.  A few comments related to the coding style and
cleanups.

> gdb/ChangeLog:
>
> 2014-11-25  Simon Marchi  <simon.marchi@ericsson.com>
>
> 	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       | 30 ++++++++++++++++++++++++++++++
>  gdb/target.h       |  4 ++++
>  3 files changed, 38 insertions(+)
>
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index df2b558..2a62e22 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();

Space between function name and ().

>    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);

Likewise.

>  }
>  
>  /* Emit notification on changing the state of record.  */
> diff --git a/gdb/target.c b/gdb/target.c
> index ab5f2b9..d6a06bd 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -528,6 +528,36 @@ target_supports_terminal_ours (void)
>    return 0;
>  }
>  
> +static void cleanup_restore_target_terminal (void *arg)
              ^
Break the line here.

New functions should have a comment describing them.

> +{
> +  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;

The "case" should be indented below the "{".

Missing whitespace between function names and ().

> +  }
> +
> +  xfree(previous_state);

Likewise.

> +}
> +
> +struct cleanup *
> +make_cleanup_restore_target_terminal (void)

Missing function comment.  Since you already described this one on the
header file, you can just say:

  /* See definition in target.h.  */

> +{
> +  enum terminal_state *ts = xmalloc(sizeof(*ts));

Whitespace between function name and ().

> +
> +  *ts = terminal_state;
> +
> +  return make_cleanup (cleanup_restore_target_terminal, ts);

You should use make_cleanup_dtor here, because "ts" may leak if the
cleanup is not called.

> +}
> +
>  static void
>  tcomplain (void)
>  {
> diff --git a/gdb/target.h b/gdb/target.h
> index d363b61..a1c3b7b 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. */

You just need to use "*" after and before the "/" in comments:

  /* This is a
     multi-line
     comment.  */

It's also missing two spaces after period.

> +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-11-25 04:59 PM, Sergio Durigan Junior wrote:
> On Tuesday, November 25 2014, Simon Marchi 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.
>>
>> I find that this fits quite well the cleanup model, so I implemented a
>> cleanup for that.
> 
> Thanks for the patch.  A few comments related to the coding style and
> cleanups.
> 
>> gdb/ChangeLog:
>>
>> 2014-11-25  Simon Marchi  <simon.marchi@ericsson.com>
>>
>> 	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       | 30 ++++++++++++++++++++++++++++++
>>  gdb/target.h       |  4 ++++
>>  3 files changed, 38 insertions(+)
>>
>> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
>> index df2b558..2a62e22 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();
> 
> Space between function name and ().
> 
>>    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);
> 
> Likewise.
> 
>>  }
>>  
>>  /* Emit notification on changing the state of record.  */
>> diff --git a/gdb/target.c b/gdb/target.c
>> index ab5f2b9..d6a06bd 100644
>> --- a/gdb/target.c
>> +++ b/gdb/target.c
>> @@ -528,6 +528,36 @@ target_supports_terminal_ours (void)
>>    return 0;
>>  }
>>  
>> +static void cleanup_restore_target_terminal (void *arg)
>               ^
> Break the line here.
> 
> New functions should have a comment describing them.
> 
>> +{
>> +  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;
> 
> The "case" should be indented below the "{".
> 
> Missing whitespace between function names and ().
> 
>> +  }
>> +
>> +  xfree(previous_state);
> 
> Likewise.
> 
>> +}
>> +
>> +struct cleanup *
>> +make_cleanup_restore_target_terminal (void)
> 
> Missing function comment.  Since you already described this one on the
> header file, you can just say:
> 
>   /* See definition in target.h.  */
> 
>> +{
>> +  enum terminal_state *ts = xmalloc(sizeof(*ts));
> 
> Whitespace between function name and ().
> 
>> +
>> +  *ts = terminal_state;
>> +
>> +  return make_cleanup (cleanup_restore_target_terminal, ts);
> 
> You should use make_cleanup_dtor here, because "ts" may leak if the
> cleanup is not called.
> 
>> +}
>> +
>>  static void
>>  tcomplain (void)
>>  {
>> diff --git a/gdb/target.h b/gdb/target.h
>> index d363b61..a1c3b7b 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. */
> 
> You just need to use "*" after and before the "/" in comments:
> 
>   /* This is a
>      multi-line
>      comment.  */
> 
> It's also missing two spaces after period.
> 
>> +extern struct cleanup *make_cleanup_restore_target_terminal (void);
>> +
>>  /* Print useful information about our terminal status, if such a thing
>>     exists.  */
>>  
>> -- 
>> 2.1.3

Oops, I had missed your reply. Thanks for the review, I just sent a V2.

Simon