From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15908 invoked by alias); 9 May 2008 01:23:14 -0000 Received: (qmail 15896 invoked by uid 22791); 9 May 2008 01:23:13 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 09 May 2008 01:22:55 +0000 Received: (qmail 3315 invoked from network); 9 May 2008 01:22:53 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 9 May 2008 01:22:53 -0000 From: Pedro Alves To: gdb-patches@sourceware.org, Eli Zaretskii Subject: Re: [RFC] 07/10 non-stop inferior control Date: Fri, 09 May 2008 02:08:00 -0000 User-Agent: KMail/1.9.9 References: <200805061649.24082.pedro@codesourcery.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200805090222.51406.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: 2008-05/txt/msg00296.txt.bz2 A Thursday 08 May 2008 12:40:00, Eli Zaretskii wrote: > > From: Pedro Alves > > Date: Tue, 6 May 2008 16:49:23 +0100 > > > > if (ecs->new_thread_event) > > { > > + if (non_stop) > > + /* Non-stop requires a behaving target. */ > > + internal_error (__FILE__, __LINE__, > > + "target misreported a new thread in non-stop mode."); > > + > > What does this internal_error mean? The wording of the error message > doesn't seem to justify an internal_error. What am I missing here? Isn't this new_thread_event handling a bandaid for some misbehaving target? My understanding was that targets should handle new thread events (and call add_thread*) themselves nowadays. There are two issues with non-stop here. First, is that I context switched before this code already, which isn't that great when the new inferior ptid isn't listed yet (the next context switch after the new thread is added to the list will store the wrong context). In non-stop, context switching should happen before adjust_pc_after_break, because that relies on prev_pc, and currently_stepping (ecs), which are per-thread, so to make the new_thread_event handling correct in non-stop, would require moving this bit: /* If it's a new process, add it to the thread database */ ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid) && !ptid_equal (ecs->ptid, minus_one_ptid) && !in_thread_list (ecs->ptid)); if (ecs->ws.kind != TARGET_WAITKIND_EXITED && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event) add_thread (ecs->ptid); To the top of handle_inferior_event, and context switch right after it, if non-stop, instead of context-switching in fetch_inferior_event. Something like: void handle_inferior_event (struct execution_control_state *ecs) { int sw_single_step_trap_p = 0; int stopped_by_watchpoint; int stepped_after_stopped_by_watchpoint = 0; /* If it's a new process, add it to the thread database */ ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid) && !ptid_equal (ecs->ptid, minus_one_ptid) && !in_thread_list (ecs->ptid)); if (ecs->ws.kind != TARGET_WAITKIND_EXITED && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event) add_thread (ecs->ptid); if (non_stop && ecs->ws.kind != TARGET_WAITKIND_IGNORE && ecs->ws.kind != TARGET_WAITKIND_EXITED && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED) /* In non-stop mode, each thread is handled individually. Switch early, so the global state is set correctly for this thread. */ context_switch (ecs->ptid); /* Cache the last pid/waitstatus. */ target_last_wait_ptid = ecs->ptid; target_last_waitstatus = ecs->ws; /* Always clear state belonging to the previous time we stopped. */ stop_stack_dummy = 0; adjust_pc_after_break (ecs); ... Since this is a new mode that will require adaptation by each target, it sounded reasonable to require that the the new_thread_event should never be hit, and that the target handles adding threads to GDB's thread list. Second is that what follows that code you pointed is target_resume (RESUME_ALL), which should never happen in non-stop mode: /* At this point, all threads are stopped (happens automatically in either the OS or the native code). Therefore we need to continue all threads in order to make progress. */ if (ecs->new_thread_event) { target_resume (RESUME_ALL, 0, TARGET_SIGNAL_0); prepare_to_wait (ecs); return; } I can certainly change that to: if (ecs->new_thread_event) { /* We may want to consider not doing a resume here in order to give the user a chance to play with the new thread. It might be good to make that a user-settable option. */ if (non_stop) /* Only resume the event thread. */ target_resume (ecs->ptid, 0, TARGET_SIGNAL_0); else /* At this point, all threads are stopped (happens automatically in either the OS or the native code). Therefore we need to continue all threads in order to make progress. */ target_resume (RESUME_ALL, 0, TARGET_SIGNAL_0); prepare_to_wait (ecs); return; } But should I ? -- Pedro Alves