[Lldb-commits] [PATCH] D119548: [lldb] Fix race condition between lldb-vscode and stop hooks executor

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I don't think CreateProcess needs a HijackListener, does it?  It doesn't generate events, it just make the connection to the server.  It is always followed by Launch or DebugProcess, which are the ones that do the real work.



================
Comment at: lldb/include/lldb/Target/Target.h:587
 
   // If listener_sp is null, the listener of the owning Debugger object will be
   // used.
----------------
Probably worth saying what the hijack_listener is for as well.


================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py:14
         '''Create the Visual Studio Code debug adaptor'''
-        self.assertTrue(os.path.exists(self.lldbVSCodeExec),
-                        'lldb-vscode must exist')
+        self.assertTrue(is_exe(self.lldbVSCodeExec), 'lldb-vscode must exist')
         log_file_path = self.getBuildArtifact('vscode.txt')
----------------
I don't know if it's worth working harder for this, but it would be confusing if lldb-vscode existed but wasn't executable, and we gave an error of "lldb-vscode must exist". Maybe just say "must exist and be executable"...


================
Comment at: lldb/source/Target/Target.cpp:3038
+  // manually, without the platform).
+  if (synchronous_execution && !launch_info.GetHijackListener())
+    launch_info.SetHijackListener(
----------------
In this context sync vrs. async is really about whether the caller expects to get a stop event for the first "real user stop", or just have Launch return when we are at the first user event w/o the caller having to spin the event loop.  A real user stop is generally "process hit a breakpoint" or "process crashed", etc.

That means we should hijack the events up to the stop at the entry point in both sync and async modes.  The odd case is when "stop at entry" is set.  In that case the "launch success" stop is also the first "real user stop". So in the "stop at entry && async" mode we need to arrange for there to be a stop event that the caller can wait on.  

The cleanest way to do this would be to always hijack the events, and then in the case where stop at entry is set in async mode, rebroadcast the stop event to the regular listener.  You could have the DebugProcess & Launch return the last event SP for this purpose.

The way the old code did it was to just undo the hijack, and return w/o calling WaitForProcessToStop (line 3084 & following) under the assumption that this would leave the last stop event unfetched and available for the caller.  But I'm not sure that's guaranteed to work.


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

https://reviews.llvm.org/D119548