[PATCH 1/2] gdb: return *this in target_waitstatus setters

[PATCH 1/2] gdb: return *this in target_waitstatus setters

[Simon Marchi via Gdb-patches]

From: Simon Marchi <simon.marchi@efficios.com>

While playing with some code creating target_waitstatus objects, I was
mildly annoyed by the fact that we can't just return a new
target_waitstatus object.  We have to do:

    target_waitstatus ws;
    ws.set_exited (123);
    return ws;

Make the setters return the "this" object as a reference, such that it's
possible to do:

    return target_waitstatus ().set_exited (123);

I initially thought of adding static creation functions, which you would
use like:

    return target_waitstatus::make_exited (123);

However, making the setters return a reference to the object achieves
pretty much the same thing, with less new code.

Change-Id: I45159b7f9fcd9db5b20603480e323020b14ed147
---
 gdb/target/waitstatus.h | 48 +++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/gdb/target/waitstatus.h b/gdb/target/waitstatus.h
index 48405d222f4e..631847732adb 100644
--- a/gdb/target/waitstatus.h
+++ b/gdb/target/waitstatus.h
@@ -218,109 +218,125 @@ struct target_waitstatus
 
   /* Setters: set the wait status kind plus any associated data.  */
 
-  void set_exited (int exit_status)
+  target_waitstatus &set_exited (int exit_status)
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_EXITED;
     m_value.exit_status = exit_status;
+    return *this;
   }
 
-  void set_stopped (gdb_signal sig)
+  target_waitstatus &set_stopped (gdb_signal sig)
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_STOPPED;
     m_value.sig = sig;
+    return *this;
   }
 
-  void set_signalled (gdb_signal sig)
+  target_waitstatus &set_signalled (gdb_signal sig)
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_SIGNALLED;
     m_value.sig = sig;
+    return *this;
   }
 
-  void set_loaded ()
+  target_waitstatus &set_loaded ()
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_LOADED;
+    return *this;
   }
 
-  void set_forked (ptid_t child_ptid)
+  target_waitstatus &set_forked (ptid_t child_ptid)
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_FORKED;
     m_value.child_ptid = child_ptid;
+    return *this;
   }
 
-  void set_vforked (ptid_t child_ptid)
+  target_waitstatus &set_vforked (ptid_t child_ptid)
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_VFORKED;
     m_value.child_ptid = child_ptid;
+    return *this;
   }
 
-  void set_execd (gdb::unique_xmalloc_ptr<char> execd_pathname)
+  target_waitstatus &set_execd (gdb::unique_xmalloc_ptr<char> execd_pathname)
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_EXECD;
     m_value.execd_pathname = execd_pathname.release ();
+    return *this;
   }
 
-  void set_vfork_done ()
+  target_waitstatus &set_vfork_done ()
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_VFORK_DONE;
+    return *this;
   }
 
-  void set_syscall_entry (int syscall_number)
+  target_waitstatus &set_syscall_entry (int syscall_number)
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_SYSCALL_ENTRY;
     m_value.syscall_number = syscall_number;
+    return *this;
   }
 
-  void set_syscall_return (int syscall_number)
+  target_waitstatus &set_syscall_return (int syscall_number)
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_SYSCALL_RETURN;
     m_value.syscall_number = syscall_number;
+    return *this;
   }
 
-  void set_spurious ()
+  target_waitstatus &set_spurious ()
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_SPURIOUS;
+    return *this;
   }
 
-  void set_ignore ()
+  target_waitstatus &set_ignore ()
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_IGNORE;
+    return *this;
   }
 
-  void set_no_history ()
+  target_waitstatus &set_no_history ()
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_NO_HISTORY;
+    return *this;
   }
 
-  void set_no_resumed ()
+  target_waitstatus &set_no_resumed ()
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_NO_RESUMED;
+    return *this;
   }
 
-  void set_thread_created ()
+  target_waitstatus &set_thread_created ()
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_THREAD_CREATED;
+    return *this;
   }
 
-  void set_thread_exited (int exit_status)
+  target_waitstatus &set_thread_exited (int exit_status)
   {
     this->reset ();
     m_kind = TARGET_WAITKIND_THREAD_EXITED;
     m_value.exit_status = exit_status;
+    return *this;
   }
 
   /* Get the kind of this wait status.  */
-- 
2.33.1

[PATCH 2/2] gdb: change store_waitstatus to return a target_waitstatus by value

