From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 899033858C56 for ; Thu, 21 Jul 2022 18:12:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 899033858C56 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 336541E21F; Thu, 21 Jul 2022 14:12:14 -0400 (EDT) Message-ID: <022c35c9-0e85-b094-a3bc-417feb203894@simark.ca> Date: Thu, 21 Jul 2022 14:12:13 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v2 18/29] gdb: clear step over information on thread exit (PR gdb/27338) Content-Language: en-US To: Pedro Alves , gdb-patches@sourceware.org References: <20220713222433.374898-1-pedro@palves.net> <20220713222433.374898-19-pedro@palves.net> From: Simon Marchi In-Reply-To: <20220713222433.374898-19-pedro@palves.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Jul 2022 18:12:16 -0000 > @@ -5428,6 +5443,117 @@ handle_no_resumed (struct execution_control_state *ecs) > return false; > } > > +/* Handle a TARGET_WAITKIND_THREAD_EXITED event. Return true if we > + handled the event and should continue waiting. Return false if we > + should stop and report the event to the user. */ > + > +static bool > +handle_thread_exited (execution_control_state *ecs) > +{ > + context_switch (ecs); > + > + /* Clear these so we don't re-start the thread stepping over a > + breakpoint/watchpoint. */ > + ecs->event_thread->stepping_over_breakpoint = 0; > + ecs->event_thread->stepping_over_watchpoint = 0; > + > + /* Maybe the thread was doing a step-over, if so release > + resources and start any further pending step-overs. > + > + If we are on a non-stop target and the thread was doing an > + in-line step, this also restarts the other threads. */ > + int ret = finish_step_over (ecs); > + > + /* finish_step_over returns true if it moves ecs' wait status > + back into the thread, so that we go handle another pending > + event before this one. But we know it never does that if > + the event thread has exited. */ > + gdb_assert (ret == 0); > + > + /* If finish_step_over started a new in-line step-over, don't > + try to restart anything else. */ > + if (step_over_info_valid_p ()) > + { > + delete_thread (ecs->event_thread); > + return true; > + } > + > + /* Maybe we are on an all-stop target and we got this event > + while doing a step-like command on another thread. If so, > + go back to doing that. If this thread was stepping, > + switch_back_to_stepped_thread will consider that the thread > + was interrupted mid-step and will try keep stepping it. We > + don't want that, the thread is gone. So clear the proceed > + status so it doesn't do that. */ > + clear_proceed_status_thread (ecs->event_thread); > + if (switch_back_to_stepped_thread (ecs)) > + { > + delete_thread (ecs->event_thread); > + return true; > + } > + > + inferior *inf = ecs->event_thread->inf; > + bool slock_applies = schedlock_applies (ecs->event_thread); > + > + delete_thread (ecs->event_thread); > + ecs->event_thread = nullptr; > + > + auto handle_as_no_resumed = [ecs] () > + { > + ecs->ws.set_no_resumed (); > + ecs->event_thread = nullptr; > + ecs->ptid = minus_one_ptid; > + return handle_no_resumed (ecs); > + }; Is it really necessary to change the nature of the event? handle_no_resumed doesn't seem to actually care about the kind in `ecs`, so maybe you could just pass `ecs` down as-is? I think it adds a layer of complexity if the ecs gets modified as we handle it, it's simpler to follow if it's immutable (other than filling in not-yet-set fields). But the difficulty I see is that normal_stop does some things when there are no resumed threads left. The check there would become a bit more complex. > diff --git a/gdb/thread.c b/gdb/thread.c > index 6ea05f70a41..a83db6b07fd 100644 > --- a/gdb/thread.c > +++ b/gdb/thread.c > @@ -401,6 +401,8 @@ thread_info::clear_pending_waitstatus () > void > thread_info::set_thread_options (gdb_thread_options thread_options) > { > + gdb_assert (this->state != THREAD_EXITED && !this->executing ()); I'd suggesting splitting this in two asserts. Simon