[lldb] Fix race condition between lldb-vscode and stop hooks executor

[ilya-nozhkin]

Summary

The race is between these two pieces of code that are executed in two separate
lldb-vscode threads (the first is in the main thread and another is in the
event-handling thread):

// lldb-vscode.cpp
g_vsc.debugger.SetAsync(false);
g_vsc.target.Launch(launch_info, error);
g_vsc.debugger.SetAsync(true);

// Target.cpp
bool old_async = debugger.GetAsyncExecution();
debugger.SetAsyncExecution(true);
debugger.GetCommandInterpreter().HandleCommands(GetCommands(), exc_ctx,
                                                options, result);
debugger.SetAsyncExecution(old_async);

The sequence that leads to the bug is this one:

1. Main thread enables synchronous mode and launches the process.
2. When the process is launched, it generates the first stop event.
3. This stop event is catched by the event-handling thread and DoOnRemoval is invoked.
4. Inside DoOnRemoval, this thread runs stop hooks. And before running stop hooks, the current synchronization mode is stored into old_async (and right now it is equal to "false").
5. The main thread finishes the launch and returns to lldb-vscode, the synchronization mode is restored to asynchronous by lldb-vscode.
6. Event-handling thread finishes stop hooks processing and restores the synchronization mode according to old_async (i.e. makes the mode synchronous)
7. And now the mode is synchronous while lldb-vscode expects it to be asynchronous. Synchronous mode forbids the process to broadcast public stop events, so, VS Code just hangs because lldb-vscode doesn't notify it about stops.

So, this diff makes the target intercept the first stop event if the process is
launched in the synchronous mode, thus preventing stop hooks execution.

The bug is only present on Windows because other platforms already
intercept this event using their own hijacking listeners.

So, this diff also fixes some problems with lldb-vscode tests on Windows to make
it possible to run the related test. Other tests still can't be enabled because
the debugged program prints something into stdout and LLDB can't intercept this
output and redirect it to lldb-vscode properly.

[labath]

I'm not sure this is fixing the problem at the right level. I mean, even if you fix the race on the synchronicity flag, it still seems like there is a zillion of other things that the two processes (event handler thread and stop hook executor) race on.

Isn't the right fix to make sure that the event gets broadcast only after the stop hooks finish their work? How does that work on other platforms?

[ilya-nozhkin]

In D119548#3318806, @labath wrote:

it still seems like there is a zillion of other things that the two processes (event handler thread and stop hook executor) race on.

Stop hooks are executed in the event handler thread, so, these two particular things don't have any conflicts. Event handler thread (and stop hooks execution inside it) races with the main lldb-vscode thread (the one that handles VS Code requests).

In D119548#3318806, @labath wrote:

Isn't the right fix to make sure that the event gets broadcast only after the stop hooks finish their work?

It would be cool because of some other reasons but it won't solve this particular race. I.e. we need some synchronization between stop hooks execution and the main lldb-vscode thread. So, even if the event is not broadcasted until stop hooks are executed (which means that they are executed by some other thread and it should not be the process' private state thread) then the main lldb-vscode thread will race with this thread that executes stop hooks.

In D119548#3318806, @labath wrote:

How does that work on other platforms?

I've only tested it on Linux and Windows, so, I don't know whether this bug is present on other platforms. On Linux, the first stop event is pushed to the event queue of lldb.PlatformLinux.DebugProcess.hijack listener but then this listener gets unsubscribed from the process events and deleted, so, the first stop event is not handled at all, DoOnRemoval method is not invoked thus stop hooks are not executed.
In the command line mode, as I understand, the synchronicity flag is not switched as often as lldb-vscode switches it, so, it seems that the race is avoided by chance.

[labath]

Ah, I'm sorry. It seems I misunderstood the nature of the problem. Let me try again.

In your description of the problem, I think the problematic part is the sequencing of steps 5 and 6. You're saying that the main thread returns from a "synchronous" launch operation even though the stop event is still being processed by the event handler thread. That doesn't seem right to me because the whole point of switching to synchronous mode was to ensure that, by the time the operation returns, the process is stopped and ready to accept additional commands. That won't work if the stop hooks are still running in the background.

So, I'd say that the bug is caused by the lack of proper ordering between these two events, and instead of making the code cope with it, the right fix is to add the synchronization. I think that could be achieved by "hijacking" the process events in the case of a synchronous launch, similar to how we do it for synchronous resumes (compare Process::Resume and Process:ResumeSynchronous) -- except that this may be slightly trickier to implement because some of our (platform-specific) launch code already hijacks these events.

[ilya-nozhkin]

Moved the body of this giant lambda to a separate method.
Also, renamed the locking method because "do with locked synchronization" looks like the synchronized mode will be locked. "Do with locked synchronicity" looks better IMO.

[ilya-nozhkin]

In D119548#3318995, @labath wrote:

I think that could be achieved by "hijacking" the process events in the case of a synchronous launch, similar to how we do it for synchronous resumes (compare Process::Resume and Process:ResumeSynchronous) -- except that this may be slightly trickier to implement because some of our (platform-specific) launch code already hijacks these events.

Oh, I didn't notice your answer when I was updating the diff. Yeah, I think, it will work perfectly even if platform-specific code hijacks events (it is done in the same thread, so, no race should happen, we only need to catch any events that weren't catched by platform-specific code). I'll try to implement this approach.

[jingham]

I'm a little confused by the analysis so far. SBTarget.Launch calls Target::Launch. That sets up a hijacker for the "stop at the first instruction" event regardless of the Sync mode. Once that succeeds, if the Debugger is set for Synchronous execution Target::Launch does:

