From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6568 invoked by alias); 3 Dec 2014 00:09:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 6554 invoked by uid 89); 3 Dec 2014 00:09:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: mail-pa0-f50.google.com Received: from mail-pa0-f50.google.com (HELO mail-pa0-f50.google.com) (209.85.220.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 03 Dec 2014 00:09:10 +0000 Received: by mail-pa0-f50.google.com with SMTP id bj1so14445928pad.23 for ; Tue, 02 Dec 2014 16:09:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=+KnDwF37xmYPrlJJxqFDXfMNfqnBtMU5bxeZHkb9beg=; b=T4NL1UghUWOKWwdUN8nACDZrxC7/otk4IPTKTsJcRwjQ4lF7wMLCMLgf2emgvaLRUt xL4GdgLlkuNDN5LxdW9AL6EhLg9SP1JX26zsgNTo0h+KTVoHSj+VyFFBytqPn4npAfxl E7C7NHNl3D6FJ7xo8j2DbP2sGD4mYkosrAgcxmHuUXQPDDlZtKQU+jg2gNp5efHZjI0w 0vqpQsZvzxficoHB1gb3mWbNezWfJlr10aE3LUsftaciAFS3J7h0yJRR5V8Jsye9vpWE I2P3HZHnYjcIFoXUzDeHZo916fUL7pRB/7pUpN3et2Bl8Jta4oxMd9qU3ov0NMTDbYHu RNDA== X-Gm-Message-State: ALoCoQkSrp9d3oKWRYKlIhBCZjMi/xUyUWUCUrbu1fiKQjojlf78opWJgDV56ApxgBncQ0t2Snci X-Received: by 10.68.106.2 with SMTP id gq2mr10689305pbb.46.1417565348454; Tue, 02 Dec 2014 16:09:08 -0800 (PST) MIME-Version: 1.0 Received: by 10.70.31.100 with HTTP; Tue, 2 Dec 2014 16:08:48 -0800 (PST) In-Reply-To: <1417558223-27328-1-git-send-email-simon.marchi@ericsson.com> References: <1417558223-27328-1-git-send-email-simon.marchi@ericsson.com> From: Patrick Palka Date: Wed, 03 Dec 2014 00:09:00 -0000 Message-ID: Subject: Re: [PATCH v2] Restore terminal state in mi_thread_exit (PR gdb/17627) To: Simon Marchi Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-12/txt/msg00041.txt.bz2 On Tue, Dec 2, 2014 at 5: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. > > 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 > --- > 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 >