* [PATCH] Return unconditionally ptid.pid () in get_ptrace_pid() for NetBSD @ 2020-03-17 16:30 Kamil Rytarowski 2020-03-17 16:39 ` Simon Marchi 0 siblings, 1 reply; 5+ messages in thread From: Kamil Rytarowski @ 2020-03-17 16:30 UTC (permalink / raw) To: gdb-patches; +Cc: simark, Kamil Rytarowski 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Return unconditionally ptid.pid () in get_ptrace_pid() for NetBSD 2020-03-17 16:30 [PATCH] Return unconditionally ptid.pid () in get_ptrace_pid() for NetBSD Kamil Rytarowski @ 2020-03-17 16:39 ` Simon Marchi 2020-03-17 17:45 ` Kamil Rytarowski 0 siblings, 1 reply; 5+ messages in thread From: Simon Marchi @ 2020-03-17 16:39 UTC (permalink / raw) To: Kamil Rytarowski, gdb-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Return unconditionally ptid.pid () in get_ptrace_pid() for NetBSD 2020-03-17 16:39 ` Simon Marchi @ 2020-03-17 17:45 ` Kamil Rytarowski 2020-03-17 19:00 ` Simon Marchi 0 siblings, 1 reply; 5+ messages in thread From: Kamil Rytarowski @ 2020-03-17 17:45 UTC (permalink / raw) To: Simon Marchi, gdb-patches [-- Attachment #1.1: Type: text/plain, Size: 3470 bytes --] 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) { 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. 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. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Return unconditionally ptid.pid () in get_ptrace_pid() for NetBSD 2020-03-17 17:45 ` Kamil Rytarowski @ 2020-03-17 19:00 ` Simon Marchi 2020-03-18 16:45 ` Kamil Rytarowski 0 siblings, 1 reply; 5+ messages in thread From: Simon Marchi @ 2020-03-17 19:00 UTC (permalink / raw) To: Kamil Rytarowski, gdb-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Return unconditionally ptid.pid () in get_ptrace_pid() for NetBSD 2020-03-17 19:00 ` Simon Marchi @ 2020-03-18 16:45 ` Kamil Rytarowski 0 siblings, 0 replies; 5+ messages in thread From: Kamil Rytarowski @ 2020-03-18 16:45 UTC (permalink / raw) To: Simon Marchi, gdb-patches [-- Attachment #1.1: Type: text/plain, Size: 4989 bytes --] On 17.03.2020 20:00, Simon Marchi wrote: > 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". > Please disregard. Refactoring (and removal assumptions about specific threading model) is out of scope of my patches. >> 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 > I've submitted a patch disabling get_ptrace_pid() for NetBSD and switching in 2 places to ptid. pid()me file. Not all ptrace(2) calls accept PID+LWP on NetBSD as here are calls that work for the whole process such as DETACH, ATTACH, PT_SET_EVENT_MASK etc. It's easier for now to go for this approach in the proposed patch. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-18 16:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-17 16:30 [PATCH] Return unconditionally ptid.pid () in get_ptrace_pid() for NetBSD Kamil Rytarowski 2020-03-17 16:39 ` Simon Marchi 2020-03-17 17:45 ` Kamil Rytarowski 2020-03-17 19:00 ` Simon Marchi 2020-03-18 16:45 ` Kamil Rytarowski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox