Re: [PATCH v2] Disable get_ptrace_pid for NetBSD

Kamil Rytarowski
2020-03-18T23:31:52Z

[-- Attachment #1.1: Type: text/plain, Size: 4060 bytes --]

On 18.03.2020 23:21, Simon Marchi wrote:
> 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. */
>

Done.

>>
>> 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?
>

It's OK.

>>
>> 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`.
>

OK.

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

Done.

> Simon
>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]