From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f46.google.com (mail-wm1-f46.google.com [209.85.128.46]) by sourceware.org (Postfix) with ESMTPS id 9DCF9385188E for ; Mon, 12 Dec 2022 20:18:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9DCF9385188E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f46.google.com with SMTP id 206-20020a1c02d7000000b003d1e906ca23so200903wmc.3 for ; Mon, 12 Dec 2022 12:18:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:references:to:subject:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=F+9qqVnZ6voB47Sz1/J1jF2ym6cr2rrj7fyutFLKkVE=; b=Q+MMcoS813jVJeEND0zFzBNKxuFs6nwz6995hJpUQOlr2U5zul48QNsr3ZwEdq+XbR XvxUxSEageonjrG3EuWmV1PehGlQ6eFP3BFtbOpwpBVEbUQOnTSmeSkhwzJHpgwWsKm+ C1RwcEGs3Z0YvEiad/e+wNPzyWJvOMeLmbSOYbEYNTs7dOmGG5N5OGz6IyU+IyGBckyq Zb8XoM/J2pkBPBAG7NKO7McGqJsTgVyvmvQ81tUvPcxPdvOO3+FGtI4tCZPDC3aS2FPK 7QBPjFEoVW/0OhJjAzTaqfWv9JCeg2ZC/mNS+vo2EF4At9lL9QrQ1i9zpGUsK5+RUg2f /DAA== X-Gm-Message-State: ANoB5pmsUAdDtHiQjCwHg+HD+xOoriPvLuN8bSN3sqkBFf/+1v4jOMl0 bdqgeHGhH5Noe1AyXx+GB6GL2r+oERKnIQ== X-Google-Smtp-Source: AA0mqf44X6XvGHmZsUUmWB0WLo2gsYGWob5w2Np9p7XC872BVa60Wzs4qX+hgQZwH7Jr0DO9YPeb1w== X-Received: by 2002:a05:600c:3d96:b0:3cf:7704:50ce with SMTP id bi22-20020a05600c3d9600b003cf770450cemr13188709wmb.38.1670876298427; Mon, 12 Dec 2022 12:18:18 -0800 (PST) Received: from ?IPv6:2001:8a0:f912:6700:afd9:8b6d:223f:6170? ([2001:8a0:f912:6700:afd9:8b6d:223f:6170]) by smtp.gmail.com with ESMTPSA id o15-20020a05600c378f00b003c6d21a19a0sm10241478wmr.29.2022.12.12.12.18.17 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 12 Dec 2022 12:18:17 -0800 (PST) From: Pedro Alves Subject: Re: [PATCH v2 18/29] gdb: clear step over information on thread exit (PR gdb/27338) To: Simon Marchi , gdb-patches@sourceware.org References: <20220713222433.374898-1-pedro@palves.net> <20220713222433.374898-19-pedro@palves.net> <022c35c9-0e85-b094-a3bc-417feb203894@simark.ca> Message-ID: Date: Mon, 12 Dec 2022 20:18:22 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <022c35c9-0e85-b094-a3bc-417feb203894@simark.ca> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,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 List-Id: On 2022-07-21 7:12 p.m., Simon Marchi wrote: >> @@ -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. Right, this is more about normal_stop, and how to communicate "this is a thread exit, but treat it really as a no-resumed event" down to normal_stop and whatever other spots that would need it. It's much simpler to just switch the event. Then you know nothing would miss it. I moved the set_last_target_status inside the lambda above, so it's all more together, and added a comment. It was this seemingly-obvious change that revealed the need for this other series, though: [PATCH 0/6] Eliminate infrun_thread_thread_exit observer https://inbox.sourceware.org/gdb-patches/20221203211338.2264994-1-pedro@palves.net/T/#t > >> 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. > Done.