[Lldb-commits] [lldb] [lldb] Removed gdbserver ports map from lldb-server (PR #104238)

[Pavel Labath via lldb-commits]

================
@@ -150,12 +153,17 @@ static void client_handle(GDBRemoteCommunicationServerPlatform &platform,
   printf("Disconnected.\n");
 }
 
-static GDBRemoteCommunicationServerPlatform::PortMap gdbserver_portmap;
-static std::mutex gdbserver_portmap_mutex;
-
 static void spawn_process_reaped(lldb::pid_t pid, int signal, int status) {
-  std::lock_guard<std::mutex> guard(gdbserver_portmap_mutex);
-  gdbserver_portmap.FreePortForProcess(pid);
+  if (g_single_pid != LLDB_INVALID_PROCESS_ID && g_single_pid == pid) {
+    // If not running as a server and the platform connection is closed
+    if (!g_terminate) {
+      g_terminate = true;
----------------
labath wrote:

I think I know the problems with detached threads pretty well (remember how I said I found bugs in lldb's dependencies -- [one of them](https://sourceware.org/bugzilla/show_bug.cgi?id=19951) is in glibc's pthread_detach). And I'm not interested in changing the design of the monitoring callback. It's detached and it's going to stay detached for a long time to come.

The thing I'm questioning is whether the code you are running inside the callback has to be detached (because the fact that the outer callback machinery is detached doesn't mean that the callback itself has to be detached).

The problems with detached threads are a runtime property. If a thread *accesses* something after it has been destroyed, you have a bug. I emphasised "access" because the access has to happen. Mere existence of a pointer/reference that would permit that access is not in itself a problem. So code like
```
int main() {
  std::condition_variable cv;
  std::thread([&]{cv.notify_one();}).detach();
  cv.wait();
  return;
}
```
is safe because even though the detached thread holds a reference to the local object (and may continue to hold it after its destroyed), the program ensures that the (only) usage of that reference happens while the object is alive. If I removed `cv.wait()` then we'd have a problem.

Why I'm mentioning this? Because this is exactly the pattern we have (or one that I think we should have) here, just instead of `cv.wait()` we have `mainloop.Run()` and `cv.notify()` is replaced by `mainloop.RequestTermination()`

If we can make sure things work this way, we're safe, or at least the (0.1%) problems are "not our problem". If we can't undetach this code, then we're exacerbating the problem, because even if someone wanted to make the undetach the monitoring callback machinery, he could make that work because the code inside it would be detached.

https://github.com/llvm/llvm-project/pull/104238