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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Feb 16 05:17:32 PST 2022

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
+        // 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);
I guess we should just delete this now, as it is never correct to hijack events this late in the game.



More information about the lldb-commits mailing list