NPL: Clean up handling of inferior exit

Summary:
lldb-server was sending the "exit" packet (W??) twice. This happened
because it was handling both the pre-exit (PTRACE_EVENT_EXIT) and
post-exit (WIFEXITED) as exit events. We had some code which was trying
to detect when we've already sent the exit packet, but this stopped
working quite a while ago.

This never really caused any problems in practice because the client
automatically closes the connection after receiving the first packet, so
the only effect of this was some warning messages about extra packets
from the lldb-server test suite, which were ignored because they didn't
fail the test.

The new test suite will be stricter about this, so I fix this issue
ignoring the first event. I think this is the correct behavior, as the
inferior is not really dead at that point, so it's premature to send the
exit packet.

There isn't an actual test yet which would verify the exit behavior, but
in my next patch I will add a test which will also test this
functionality.

Reviewers: eugene

Subscribers: lldb-commits

Differential Revision: https://reviews.llvm.org/D41069

llvm-svn: 320961

diff --git a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
index decd3af..136af36 100644 (file)
--- a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -412,46 +412,20 @@ void NativeProcessLinux::MonitorCallback(lldb::pid_t pid, bool exited,
 
   // Handle when the thread exits.
   if (exited) {
-    LLDB_LOG(log, "got exit signal({0}) , tid = {1} ({2} main thread)", signal,
-             pid, is_main_thread ? "is" : "is not");
+    LLDB_LOG(log,
+             "got exit signal({0}) , tid = {1} ({2} main thread), process "
+             "state = {3}",
+             signal, pid, is_main_thread ? "is" : "is not", GetState());
 
     // This is a thread that exited.  Ensure we're not tracking it anymore.
-    const bool thread_found = StopTrackingThread(pid);
+    StopTrackingThread(pid);
 
     if (is_main_thread) {
-      // We only set the exit status and notify the delegate if we haven't
-      // already set the process
-      // state to an exited state.  We normally should have received a SIGTRAP |
-      // (PTRACE_EVENT_EXIT << 8)
-      // for the main thread.
-      const bool already_notified = (GetState() == StateType::eStateExited) ||
-                                    (GetState() == StateType::eStateCrashed);
-      if (!already_notified) {
-        LLDB_LOG(
-            log,
-            "tid = {0} handling main thread exit ({1}), expected exit state "
-            "already set but state was {2} instead, setting exit state now",
-            pid,
-            thread_found ? "stopped tracking thread metadata"
-                         : "thread metadata not found",
-            GetState());
-        // The main thread exited.  We're done monitoring.  Report to delegate.
-        SetExitStatus(status, true);
+      // The main thread exited.  We're done monitoring.  Report to delegate.
+      SetExitStatus(status, true);
 
-        // Notify delegate that our process has exited.
-        SetState(StateType::eStateExited, true);
-      } else
-        LLDB_LOG(log, "tid = {0} main thread now exited (%s)", pid,
-                 thread_found ? "stopped tracking thread metadata"
-                              : "thread metadata not found");
-    } else {
-      // Do we want to report to the delegate in this case?  I think not.  If
-      // this was an orderly thread exit, we would already have received the
-      // SIGTRAP | (PTRACE_EVENT_EXIT << 8) signal, and we would have done an
-      // all-stop then.
-      LLDB_LOG(log, "tid = {0} handling non-main thread exit (%s)", pid,
-               thread_found ? "stopped tracking thread metadata"
-                            : "thread metadata not found");
+      // Notify delegate that our process has exited.
+      SetState(StateType::eStateExited, true);
     }
     return;
   }
@@ -662,10 +636,8 @@ void NativeProcessLinux::MonitorSIGTRAP(const siginfo_t &info,
   case (SIGTRAP | (PTRACE_EVENT_EXIT << 8)): {
     // The inferior process or one of its threads is about to exit.
     // We don't want to do anything with the thread so we just resume it. In
-    // case we
-    // want to implement "break on thread exit" functionality, we would need to
-    // stop
-    // here.
+    // case we want to implement "break on thread exit" functionality, we would
+    // need to stop here.
 
     unsigned long data = 0;
     if (GetEventMessage(thread.GetID(), &data).Fail())
@@ -677,18 +649,14 @@ void NativeProcessLinux::MonitorSIGTRAP(const siginfo_t &info,
              data, WIFEXITED(data), WIFSIGNALED(data), thread.GetID(),
              is_main_thread);
 
-    if (is_main_thread)
-      SetExitStatus(WaitStatus::Decode(data), true);
 
     StateType state = thread.GetState();
     if (!StateIsRunningState(state)) {
       // Due to a kernel bug, we may sometimes get this stop after the inferior
-      // gets a
-      // SIGKILL. This confuses our state tracking logic in ResumeThread(),
-      // since normally,
-      // we should not be receiving any ptrace events while the inferior is
-      // stopped. This
-      // makes sure that the inferior is resumed and exits normally.
+      // gets a SIGKILL. This confuses our state tracking logic in
+      // ResumeThread(), since normally, we should not be receiving any ptrace
+      // events while the inferior is stopped. This makes sure that the inferior
+      // is resumed and exits normally.
       state = eStateRunning;
     }
     ResumeThread(thread, state, LLDB_INVALID_SIGNAL_NUMBER);

diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
index 23c1547..4134005 100644 (file)
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -825,6 +825,7 @@ void GDBRemoteCommunicationServerLLGS::HandleInferiorState_Exited(
 
   // We are ready to exit the debug monitor.
   m_exit_now = true;
+  m_mainloop.RequestTermination();
 }
 
 void GDBRemoteCommunicationServerLLGS::HandleInferiorState_Stopped(

diff --git a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp
index c414eee..73ded85 100644 (file)
--- a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp
+++ b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp
@@ -40,6 +40,9 @@ TestClient::TestClient(std::unique_ptr<Connection> Conn) {
 }
 
 TestClient::~TestClient() {
+  if (!IsConnected())
+    return;
+
   std::string response;
   // Debugserver (non-conformingly?) sends a reply to the k packet instead of
   // simply closing the connection.
@@ -242,6 +245,18 @@ Error TestClient::Continue(StringRef message) {
     return E;
 
   m_stop_reply = std::move(*creation);
+  if (!isa<StopReplyStop>(m_stop_reply)) {
+    StringExtractorGDBRemote R;
+    PacketResult result = ReadPacket(R, GetPacketTimeout(), false);
+    if (result != PacketResult::ErrorDisconnected) {
+      return make_error<StringError>(
+          formatv("Expected connection close after receiving {0}. Got {1}/{2} "
+                  "instead.",
+                  response, result, R.GetStringRef())
+              .str(),
+          inconvertibleErrorCode());
+    }
+  }
   return Error::success();
 }