[PATCH] Disable get_ptrace_pid for NetBSD

[PATCH] Disable get_ptrace_pid for NetBSD

[Kamil Rytarowski]

Unlike most other Operating Systems, NetBSD tracks both pid and lwp.
The process id on NetBSD is stored always in the pid field of ptid.

gdb/ChangeLog:

	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
	* inf-ptrace.c: Likewise.
	* (gdb_ptrace): Add.
	* (inf_ptrace_target::resume): Update.
	* (inf_ptrace_target::xfer_partial): Likewise.
---
 gdb/ChangeLog    |  8 ++++++++
 gdb/inf-ptrace.c | 32 +++++++++++++++++++++++---------
 gdb/inf-ptrace.h |  2 ++
 3 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 84964dc00ac..406414cee76 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2020-03-18  Kamil Rytarowski  <n54@gmx.com>
+
+	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
+	* inf-ptrace.c: Likewise.
+	* (gdb_ptrace): Add.
+	* (inf_ptrace_target::resume): Update.
+	* (inf_ptrace_target::xfer_partial): Likewise.
+
 2020-03-17  Kamil Rytarowski  <n54@gmx.com>

 	* regformats/regdef.h: Put reg in gdb namespace.
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index db17a76d946..304a749f3f3 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -37,6 +37,18 @@

 \f

+static int
+gdb_ptrace (PTRACE_TYPE_ARG1 request, ptid_t ptid, PTRACE_TYPE_ARG3 addr,
+	    PTRACE_TYPE_ARG4 data)
+{
+#ifdef __NetBSD__
+  return ptrace (request, ptid.pid (), addr, data);
+#else
+  pid_t pid = get_ptrace_pid (ptid);
+  return ptrace (request, pid, addr, data);
+#endif
+}
+
 /* A unique_ptr helper to unpush a target.  */

 struct target_unpusher
@@ -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.  */

 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;

   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;
@@ -588,7 +602,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;
diff --git a/gdb/inf-ptrace.h b/gdb/inf-ptrace.h
index dd0733736f2..340d41d8beb 100644
--- a/gdb/inf-ptrace.h
+++ b/gdb/inf-ptrace.h
@@ -78,9 +78,11 @@ struct inf_ptrace_target : public inf_child_target
   void detach_success (inferior *inf);
 };

+#ifndef __NetBSD__
 /* Return which PID to pass to ptrace in order to observe/control the
    tracee identified by PTID.  */

 extern pid_t get_ptrace_pid (ptid_t);
+#endif

 #endif
--
2.25.0

Re: [PATCH] Disable get_ptrace_pid for NetBSD

[Kamil Rytarowski]

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

On 18.03.2020 22:49, Kamil Rytarowski wrote:
> Unlike most other Operating Systems, NetBSD tracks both pid and lwp.
> The process id on NetBSD is stored always in the pid field of ptid.
> 

There is is one bug.

I'm going to fix and submit a new version.

> gdb/ChangeLog:
> 
> 	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
> 	* inf-ptrace.c: Likewise.
> 	* (gdb_ptrace): Add.
> 	* (inf_ptrace_target::resume): Update.
> 	* (inf_ptrace_target::xfer_partial): Likewise.
> ---
>  gdb/ChangeLog    |  8 ++++++++
>  gdb/inf-ptrace.c | 32 +++++++++++++++++++++++---------
>  gdb/inf-ptrace.h |  2 ++
>  3 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 84964dc00ac..406414cee76 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,11 @@
> +2020-03-18  Kamil Rytarowski  <n54@gmx.com>
> +
> +	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
> +	* inf-ptrace.c: Likewise.
> +	* (gdb_ptrace): Add.
> +	* (inf_ptrace_target::resume): Update.
> +	* (inf_ptrace_target::xfer_partial): Likewise.
> +
>  2020-03-17  Kamil Rytarowski  <n54@gmx.com>
> 
>  	* regformats/regdef.h: Put reg in gdb namespace.
> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> index db17a76d946..304a749f3f3 100644
> --- a/gdb/inf-ptrace.c
> +++ b/gdb/inf-ptrace.c
> @@ -37,6 +37,18 @@
> 
>  \f
> 
> +static int
> +gdb_ptrace (PTRACE_TYPE_ARG1 request, ptid_t ptid, PTRACE_TYPE_ARG3 addr,
> +	    PTRACE_TYPE_ARG4 data)
> +{
> +#ifdef __NetBSD__
> +  return ptrace (request, ptid.pid (), addr, data);
> +#else
> +  pid_t pid = get_ptrace_pid (ptid);
> +  return ptrace (request, pid, addr, data);
> +#endif
> +}
> +
>  /* A unique_ptr helper to unpush a target.  */
> 
>  struct target_unpusher
> @@ -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.  */
> 
>  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;
> 
>    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;
> @@ -588,7 +602,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;
> diff --git a/gdb/inf-ptrace.h b/gdb/inf-ptrace.h
> index dd0733736f2..340d41d8beb 100644
> --- a/gdb/inf-ptrace.h
> +++ b/gdb/inf-ptrace.h
> @@ -78,9 +78,11 @@ struct inf_ptrace_target : public inf_child_target
>    void detach_success (inferior *inf);
>  };
> 
> +#ifndef __NetBSD__
>  /* Return which PID to pass to ptrace in order to observe/control the
>     tracee identified by PTID.  */
> 
>  extern pid_t get_ptrace_pid (ptid_t);
> +#endif
> 
>  #endif
> --
> 2.25.0
> 



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

