Re: [EXTERNAL] Re: Issues With Thread Events In User Mode GDBServer

[Simon Marchi <simark@simark.ca>]

On 9/30/22 17:08, Bill Messmer wrote:
> Simon,
> 
> Apologies for the delay in response.  I finally had a bit of time to debug through gdbserver while trying to get all of this working on my side...
> 
> linux_process_target::wait_1 does *NOT* call stop_all_lwps at all (even in full stop mode) if the event is a termination event.  The relevant block is the large
> 
>       if (WIFEXITED (w) || WIFSIGNALED (w))
>         {
> 
>         ...
> 
>           if (ourstatus->kind () == TARGET_WAITKIND_EXITED)
>             return filter_exit_event (event_child, ourstatus);
> 
>           return ptid_of (current_thread);
>         }
> 
> I went and tweaked this to:
> 
>       if (WIFEXITED (w) || WIFSIGNALED (w))
>         {
> 
>         ...
> 
>           if (ourstatus->kind () == TARGET_WAITKIND_EXITED)
>            result = filter_exit_event (event_child, ourstatus);
> 
>           result = ptid_of (current_thread);

Not sure about that last line.  If filter_exit_event has deleted the
current thread, I guess current_thread will be nullptr?

> 
>           if (!non_stop)
>             {
>               stop_all_lwps(0, NULL);

Hmm, if filter_exit_event transforms the event into an ignore, I guess
you don't want to stop_all_lwps, because we won't stop nor report
anything to GDB.

I don't think we need to call stop_all_threads if we are in the
WSIGNALLED case, I guess that one can only be reported for the leader
once all the other threads have already reported exit (though I didn't
verify).

That's what I have so far, including my hack in remote.c:


From c6ea8b47328a6236d74f80f3a5cb6eac78276af5 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Wed, 19 Oct 2022 11:57:20 -0400
Subject: [PATCH] fix

Change-Id: Iaac45d828116b65ea2812469da2d395f70f015d9
---
 gdb/remote.c           | 2 ++
 gdbserver/linux-low.cc | 9 ++++++++-
 gdbserver/server.cc    | 3 ++-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 17c2d17c8fe8..24816cb2704c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4776,6 +4776,8 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
   if (packet_support (PACKET_QAllow) != PACKET_DISABLE)
     set_permissions ();

+  this->thread_events (1);
+
   /* gdbserver < 7.7 (before its fix from 2013-12-11) did reply to any
      unknown 'v' packet with string "OK".  "OK" gets interpreted by GDB
      as a reply to known packet.  For packet "vFile:setfs:" it is an
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 4754366d4436..898e948d911d 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -3020,7 +3020,14 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
 	}

       if (ourstatus->kind () == TARGET_WAITKIND_EXITED)
-	return filter_exit_event (event_child, ourstatus);
+	{
+	  ptid_t ptid_ = filter_exit_event (event_child, ourstatus);
+
+	  if (!non_stop && ourstatus->kind () == TARGET_WAITKIND_THREAD_EXITED)
+	    stop_all_lwps (0, nullptr);
+
+	  return ptid_;
+	}

       return ptid_of (current_thread);
     }
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 366a843ea894..d0cf80915ee1 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2927,7 +2927,8 @@ resume (struct thread_resume *actions, size_t num_actions)

       if (cs.last_status.kind () != TARGET_WAITKIND_EXITED
 	  && cs.last_status.kind () != TARGET_WAITKIND_SIGNALLED
-	  && cs.last_status.kind () != TARGET_WAITKIND_NO_RESUMED)
+	  && cs.last_status.kind () != TARGET_WAITKIND_NO_RESUMED
+	  && cs.last_status.kind () != TARGET_WAITKIND_THREAD_EXITED)
 	current_thread->last_status = cs.last_status;

       /* From the client's perspective, all-stop mode always stops all
-- 
2.38.0


>             }
> 
>           return result;
>         }
> 
> With the tests I have, things appear to largely work as a I'd expect after making these changes.  Again -- I have little familiarity with GDBServer, so I don't know if I'm missing something here.
> 
> If this seems reasonably correct to you -- I'm happy to submit a patch.

It's really hard to say.  This is the kind of problem that gets you
scratching your head for week, trying to think of all the possible
cases.

If we want to support thread events in all-stop mode (by that I mean the
all-stop version of the remote protocol and therefore mode of operation
of GDBserver), I think the route forward would be:

 - Add a mode to GDB (maybe a maintenance setting) to tell targets to
   always report thread events
 - Write some tests that use it

Making it work will need some modifications on the GDB side, as GDB
currently doesn't handle thread exist stop notifications (small case
`w`) when using the all-stop remote protocol.  With my patch above
applied, when a thread exits, I get:

    warning: Invalid remote reply: w0;pb8251.b826b

We would need to teach remote_target::wait_as about it.

We may also need modifications to the local Linux debugging target
(linux-nat.c), I don't know if it will work out of the box when asked to
report thread events in all-stop.

Simon