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 BC3E3385DC1B for ; Sun, 26 Apr 2020 21:14:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org BC3E3385DC1B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.193] (unknown [192.222.164.54]) (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 B242E1E581; Sun, 26 Apr 2020 17:14:35 -0400 (EDT) Subject: Re: [PATCH] Add basic event handling in the NetBSD target To: Kamil Rytarowski , gdb-patches@sourceware.org References: <20200417144508.6366-1-n54@gmx.com> <2eaea1b8-215c-c628-bedc-4b70900c18f9@simark.ca> From: Simon Marchi Message-ID: Date: Sun, 26 Apr 2020 17:14:34 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Sun, 26 Apr 2020 21:14:39 -0000 On 2020-04-24 6:09 a.m., Kamil Rytarowski wrote: >> I know that you'll say it's because FreeBSD does it this way, but still it surprised >> me. If the core of GDB asks to resume one specific thread, it doesn't mean you should >> suspend the other threads, they should just be left in whatever state they are. Is there >> a reason to stop the other threads here? >> > > In Linux there can be non-stop mode and threads are managed (stopped, > started, etc) individually. On NetBSD we can start and stop the whole > process, regardless of the number of threads in it. > > We need to set the state of all threads before PT_CONTINUE (or assume > unchanged state of them). > > If a thread is not expected to be suspended, we shall set its flag to > resume. The same for single-step (PT_STEP). This approach simplifies the > implementation as I don't need to track internally which thread has what > status and merely save a few syscalls. Ok. >>> + } >>> + else >>> + { >>> + /* If ptid is a wildcard, resume all matching threads (they won't run >>> + until the process is continued however). */ >>> + for (thread_info *tp : all_non_exited_threads (this, ptid)) >>> + if (ptrace (PT_RESUME, tp->ptid.pid (), NULL, tp->ptid.lwp ()) == -1) >>> + perror_with_name (("ptrace")); >>> + ptid = inferior_ptid; >> >> Can you explain this? Ideally the resume method should not rely on inferior_ptid. We >> are working on reducing the dependencies on this global, in favor of passing the context >> by parameters. Here, the `ptid` parameter should be enough. >> > > The caller can pass minus_one_ptid, which makes no sense for NetBSD and > certainly in the multi-target support. > > Scenarios, as I can see them on NetBSD: > > - passing ptid_t (pid, 0, 0) -> resume the whole process with all the > threads > - passing ptid_t (pid, lwp, 0) -> resume pid::lwp with all the other > threads suspended in pid (`set scheduler-lock') > - passing ptid_t (-1, ?, ?) -> confusion to me so I try to find any > fallback > > If we can abandon the -1 case, I can drop the inferior_ptid usage. If > the core no longer passes -1 (it could be), we shall cleanup existing > code that handles this as it could happen. If a refactoring can happen, > this shall be followed by addition of gdb_assert(ptid != minus_one_ptid). Or a 4th one: resume all threads of all processes that this target manages. I don't know off-hand if the core of GDB today can pass minus_one_ptid. You could add such an assert, run all your tests, and if everything works, conclude it's not needed. If the assert is hit, you'll know why. >>> + /* The common code passes WNOHANG that leads to crashes, overwrite it. */ >>> + pid = waitpid (ptid.pid (), &status, 0); >> >> What is crashing exactly? >> > > The core code asks to waitpid() with WNOHANG, receives pid=0 (nobody) > and crashes. > > It's a generic bug that shall be fixed... but it looks like nobody > really obeys the core and the 3rd argument is overwritten (compare: > rs6000-nat.c, obsd-nat.c, inf-ptrace.c or Linux...). > > I've landed into more similar generic nuances during my effort on GDB, > that handling events promptly can crash the core and I need to change > the behavior to emulate other kernels. > > It would be better to fix the core, but it is not NetBSD's fault and not > a blocker for NetBSD support. > > If there is interest in fixing it, I can file a bug report > independently. I don't pledge to work on it myself (at least as long as > I can merely focus on one kernel support). Ok, I'm fine with doing this and documenting it somewhere. Simon