[Simon Marchi via Gdb-patches]

store_waitstatus is basically a translation function between a status
integer and an equivalent target_waitstatus object.  It would make sense
for it to take the integer as a parameter and return the
target_waitstatus by value.  Do that, and rename to
host_status_to_waitstatus.  Users can then do:

  ws = host_status_to_waitstatus (status)

which does the right thing, given the move constructor of
target_waitstatus.

Change-Id: I7a07d59d3dc19d3ed66929642f82f44f3e85d61b
---
 gdb/gnu-nat.c        |  2 +-
 gdb/inf-child.c      | 17 +++++++++--------
 gdb/inf-child.h      |  5 ++---
 gdb/inf-ptrace.c     |  3 ++-
 gdb/linux-nat.c      |  6 +++---
 gdb/netbsd-nat.c     |  2 +-
 gdb/procfs.c         |  2 +-
 gdb/rs6000-aix-nat.c |  2 +-
 8 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 13127cd82461..f24e1c523c6b 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -1821,7 +1821,7 @@ S_proc_wait_reply (mach_port_t reply, kern_return_t err,
     }
   else if (pid == inf->pid)
     {
-      store_waitstatus (&inf->wait.status, status);
+      inf->wait.status = host_address_to_string (status);
       if (inf->wait.status.kind () == TARGET_WAITKIND_STOPPED)
 	/* The process has sent us a signal, and stopped itself in a sane
 	   state pending our actions.  */
diff --git a/gdb/inf-child.c b/gdb/inf-child.c
index 5e821f455982..b65bbf300747 100644
--- a/gdb/inf-child.c
+++ b/gdb/inf-child.c
@@ -51,18 +51,19 @@ inf_child_target::info () const
   return inf_child_target_info;
 }
 
-/* Helper function for child_wait and the derivatives of child_wait.
-   HOSTSTATUS is the waitstatus from wait() or the equivalent; store our
-   translation of that in OURSTATUS.  */
-void
-store_waitstatus (struct target_waitstatus *ourstatus, int hoststatus)
+/* See inf-child.h.  */
+
+target_waitstatus
+host_status_to_waitstatus (int hoststatus)
 {
   if (WIFEXITED (hoststatus))
-    ourstatus->set_exited (WEXITSTATUS (hoststatus));
+    return target_waitstatus ().set_exited (WEXITSTATUS (hoststatus));
   else if (!WIFSTOPPED (hoststatus))
-    ourstatus->set_signalled (gdb_signal_from_host (WTERMSIG (hoststatus)));
+    return target_waitstatus ().set_signalled
+      (gdb_signal_from_host (WTERMSIG (hoststatus)));
   else
-    ourstatus->set_stopped (gdb_signal_from_host (WSTOPSIG (hoststatus)));
+    return target_waitstatus ().set_stopped
+      (gdb_signal_from_host (WSTOPSIG (hoststatus)));
 }
 
 inf_child_target::~inf_child_target ()
diff --git a/gdb/inf-child.h b/gdb/inf-child.h
index aa33c5381381..1e009b6b74ae 100644
--- a/gdb/inf-child.h
+++ b/gdb/inf-child.h
@@ -104,10 +104,9 @@ class inf_child_target
   void maybe_unpush_target ();
 };
 
-/* Functions for helping to write a native target.  */
+/* Convert the host wait(2) status to a target_waitstatus.  */
 
-/* This is for native targets which use a unix/POSIX-style waitstatus.  */
-extern void store_waitstatus (struct target_waitstatus *, int);
+extern target_waitstatus host_status_to_waitstatus (int hoststatus);
 
 /* Register TARGET as native target and set it up to respond to the
    "target native" command.  */
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 852636ba646f..2e7a03c63f5e 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -334,7 +334,8 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
     }
   while (pid == -1);
 
-  store_waitstatus (ourstatus, status);
+  *ourstatus = host_status_to_waitstatus (status);
+
   return ptid_t (pid);
 }
 
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index fbb60a398b0c..656a0975dddf 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2159,7 +2159,7 @@ wait_lwp (struct lwp_info *lp)
 		 process is gone.  Store the status to report to the
 		 core.  Store it in lp->waitstatus, because lp->status
 		 would be ambiguous (W_EXITCODE(0,0) == 0).  */