[PATCH v2] Disable get_ptrace_pid for NetBSD

[Kamil Rytarowski]

Unlike most other Operating Systems, NetBSD tracks both pid and lwp.
The process id on NetBSD is stored always in the pid field of ptid.

gdb/ChangeLog:

	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
	* inf-ptrace.c: Likewise.
	* (gdb_ptrace): Add.
	* (inf_ptrace_target::resume): Update.
	* (inf_ptrace_target::xfer_partial): Likewise.
---
 gdb/ChangeLog    |  8 ++++++++
 gdb/inf-ptrace.c | 34 ++++++++++++++++++++++++----------
 gdb/inf-ptrace.h |  2 ++
 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 84964dc00ac..406414cee76 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2020-03-18  Kamil Rytarowski  <n54@gmx.com>
+
+	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
+	* inf-ptrace.c: Likewise.
+	* (gdb_ptrace): Add.
+	* (inf_ptrace_target::resume): Update.
+	* (inf_ptrace_target::xfer_partial): Likewise.
+
 2020-03-17  Kamil Rytarowski  <n54@gmx.com>

 	* regformats/regdef.h: Put reg in gdb namespace.
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index db17a76d946..931192bd0db 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -37,6 +37,18 @@

 \f

+static int
+gdb_ptrace (PTRACE_TYPE_ARG1 request, ptid_t ptid, PTRACE_TYPE_ARG3 addr,
+	    PTRACE_TYPE_ARG4 data)
+{
+#ifdef __NetBSD__
+  return ptrace (request, ptid.pid (), addr, data);
+#else
+  pid_t pid = get_ptrace_pid (ptid);
+  return ptrace (request, pid, addr, data);
+#endif
+}
+
 /* A unique_ptr helper to unpush a target.  */

 struct target_unpusher
@@ -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.  */

 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;

   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;

@@ -588,7 +602,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;
diff --git a/gdb/inf-ptrace.h b/gdb/inf-ptrace.h
index dd0733736f2..340d41d8beb 100644
--- a/gdb/inf-ptrace.h
+++ b/gdb/inf-ptrace.h
@@ -78,9 +78,11 @@ struct inf_ptrace_target : public inf_child_target
   void detach_success (inferior *inf);
 };

+#ifndef __NetBSD__
 /* Return which PID to pass to ptrace in order to observe/control the
    tracee identified by PTID.  */

 extern pid_t get_ptrace_pid (ptid_t);
+#endif

 #endif
--
2.25.0

Re: [PATCH v2] Disable get_ptrace_pid for NetBSD

[Simon Marchi]

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

[PATCH v3] Disable get_ptrace_pid for NetBSD

[Kamil Rytarowski]

Unlike most other Operating Systems, NetBSD tracks both pid and lwp.
The process id on NetBSD is stored always in the pid field of ptid.

gdb/ChangeLog:

	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
	* inf-ptrace.c: Likewise.
	* (gdb_ptrace): Add.
	* (inf_ptrace_target::resume): Update.
	* (inf_ptrace_target::xfer_partial): Likewise.
	* (inf_ptrace_peek_poke): Change argument `pid' to `ptid'.
	* (inf_ptrace_peek_poke): Update.
---
 gdb/ChangeLog    | 10 ++++++++++
 gdb/inf-ptrace.c | 45 +++++++++++++++++++++++++++++----------------
 gdb/inf-ptrace.h |  7 ++++++-
 3 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 84964dc00ac..f09b4d154c5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2020-03-18  Kamil Rytarowski  <n54@gmx.com>