if (synchronous_execution) {
  // Now we have handled the stop-from-attach, and we are just
  // switching to a synchronous resume.  So we should switch to the
  // SyncResume hijacker.
  m_process_sp->RestoreProcessEvents();
  m_process_sp->ResumeSynchronous(stream);

and ResumeSynchronous sets up a Hijacker to wait for and consume the stop event for the first "user stop" after the launch:

ListenerSP listener_sp(
    Listener::MakeListener(g_resume_sync_name));
HijackProcessEvents(listener_sp);

Status error = PrivateResume();
if (error.Success()) {
  StateType state = WaitForProcessToStop(llvm::None, nullptr, true,
                                         listener_sp, stream);

This is the same routine used by all the "target running" commands in sync mode. WaitForProcessToStop won't return till the process has stopped, and the ResumeSync hijacker will consume the event.

So I don't see how your event listening thread could have heard about the second stop event as the events should not have been sent to it.

[jingham]

Note, however, that if you are doing a Synchronous launch, you won't ever get an event telling you the process has stopped. Maybe that's what is causing the actual confusion? If you are waiting on a stop event after a synchronous Launch, you won't get one. But that's pretty much what you asked for: "run this command w/o my having to spin an event loop to handle it..." The way you know that a synchronous Lauch has stopped is because the command returned successfully.

I don't think this patch is solving the right problem.

[clayborg]

FYI: I have a patch coming up that will add the ability to not have to play with the async mode when launching or attaching. I will try and post that diff here, it should be up today.

We were running into a race condition when we used "launchCommands" or "attachCommands" as they are not executed in sync mode, so we ended up getting events before the IDE was ready. I am adding code to wait for the process to stop (we always stop at entry when launching via lldb-vscode) after the "launchCommands" or "attachCommands", and we might be able to get rid of the setting of async mode before and after launch.

[ilya-nozhkin]

In D119548#3321357, @jingham wrote:

SBTarget.Launch calls Target::Launch. That sets up a hijacker for the "stop at the first instruction" event regardless of the Sync mode.

The problem that it sets up this hijacker too late. See this part of the log (lines starting with // are my comments):

// The process has been launched successfully, its internal hijacker is removed.
 0x000002875FCB1AE0 Broadcaster("lldb.process")::RestoreBroadcaster (about to pop listener("LaunchEventHijack")=0x000002875FFCF890)
 Process::SetPublicState (state = stopped, restarted = 0)
 Process::SetPublicState (stopped) -- unlocking run lock
 Process::lldb_private::Process::StartPrivateStateThread() starting private state thread

// Some internal magic happens inside the private state thread.
 Process::lldb_private::Process::ControlPrivateStateThread (signal = 4)
 Sending control event of type: 4.
 0000028760372F90 Broadcaster("lldb.process.internal_state_control_broadcaster")::BroadcastEvent (event_sp = {0000028760571460 Event: broadcaster = 000002876047C4C0 (lldb.process.internal_state_control_broadcaster), type = 0x00000004 (control-resume), data = {Generic Event Data}}, unique =0) hijack = 0000000000000000
 000002875FFD00D0 Listener('lldb.process.internal_state_listener')::AddEvent (event_sp = {0000028760571460})
 Process::lldb_private::Process::RunPrivateStateThread (arg = 000002876047C350, pid = 9944) thread starting...
 timeout = <infinite>, event_sp)...
 this = 0x000002875FFD00D0, timeout = <infinite> for lldb.process.internal_state_listener
 000002875FFD00D0 'lldb.process.internal_state_listener' Listener::FindNextEventInternal(broadcaster=000002876047C4C0, broadcaster_names=0000000000000000[0], event_type_mask=0x00000000, remove=1) event 0000028760571460
 Process::lldb_private::Process::RunPrivateStateThread (arg = 000002876047C350, pid = 9944) got a control event: 4
 000002875FFCF5D0 Listener::StartListeningForEvents (broadcaster = 000002876047C9C8, mask = 0x00000020) acquired_mask = 0x00000020 for Communication::SyncronizeWithReadThread
 timeout = <infinite>, event_sp)...
 this = 0x000002875FFD00D0, timeout = <infinite> for lldb.process.internal_state_listener
 Process::ShouldBroadcastEvent (00000287604D4CB0) => new state: stopped, last broadcast state: stopped - YES
 Process::lldb_private::Process::HandlePrivateEvent (pid = 9944) broadcasting new state stopped (old state stopped) to public

// Process broadcasts the stop event.
 000002875FCB1AE0 Broadcaster("lldb.process")::BroadcastEvent (event_sp = {00000287604D4CB0 Event: broadcaster = 000002876047C378 (lldb.process), type = 0x00000001 (state-changed), data = { process = 000002876047C350 (pid = 9944), state = stopped}}, unique =0) hijack = 0000000000000000

// It is catched by "lldb.Debugger"!
 000002875DB0BD40 Listener('lldb.Debugger')::AddEvent (event_sp = {00000287604D4CB0})

// And only now the target adds its hijacking listener!
 0x000002875FCB1AE0 Broadcaster("lldb.process")::HijackBroadcaster (listener("lldb.Target.Launch.hijack")=0x000002875FFCFE10)

[ilya-nozhkin]

In D119548#3321376, @clayborg wrote:

FYI: I have a patch coming up that will add the ability to not have to play with the async mode when launching or attaching. I will try and post that diff here, it should be up today.

We were running into a race condition when we used "launchCommands" or "attachCommands" as they are not executed in sync mode, so we ended up getting events before the IDE was ready. I am adding code to wait for the process to stop (we always stop at entry when launching via lldb-vscode) after the "launchCommands" or "attachCommands", and we might be able to get rid of the setting of async mode before and after launch.

Oh, that's great to hear! It will probably solve this race condition on stop hooks as well.
Then, I think, I'd rather wait for your changes and run my test case against them.

[clayborg]

Can anyone try out the following patch and see if this stops the race condition

https://reviews.llvm.org/D119797

It might solve the problem.

[jingham]

In D119548#3321397, @ilya-nozhkin wrote:

In D119548#3321357, @jingham wrote:

SBTarget.Launch calls Target::Launch. That sets up a hijacker for the "stop at the first instruction" event regardless of the Sync mode.

The problem that it sets up this hijacker too late. See this part of the log (lines starting with // are my comments):

// The process has been launched successfully, its internal hijacker is removed.
 0x000002875FCB1AE0 Broadcaster("lldb.process")::RestoreBroadcaster (about to pop listener("LaunchEventHijack")=0x000002875FFCF890)
 Process::SetPublicState (state = stopped, restarted = 0)
 Process::SetPublicState (stopped) -- unlocking run lock
 Process::lldb_private::Process::StartPrivateStateThread() starting private state thread

// Some internal magic happens inside the private state thread.
 Process::lldb_private::Process::ControlPrivateStateThread (signal = 4)
 Sending control event of type: 4.
 0000028760372F90 Broadcaster("lldb.process.internal_state_control_broadcaster")::BroadcastEvent (event_sp = {0000028760571460 Event: broadcaster = 000002876047C4C0 (lldb.process.internal_state_control_broadcaster), type = 0x00000004 (control-resume), data = {Generic Event Data}}, unique =0) hijack = 0000000000000000
 000002875FFD00D0 Listener('lldb.process.internal_state_listener')::AddEvent (event_sp = {0000028760571460})
 Process::lldb_private::Process::RunPrivateStateThread (arg = 000002876047C350, pid = 9944) thread starting...
 timeout = <infinite>, event_sp)...
 this = 0x000002875FFD00D0, timeout = <infinite> for lldb.process.internal_state_listener
 000002875FFD00D0 'lldb.process.internal_state_listener' Listener::FindNextEventInternal(broadcaster=000002876047C4C0, broadcaster_names=0000000000000000[0], event_type_mask=0x00000000, remove=1) event 0000028760571460
 Process::lldb_private::Process::RunPrivateStateThread (arg = 000002876047C350, pid = 9944) got a control event: 4
 000002875FFCF5D0 Listener::StartListeningForEvents (broadcaster = 000002876047C9C8, mask = 0x00000020) acquired_mask = 0x00000020 for Communication::SyncronizeWithReadThread
 timeout = <infinite>, event_sp)...
 this = 0x000002875FFD00D0, timeout = <infinite> for lldb.process.internal_state_listener
 Process::ShouldBroadcastEvent (00000287604D4CB0) => new state: stopped, last broadcast state: stopped - YES
 Process::lldb_private::Process::HandlePrivateEvent (pid = 9944) broadcasting new state stopped (old state stopped) to public