-	      store_waitstatus (&lp->waitstatus, status);
+	      lp->waitstatus = host_status_to_waitstatus (status);
 	      return 0;
 	    }
 
@@ -2932,7 +2932,7 @@ linux_nat_filter_event (int lwpid, int status)
 
       /* Store the pending event in the waitstatus, because
 	 W_EXITCODE(0,0) == 0.  */
-      store_waitstatus (&lp->waitstatus, status);
+      lp->waitstatus = host_status_to_waitstatus (status);
       return;
     }
 
@@ -3306,7 +3306,7 @@ linux_nat_wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
       lp->waitstatus.set_ignore ();
     }
   else
-    store_waitstatus (ourstatus, status);
+    *ourstatus = host_status_to_waitstatus (status);
 
   linux_nat_debug_printf ("exit");
 
diff --git a/gdb/netbsd-nat.c b/gdb/netbsd-nat.c
index e06c036fcdd3..7dfc586fc7b1 100644
--- a/gdb/netbsd-nat.c
+++ b/gdb/netbsd-nat.c
@@ -560,7 +560,7 @@ nbsd_wait (ptid_t ptid, struct target_waitstatus *ourstatus,
   if (pid == -1)
     perror_with_name (_("Child process unexpectedly missing"));
 
-  store_waitstatus (ourstatus, status);
+  *ourstatus = host_status_to_waitstatus (status);
   return pid;
 }
 
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 2c96919dcebd..d77458b6c6ab 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -2353,7 +2353,7 @@ procfs_target::wait (ptid_t ptid, struct target_waitstatus *status,
 	}
 
       if (status)
-	store_waitstatus (status, wstat);
+	*status = host_status_to_waitstatus (wstat);
     }
 
   return retval;
diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
index 72e59e5e4844..d74211f048e6 100644
--- a/gdb/rs6000-aix-nat.c
+++ b/gdb/rs6000-aix-nat.c
@@ -539,7 +539,7 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
     ourstatus->set_spurious ();
   /* A normal waitstatus.  Let the usual macros deal with it.  */
   else
-    store_waitstatus (ourstatus, status);
+    *ourstatus = host_status_to_waitstatus (status);
 
   return ptid_t (pid);
 }
-- 
2.33.1

Re: [PATCH 2/2] gdb: change store_waitstatus to return a target_waitstatus by value

[John Baldwin]