+
+	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
+	* inf-ptrace.c: Likewise.
+	* (gdb_ptrace): Add.
+	* (inf_ptrace_target::resume): Update.
+	* (inf_ptrace_target::xfer_partial): Likewise.
+	* (inf_ptrace_peek_poke): Change argument `pid' to `ptid'.
+	* (inf_ptrace_peek_poke): Update.
+
 2020-03-17  Kamil Rytarowski  <n54@gmx.com>

 	* regformats/regdef.h: Put reg in gdb namespace.
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index db17a76d946..4003b888616 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -37,6 +37,18 @@

 \f

+static int
+gdb_ptrace (PTRACE_TYPE_ARG1 request, ptid_t ptid, PTRACE_TYPE_ARG3 addr,
+	    PTRACE_TYPE_ARG4 data)
+{
+#ifdef __NetBSD__
+  return ptrace (request, ptid.pid (), addr, data);
+#else
+  pid_t pid = get_ptrace_pid (ptid);
+  return ptrace (request, pid, addr, data);
+#endif
+}
+
 /* A unique_ptr helper to unpush a target.  */

 struct target_unpusher
@@ -313,8 +325,9 @@ inf_ptrace_target::kill ()
   target_mourn_inferior (inferior_ptid);
 }

-/* Return which PID to pass to ptrace in order to observe/control the
-   tracee identified by PTID.  */
+#ifndef __NetBSD__
+
+/* See inf-ptrace.h.  */

 pid_t
 get_ptrace_pid (ptid_t ptid)
@@ -328,6 +341,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 +350,14 @@ 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 ();
+    ptid = inferior_ptid;
   else
-    pid = get_ptrace_pid (ptid);
+    ptid = ptid_t (inferior_ptid.pid ());

   if (catch_syscall_enabled () > 0)
     request = PT_SYSCALL;
@@ -365,7 +378,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"));
 }
@@ -460,7 +473,7 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
    be non-null.  Return the number of transferred bytes.  */

 static ULONGEST
