From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1284 invoked by alias); 17 Mar 2009 17:25:08 -0000 Received: (qmail 1252 invoked by uid 22791); 17 Mar 2009 17:25:06 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_44,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; Tue, 17 Mar 2009 17:24:57 +0000 Received: (qmail 16862 invoked from network); 17 Mar 2009 17:24:54 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 17 Mar 2009 17:24:54 -0000 From: Pedro Alves To: Vladimir Prus Subject: Re: [1/3] broken -thread-info output in non-stop mode, suppress_stop_observer Date: Tue, 17 Mar 2009 17:27:00 -0000 User-Agent: KMail/1.9.10 Cc: gdb-patches@sourceware.org References: <200903170621.02808.pedro@codesourcery.com> <200903171026.31197.vladimir@codesourcery.com> In-Reply-To: <200903171026.31197.vladimir@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200903171724.54512.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-03/txt/msg00313.txt.bz2 On Tuesday 17 March 2009 07:26:29, Vladimir Prus wrote: > On Tuesday 17 March 2009 09:21:02 Pedro Alves wrote: > > This patch removes the suppress_stop_observer global. As indicated > > in the comments I'm removing, this was problematic for non-stop > > mode. > > > > To fix the broken -thread-info output mentioned here: > > > > http://sourceware.org/ml/gdb-patches/2009-03/msg00236.html > > > > ... I wanted to add a thread_info->in_infcall flag to use in > > the 3rd patch in the series. That meant I needed to get rid of > > the suppress_resume_observer global, which then brings > > us to this. > > > > Vladimir, et al, how does this look? > > I did not look very carefully at details of conditions, but the > general idea looks solid. One detail is the new command you > add in infrun.c -- can it explicitly write down *why* we need to > suppress *stopped at this point, when doing finish? Certainly, here it is. Thank you. Hmmm, I do wonder if we could rewrite "finish" to not rely on bpstat_find_breakpoint after normal_stop, but to instead add a new BPSTAT_WHAT_FINISH and handle it from within handle_inferior_event's internal breakpoint handling switch (where we handle BPSTAT_WHAT_STEP_RESUME, etc.). Since that runs before normal_stop, we wouldn't have to suppress the normal_stop observer. ? -- Pedro Alves 2009-03-17 Pedro Alves * infcall.c (run_inferior_call): Remove references to suppress_stop_observer. * infcmd.c (suppress_stop_observer): Delete. (finish_command_continuation): Remove NOTE. Don't clear suppress_stop_observer anymore. (finish_command_continuation_free_arg): Likewise. (finish_forward): Remove references to suppress_stop_observer. Call normal_stop observer if we haven't already. * inferior.h (suppress_stop_observer): Delete. * infrun.c (normal_stop): When deciding to suppress the normal_stop observer, check for proceed_to_finish instead of suppress_stop_observer. --- gdb/infcall.c | 3 --- gdb/infcmd.c | 24 ++++++++---------------- gdb/inferior.h | 4 ---- gdb/infrun.c | 24 +++++++++++++++++++----- 4 files changed, 27 insertions(+), 28 deletions(-) Index: src/gdb/infcall.c =================================================================== --- src.orig/gdb/infcall.c 2009-03-17 05:25:17.000000000 +0000 +++ src/gdb/infcall.c 2009-03-17 16:14:06.000000000 +0000 @@ -331,7 +331,6 @@ run_inferior_call (struct thread_info *c volatile struct gdb_exception e; int saved_async = 0; int saved_suppress_resume_observer = suppress_resume_observer; - int saved_suppress_stop_observer = suppress_stop_observer; ptid_t call_thread_ptid = call_thread->ptid; char *saved_target_shortname = xstrdup (target_shortname); @@ -344,7 +343,6 @@ run_inferior_call (struct thread_info *c saved_async = target_async_mask (0); suppress_resume_observer = 1; - suppress_stop_observer = 1; TRY_CATCH (e, RETURN_MASK_ALL) proceed (real_pc, TARGET_SIGNAL_0, 0); @@ -355,7 +353,6 @@ run_inferior_call (struct thread_info *c call_thread = NULL; suppress_resume_observer = saved_suppress_resume_observer; - suppress_stop_observer = saved_suppress_stop_observer; /* Don't restore the async mask if the target has changed, saved_async is for the original target. */ Index: src/gdb/infcmd.c =================================================================== --- src.orig/gdb/infcmd.c 2009-03-17 05:25:19.000000000 +0000 +++ src/gdb/infcmd.c 2009-03-17 16:14:06.000000000 +0000 @@ -169,8 +169,6 @@ struct gdb_environ *inferior_environ; /* When set, no calls to target_resumed observer will be made. */ int suppress_resume_observer = 0; -/* When set, normal_stop will not call the normal_stop observer. */ -int suppress_stop_observer = 0; /* Accessor routines. */ @@ -1346,13 +1344,16 @@ static void finish_command_continuation (void *arg) { struct finish_command_continuation_args *a = arg; - + struct thread_info *tp = NULL; bpstat bs = NULL; if (!ptid_equal (inferior_ptid, null_ptid) && target_has_execution && is_stopped (inferior_ptid)) - bs = inferior_thread ()->stop_bpstat; + { + tp = inferior_thread (); + bs = tp->stop_bpstat; + } if (bpstat_find_breakpoint (bs, a->breakpoint) != NULL && a->function != NULL) @@ -1369,22 +1370,15 @@ finish_command_continuation (void *arg) } /* We suppress normal call of normal_stop observer and do it here so - that that *stopped notification includes the return value. */ - /* NOTE: This is broken in non-stop mode. There is no guarantee the - next stop will be in the same thread that we started doing a - finish on. This suppressing (or some other replacement means) - should be a thread property. */ - observer_notify_normal_stop (bs, 1 /* print frame */); - suppress_stop_observer = 0; + that the *stopped notification includes the return value. */ + if (bs != NULL && tp->proceed_to_finish) + observer_notify_normal_stop (bs, 1 /* print frame */); delete_breakpoint (a->breakpoint); } static void finish_command_continuation_free_arg (void *arg) { - /* NOTE: See finish_command_continuation. This would go away, if - this suppressing is made a thread property. */ - suppress_stop_observer = 0; xfree (arg); } @@ -1469,8 +1463,6 @@ finish_forward (struct symbol *function, old_chain = make_cleanup_delete_breakpoint (breakpoint); tp->proceed_to_finish = 1; /* We want stop_registers, please... */ - make_cleanup_restore_integer (&suppress_stop_observer); - suppress_stop_observer = 1; proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0); cargs = xmalloc (sizeof (*cargs)); Index: src/gdb/inferior.h =================================================================== --- src.orig/gdb/inferior.h 2009-03-17 05:25:19.000000000 +0000 +++ src/gdb/inferior.h 2009-03-17 16:14:06.000000000 +0000 @@ -368,10 +368,6 @@ extern int debug_displaced; void displaced_step_dump_bytes (struct ui_file *file, const gdb_byte *buf, size_t len); - -/* When set, normal_stop will not call the normal_stop observer. */ -extern int suppress_stop_observer; - /* When set, no calls to target_resumed observer will be made. */ extern int suppress_resume_observer; Index: src/gdb/infrun.c =================================================================== --- src.orig/gdb/infrun.c 2009-03-17 05:25:21.000000000 +0000 +++ src/gdb/infrun.c 2009-03-17 16:24:17.000000000 +0000 @@ -4433,11 +4433,25 @@ Further execution is probably impossible done: annotate_stopped (); - if (!suppress_stop_observer - && !(target_has_execution - && last.kind != TARGET_WAITKIND_SIGNALLED - && last.kind != TARGET_WAITKIND_EXITED - && inferior_thread ()->step_multi)) + + /* Suppress the stop observer if we're in the middle of: + + - a step n (n > 1), as there still more steps to be done. + + - a "finish" command, as the observer will be called in + finish_command_continuation, so it can include the inferior + function's return value. + + - calling an inferior function, as we pretend we inferior didn't + run at all. The return value of the call is handled by the + expression evaluator, through call_function_by_hand. */ + + if (!target_has_execution + || last.kind == TARGET_WAITKIND_SIGNALLED + || last.kind == TARGET_WAITKIND_EXITED + || (!inferior_thread ()->step_multi + && !(inferior_thread ()->stop_bpstat + && inferior_thread ()->proceed_to_finish))) { if (!ptid_equal (inferior_ptid, null_ptid)) observer_notify_normal_stop (inferior_thread ()->stop_bpstat,