On 12/1/21 5:40 PM, Simon Marchi via Gdb-patches wrote:
> store_waitstatus is basically a translation function between a status
> integer and an equivalent target_waitstatus object.  It would make sense
> for it to take the integer as a parameter and return the
> target_waitstatus by value.  Do that, and rename to
> host_status_to_waitstatus.  Users can then do:
> 
>    ws = host_status_to_waitstatus (status)
> 
> which does the right thing, given the move constructor of
> target_waitstatus.
> 
> Change-Id: I7a07d59d3dc19d3ed66929642f82f44f3e85d61b
> ---
>   gdb/gnu-nat.c        |  2 +-
>   gdb/inf-child.c      | 17 +++++++++--------
>   gdb/inf-child.h      |  5 ++---
>   gdb/inf-ptrace.c     |  3 ++-
>   gdb/linux-nat.c      |  6 +++---
>   gdb/netbsd-nat.c     |  2 +-
>   gdb/procfs.c         |  2 +-
>   gdb/rs6000-aix-nat.c |  2 +-
>   8 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
> index 13127cd82461..f24e1c523c6b 100644
> --- a/gdb/gnu-nat.c
> +++ b/gdb/gnu-nat.c
> @@ -1821,7 +1821,7 @@ S_proc_wait_reply (mach_port_t reply, kern_return_t err,
>       }
>     else if (pid == inf->pid)
>       {
> -      store_waitstatus (&inf->wait.status, status);
> +      inf->wait.status = host_address_to_string (status);

Typo?  (host_address_to_string vs host_address_to_waitstatus)

These two patches look good to me otherwise.

-- 
John Baldwin

Re: [PATCH 2/2] gdb: change store_waitstatus to return a target_waitstatus by value

[Simon Marchi via Gdb-patches]

On 2021-12-02 12:20, John Baldwin wrote:
> On 12/1/21 5:40 PM, Simon Marchi via Gdb-patches wrote:
>> store_waitstatus is basically a translation function between a status
>> integer and an equivalent target_waitstatus object.  It would make sense
>> for it to take the integer as a parameter and return the
>> target_waitstatus by value.  Do that, and rename to
>> host_status_to_waitstatus.  Users can then do:
>>
>>    ws = host_status_to_waitstatus (status)
>>
>> which does the right thing, given the move constructor of
>> target_waitstatus.
>>
>> Change-Id: I7a07d59d3dc19d3ed66929642f82f44f3e85d61b
>> ---
>>   gdb/gnu-nat.c        |  2 +-
>>   gdb/inf-child.c      | 17 +++++++++--------
>>   gdb/inf-child.h      |  5 ++---
>>   gdb/inf-ptrace.c     |  3 ++-
>>   gdb/linux-nat.c      |  6 +++---
>>   gdb/netbsd-nat.c     |  2 +-
>>   gdb/procfs.c         |  2 +-
>>   gdb/rs6000-aix-nat.c |  2 +-
>>   8 files changed, 20 insertions(+), 19 deletions(-)
>>
>> diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
>> index 13127cd82461..f24e1c523c6b 100644
>> --- a/gdb/gnu-nat.c
>> +++ b/gdb/gnu-nat.c
>> @@ -1821,7 +1821,7 @@ S_proc_wait_reply (mach_port_t reply, kern_return_t err,
>>       }
>>     else if (pid == inf->pid)
>>       {
>> -      store_waitstatus (&inf->wait.status, status);
>> +      inf->wait.status = host_address_to_string (status);
> 
> Typo?  (host_address_to_string vs host_address_to_waitstatus)
> 
> These two patches look good to me otherwise.
> 

Oops, the auto-complete played a trick on me.  Will fix locally, thanks
for taking a look.

Simon

Re: [PATCH 1/2] gdb: return *this in target_waitstatus setters

[Pedro Alves]

On 2021-12-02 01:40, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> While playing with some code creating target_waitstatus objects, I was
> mildly annoyed by the fact that we can't just return a new
> target_waitstatus object.  We have to do:
> 
>     target_waitstatus ws;
>     ws.set_exited (123);
>     return ws;
> 
> Make the setters return the "this" object as a reference, such that it's
> possible to do:
> 
>     return target_waitstatus ().set_exited (123);
> 
> I initially thought of adding static creation functions, which you would
> use like:
> 
>     return target_waitstatus::make_exited (123);
> 
> However, making the setters return a reference to the object achieves
> pretty much the same thing, with less new code.

I think the static methods would be nicer (I'd maybe drop the "make_" to keep
it short), but I'm fine with this too, it doesn't seem mutually exclusive.

Series LGTM.

Re: [PATCH 1/2] gdb: return *this in target_waitstatus setters

[Simon Marchi via Gdb-patches]

On 2021-12-03 08:10, Pedro Alves wrote:
> On 2021-12-02 01:40, Simon Marchi via Gdb-patches wrote:
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> While playing with some code creating target_waitstatus objects, I was
>> mildly annoyed by the fact that we can't just return a new
>> target_waitstatus object.  We have to do:
>>
>>     target_waitstatus ws;
>>     ws.set_exited (123);
>>     return ws;
>>
>> Make the setters return the "this" object as a reference, such that it's
>> possible to do:
>>
>>     return target_waitstatus ().set_exited (123);
>>
>> I initially thought of adding static creation functions, which you would
>> use like:
>>
>>     return target_waitstatus::make_exited (123);
>>
>> However, making the setters return a reference to the object achieves
>> pretty much the same thing, with less new code.
> 
> I think the static methods would be nicer (I'd maybe drop the "make_" to keep
> it short), but I'm fine with this too, it doesn't seem mutually exclusive.

I started with this idea to add static methods and remove the setters,
making target_waitstatus essentially immutable, except through the
assignment operator.  So to change an existing target_waitstatus object,
you would do use static method + assignment operator:

  lp->waitstatus = target_waitstatus::make_stopped (...);

instead of today:

  lp->waitstatus.set_stopped ();

But that would have needed a lot of unnecessary churn again, updating
all existing calls (which I did when introducing the setters).  And
having two ways of setting a target_waitstatus seems redundant, I prefer
if there's just one way.  It makes the code more consistent, easier to
grep for (you can grep for `set_exited`, for example), etc.  So I went
for the solution presented in this patch, but if someone wants to
convert to static methods later, I will be fine with it.

> Series LGTM.

Thanks, pushed.

Simon