-inf_ptrace_peek_poke (pid_t pid, gdb_byte *readbuf,
+inf_ptrace_peek_poke (ptid_t ptid, gdb_byte *readbuf,
 		      const gdb_byte *writebuf,
 		      ULONGEST addr, ULONGEST len)
 {
@@ -491,8 +504,8 @@ inf_ptrace_peek_poke (pid_t pid, gdb_byte *readbuf,
       if (readbuf != NULL || chunk < sizeof (PTRACE_TYPE_RET))
 	{
 	  errno = 0;
-	  buf.word = ptrace (PT_READ_I, pid,
-			     (PTRACE_TYPE_ARG3)(uintptr_t) addr, 0);
+	  buf.word = gdb_ptrace (PT_READ_I, ptid,
+				 (PTRACE_TYPE_ARG3)(uintptr_t) addr, 0);
 	  if (errno != 0)
 	    break;
 	  if (readbuf != NULL)
@@ -502,15 +515,15 @@ inf_ptrace_peek_poke (pid_t pid, gdb_byte *readbuf,
 	{
 	  memcpy (buf.byte + skip, writebuf + n, chunk);
 	  errno = 0;
-	  ptrace (PT_WRITE_D, pid, (PTRACE_TYPE_ARG3)(uintptr_t) addr,
+	  gdb_ptrace (PT_WRITE_D, ptid, (PTRACE_TYPE_ARG3)(uintptr_t) addr,
 		  buf.word);
 	  if (errno != 0)
 	    {
 	      /* Using the appropriate one (I or D) is necessary for
 		 Gould NP1, at least.  */
 	      errno = 0;
-	      ptrace (PT_WRITE_I, pid, (PTRACE_TYPE_ARG3)(uintptr_t) addr,
-		      buf.word);
+	      gdb_ptrace (PT_WRITE_I, ptid, (PTRACE_TYPE_ARG3)(uintptr_t) addr,
+			  buf.word);
 	      if (errno != 0)
 		break;
 	    }
@@ -528,7 +541,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 +565,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 +578,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, readbuf, writebuf,
 					  offset, len);
       return *xfered_len != 0 ? TARGET_XFER_OK : TARGET_XFER_EOF;

@@ -588,7 +601,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;
diff --git a/gdb/inf-ptrace.h b/gdb/inf-ptrace.h
index dd0733736f2..dea82d005e3 100644
--- a/gdb/inf-ptrace.h
+++ b/gdb/inf-ptrace.h
@@ -78,9 +78,14 @@ struct inf_ptrace_target : public inf_child_target
   void detach_success (inferior *inf);
 };

+#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.  */

 extern pid_t get_ptrace_pid (ptid_t);
+#endif

 #endif
--
2.25.0

Re: [PATCH v2] Disable get_ptrace_pid for NetBSD

[Kamil Rytarowski]

[-- 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 --]

Re: [PATCH v3] Disable get_ptrace_pid for NetBSD

[Simon Marchi]

On 2020-03-18 7:16 p.m., Kamil Rytarowski wrote:
> @@ -336,15 +350,14 @@ 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 ();
> +    ptid = inferior_ptid;
>    else
> -    pid = get_ptrace_pid (ptid);
> +    ptid = ptid_t (inferior_ptid.pid ());

That's not what I meant.  To keep the existing behavior, I believe it should be:

  if (minus_one_ptid == ptid)
    /* Resume all threads.  Traditionally ptrace() only supports
       single-threaded processes, so simply resume the inferior.  */
    ptid = ptid_t (inferior_ptid.pid ());

> 
>    if (catch_syscall_enabled () > 0)
>      request = PT_SYSCALL;
> @@ -365,7 +378,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"));
>  }

I'm getting this:

/home/simark/src/binutils-gdb/gdb/inf-ptrace.c: In member function ‘virtual void inf_ptrace_target::resume(ptid_t, int, gdb_signal)’:
/home/simark/src/binutils-gdb/gdb/inf-ptrace.c:379:15: error: invalid conversion from ‘int’ to ‘__ptrace_request’ [-fpermissive]
  379 |   gdb_ptrace (request, ptid, (PTRACE_TYPE_ARG3)1, gdb_signal_to_host (signal));
      |               ^~~~~~~
      |               |
      |               int
/home/simark/src/binutils-gdb/gdb/inf-ptrace.c:41:30: note:   initializing argument 1 of ‘int gdb_ptrace(__ptrace_request, ptid_t, long int, long int)’
   41 | gdb_ptrace (PTRACE_TYPE_ARG1 request, ptid_t ptid, PTRACE_TYPE_ARG3 addr,


We would need to change the type of the variable `ret` to PTRACE_TYPE_ARG1.

I'll make a test run on Linux with those changes to check if there's any regression.

Simon

Re: [PATCH v3] Disable get_ptrace_pid for NetBSD

[Kamil Rytarowski]

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

On 19.03.2020 02:13, Simon Marchi wrote:
> On 2020-03-18 7:16 p.m., Kamil Rytarowski wrote:
>> @@ -336,15 +350,14 @@ 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 ();
>> +    ptid = inferior_ptid;
>>    else
>> -    pid = get_ptrace_pid (ptid);
>> +    ptid = ptid_t (inferior_ptid.pid ());
> 
> That's not what I meant.  To keep the existing behavior, I believe it should be:
> 
>   if (minus_one_ptid == ptid)
>     /* Resume all threads.  Traditionally ptrace() only supports
>        single-threaded processes, so simply resume the inferior.  */
>     ptid = ptid_t (inferior_ptid.pid ());
> 

OK.

>>
>>    if (catch_syscall_enabled () > 0)
>>      request = PT_SYSCALL;
>> @@ -365,7 +378,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"));
>>  }
> 
> I'm getting this:
> 
> /home/simark/src/binutils-gdb/gdb/inf-ptrace.c: In member function ‘virtual void inf_ptrace_target::resume(ptid_t, int, gdb_signal)’:
> /home/simark/src/binutils-gdb/gdb/inf-ptrace.c:379:15: error: invalid conversion from ‘int’ to ‘__ptrace_request’ [-fpermissive]
>   379 |   gdb_ptrace (request, ptid, (PTRACE_TYPE_ARG3)1, gdb_signal_to_host (signal));
>       |               ^~~~~~~
>       |               |
>       |               int
> /home/simark/src/binutils-gdb/gdb/inf-ptrace.c:41:30: note:   initializing argument 1 of ‘int gdb_ptrace(__ptrace_request, ptid_t, long int, long int)’
>    41 | gdb_ptrace (PTRACE_TYPE_ARG1 request, ptid_t ptid, PTRACE_TYPE_ARG3 addr,
> 
> 
> We would need to change the type of the variable `ret` to PTRACE_TYPE_ARG1.
> 
> I'll make a test run on Linux with those changes to check if there's any regression.
> 

I presume you mean variable `request', not `ret'.

Done.

> Simon
> 



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

[PATCH v4] Disable get_ptrace_pid for NetBSD

[Kamil Rytarowski]

Unlike most other Operating Systems, NetBSD tracks both pid and lwp.
The process id on NetBSD is stored always in the pid field of ptid.

gdb/ChangeLog:

	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
	* inf-ptrace.c: Likewise.
	* (gdb_ptrace): Add.
	* (inf_ptrace_target::resume): Update.
	* (inf_ptrace_target::xfer_partial): Likewise.
	* (inf_ptrace_peek_poke): Change argument `pid' to `ptid'.
	* (inf_ptrace_peek_poke): Update.
---
 gdb/ChangeLog    | 10 ++++++++++
 gdb/inf-ptrace.c | 47 +++++++++++++++++++++++++++++------------------
 gdb/inf-ptrace.h |  7 ++++++-
 3 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7f87eceaf70..78c6893cfe8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2020-03-19  Kamil Rytarowski  <n54@gmx.com>
+
+	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
+	* inf-ptrace.c: Likewise.
+	* (gdb_ptrace): Add.
+	* (inf_ptrace_target::resume): Update.
+	* (inf_ptrace_target::xfer_partial): Likewise.
+	* (inf_ptrace_peek_poke): Change argument `pid' to `ptid'.
+	* (inf_ptrace_peek_poke): Update.
+
 2020-03-19  Andrew Burgess  <andrew.burgess@embecosm.com>

 	* remote.c (remote_target::process_stop_reply): Handle events for
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index db17a76d946..6c338cd5d14 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -37,6 +37,18 @@

 \f

+static int
+gdb_ptrace (PTRACE_TYPE_ARG1 request, ptid_t ptid, PTRACE_TYPE_ARG3 addr,
+	    PTRACE_TYPE_ARG4 data)
+{
+#ifdef __NetBSD__
+  return ptrace (request, ptid.pid (), addr, data);
+#else
+  pid_t pid = get_ptrace_pid (ptid);
+  return ptrace (request, pid, addr, data);
+#endif
+}
+
 /* A unique_ptr helper to unpush a target.  */

 struct target_unpusher
@@ -313,8 +325,9 @@ inf_ptrace_target::kill ()
   target_mourn_inferior (inferior_ptid);
 }

-/* Return which PID to pass to ptrace in order to observe/control the
-   tracee identified by PTID.  */
+#ifndef __NetBSD__
+
+/* See inf-ptrace.h.  */

 pid_t
 get_ptrace_pid (ptid_t ptid)
@@ -328,6 +341,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 +350,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;
+  PTRACE_TYPE_ARG1 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 = ptid_t (inferior_ptid.pid ());

   if (catch_syscall_enabled () > 0)
     request = PT_SYSCALL;
@@ -365,7 +376,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"));
 }
@@ -460,7 +471,7 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
    be non-null.  Return the number of transferred bytes.  */

 static ULONGEST
-inf_ptrace_peek_poke (pid_t pid, gdb_byte *readbuf,
+inf_ptrace_peek_poke (ptid_t ptid, gdb_byte *readbuf,
 		      const gdb_byte *writebuf,
 		      ULONGEST addr, ULONGEST len)
 {
@@ -491,8 +502,8 @@ inf_ptrace_peek_poke (pid_t pid, gdb_byte *readbuf,
       if (readbuf != NULL || chunk < sizeof (PTRACE_TYPE_RET))
 	{
 	  errno = 0;
-	  buf.word = ptrace (PT_READ_I, pid,
-			     (PTRACE_TYPE_ARG3)(uintptr_t) addr, 0);
+	  buf.word = gdb_ptrace (PT_READ_I, ptid,
+				 (PTRACE_TYPE_ARG3)(uintptr_t) addr, 0);
 	  if (errno != 0)
 	    break;
 	  if (readbuf != NULL)
@@ -502,15 +513,15 @@ inf_ptrace_peek_poke (pid_t pid, gdb_byte *readbuf,
 	{
 	  memcpy (buf.byte + skip, writebuf + n, chunk);
 	  errno = 0;
-	  ptrace (PT_WRITE_D, pid, (PTRACE_TYPE_ARG3)(uintptr_t) addr,
+	  gdb_ptrace (PT_WRITE_D, ptid, (PTRACE_TYPE_ARG3)(uintptr_t) addr,
 		  buf.word);
 	  if (errno != 0)
 	    {
 	      /* Using the appropriate one (I or D) is necessary for
 		 Gould NP1, at least.  */
 	      errno = 0;
-	      ptrace (PT_WRITE_I, pid, (PTRACE_TYPE_ARG3)(uintptr_t) addr,
-		      buf.word);
+	      gdb_ptrace (PT_WRITE_I, ptid, (PTRACE_TYPE_ARG3)(uintptr_t) addr,
+			  buf.word);
 	      if (errno != 0)
 		break;
 	    }
@@ -528,7 +539,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 +563,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 +576,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, readbuf, writebuf,
 					  offset, len);
       return *xfered_len != 0 ? TARGET_XFER_OK : TARGET_XFER_EOF;

@@ -588,7 +599,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;
diff --git a/gdb/inf-ptrace.h b/gdb/inf-ptrace.h
index dd0733736f2..dea82d005e3 100644
--- a/gdb/inf-ptrace.h
+++ b/gdb/inf-ptrace.h
@@ -78,9 +78,14 @@ struct inf_ptrace_target : public inf_child_target
   void detach_success (inferior *inf);
 };

+#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.  */

 extern pid_t get_ptrace_pid (ptid_t);
+#endif

 #endif
--
2.25.0

Re: [PATCH v4] Disable get_ptrace_pid for NetBSD

[Simon Marchi]

On 2020-03-19 8:28 a.m., Kamil Rytarowski wrote:
> Unlike most other Operating Systems, NetBSD tracks both pid and lwp.
> The process id on NetBSD is stored always in the pid field of ptid.
> 
> gdb/ChangeLog:
> 
> 	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
> 	* inf-ptrace.c: Likewise.
> 	* (gdb_ptrace): Add.
> 	* (inf_ptrace_target::resume): Update.
> 	* (inf_ptrace_target::xfer_partial): Likewise.
> 	* (inf_ptrace_peek_poke): Change argument `pid' to `ptid'.
> 	* (inf_ptrace_peek_poke): Update.

Hmm, this breaks simple debugging on Linux:

$ ./gdb --data-directory=data-directory a.out -ex start
Reading symbols from a.out...
Temporary breakpoint 1 at 0x4004da: file test.c, line 2.
Starting program: /home/smarchi/build/binutils-gdb/gdb/a.out

Program received signal SIGILL, Illegal instruction.
0x00007ffff7dda96d in dl_main (phdr=<optimized out>, phnum=<optimized out>, user_entry=<optimized out>, auxv=<optimized out>) at rtld.c:1517
1517    rtld.c: No such file or directory.


I haven't figured out why by inspecting the code yet, I'll try to debug it later today.

Simon

Re: [PATCH v4] Disable get_ptrace_pid for NetBSD

[Simon Marchi]

On 2020-03-19 8:55 a.m., Simon Marchi wrote:
> On 2020-03-19 8:28 a.m., Kamil Rytarowski wrote:
>> Unlike most other Operating Systems, NetBSD tracks both pid and lwp.
>> The process id on NetBSD is stored always in the pid field of ptid.
>>
>> gdb/ChangeLog:
>>
>> 	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
>> 	* inf-ptrace.c: Likewise.
>> 	* (gdb_ptrace): Add.
>> 	* (inf_ptrace_target::resume): Update.
>> 	* (inf_ptrace_target::xfer_partial): Likewise.
>> 	* (inf_ptrace_peek_poke): Change argument `pid' to `ptid'.
>> 	* (inf_ptrace_peek_poke): Update.
> 
> Hmm, this breaks simple debugging on Linux:
> 
> $ ./gdb --data-directory=data-directory a.out -ex start
> Reading symbols from a.out...
> Temporary breakpoint 1 at 0x4004da: file test.c, line 2.
> Starting program: /home/smarchi/build/binutils-gdb/gdb/a.out
> 
> Program received signal SIGILL, Illegal instruction.
> 0x00007ffff7dda96d in dl_main (phdr=<optimized out>, phnum=<optimized out>, user_entry=<optimized out>, auxv=<optimized out>) at rtld.c:1517
> 1517    rtld.c: No such file or directory.
> 
> 
> I haven't figured out why by inspecting the code yet, I'll try to debug it later today.
> 
> Simon
> 

Ah, it's because ptrace returns long and not int on GNU/Linux.  So when we want to read
a 64-bits word from memory, it gets truncated.  gdb_ptrace should return PTRACE_TYPE_RET.

In fact, to be consistent, all these gdb_ptrace functions should be changed to return
PTRACE_TYPE_RET (as a separate patch).

Simon

[PATCH v5] Disable get_ptrace_pid for NetBSD

[Kamil Rytarowski]

Unlike most other Operating Systems, NetBSD tracks both pid and lwp.
The process id on NetBSD is stored always in the pid field of ptid.

gdb/ChangeLog:

	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
	* inf-ptrace.c: Likewise.
	* (gdb_ptrace): Add.
	* (inf_ptrace_target::resume): Update.
	* (inf_ptrace_target::xfer_partial): Likewise.
	* (inf_ptrace_peek_poke): Change argument `pid' to `ptid'.
	* (inf_ptrace_peek_poke): Update.
---
 gdb/ChangeLog    |  9 +++++++++
 gdb/inf-ptrace.c | 47 +++++++++++++++++++++++++++++------------------
 gdb/inf-ptrace.h |  7 ++++++-
 3 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0955d648e79..c5119224415 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2020-03-19  Kamil Rytarowski  <n54@gmx.com>
+	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
+	* inf-ptrace.c: Likewise.
+	* (gdb_ptrace): Add.
+	* (inf_ptrace_target::resume): Update.
+	* (inf_ptrace_target::xfer_partial): Likewise.
+	* (inf_ptrace_peek_poke): Change argument `pid' to `ptid'.
+	* (inf_ptrace_peek_poke): Update.
+
 2020-03-19  Kamil Rytarowski  <n54@gmx.com>

 	* x86-bsd-nat.c (gdb_ptrace): New.
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index db17a76d946..a6a77ef9d31 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -37,6 +37,18 @@

 \f

+static PTRACE_TYPE_RET
+gdb_ptrace (PTRACE_TYPE_ARG1 request, ptid_t ptid, PTRACE_TYPE_ARG3 addr,
+	    PTRACE_TYPE_ARG4 data)
+{
+#ifdef __NetBSD__
+  return ptrace (request, ptid.pid (), addr, data);
+#else
+  pid_t pid = get_ptrace_pid (ptid);
+  return ptrace (request, pid, addr, data);
+#endif
+}
+
 /* A unique_ptr helper to unpush a target.  */

 struct target_unpusher
@@ -313,8 +325,9 @@ inf_ptrace_target::kill ()
   target_mourn_inferior (inferior_ptid);
 }

-/* Return which PID to pass to ptrace in order to observe/control the
-   tracee identified by PTID.  */
+#ifndef __NetBSD__
+
+/* See inf-ptrace.h.  */

 pid_t
 get_ptrace_pid (ptid_t ptid)
@@ -328,6 +341,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 +350,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;
+  PTRACE_TYPE_ARG1 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 = ptid_t (inferior_ptid.pid ());

   if (catch_syscall_enabled () > 0)
     request = PT_SYSCALL;
@@ -365,7 +376,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"));
 }
@@ -460,7 +471,7 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
    be non-null.  Return the number of transferred bytes.  */

 static ULONGEST
-inf_ptrace_peek_poke (pid_t pid, gdb_byte *readbuf,
+inf_ptrace_peek_poke (ptid_t ptid, gdb_byte *readbuf,
 		      const gdb_byte *writebuf,
 		      ULONGEST addr, ULONGEST len)
 {
@@ -491,8 +502,8 @@ inf_ptrace_peek_poke (pid_t pid, gdb_byte *readbuf,
       if (readbuf != NULL || chunk < sizeof (PTRACE_TYPE_RET))
 	{
 	  errno = 0;
-	  buf.word = ptrace (PT_READ_I, pid,
-			     (PTRACE_TYPE_ARG3)(uintptr_t) addr, 0);
+	  buf.word = gdb_ptrace (PT_READ_I, ptid,
+				 (PTRACE_TYPE_ARG3)(uintptr_t) addr, 0);
 	  if (errno != 0)
 	    break;
 	  if (readbuf != NULL)
@@ -502,15 +513,15 @@ inf_ptrace_peek_poke (pid_t pid, gdb_byte *readbuf,
 	{
 	  memcpy (buf.byte + skip, writebuf + n, chunk);
 	  errno = 0;
-	  ptrace (PT_WRITE_D, pid, (PTRACE_TYPE_ARG3)(uintptr_t) addr,
+	  gdb_ptrace (PT_WRITE_D, ptid, (PTRACE_TYPE_ARG3)(uintptr_t) addr,
 		  buf.word);
 	  if (errno != 0)
 	    {
 	      /* Using the appropriate one (I or D) is necessary for
 		 Gould NP1, at least.  */
 	      errno = 0;
-	      ptrace (PT_WRITE_I, pid, (PTRACE_TYPE_ARG3)(uintptr_t) addr,
-		      buf.word);
+	      gdb_ptrace (PT_WRITE_I, ptid, (PTRACE_TYPE_ARG3)(uintptr_t) addr,
+			  buf.word);
 	      if (errno != 0)
 		break;
 	    }
@@ -528,7 +539,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 +563,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 +576,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, readbuf, writebuf,
 					  offset, len);
       return *xfered_len != 0 ? TARGET_XFER_OK : TARGET_XFER_EOF;

@@ -588,7 +599,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;
diff --git a/gdb/inf-ptrace.h b/gdb/inf-ptrace.h
index dd0733736f2..dea82d005e3 100644
--- a/gdb/inf-ptrace.h
+++ b/gdb/inf-ptrace.h
@@ -78,9 +78,14 @@ struct inf_ptrace_target : public inf_child_target
   void detach_success (inferior *inf);
 };

+#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.  */

 extern pid_t get_ptrace_pid (ptid_t);
+#endif

 #endif
--
2.25.0

Re: [PATCH v4] Disable get_ptrace_pid for NetBSD

[Kamil Rytarowski]

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

On 19.03.2020 16:30, Simon Marchi wrote:
> On 2020-03-19 8:55 a.m., Simon Marchi wrote:
>> On 2020-03-19 8:28 a.m., Kamil Rytarowski wrote:
>>> Unlike most other Operating Systems, NetBSD tracks both pid and lwp.
>>> The process id on NetBSD is stored always in the pid field of ptid.
>>>
>>> gdb/ChangeLog:
>>>
>>> 	* inf-ptrace.h: Disable get_ptrace_pid on NetBSD.
>>> 	* inf-ptrace.c: Likewise.
>>> 	* (gdb_ptrace): Add.
>>> 	* (inf_ptrace_target::resume): Update.
>>> 	* (inf_ptrace_target::xfer_partial): Likewise.
>>> 	* (inf_ptrace_peek_poke): Change argument `pid' to `ptid'.
>>> 	* (inf_ptrace_peek_poke): Update.
>>
>> Hmm, this breaks simple debugging on Linux:
>>
>> $ ./gdb --data-directory=data-directory a.out -ex start
>> Reading symbols from a.out...
>> Temporary breakpoint 1 at 0x4004da: file test.c, line 2.
>> Starting program: /home/smarchi/build/binutils-gdb/gdb/a.out
>>
>> Program received signal SIGILL, Illegal instruction.
>> 0x00007ffff7dda96d in dl_main (phdr=<optimized out>, phnum=<optimized out>, user_entry=<optimized out>, auxv=<optimized out>) at rtld.c:1517
>> 1517    rtld.c: No such file or directory.
>>
>>
>> I haven't figured out why by inspecting the code yet, I'll try to debug it later today.
>>
>> Simon
>>
> 
> Ah, it's because ptrace returns long and not int on GNU/Linux.  So when we want to read
> a 64-bits word from memory, it gets truncated.  gdb_ptrace should return PTRACE_TYPE_RET.
> 
> In fact, to be consistent, all these gdb_ptrace functions should be changed to return
> PTRACE_TYPE_RET (as a separate patch).
> 

Done in v5.

I will follow up with other gdb_ptrace() instances next.

> Simon
> 



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

Re: [PATCH v5] Disable get_ptrace_pid for NetBSD

[Simon Marchi]

On 2020-03-19 11:45 a.m., Kamil Rytarowski wrote:
> Unlike most other Operating Systems, NetBSD tracks both pid and lwp.
> The process id on NetBSD is stored always in the pid field of ptid.

I did a test run and it looks fine, this patch is ok to push.

Simon

Re: [PATCH v4] Disable get_ptrace_pid for NetBSD

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Ah, it's because ptrace returns long and not int on GNU/Linux.
Simon> So when we want to read a 64-bits word from memory, it gets
Simon> truncated.  gdb_ptrace should return PTRACE_TYPE_RET.

FWIW it's also fine to just return long from our wrapper; at least if
all known ptrace implementations return some integer type.

Simon> In fact, to be consistent, all these gdb_ptrace functions should
Simon> be changed to return PTRACE_TYPE_RET (as a separate patch).

Now that we have C++ we could probably get rid of these autoconf checks
and just use the template instantiation or overloading tricks.

Tom