// Process broadcasts the stop event.
 000002875FCB1AE0 Broadcaster("lldb.process")::BroadcastEvent (event_sp = {00000287604D4CB0 Event: broadcaster = 000002876047C378 (lldb.process), type = 0x00000001 (state-changed), data = { process = 000002876047C350 (pid = 9944), state = stopped}}, unique =0) hijack = 0000000000000000

// It is catched by "lldb.Debugger"!
 000002875DB0BD40 Listener('lldb.Debugger')::AddEvent (event_sp = {00000287604D4CB0})

// And only now the target adds its hijacking listener!
 0x000002875FCB1AE0 Broadcaster("lldb.process")::HijackBroadcaster (listener("lldb.Target.Launch.hijack")=0x000002875FFCFE10)

That can't be it. Target::Launch consumes the "stop at entry" event when it calls WaitForProcessToStop. And then we set the Resume hijacker before we resume the process, so there weren't any other stop events around at that point.

It's also odd that we choose to broadcast a stop event when the state is stopped. We don't usually broadcast duplicate events. Somewhere we must be being helpful and sending out a priming event when the private state thread gets resumed. But I couldn't find where we were doing that on a quick pass.

[ilya-nozhkin]

In D119548#3321452, @clayborg wrote:

Can anyone try out the following patch and see if this stops the race condition

https://reviews.llvm.org/D119797

It might solve the problem.

Yeah, now the test passes and VS Code doesn't fall into infinite waiting loop.
I'm still concerned about that weird event dispatching happening during the launch, but lldb-vscode now works, so, I can close this review if you want to investigate this bug sometime later.

In D119548#3321459, @jingham wrote:

Target::Launch consumes the "stop at entry" event when it calls WaitForProcessToStop.

