From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id yBsuNEUp42DSYgAAWB0awg (envelope-from ) for ; Mon, 05 Jul 2021 11:46:13 -0400 Received: by simark.ca (Postfix, from userid 112) id D3A3A1F1F2; Mon, 5 Jul 2021 11:46:13 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id DFCCB1E940 for ; Mon, 5 Jul 2021 11:46:12 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A923A3840008 for ; Mon, 5 Jul 2021 15:46:12 +0000 (GMT) Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by sourceware.org (Postfix) with ESMTPS id 6C0AA3840017 for ; Mon, 5 Jul 2021 15:45:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6C0AA3840017 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-f50.google.com with SMTP id g10so5857514wmh.2 for ; Mon, 05 Jul 2021 08:45:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=KRzHMKC/zEKBBn02jpiLovwDy3VPKSJLuCJNQwQZwI8=; b=HLevKJSEDS21z2mUMhHep4pvFy6cTiRAvYMd6bkXyuWmzsjXuuDnViIuHZ09ngWAWh 7+W8rmpRJl6x/Qc6wmZw4QUCU+KV6r/Vsb/bBdYzgEdu8TjH5cY+dS8W0QpMmBa5kaEP HovOlumZMDZNMDzCpn6buBV58PIe75hcYwXPWHuQa3lAv1blEG4+guyAqQ2VQ0CMTWxN AGP0hIDhmghNAlJGHP/2JI8Z/34f8UKvkohKR1JSBtzfD5RS5j3IFSJ58+OU86vgGiny 2nVqgS8J6hCLt+/sSkPRWUDsaz9wkbXJw5R6IAsz+qPRwOn8FjogtNoW77Xsd478n6eq jATg== X-Gm-Message-State: AOAM531vaD3h9k4pv/XRWoBf06XE1mtvwfw8jgM25CbQKUTeecUZ6EHd SgAlrHV3nOVUDyNYmDbf/zeC/HvGEIVm7Q== X-Google-Smtp-Source: ABdhPJyqVXbskPybNFJsR7kJb4F6oKLF5GuEDWZ+FN+Bxvoy83bAz3Ae5hLj/LC6LtnpsgTPlrgKdg== X-Received: by 2002:a1c:1f12:: with SMTP id f18mr15559444wmf.183.1625499930883; Mon, 05 Jul 2021 08:45:30 -0700 (PDT) Received: from ?IPv6:2001:8a0:f932:6a00:46bc:d03b:7b3a:2227? ([2001:8a0:f932:6a00:46bc:d03b:7b3a:2227]) by smtp.gmail.com with ESMTPSA id f6sm13308375wrs.13.2021.07.05.08.45.29 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 05 Jul 2021 08:45:29 -0700 (PDT) Subject: Re: [PATCH 06/11] gdb: make thread_info::suspend private, add getters / setters From: Pedro Alves To: Simon Marchi , gdb-patches@sourceware.org References: <20210622165704.2404007-1-simon.marchi@polymtl.ca> <20210622165704.2404007-7-simon.marchi@polymtl.ca> Message-ID: <9d006e6e-d232-9c46-4485-f0e888e9ed29@palves.net> Date: Mon, 5 Jul 2021 16:45:28 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210622165704.2404007-7-simon.marchi@polymtl.ca> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" On 2021-06-22 5:56 p.m., Simon Marchi via Gdb-patches wrote: > A following patch will want to take some action when a pending wait > status is set on or removed from a thread. Add a getter and a setter on > thread_info for the pending waitstatus, so that we can add some code in > the setter later. > > The thing is, the pending wait status field is in the > thread_suspend_state, along with other fields that we need to backup > before and restore after the thread does an inferior function call. > Therefore, make the thread_suspend_state member private > (thread_info::suspend becomes thread_info::m_suspend), and add getters / > setters for all of its fields: > > - pending wait status > - stop signal > - stop reason > - stop pc > > For the pending wait status, add the additional has_pending_waitstatus > and clear_pending_waitstatus methods. > > I think this makes the thread_info interface a bit nicer, because we > now access the fields as: > > thread->stop_pc () > > rather than > > thread->suspend.stop_pc > > The stop_pc field being in the `suspend` structure is an implementation > detail of thread_info that callers don't need to be aware of. > > For the backup / restore of the thread_suspend_state structure, add > save_suspend_to and restore_suspend_from methods. You might wonder why > `save_suspend_to`, as opposed to a simple getter like > > thread_suspend_state &suspend (); > > I want to make it clear that this is to be used only for backing up and > restoring the suspend state, _not_ to access fields like: > > thread->suspend ()->stop_pc > > Adding some getters / setters allows adding some assertions. I find > that this helps understand how things are supposed to work. Add: > > - When getting the pending status (pending_waitstatus method), ensure > that there is a pending status. > - When setting a pending status (set_pending_waitstatus method), ensure > there is no pending status. > > There is one case I found where this wasn't true - in > remote_target::process_initial_stop_replies - which needed adjustments > to respect that contract. I think it's because > process_initial_stop_replies is kind of (ab)using the > thread_info::suspend::waitstatus to store some statuses temporarily, for > its internal use (statuses it doesn't intent on leaving pending). > > process_initial_stop_replies pulls out stop replies received during the > initial connection using target_wait. It always stores the received > event in `evthread->suspend.waitstatus`. But it only sets > waitstatus_pending_p, if it deems the event interesting enough to leave > pending, to be reported to the core: > > if (ws.kind != TARGET_WAITKIND_STOPPED > || ws.value.sig != GDB_SIGNAL_0) > evthread->suspend.waitstatus_pending_p = 1; > > It later uses this flag a bit below, to choose which thread to make the > "selected" one: > > if (selected == NULL > && thread->suspend.waitstatus_pending_p) > selected = thread; > > And ultimately that's used if the user-visible mode is all-stop, so that > we print the stop for that interesting thread: > > /* In all-stop, we only print the status of one thread, and leave > others with their status pending. */ > if (!non_stop) > { > thread_info *thread = selected; > if (thread == NULL) > thread = lowest_stopped; > if (thread == NULL) > thread = first; > > print_one_stopped_thread (thread); > } > > But in any case (all-stop or non-stop), print_one_stopped_thread needs > to access the waitstatus value of these threads that don't have a > pending waitstatus (those that had TARGET_WAITKIND_STOPPED + > GDB_SIGNAL_0). This doesn't work with the assertions I've > put. > > So, change the code to only set the thread's wait status if it is an > interesting one that we are going to leave pending. If the thread > stopped due to a non-interesting event (TARGET_WAITKIND_STOPPED + > GDB_SIGNAL_0), don't store it. Adjust print_one_stopped_thread to > understand that if a thread has no pending waitstatus, it's because it > stopped with TARGET_WAITKIND_STOPPED + GDB_SIGNAL_0. > > The call to set_last_target_status also uses the pending waitstatus. > However, given that the pending waitstatus for the thread may have been > cleared in print_one_stopped_thread (and that there might not even be a > pending waitstatus in the first place, as explained above), it is no > longer possible to do it at this point. To fix that, move the call to > set_last_target_status in print_one_stopped_thread. I think this will > preserve the existing behavior, because set_last_target_status is > currently using the current thread's wait status. And the current > thread is the last one for which print_one_stopped_thread is called. So > by calling set_last_target_status in print_one_stopped_thread, we'll get > the same result. set_last_target_status will possibly be called > multiple times, but only the last call will matter. It just means > possibly more calls to set_last_target_status, but those are cheap. > > gdb/ChangeLog: > > * gdbthread.h (class thread_info) restore_suspend_from, stop_pc, set_stop_pc, > has_pending_waitstatus, pending_waitstatus, > set_pending_waitstatus, clear_pending_waitstatus, stop_signal, > set_stop_signal, stop_reason, set_stop_reason): New. > : Rename to... > : ... this, make private. Adjust all uses to use one > of the new methods above. > * thread.c (thread_info::set_pending_waitstatus, > thread_info::clear_pending_waitstatus): New. > * remote.c (class remote_target) : New > method. > (print_one_stopped_thread): Rename to... > (remote_target::print_one_stopped_thread): ... this. Assume > that if thread has no pending waitstatus, it's > TARGET_WAITKIND_STOPPED + GDB_SIGNAL_0. Call > set_last_target_status. > (remote_target::process_initial_stop_replies): Don't set pending > waitstatus if TARGET_WAITKIND_STOPPED + GDB_SIGNAL_0. Don't > call set_last_target_status. > (is_pending_fork_parent): Constify param. > (thread_pending_fork_status): Constify return. > (is_pending_fork_parent_thread): Adjust. > (remote_target::remove_new_fork_children): Adjust. OK.