Re: [PATCH] MIPS/GDB/linux-nat.c: Fix a child detach condition for uClibc-ng

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Sergey Korolev <s.korolev@ndmsystems.com> [2018-06-15 01:54:12 +0300]:

> Current implementation expects that WIFSTOPPED (W_STOPCODE (0)) is true,
> but in the uClibc-ng it is false on MIPS. The patch adds a "detach"
> helper variable to avoid this corner case: WIFSTOPPED applied
> only to a status filled by a waitpid call that should never return
> the status with zero stop signal.

I took a quick look through this patch, and have a little feedback.

You might want to expand your commit message to explain _why_ MIPS is
different (having a signal 127), I didn't know this, and it puzzled me
for a while as to why your above text didn't just indicate a bug in
uClibc-ng :)

> ---
>  gdb/linux-nat.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 445b59fa4a..916de2d335 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -468,6 +468,7 @@ linux_nat_target::follow_fork (int follow_child, int
> detach_fork)
>        /* Detach new forked process?  */
>        if (detach_fork)
>   {
> +   int detach = 1;

There's already a detach_fork variable in this function, so maybe a
more descriptive name would make thing clearer.

>     struct cleanup *old_chain = make_cleanup (delete_lwp_cleanup,
>       child_lp);
> 
> @@ -492,9 +493,11 @@ linux_nat_target::follow_fork (int follow_child, int
> detach_fork)
>   perror_with_name (_("Couldn't do single step"));
>         if (my_waitpid (child_pid, &status, 0) < 0)
>   perror_with_name (_("Couldn't wait vfork process"));
> +       else
> + detach = WIFSTOPPED (status);
>       }
> 
> -   if (WIFSTOPPED (status))
> +   if (detach)
>       {
>         int signo;

I wonder if we should move the status into the scope of the call to
WIFSTOPPED, and avoid making any calls on it unless we know it has
been filled in.

Such a thing might look like this:


[PATCH] gdb: Avoid using W_STOPCODE(0) as this is ambiguous on MIPS

The MIPS target supports 127 signals, and this can create an ambiguity
in process wait statuses.  A status value of 0x007f could potentially
indicate a process that has exited with signal 127, or a process that
has stopped with signal 0.

In uClibc-ng the interpretation of 0x007f is that the process has
exited with signal 127 rather than stopped with signal 0, and so,
WIFSTOPPED (W_STOPCODE (0)) will be false rather than true as it would
be on most other platforms.

Given that it's pretty easy to avoid using W_STOPCODE (0), lets do that.

gdb/ChangeLog:

	* linux-nat.c (linux_nat_target::follow_fork): Avoid using
	'W_STOPCODE (0)' as this could be ambiguous.
---
 gdb/ChangeLog   |  6 ++++++
 gdb/linux-nat.c | 15 +++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index af38a2a1a5b..4de19770f42 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -448,7 +448,6 @@ linux_nat_target::follow_fork (int follow_child, int detach_fork)
   if (!follow_child)
     {
       struct lwp_info *child_lp = NULL;
-      int status = W_STOPCODE (0);
       int has_vforked;
       ptid_t parent_ptid, child_ptid;
       int parent_pid, child_pid;
@@ -468,6 +467,8 @@ linux_nat_target::follow_fork (int follow_child, int detach_fork)
       /* Detach new forked process?  */
       if (detach_fork)
 	{
+	  int child_stop_signal = 0;
+	  bool detach_child = true;
 	  struct cleanup *old_chain = make_cleanup (delete_lwp_cleanup,
 						    child_lp);
 
@@ -487,18 +488,24 @@ linux_nat_target::follow_fork (int follow_child, int detach_fork)
 	  if (!gdbarch_software_single_step_p (target_thread_architecture
 					       (parent_ptid)))
 	    {
+	      int status;
+
 	      linux_disable_event_reporting (child_pid);
 	      if (ptrace (PTRACE_SINGLESTEP, child_pid, 0, 0) < 0)
 		perror_with_name (_("Couldn't do single step"));
 	      if (my_waitpid (child_pid, &status, 0) < 0)
 		perror_with_name (_("Couldn't wait vfork process"));
+	      else
+		{
+		  detach_child = WIFSTOPPED (status);
+		  child_stop_signal = WSTOPSIG (status);
+		}
 	    }
 
-	  if (WIFSTOPPED (status))
+	  if (detach_child)
 	    {
-	      int signo;
+	      int signo = child_stop_signal;
 
-	      signo = WSTOPSIG (status);
 	      if (signo != 0
 		  && !signal_pass_state (gdb_signal_from_host (signo)))
 		signo = 0;
-- 
2.14.4