[Lldb-commits] [PATCH] D119797: Fix race condition when launching and attaching.

Thu Feb 17 10:17:52 PST 2022

clayborg marked 2 inline comments as done.
clayborg added a comment.

In D119797#3321924 <https://reviews.llvm.org/D119797#3321924>, @labath wrote:

> Instead of polling, would it be possible for the event handler thread to send a notification (through a condition variable, std::future, etc.), and have the main thread wait for that?

I tried doing this, but was unable to make it work because attaching doesn't send the a eStateStopped event for the process' stop ID of 1 where launching does. Probably a bug that can be fixed in LLDB at some point, but attaching can be done in so many ways that I didn't want to hold up this patch for this.

In D119797#3324277 <https://reviews.llvm.org/D119797#3324277>, @wallace wrote:

> I agree with labath. Specially because I wouldn't like the user having aborted debug sessions if their setup takes longer than 10 seconds to get the process ready, which I imagine might happen with android or oculus devices using custom launch configs. What about using a condition variable and at the same time issue a progress notification mentioning that the LLDB is waiting for the process to stop? They could stop LLDB manually if something is not working properly for them in that case.

I default to 30 seconds now and I have added a new "timeout" launch configuration argument that _can_ be specified and the timeout can be made longer. Let me know if the 30 second default timeout should be increased.

================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:9

+#include <unistd.h>
 #include <chrono>
----------------
ilya-nozhkin wrote:
> Incompatible with MSVC. Maybe use std::chrono instead?
Done in the current updated patch, thanks for pointing this out.

================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:545
+  }
+  const useconds_t usleep_interval = 250 * 10000; // 250 ms internal
+  const useconds_t count = (seconds * 1000 * 1000)/usleep_interval;
----------------
yinghuitan wrote:
> Is this a typo? It should be 1000 instead of 10000, right?
Fixed by using std::chrono

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119797