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 79E4C393741C for ; Tue, 17 Mar 2020 19:00:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 79E4C393741C 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.11] (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 C9EC91E5F8; Tue, 17 Mar 2020 15:00:16 -0400 (EDT) Subject: Re: [PATCH] Return unconditionally ptid.pid () in get_ptrace_pid() for NetBSD To: Kamil Rytarowski , gdb-patches@sourceware.org References: <20200317163020.28790-1-n54@gmx.com> <597c7d5f-dfd5-53a8-3369-4042d4cd653a@simark.ca> <72744690-5add-413e-a4cf-ada6cf8bd5e9@gmx.com> From: Simon Marchi Message-ID: Date: Tue, 17 Mar 2020 15:00:16 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <72744690-5add-413e-a4cf-ada6cf8bd5e9@gmx.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US-large Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-25.0 required=5.0 tests=GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS 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: Tue, 17 Mar 2020 19:00:18 -0000 On 2020-03-17 1:45 p.m., Kamil Rytarowski wrote: > On 17.03.2020 17:39, Simon Marchi wrote: >> On 2020-03-17 12:30 p.m., Kamil Rytarowski wrote: >>> NetBSD tracks the PID and LWP pair separately and both values are >>> needed and meaningful. >>> --- >>> gdb/inf-ptrace.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c >>> index db17a76d946..6a6cb554ba7 100644 >>> --- a/gdb/inf-ptrace.c >>> +++ b/gdb/inf-ptrace.c >>> @@ -321,10 +321,14 @@ get_ptrace_pid (ptid_t ptid) >>> { >>> pid_t pid; >>> >>> +#if !defined(__NetBSD__) >>> /* If we have an LWPID to work with, use it. Otherwise, we're >>> - dealing with a non-threaded program/target. */ >>> + dealing with a non-threaded program/target. >>> + >>> + NetBSD tracks the PID and LWP pair separately. */ >>> pid = ptid.lwp (); >>> if (pid == 0) >>> +#endif >>> pid = ptid.pid (); >>> return pid; >>> } >>> -- >>> 2.25.0 >>> >> >> I think you should just avoid using get_ptrace_pid on NetBSD altogether, since >> it is meant for OSes that require passing a single thread identifier to ptrace >> (whereas NetBSD requires the (pid, lwp) pair). >> >> Even with this modification in get_ptrace_pid, you need to change all the ptrace >> call sites to pass the lwp on top of it. >> >> I would suggest to instead #ifdef out get_ptrace_pid entirely on NetBSD, to avoid >> using it by mistake, and just replace all ptrace call sites possibly used on BSD >> to be >> >> ptrace (request, ptid.pid (), addr, ptid.lwp ()); >> >> This matches what I suggested in: >> >> https://sourceware.org/pipermail/gdb-patches/2020-March/166735.html >> >> Simon >> > > Avoiding is possibly nice.. however in the current code it is much more > intrusive. We would need to patch now generic and OS/CPU specific code > (some of that is also shared with other OSs due to legacy reasons). > > I think it is much cleaner to return ptid. pid() for NetBSD and reflect > the meaning of get_ptrace_pid(). > > If I follow your advice I end up with ifdefs like here: > > diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c > index b63a1bf88ef..a5d9c1d10ea 100644 > --- a/gdb/inf-ptrace.c > +++ b/gdb/inf-ptrace.c > @@ -349,7 +349,11 @@ inf_ptrace_target::resume (ptid_t ptid, int step, > enum gdb_signal signal) > single-threaded processes, so simply resume the inferior. */ > pid = inferior_ptid.pid (); > else > +#ifdef __NetBSD__ > + pid = ptid. pid(); > +#else > pid = get_ptrace_pid (ptid); > +#endif > > if (catch_syscall_enabled () > 0) > request = PT_SYSCALL; > @@ -533,7 +537,11 @@ inf_ptrace_target::xfer_partial (enum target_object > object, > const gdb_byte *writebuf, > ULONGEST offset, ULONGEST len, ULONGEST *xfered_len) > { > +#ifdef __NetBSD__ > + pid_t pid = inferior_ptid. pid(); > +#else > pid_t pid = get_ptrace_pid (inferior_ptid); > +#endif > > switch (object) > { I was thinking more about using a "gdb_ptrace" function in this file, as you have added in the other patch. The only ifdef would be in that function. get_ptrace_pid would only be used in the !__NetBSD__ branch of that function. inf_ptrace_target::xfer_partial and inf_ptrace_target::resume would just call gdb_ptrace, passing the right ptid. > Maintaining that will be certainly harder and it will be prone to > recurring regressions. > > If we want to take the route of cleanups and refactoring I think it > would be better to rethink the pid,lwp separation in Linux; but that is > much beyond the scope of my patches. I'm not sure what do mean by "rethink the pid,lwp separation in Linux". > Last but not least, get_ptrace_pid() would work now for NetBSD literally > as specified in the function name now... just extracting pid from ptid, > not calculating it from lwp/pid. It's now questionable whether a wrapper > function is still needed, but that would be optimized to .pid () in future. Yeah, but since NetBSD doesn't need get_ptrace_pid, I'd like if we could avoid making get_ptrace_pid more complex and if the ifdefs were concentrated around the ptrace calls (ideally, in as few spots possible, thanks to the gdb_ptrace functions). Simon