[Lldb-commits] [PATCH] D72813: Fixes to lldb's eLaunchFlagLaunchInTTY feature on macOS

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda marked 3 inline comments as done.
jasonmolenda added a comment.

Thanks for the review Greg.  To really do this reliably, we probably either need to use a different kernel API, or push this down into debugserver where we can get the process' task port and inspect what its status is.  This patch is more a preliminary one to get it working much of the time -- getting it to always work is going to require a slightly different approach.



================
Comment at: lldb/source/Host/macosx/objcxx/Host.mm:154-155
+      // started, and stop at dyld_start, before we attach.
+      const int short_sleep = 100000; // 0.1 seconds
+      ::usleep(short_sleep);
+
----------------
clayborg wrote:
> This seems racy still?
It is.  darwin-debug is going to call posix_spawn after this, which exec's the app binary, which is launched and stopped at dyld_start.  At the same time, lldb is going to send the pid to debugserver which is going to attach to it.  In my own testing, the lldb+debugserver work was much slower than the app binary launch even without the sleep. 

I'm talking with the kernel folks to see if there's any way we could do better.  The best approach may be to push all this logic down into debugserver which can task_for_pid and check the task suspend count directly.  There are some other reasons why a task suspend count may be nonzero, but mostly it means we're done launching.


================
Comment at: lldb/source/Host/macosx/objcxx/Host.mm:159
-  const int time_delta_usecs = 100000;
-  const int num_retries = timeout_in_seconds / time_delta_usecs;
-  for (int i = 0; i < num_retries; i++) {
----------------
clayborg wrote:
> So if we fix this line to be:
> 
> ```
> const int num_retries = timeout_in_seconds * USEC_PER_SEC / time_delta_usecs;
> ```
> 
> This doesn't work? I am assuming you tried this?
That would make the loop execute more than 0 times but the return value from proc_pidinfo is the size of the struct read, not an errno value like this is treating it as.  And the app will not have a pbi_status of SSTOP when the task is suspended.  There's no way to detect this via the BSD side of the kernel APIs as near as I can find.


================
Comment at: lldb/tools/darwin-debug/darwin-debug.cpp:161
+    perror("error: send (socket_fd, pid_str, pid_str_len, 0)");
+    exit(1);
+  }
----------------
clayborg wrote:
> "close(socket_fd)" here if we fail and are going to exit?
Good point.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72813/new/

https://reviews.llvm.org/D72813