Re: [PATCH v2] Disable get_ptrace_pid for NetBSD

Simon Marchi
2020-03-18T22:21:07Z

On 2020-03-18 5:55 p.m., Kamil Rytarowski wrote:
> @@ -313,8 +325,12 @@ inf_ptrace_target::kill ()
> target_mourn_inferior (inferior_ptid);
> }
>
> +#ifndef __NetBSD__
> /* Return which PID to pass to ptrace in order to observe/control the
> - tracee identified by PTID. */
> + tracee identified by PTID.
> +
> + Unlike most other Operating Systems, NetBSD tracks both pid and lwp
> + and avoids this function. */

To align this function with our current way of documenting functions, please move
this comment to the declaration of get_ptrace_pid, in inf-ptrace.h, and put this here:

/* See inf-ptrace.h. */

>
> pid_t
> get_ptrace_pid (ptid_t ptid)
> @@ -328,6 +344,7 @@ get_ptrace_pid (ptid_t ptid)
> pid = ptid.pid ();
> return pid;
> }
> +#endif
>
> /* Resume execution of thread PTID, or all threads if PTID is -1. If
> STEP is nonzero, single-step it. If SIGNAL is nonzero, give it
> @@ -336,15 +353,12 @@ get_ptrace_pid (ptid_t ptid)
> void
> inf_ptrace_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
> {
> - pid_t pid;
> int request;
>
> if (minus_one_ptid == ptid)
> /* Resume all threads. Traditionally ptrace() only supports
> single-threaded processes, so simply resume the inferior. */
> - pid = inferior_ptid.pid ();
> - else
> - pid = get_ptrace_pid (ptid);
> + ptid = inferior_ptid;

I'm not certain about this part (which I suggested). With the existing code,
if inferior_ptid is {pid=10, lwp=20}, we'll call ptrace with pid=10. With the
patch, we'll call ptrace with pid=20.

I'm not entirely sure if/when resume can be called with ptid == minus_one_ptid.
And if it's called with minus_one_ptid, is it possible that ptid.pid != ptid.lwp?

In any case, it's probably safer to do:

ptid = ptid_t (inferior_ptid.pid ());

to retain the existing behavior. WDYT?

>
> if (catch_syscall_enabled () > 0)
> request = PT_SYSCALL;
> @@ -365,7 +379,7 @@ inf_ptrace_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
> where it was. If GDB wanted it to start some other way, we have
> already written a new program counter value to the child. */
> errno = 0;
> - ptrace (request, pid, (PTRACE_TYPE_ARG3)1, gdb_signal_to_host (signal));
> + gdb_ptrace (request, ptid, (PTRACE_TYPE_ARG3)1, gdb_signal_to_host (signal));
> if (errno != 0)
> perror_with_name (("ptrace"));
> }
> @@ -528,7 +542,7 @@ inf_ptrace_target::xfer_partial (enum target_object object,
> const gdb_byte *writebuf,
> ULONGEST offset, ULONGEST len, ULONGEST *xfered_len)
> {
> - pid_t pid = get_ptrace_pid (inferior_ptid);
> + ptid_t ptid = inferior_ptid;
>
> switch (object)
> {
> @@ -552,7 +566,7 @@ inf_ptrace_target::xfer_partial (enum target_object object,
> piod.piod_len = len;
>
> errno = 0;
> - if (ptrace (PT_IO, pid, (caddr_t)&piod, 0) == 0)
> + if (gdb_ptrace (PT_IO, ptid, (caddr_t)&piod, 0) == 0)
> {
> /* Return the actual number of bytes read or written. */
> *xfered_len = piod.piod_len;
> @@ -565,7 +579,7 @@ inf_ptrace_target::xfer_partial (enum target_object object,
> return TARGET_XFER_EOF;
> }
> #endif
> - *xfered_len = inf_ptrace_peek_poke (pid, readbuf, writebuf,
> + *xfered_len = inf_ptrace_peek_poke (ptid.pid (), readbuf, writebuf,
> offset, len);
> return *xfered_len != 0 ? TARGET_XFER_OK : TARGET_XFER_EOF;

I'm pretty sure that this change is not right. For non-NetBSD targets, this
used to pass `inferior_ptid.lwp` (the result of get_ptrace_pid). Now, we pass
`inferior_ptid.pid`.

I think you'll need to pass the ptid to inf_ptrace_peek_poke and use gdb_ptrace
inside that function too.

Simon