From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4109 invoked by alias); 21 May 2009 00:41:59 -0000 Received: (qmail 4097 invoked by uid 22791); 21 May 2009 00:41:57 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_32,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 21 May 2009 00:41:51 +0000 Received: (qmail 16061 invoked from network); 21 May 2009 00:41:49 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 21 May 2009 00:41:49 -0000 From: Pedro Alves To: Jan Kratochvil Subject: Re: [patch] Fix gdb.mi internal_error on killing inferior Date: Thu, 21 May 2009 00:41:00 -0000 User-Agent: KMail/1.9.10 Cc: gdb-patches@sourceware.org References: <20090520202749.GA17984@host0.dyn.jankratochvil.net> <200905202143.17772.pedro@codesourcery.com> <20090520213441.GA32109@host0.dyn.jankratochvil.net> In-Reply-To: <20090520213441.GA32109@host0.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200905210142.10924.pedro@codesourcery.com> X-IsSubscribed: yes 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 X-SW-Source: 2009-05/txt/msg00444.txt.bz2 On Wednesday 20 May 2009 22:34:41, Jan Kratochvil wrote: > On Wed, 20 May 2009 22:43:17 +0200, Pedro Alves wrote: > > Weird, I don't see this happening. > > Another reproducer + a backtrace below: > echo -e '#include \nint main(void){pause();return 0;}'|gcc -Wall -o pauset -x c -pthread -;echo -e '-gdb-set target-async 1\n-exec-run &\n-target-attach\ny' >cmd;../gdb -nx -i=mi ./pauset <./cmd > Thanks. This one is tripping on 10 year old FIXMEs. You've started the target in the background (-exec-run &), meaning that GDB shouldn't try to set the inferior's terminal modes, yet, attach->kill ended up calling target_terminal_ours, and terminal_ours_1 claims that the target got the terminal. This patches fixes your test above for me. I'm running the full testsuite on {sync|async}/{remote|native}, after having run a few smoke test .exp's and seeing no problem. Does it work for you? -- Pedro Alves 2009-05-21 Pedro Alves * linux-nat.c (linux_nat_terminal_inferior) (linux_nat_terminal_ours): Don't check sync_execution. * remote.c (remote_terminal_inferior, remote_terminal_ours): Don't check sync_execution. Update comments. * target.c (target_terminal_inferior): New. * target.h (target_terminal_inferior): Delete macro, and declare as function. * event-top.c (async_disable_stdin): Make idempotent. Don't give the target the terminal here. * inflow.c (terminal_ours_1): Don't return early without setting `terminal_is_ours'. --- gdb/event-top.c | 13 +++++-------- gdb/inflow.c | 5 ++--- gdb/linux-nat.c | 10 +--------- gdb/remote.c | 22 ++++++---------------- gdb/target.c | 12 ++++++++++++ gdb/target.h | 3 +-- 6 files changed, 27 insertions(+), 38 deletions(-) Index: src/gdb/linux-nat.c =================================================================== --- src.orig/gdb/linux-nat.c 2009-05-21 00:56:59.000000000 +0100 +++ src/gdb/linux-nat.c 2009-05-21 01:18:04.000000000 +0100 @@ -4366,14 +4366,9 @@ linux_nat_terminal_inferior (void) return; } - /* GDB should never give the terminal to the inferior, if the - inferior is running in the background (run&, continue&, etc.). - This check can be removed when the common code is fixed. */ - if (!sync_execution) - return; - terminal_inferior (); + /* Calls to target_terminal_*() are meant to be idempotent. */ if (!async_terminal_is_ours) return; @@ -4399,9 +4394,6 @@ linux_nat_terminal_ours (void) but claiming it sure should. */ terminal_ours (); - if (!sync_execution) - return; - if (async_terminal_is_ours) return; Index: src/gdb/remote.c =================================================================== --- src.orig/gdb/remote.c 2009-05-21 00:56:59.000000000 +0100 +++ src/gdb/remote.c 2009-05-21 01:16:00.000000000 +0100 @@ -4078,19 +4078,12 @@ remote_terminal_inferior (void) /* Nothing to do. */ return; - /* FIXME: cagney/1999-09-27: Shouldn't need to test for - sync_execution here. This function should only be called when - GDB is resuming the inferior in the forground. A background - resume (``run&'') should leave GDB in control of the terminal and - consequently should not call this code. */ - if (!sync_execution) - return; - /* FIXME: cagney/1999-09-27: Closely related to the above. Make - calls target_terminal_*() idenpotent. The event-loop GDB talking - to an asynchronous target with a synchronous command calls this - function from both event-top.c and infrun.c/infcmd.c. Once GDB - stops trying to transfer the terminal to the target when it - shouldn't this guard can go away. */ + /* FIXME: cagney/1999-09-27: Make calls to target_terminal_*() + idempotent. The event-loop GDB talking to an asynchronous target + with a synchronous command calls this function from both + event-top.c and infrun.c/infcmd.c. Once GDB stops trying to + transfer the terminal to the target when it shouldn't this guard + can go away. */ if (!remote_async_terminal_ours_p) return; delete_file_handler (input_fd); @@ -4109,9 +4102,6 @@ remote_terminal_ours (void) return; /* See FIXME in remote_terminal_inferior. */ - if (!sync_execution) - return; - /* See FIXME in remote_terminal_inferior. */ if (remote_async_terminal_ours_p) return; cleanup_sigint_signal_handler (NULL); Index: src/gdb/target.c =================================================================== --- src.orig/gdb/target.c 2009-05-21 00:56:59.000000000 +0100 +++ src/gdb/target.c 2009-05-21 00:57:28.000000000 +0100 @@ -301,6 +301,18 @@ target_create_inferior (char *exec_file, "could not find a target to create inferior"); } +void +target_terminal_inferior (void) +{ + /* A background resume (``run&'') should leave GDB in control of the + terminal. */ + if (target_is_async_p () && !sync_execution) + return; + + /* If GDB is resuming the inferior in the foreground, install + inferior's terminal modes. */ + (*current_target.to_terminal_inferior) (); +} static int nomemory (CORE_ADDR memaddr, char *myaddr, int len, int write, Index: src/gdb/target.h =================================================================== --- src.orig/gdb/target.h 2009-05-21 00:56:59.000000000 +0100 +++ src/gdb/target.h 2009-05-21 00:57:28.000000000 +0100 @@ -754,8 +754,7 @@ extern void print_section_info (struct t /* Put the inferior's terminal settings into effect. This is preparation for starting or resuming the inferior. */ -#define target_terminal_inferior() \ - (*current_target.to_terminal_inferior) () +extern void target_terminal_inferior (void); /* Put some of our terminal settings into effect, enough to get proper results from our output, Index: src/gdb/event-top.c =================================================================== --- src.orig/gdb/event-top.c 2009-05-21 00:56:59.000000000 +0100 +++ src/gdb/event-top.c 2009-05-21 00:57:28.000000000 +0100 @@ -458,14 +458,11 @@ async_enable_stdin (void) void async_disable_stdin (void) { - sync_execution = 1; - push_prompt ("", "", ""); - /* FIXME: cagney/1999-09-27: At present this call is technically - redundant since infcmd.c and infrun.c both already call - target_terminal_inferior(). As the terminal handling (in - sync/async mode) is refined, the duplicate calls can be - eliminated (Here or in infcmd.c/infrun.c). */ - target_terminal_inferior (); + if (!sync_execution) + { + sync_execution = 1; + push_prompt ("", "", ""); + } } Index: src/gdb/inflow.c =================================================================== --- src.orig/gdb/inflow.c 2009-05-21 00:57:55.000000000 +0100 +++ src/gdb/inflow.c 2009-05-21 00:57:56.000000000 +0100 @@ -361,6 +361,8 @@ terminal_ours_1 (int output_only) if (terminal_is_ours) return; + terminal_is_ours = 1; + /* Checking inferior->run_terminal is necessary so that if GDB is running in the background, it won't block trying to do the ioctl()'s below. Checking gdb_has_a_terminal @@ -371,7 +373,6 @@ terminal_ours_1 (int output_only) if (inf->terminal_info->run_terminal != NULL || gdb_has_a_terminal () == 0) return; - if (!terminal_is_ours) { #ifdef SIGTTOU /* Ignore this signal since it will happen when we try to set the @@ -380,8 +381,6 @@ terminal_ours_1 (int output_only) #endif int result; - terminal_is_ours = 1; - #ifdef SIGTTOU if (job_control) osigttou = (void (*)()) signal (SIGTTOU, SIG_IGN);