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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.



================
Comment at: lldb/source/Target/Process.cpp:2538-2539
         RestoreProcessEvents();
+        // Releasing the scope guard to avoid double restoring.
+        on_exit.release();
         SetPublicState(state, false);
----------------
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.


================
Comment at: lldb/source/Target/Target.cpp:202
                                              llvm::StringRef plugin_name,
                                              const FileSpec *crash_file,
                                              bool can_connect) {
----------------
Add an extra argument here.


================
Comment at: lldb/source/Target/Target.cpp:3044-3045
+    // they invoke Process::Launch.
+    m_hijacker_on_process_creation_sp =
+        Listener::MakeListener(hijacking_listener_name);
+    launch_info.SetHijackListener(m_hijacker_on_process_creation_sp);
----------------
Remove this. Make sure the listener in the launch_info is plumbed to the launch function.


================
Comment at: lldb/source/Target/Target.cpp:3069
     m_process_sp =
         GetPlatform()->DebugProcess(launch_info, debugger, *this, error);
 
----------------
Remove hijack listener creation from DebugProcess implementations (you can replace with an assert if you want).


================
Comment at: lldb/source/Target/Target.cpp:3106-3107
+    hijack_listener_sp = Listener::MakeListener(hijacking_listener_name);
     launch_info.SetHijackListener(hijack_listener_sp);
     m_process_sp->HijackProcessEvents(hijack_listener_sp);
   }
----------------
I guess we should just delete this now, as it is never correct to hijack events this late in the game.


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

https://reviews.llvm.org/D119548