Actually, there is a gap between returning from Process::Launch and setting up the target's hijacking listener. I think, the stop event sneaks past the target right during this time interval. So, Target::Launch can't consume it because there is nothing to consume (i.e. the event has been added to another listener's queue). But I agree that it's odd that this event is broadcasted at all.

[labath]

In D119548#3321397, @ilya-nozhkin wrote:

In D119548#3321357, @jingham wrote:

SBTarget.Launch calls Target::Launch. That sets up a hijacker for the "stop at the first instruction" event regardless of the Sync mode.

The problem that it sets up this hijacker too late.

Yes, so maybe the fix is to set the hijacker early (?)

That is what happens in the linux case -- hijacking is set up in PlatformPOSIX::DebugProcess, before the process is launched. If windows does not do that, then that would explain the bug and the differences between the platforms.

[jingham]

In D119548#3321936, @labath wrote:

In D119548#3321397, @ilya-nozhkin wrote:

In D119548#3321357, @jingham wrote:

SBTarget.Launch calls Target::Launch. That sets up a hijacker for the "stop at the first instruction" event regardless of the Sync mode.

The problem that it sets up this hijacker too late.

Yes, so maybe the fix is to set the hijacker early (?)

That is what happens in the linux case -- hijacking is set up in PlatformPOSIX::DebugProcess, before the process is launched. If windows does not do that, then that would explain the bug and the differences between the platforms.

Ah, I see. I was focusing on the second event, not the original stop one.

The current code is relying on getting the ProcessSP & calling HijackProcessEvents on it, but as you say, that's too late. Instead, we should put a Listener in the ProcessLaunchInfo before we do any of the CreateProcess stuff. All the calls to actually make the process in Target::Launch take a ProcessLaunchInfo, and should obey its listener. So that seems the right way to ensure that the process events get captured for this initial stop before they get a chance to leak out. And then we wouldn't have to rely on the individual platforms to set this up themselves.

[ilya-nozhkin]

Implemented the approach suggested by @labath. I.e. now the target's hijacking listener is activated before Process::Launch.

There are actually two ways of implementing that. The first of them (which is used in this diff) is to make the target activate the hijacking listener right after the process creation. The second one is to specify the hijacking listener in the launch info and then pass it to the platform, but it requires to modify all platforms to force them to activate the hijacking listener passed via launch info (currently, no platform actually does that). I've chosen the first way because it seems to be reliable and safe. I.e. the target won't depend on platforms, so, it'll be harder to break event dispatching again by some non-related modifications of platform's code.

I've also decided not to remove platform-specific hijacking listeners yet because some platforms define a hijacking listener even in asynchronous mode and then use it for waiting for the process to stop (for example, PlatformQemuUser does that). I'll try to test tomorrow whether it is possible to remove hijacking listeners at least from some subset of platforms.

[labath]

Doing it in the common code is a great idea, and it will make the code more robust. However, instead of "sneaking" the listener through a member variable, it would be better to pass it through function arguments instead (normally in the LaunchInfo struct, and as a probably as separate argument for the last mile). I believe this is what Jim had in mind as well.

See inline comments for details.

lldb/source/Target/Process.cpp

2528–2529

It would be better if this could be achieved by restructuring the code so that the hijacking is really scope-based. E.g., moving the "file doesn't exist" check up front and making it an early exit, moving some stuff into helper functions, or whatever.

lldb/source/Target/Target.cpp

202

Add an extra argument here.

3043–3044

Remove this. Make sure the listener in the launch_info is plumbed to the launch function.

3056

Remove hijack listener creation from DebugProcess implementations (you can replace with an assert if you want).

3087–3088

I guess we should just delete this now, as it is never correct to hijack events this late in the game.

[jingham]

In D119548#3325999, @labath wrote:

Doing it in the common code is a great idea, and it will make the code more robust. However, instead of "sneaking" the listener through a member variable, it would be better to pass it through function arguments instead (normally in the LaunchInfo struct, and as a probably as separate argument for the last mile). I believe this is what Jim had in mind as well.

Yup!

See inline comments for details.

[ilya-nozhkin]

Applied requested changes.

[jingham]

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.

lldb/include/lldb/Target/Target.h

587

Probably worth saying what the hijack_listener is for as well.

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py

14

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"...

lldb/source/Target/Target.cpp

3038

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.

[labath]

In D119548#3327751, @jingham wrote:

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.

CreateProcess is called from within DebugProcess, but DebugProcess is virtual/platform-specific. That makes CreateProcess a handy centralized place to do this, particularly if we're *always* going to be hijacking events for the initial creation (I'm not sure what's our attach story).

[ilya-nozhkin]

Made requested changes.

I agree with @jingham that if the listener is passed via launch info then we don't need to activate it in CreateProcess. I mean, activating it in CreateProcess was a hack that allowed me to hijack events regardless of platform implementation. But if the listener is passed via launch info then platforms should redirect it to CreateProcess anyway. So, why not activate the listener in the platform's code? On the one hand, it creates unnecessary code duplication but on the other hand, it makes responsibilities of CreateProcess clearer. A possible compromise is to create a new method Target::PrepareProcessForLaunch and set up the hijacker there.

[clayborg]

Try out the https://reviews.llvm.org/D119797 patch and see if this fixes this issue? I have just updated it after responding to comments.

[ilya-nozhkin]

In D119548#3330226, @clayborg wrote:

Try out the https://reviews.llvm.org/D119797 patch and see if this fixes this issue? I have just updated it after responding to comments.

Yes, it fixes this particular race. But I think, that the issue is not only with stop hooks and lldb-vscode. Some other tools can do something similar to lldb-vscode and get the same issue. So, I think, it's still worth fixing the event dispatching.

lldb/source/Target/Process.cpp

2458

Btw, this seems to be the line that sends the public stop event. So, adding synchronicity check to the condition should fix the issue as well. But I'd keep the target's hijacking listener too because we still want to intercept all event during the launch, not to remove them.

[labath]

In D119548#3330773, @ilya-nozhkin wrote:

In D119548#3330226, @clayborg wrote:

Try out the https://reviews.llvm.org/D119797 patch and see if this fixes this issue? I have just updated it after responding to comments.

Yes, it fixes this particular race. But I think, that the issue is not only with stop hooks and lldb-vscode. Some other tools can do something similar to lldb-vscode and get the same issue. So, I think, it's still worth fixing the event dispatching.

Definitely worth fixing this too.

In D119548#3329785, @ilya-nozhkin wrote:

Made requested changes.

I agree with @jingham that if the listener is passed via launch info then we don't need to activate it in CreateProcess.

Sounds good. I'm happy with this if Jim is.

[jingham]

Yes, that looks great. Thanks for persisting!

[labath]

Do you need someone to commit this for you? (I can probably do that tomorrow)

[ilya-nozhkin]

In D119548#3334776, @labath wrote:

Do you need someone to commit this for you? (I can probably do that tomorrow)

Yes, that would be nice. I don't have commit access.

[labath]

Committed. Thanks for the great patch.

lldb/include/lldb/Target/Process.h

private:
   ///
   /// \return
   ///     Returns \b true if the event should be reported to the
   ///     user, \b false otherwise.
   bool ShouldBroadcastEvent(Event *event_ptr);
 
   void ControlPrivateStateThread(uint32_t signal);
 
+  Status LaunchPrivate(ProcessLaunchInfo &launch_info, lldb::StateType &state,
+                       lldb::EventSP &event_sp);
+
   Process(const Process &) = delete;
   const Process &operator=(const Process &) = delete;
 };
 
 /// RAII guard that should be acquired when an utility function is called within
 /// a given process.
 class UtilityFunctionScope {
   Process *m_process;

lldb/include/lldb/Target/Target.h

public:
   /// been loaded or parsed up to this point at which this function
   /// is called, so this is a good way to see what has been parsed
   /// in a target.
   ///
   /// \param[in] s
   ///     The stream to which to dump the object description.
   void Dump(Stream *s, lldb::DescriptionLevel description_level);
 
   // If listener_sp is null, the listener of the owning Debugger object will be
   // used.
-  const lldb::ProcessSP &CreateProcess(lldb::ListenerSP listener_sp,
-                                       llvm::StringRef plugin_name,
-                                       const FileSpec *crash_file,
-                                       bool can_connect);
+  const lldb::ProcessSP &
+  CreateProcess(lldb::ListenerSP listener_sp, llvm::StringRef plugin_name,
+                const FileSpec *crash_file, bool can_connect,
+                lldb::ListenerSP hijack_listener_sp = {});
 
   const lldb::ProcessSP &GetProcessSP() const;
 
   bool IsValid() { return m_valid; }
 
   void Destroy();
 
   Status Launch(ProcessLaunchInfo &launch_info,

lldb/packages/Python/lldbsuite/test/lldbtest.py

def pointer_size():
     """Return the pointer size of the host system."""
     import ctypes
     a_pointer = ctypes.c_void_p(0xffff)
     return 8 * ctypes.sizeof(a_pointer)
 
 
 def is_exe(fpath):
     """Returns true if fpath is an executable."""
+    if fpath == None:
+        return False
+    if sys.platform == 'win32':
+        if not fpath.endswith(".exe"):
+            fpath += ".exe"
     return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
 
 
 def which(program):
     """Returns the full path to a program; None otherwise."""
     fpath, fname = os.path.split(program)
     if fpath:
         if is_exe(program):

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py

 
 from lldbsuite.test.lldbtest import *
 import os
 import vscode
 import time
 
 
 class VSCodeTestCaseBase(TestBase):
 
     NO_DEBUG_INFO_TESTCASE = True
 
     def create_debug_adaptor(self, lldbVSCodeEnv=None):
         '''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')
         self.vscode = vscode.DebugAdaptor(
             executable=self.lldbVSCodeExec, init_commands=self.setUpCommands(),
             log_file=log_file_path, env=lldbVSCodeEnv)
 
     def build_and_create_debug_adaptor(self, lldbVSCodeEnv=None):
         self.build()
         self.create_debug_adaptor(lldbVSCodeEnv)

lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

lldb::ProcessSP PlatformPOSIX::DebugProcess(ProcessLaunchInfo &launch_info,
   // group, since then we can handle ^C interrupts ourselves w/o having to
   // worry about the target getting them as well.
   launch_info.SetLaunchInSeparateProcessGroup(true);
 
   // Now create the gdb-remote process.
   LLDB_LOG(log, "having target create process with gdb-remote plugin");
   process_sp =
       target.CreateProcess(launch_info.GetListener(), "gdb-remote", nullptr,
-                            true);
+                           true, launch_info.GetHijackListener());
 
   if (!process_sp) {
     error.SetErrorString("CreateProcess() failed for gdb-remote process");
     LLDB_LOG(log, "error: {0}", error);
     return process_sp;
   }
 
   LLDB_LOG(log, "successfully created process");
-  // Adjust launch for a hijacker.
-  ListenerSP listener_sp;
-  if (!launch_info.GetHijackListener()) {
-    LLDB_LOG(log, "setting up hijacker");
-    listener_sp =
-        Listener::MakeListener("lldb.PlatformLinux.DebugProcess.hijack");
-    launch_info.SetHijackListener(listener_sp);
-    process_sp->HijackProcessEvents(listener_sp);
-  }
 
   // Log file actions.
   if (log) {
     LLDB_LOG(log, "launching process with the following file actions:");
     StreamString stream;
     size_t i = 0;
     const FileAction *file_action;
     while ((file_action = launch_info.GetFileActionAtIndex(i++)) != nullptr) {
       file_action->Dump(stream);
       LLDB_LOG(log, "{0}", stream.GetData());
       stream.Clear();
     }
   }
 
   // Do the launch.
   error = process_sp->Launch(launch_info);
   if (error.Success()) {
-    // Handle the hijacking of process events.
-    if (listener_sp) {
-      const StateType state = process_sp->WaitForProcessToStop(
-          llvm::None, nullptr, false, listener_sp);
-
-      LLDB_LOG(log, "pid {0} state {0}", process_sp->GetID(), state);
-    }
-
     // Hook up process PTY if we have one (which we should for local debugging
     // with llgs).
     int pty_fd = launch_info.GetPTY().ReleasePrimaryFileDescriptor();
     if (pty_fd != PseudoTerminal::invalid_fd) {
       process_sp->SetSTDIOFileDescriptor(pty_fd);
       LLDB_LOG(log, "hooked up STDIO pty to process");
     } else
       LLDB_LOG(log, "not using process STDIO pty");

lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp

lldb::ProcessSP PlatformQemuUser::DebugProcess(ProcessLaunchInfo &launch_info,
 
   error = Host::LaunchProcess(launch_info);
   if (error.Fail())
     return nullptr;
 
   ProcessSP process_sp = target.CreateProcess(
       launch_info.GetListener(),
       process_gdb_remote::ProcessGDBRemote::GetPluginNameStatic(), nullptr,
-      true);
-
-  ListenerSP listener_sp =
-      Listener::MakeListener("lldb.platform_qemu_user.debugprocess");
-  launch_info.SetHijackListener(listener_sp);
-  Process::ProcessEventHijacker hijacker(*process_sp, listener_sp);
+      true, launch_info.GetHijackListener());
 
   error = process_sp->ConnectRemote(("unix-connect://" + socket_path).str());
   if (error.Fail())
     return nullptr;
 
   if (launch_info.GetPTY().GetPrimaryFileDescriptor() !=
       PseudoTerminal::invalid_fd)
     process_sp->SetSTDIOFileDescriptor(
         launch_info.GetPTY().ReleasePrimaryFileDescriptor());
 
-  process_sp->WaitForProcessToStop(llvm::None, nullptr, false, listener_sp);
   return process_sp;
 }
 
 Environment PlatformQemuUser::GetEnvironment() {
   Environment env = Host::GetEnvironment();
   for (const auto &KV : GetGlobalProperties().GetTargetEnvVars())
     env[KV.first()] = KV.second;
   return env;
 }

lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp

ProcessSP PlatformWindows::DebugProcess(ProcessLaunchInfo &launch_info,
 
   if (launch_info.GetProcessID() != LLDB_INVALID_PROCESS_ID) {
     // This is a process attach.  Don't need to launch anything.
     ProcessAttachInfo attach_info(launch_info);
     return Attach(attach_info, debugger, &target, error);
   } else {
     ProcessSP process_sp = target.CreateProcess(
         launch_info.GetListener(), launch_info.GetProcessPluginName(), nullptr,
-        false);
+        false, launch_info.GetHijackListener());
 
     // We need to launch and attach to the process.
     launch_info.GetFlags().Set(eLaunchFlagDebug);
     if (process_sp)
       error = process_sp->Launch(launch_info);
 
     return process_sp;
   }

lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp

if (IsConnected()) {
       std::string connect_url;
       if (!LaunchGDBServer(debugserver_pid, connect_url)) {
         error.SetErrorStringWithFormat("unable to launch a GDB server on '%s'",
                                        GetHostname());
       } else {
         // The darwin always currently uses the GDB remote debugger plug-in
         // so even when debugging locally we are debugging remotely!
         process_sp = target.CreateProcess(launch_info.GetListener(),
-                                          "gdb-remote", nullptr, true);
+                                          "gdb-remote", nullptr, true,
+                                          launch_info.GetHijackListener());
 
         if (process_sp) {
           error = process_sp->ConnectRemote(connect_url.c_str());
           // Retry the connect remote one time...
           if (error.Fail())
             error = process_sp->ConnectRemote(connect_url.c_str());
           if (error.Success())
             error = process_sp->Launch(launch_info);

lldb/source/Target/Process.cpp

void Process::LoadOperatingSystemPlugin(bool flush) {
   if (flush)
     m_thread_list.Clear();
   m_os_up.reset(OperatingSystem::FindPlugin(this, nullptr));
   if (flush)
     Flush();
 }
 
 Status Process::Launch(ProcessLaunchInfo &launch_info) {
+  StateType state_after_launch = eStateInvalid;
+  EventSP first_stop_event_sp;
+  Status status =
+      LaunchPrivate(launch_info, state_after_launch, first_stop_event_sp);
+  if (status.Fail())
+    return status;
+
+  if (state_after_launch != eStateStopped &&
+      state_after_launch != eStateCrashed)
+    return Status();
+
+  // Note, the stop event was consumed above, but not handled. This
+  // was done to give DidLaunch a chance to run. The target is either
+  // stopped or crashed. Directly set the state.  This is done to
+  // prevent a stop message with a bunch of spurious output on thread
+  // status, as well as not pop a ProcessIOHandler.
+  SetPublicState(state_after_launch, false);
+
+  if (PrivateStateThreadIsValid())
+    ResumePrivateStateThread();
+  else
+    StartPrivateStateThread();
+
+  // Target was stopped at entry as was intended. Need to notify the
+  // listeners about it.
+  if (state_after_launch == eStateStopped &&
+      launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
+    HandlePrivateEvent(first_stop_event_sp);
+
+  return Status();
+}
+
+Status Process::LaunchPrivate(ProcessLaunchInfo &launch_info, StateType &state,
+                              EventSP &event_sp) {
   Status error;
   m_abi_sp.reset();
   m_dyld_up.reset();
   m_jit_loaders_up.reset();
   m_system_runtime_up.reset();
   m_os_up.reset();
   m_process_input_reader.reset();
 
   Module *exe_module = GetTarget().GetExecutableModulePointer();
 
   // The "remote executable path" is hooked up to the local Executable
   // module.  But we should be able to debug a remote process even if the
   // executable module only exists on the remote.  However, there needs to
   // be a way to express this path, without actually having a module.
   // The way to do that is to set the ExecutableFile in the LaunchInfo.
   // Figure that out here:
-  
   FileSpec exe_spec_to_use;
   if (!exe_module) {
     if (!launch_info.GetExecutableFile()) {
       error.SetErrorString("executable module does not exist");
       return error;
     }
     exe_spec_to_use = launch_info.GetExecutableFile();
   } else
     exe_spec_to_use = exe_module->GetFileSpec();
-  
   if (exe_module && FileSystem::Instance().Exists(exe_module->GetFileSpec())) {
     // Install anything that might need to be installed prior to launching.
     // For host systems, this will do nothing, but if we are connected to a
     // remote platform it will install any needed binaries
     error = GetTarget().Install(&launch_info);
     if (error.Fail())
       return error;
   }
+
   // Listen and queue events that are broadcasted during the process launch.
   ListenerSP listener_sp(Listener::MakeListener("LaunchEventHijack"));
   HijackProcessEvents(listener_sp);
   auto on_exit = llvm::make_scope_exit([this]() { RestoreProcessEvents(); });
 
   if (PrivateStateThreadIsValid())
     PausePrivateStateThread();
 
   error = WillLaunch(exe_module);
-  if (error.Success()) {
-    const bool restarted = false;
-    SetPublicState(eStateLaunching, restarted);
-    m_should_detach = false;
+  if (error.Fail()) {
+    std::string local_exec_file_path = exe_spec_to_use.GetPath();
+    return Status("file doesn't exist: '%s'", local_exec_file_path.c_str());
+  }
+
 
-    if (m_public_run_lock.TrySetRunning()) {
-      // Now launch using these arguments.
-      error = DoLaunch(exe_module, launch_info);
-    } else {
-      // This shouldn't happen
-      error.SetErrorString("failed to acquire process run lock");
-    }
 
-    if (error.Fail()) {
-      if (GetID() != LLDB_INVALID_PROCESS_ID) {
-        SetID(LLDB_INVALID_PROCESS_ID);
-        const char *error_string = error.AsCString();
-        if (error_string == nullptr)
-          error_string = "launch failed";
-        SetExitStatus(-1, error_string);
-      }
-    } else {
-      EventSP event_sp;
+    return error;
+  }
 
-      // Now wait for the process to launch and return control to us, and then
-      // call DidLaunch:
-      StateType state = WaitForProcessStopPrivate(event_sp, seconds(10));
+  state = WaitForProcessStopPrivate(event_sp, seconds(10));
 
-      if (state == eStateInvalid || !event_sp) {
-        // We were able to launch the process, but we failed to catch the
-        // initial stop.
-        error.SetErrorString("failed to catch stop after launch");
-        SetExitStatus(0, "failed to catch stop after launch");
-        Destroy(false);
-      } else if (state == eStateStopped || state == eStateCrashed) {
-        DidLaunch();
+    SetExitStatus(0, error.AsCString());
+    return error;
+  }
+
+  if (state == eStateExited) {
+    // We exited while trying to launch somehow.  Don't call DidLaunch
+    // as that's not likely to work, and return an invalid pid.
+    HandlePrivateEvent(event_sp);
+    return Status();
+  }
+
+  if (state == eStateStopped || state == eStateCrashed) {
 
-        DynamicLoader *dyld = GetDynamicLoader();
-        if (dyld)
-          dyld->DidLaunch();
 
-        GetJITLoaders().DidLaunch();
 
-        SystemRuntime *system_runtime = GetSystemRuntime();
-        if (system_runtime)
-          system_runtime->DidLaunch();
 
-        if (!m_os_up)
-          LoadOperatingSystemPlugin(false);
 
-        // We successfully launched the process and stopped, now it the
-        // right time to set up signal filters before resuming.
-        UpdateAutomaticSignalFiltering();
-
-        // Note, the stop event was consumed above, but not handled. This
-        // was done to give DidLaunch a chance to run. The target is either
-        // stopped or crashed. Directly set the state.  This is done to
-        // prevent a stop message with a bunch of spurious output on thread
-        // status, as well as not pop a ProcessIOHandler.
-        // We are done with the launch hijack listener, and this stop should
-        // go to the public state listener:
-        RestoreProcessEvents();
-        SetPublicState(state, false);
-
-        if (PrivateStateThreadIsValid())
-          ResumePrivateStateThread();
-        else
-          StartPrivateStateThread();
-
-        // Target was stopped at entry as was intended. Need to notify the
-        // listeners about it.
-        if (state == eStateStopped &&
-            launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
-          HandlePrivateEvent(event_sp);
-      } else if (state == eStateExited) {
-        // We exited while trying to launch somehow.  Don't call DidLaunch
-        // as that's not likely to work, and return an invalid pid.
-        HandlePrivateEvent(event_sp);
-      }
-    }
-  } else {
-    std::string local_exec_file_path = exe_spec_to_use.GetPath();
-    error.SetErrorStringWithFormat("file doesn't exist: '%s'",
-                                   local_exec_file_path.c_str());
+    return Status();
   }
 
-  return error;
+  return Status("Unexpected process state after the launch: %s, expected %s, "
+                "%s, %s or %s",
+                StateAsCString(state), StateAsCString(eStateInvalid),
+                StateAsCString(eStateExited), StateAsCString(eStateStopped),
+                StateAsCString(eStateCrashed));
 }
 
 Status Process::LoadCore() {
   Status error = DoLoadCore();
   if (error.Success()) {
     ListenerSP listener_sp(
         Listener::MakeListener("lldb.process.load_core_listener"));
     HijackProcessEvents(listener_sp);

lldb/source/Target/Target.cpp

if (m_process_sp) {
     CleanupProcess();
 
     m_process_sp.reset();
   }
 }
 
 const lldb::ProcessSP &Target::CreateProcess(ListenerSP listener_sp,
                                              llvm::StringRef plugin_name,
                                              const FileSpec *crash_file,
-                                             bool can_connect) {
+                                             bool can_connect,
+                                             ListenerSP hijack_listener_sp) {
   if (!listener_sp)
     listener_sp = GetDebugger().GetListener();
   DeleteCurrentProcess();
   m_process_sp = Process::FindPlugin(shared_from_this(), plugin_name,
                                      listener_sp, crash_file, can_connect);
+
+  if (m_process_sp && hijack_listener_sp)
+    m_process_sp->HijackProcessEvents(hijack_listener_sp);
+
   return m_process_sp;
 }
 
 const lldb::ProcessSP &Target::GetProcessSP() const { return m_process_sp; }
 
 lldb::REPLSP Target::GetREPL(Status &err, lldb::LanguageType language,
                              const char *repl_options, bool can_create) {
   if (language == eLanguageTypeUnknown)
if (launch_info.GetFlags().Test(eLaunchFlagLaunchInTTY)) {
           "can't launch in tty when launching through a remote connection");
       return error;
     }
   }
 
   if (!launch_info.GetArchitecture().IsValid())
     launch_info.GetArchitecture() = GetArchitecture();
 
+  // Hijacking events of the process to be created to be sure that all events
+  // until the first stop are intercepted (in case if platform doesn't define
+  // its own hijacking listener or if the process is created by the target
+  // manually, without the platform).
+  if (synchronous_execution && !launch_info.GetHijackListener())
+    launch_info.SetHijackListener(
+        Listener::MakeListener("lldb.Target.Launch.hijack"));
+
   // If we're not already connected to the process, and if we have a platform
   // that can launch a process for debugging, go ahead and do that here.
   if (state != eStateConnected && platform_sp &&
       platform_sp->CanDebugProcess() && !launch_info.IsScriptedProcess()) {
     LLDB_LOGF(log, "Target::%s asking the platform to debug the process",
               __FUNCTION__);
 
     // If there was a previous process, delete it before we make the new one.
     // One subtle point, we delete the process before we release the reference
     // to m_process_sp.  That way even if we are the last owner, the process
     // will get Finalized before it gets destroyed.
     DeleteCurrentProcess();
 
     m_process_sp =
         GetPlatform()->DebugProcess(launch_info, debugger, *this, error);
 
   } else {
     LLDB_LOGF(log,
               "Target::%s the platform doesn't know how to debug a "
               "process, getting a process plugin to do this for us.",
               __FUNCTION__);
 
     if (state == eStateConnected) {
       assert(m_process_sp);
     } else {
       // Use a Process plugin to construct the process.
       const char *plugin_name = launch_info.GetProcessPluginName();
-      CreateProcess(launch_info.GetListener(), plugin_name, nullptr, false);
+      CreateProcess(launch_info.GetListener(), plugin_name, nullptr, false,
+                    launch_info.GetHijackListener());
     }
 
     // Since we didn't have a platform launch the process, launch it here.
     if (m_process_sp)
       error = m_process_sp->Launch(launch_info);
   }
 
   if (!m_process_sp && error.Success())
     error.SetErrorString("failed to launch or debug process");
 
   if (!error.Success())
     return error;
 
-  auto at_exit =
-      llvm::make_scope_exit([&]() { m_process_sp->RestoreProcessEvents(); });
-
   if (!synchronous_execution &&
-      launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
+      launch_info.GetFlags().Test(eLaunchFlagStopAtEntry)) {
+    m_process_sp->RestoreProcessEvents();
     return error;
-
-  ListenerSP hijack_listener_sp(launch_info.GetHijackListener());
-  if (!hijack_listener_sp) {
-    hijack_listener_sp = Listener::MakeListener("lldb.Target.Launch.hijack");
-    launch_info.SetHijackListener(hijack_listener_sp);
-    m_process_sp->HijackProcessEvents(hijack_listener_sp);
   }
 
-  switch (m_process_sp->WaitForProcessToStop(llvm::None, nullptr, false,
-                                             hijack_listener_sp, nullptr)) {
+  state = m_process_sp->WaitForProcessToStop(llvm::None, nullptr, false,
+                                             launch_info.GetHijackListener());
+  m_process_sp->RestoreProcessEvents();
+
+  switch (state) {
   case eStateStopped: {
     if (launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
       break;
-    if (synchronous_execution) {
+    if (synchronous_execution)
       // Now we have handled the stop-from-attach, and we are just
       // switching to a synchronous resume.  So we should switch to the
       // SyncResume hijacker.
-      m_process_sp->RestoreProcessEvents();
       m_process_sp->ResumeSynchronous(stream);
-    } else {
-      m_process_sp->RestoreProcessEvents();
+    else
       error = m_process_sp->PrivateResume();
-    }
     if (!error.Success()) {
       Status error2;
       error2.SetErrorStringWithFormat(
           "process resume at entry point failed: %s", error.AsCString());
       error = error2;
     }
   } break;
   case eStateExited: {

lldb/test/API/tools/lldb-vscode/stop-hooks/Makefile

+C_SOURCES := main.c
+
+include Makefile.rules

lldb/test/API/tools/lldb-vscode/stop-hooks/TestVSCode_stop_hooks.py

+"""
+Test stop hooks
+"""
+
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+import lldbvscode_testcase
+
+
+class TestVSCode_stop_hooks(lldbvscode_testcase.VSCodeTestCaseBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipIfRemote
+    def test_stop_hooks_before_run(self):
+        '''
+            Test that there is no race condition between lldb-vscode and
+            stop hooks executor
+        '''
+        program = self.getBuildArtifact("a.out")
+        preRunCommands = ['target stop-hook add -o help']
+        self.build_and_launch(program, stopOnEntry=True, preRunCommands=preRunCommands)
+
+        # The first stop is on entry.
+        self.continue_to_next_stop()
+
+        breakpoint_ids = self.set_function_breakpoints(['main'])
+        # This request hangs if the race happens, because, in that case, the
+        # command interpreter is in synchronous mode while lldb-vscode expects
+        # it to be in asynchronous mode, so, the process doesn't send the stop
+        # event to "lldb.Debugger" listener (which is monitored by lldb-vscode).
+        self.continue_to_breakpoints(breakpoint_ids)
+
+        self.continue_to_exit()

lldb/test/API/tools/lldb-vscode/stop-hooks/main.c

+int main() { return 0; }