Fix -gdb-exit to detach if was attached or destroy otherwise (MI)

[ki.stfu]

Summary

This patch fixes -gdb-exit for locally target. It includes the following changes:

1. Fix Process::Finalize
2. Use SBProcess::Destroy in -gdb-exit

[clayborg]

We should not add SBProcess::GetShouldDetach() and we should fix the Process::DoDestroy() functions to all look at the Process::GetShouldDetach() and each instance should do the right thing (detach or kill) based on that setting. This isn't something we should pass out of the API. See inlined comments above.

include/lldb/API/SBProcess.h

326–327 ↗

(On Diff #21845)

Remove this.

source/API/SBProcess.cpp

1404–1412 ↗

(On Diff #21845)

Remove this.

tools/lldb-mi/MICmdCmdMiscellanous.cpp

77–78

Remove this and always call sbProcess.Destroy(). We will need to modify the internal plug-ins to "do the right thing" when destroy is called.

[ki.stfu]

tools/lldb-mi/MICmdCmdMiscellanous.cpp

77–78

Please, look at Process::Finalize:

823 void
824 Process::Finalize()
825 {
826     switch (GetPrivateState())
827     {
828         case eStateConnected:
829         case eStateAttaching:
830         case eStateLaunching:
831         case eStateStopped:
832         case eStateRunning:
833         case eStateStepping:
834         case eStateCrashed:
835         case eStateSuspended:
836             if (GetShouldDetach())
837             {
838                 // FIXME: This will have to be a process setting:
839                 bool keep_stopped = false;
840                 Detach(keep_stopped);
841             }
842             else
843                 Destroy();
844             break;
845 
846         case eStateInvalid:
847         case eStateUnloaded:
848         case eStateDetached:
849         case eStateExited:
850             break;
851     }

Seems that Destroy() should be called if not GetShouldDetach(). Right?

[clayborg]

Process::Finalize() only gets called when the lldb_private::Process destructor is called. We should do similar logic in lldb_private::Process::Destroy(). Currently Process::Destroy() calls the virtual WillDestrory(), DoDestroy() and DidDestroy(), but we should probably fix Process::Destroy() to do more generic logic and check the GetShouldDetach() and call Process::Detach() first before calling the subclass DoDestroy()...

[ki.stfu]

In D8298#139981, @clayborg wrote:

Process::Finalize() only gets called when the lldb_private::Process destructor is called. We should do similar logic in lldb_private::Process::Destroy(). Currently Process::Destroy() calls the virtual WillDestrory(), DoDestroy() and DidDestroy(), but we should probably fix Process::Destroy() to do more generic logic and check the GetShouldDetach() and call Process::Detach() first before calling the subclass DoDestroy()...

What is the difference between Process::Detach/Halt/Destroy?

[ki.stfu]

Call Process::Detach from Process::Destroy if needed

I'm not sure about this patch and I think it isn't ready.

As I understood Detach() detaches without cleanup, Halt() kills a target without cleanup, but Destroy() detaches or kills a target (according to GetShouldDetach()) and performs a cleanup after that. Therefore I think Destroy should be like following:

Destroy()
{
    if (GetShouldDetach())
        Detach();
    else
        Halt();

    CleanupObject();
}

[clayborg]

Remove the switch statement as mentioned in inline comments and we are good to go.

source/Target/Process.cpp

824–826

This switch is no longer needed, just replace with:

Destroy();

[ki.stfu]

source/Target/Process.cpp

824–826

I thought it's a smart optimization to avoid superfluous actions when target was Unloaded/Detached/Exited. Why you wanna remove it?

[ki.stfu]

Remove switch() in Process::Finalize

[clayborg]

Looks good.

[ki.stfu]

Fix a comment

[ki.stfu]

Hello @clayborg,

I added the switch() back in Process::Finalize() in r233258, because it caused an error:

$ bin/lldb
(lldb) target create ~/p/hello
Current executable set to '~/p/hello' (x86_64).
(lldb) r
Assertion failed: (!"bad_weak_ptr"), function shared_ptr, file /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory, line 5174.
Abort trap: 6

backtrace:

$ lldb bin/lldb -- ~/p/hello
(lldb) target create "bin/lldb"
Current executable set to 'bin/lldb' (x86_64).
(lldb) settings set -- target.run-args  "/Users/IliaK/p/hello"
(lldb) r
Process 50068 launched: '/Users/IliaK/p/llvm/build_ninja/bin/lldb' (x86_64)
Process 50068 stopped
* thread #1: tid = 0xbfd4c, 0x00007fff5fc0d28e, stop reason = signal SIGSTOP
    frame #0: 0x00007fff5fc0d28e
-> 0x7fff5fc0d28e:  pushq  %rbp
   0x7fff5fc0d28f:  movq   %rsp, %rbp
   0x7fff5fc0d292:  popq   %rbp
   0x7fff5fc0d293:  retq   
(lldb) c
Process 50068 resuming
(lldb) target create "/Users/IliaK/p/hello"
Current executable set to '/Users/IliaK/p/hello' (x86_64).
(lldb) r
Assertion failed: (!"bad_weak_ptr"), function shared_ptr, file /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory, line 5174.
Process 50068 stopped
* thread #1: tid = 0xbfd4c, 0x00007fff9b42f286 libsystem_kernel.dylib`__pthread_kill + 10, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
    frame #0: 0x00007fff9b42f286 libsystem_kernel.dylib`__pthread_kill + 10
libsystem_kernel.dylib`__pthread_kill + 10:
-> 0x7fff9b42f286:  jae    0x7fff9b42f290            ; __pthread_kill + 20
   0x7fff9b42f288:  movq   %rax, %rdi
   0x7fff9b42f28b:  jmp    0x7fff9b42ac53            ; cerror_nocancel
   0x7fff9b42f290:  retq   
(lldb) bt
* thread #1: tid = 0xbfd4c, 0x00007fff9b42f286 libsystem_kernel.dylib`__pthread_kill + 10, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x00007fff9b42f286 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff8f28c42f libsystem_pthread.dylib`pthread_kill + 90
    frame #2: 0x00000001014f3d9b liblldb.3.7.0.dylib`raise(sig=6) + 27 at Signals.inc:524
    frame #3: 0x00000001014f3e52 liblldb.3.7.0.dylib`abort + 18 at Signals.inc:541
    frame #4: 0x00000001014f3e31 liblldb.3.7.0.dylib`__assert_rtn(func=0x0000000103c4e976, file=0x0000000103c4e981, line=5174, expr=0x0000000103c4e9f1) + 129 at Signals.inc:537
    frame #5: 0x000000010002d3c3 liblldb.3.7.0.dylib`std::__1::shared_ptr<lldb_private::Process>::shared_ptr<lldb_private::Process>(this=0x00007fff5fbf9ec8, __r=0x0000000109293220, (null)=(__for_bool_ = 0)) + 163 at memory:5174
    frame #6: 0x000000010002d313 liblldb.3.7.0.dylib`std::__1::shared_ptr<lldb_private::Process>::shared_ptr<lldb_private::Process>(this=0x00007fff5fbf9ec8, __r=0x0000000109293220, (null)=(__for_bool_ = 0)) + 35 at memory:5176
    frame #7: 0x0000000101bd3121 liblldb.3.7.0.dylib`lldb_private::Process::SetPrivateState(lldb::StateType) [inlined] std::__1::enable_shared_from_this<lldb_private::Process>::shared_from_this(this=0x0000000109293220) + 51 at memory:5240
    frame #8: 0x0000000101bd30ee liblldb.3.7.0.dylib`lldb_private::Process::SetPrivateState(this=0x0000000109293200, new_state=eStateDetached) + 862 at Process.cpp:1785
    frame #9: 0x0000000101fe7266 liblldb.3.7.0.dylib`ProcessKDP::DoDetach(this=0x0000000109293200, keep_stopped=false) + 342 at ProcessKDP.cpp:663
    frame #10: 0x0000000101fe7390 liblldb.3.7.0.dylib`ProcessKDP::DoDestroy(this=0x0000000109293200) + 48 at ProcessKDP.cpp:675
    frame #11: 0x0000000101bce7a3 liblldb.3.7.0.dylib`lldb_private::Process::Destroy(this=0x0000000109293200) + 419 at Process.cpp:3959
    frame #12: 0x0000000101bcdb86 liblldb.3.7.0.dylib`lldb_private::Process::Finalize(this=0x0000000109293200) + 54 at Process.cpp:826
    frame #13: 0x0000000101fe4e3a liblldb.3.7.0.dylib`ProcessKDP::~ProcessKDP(this=0x0000000109293200) + 106 at ProcessKDP.cpp:203
    frame #14: 0x0000000101fe4f05 liblldb.3.7.0.dylib`ProcessKDP::~ProcessKDP(this=0x0000000109293200) + 21 at ProcessKDP.cpp:197
    frame #15: 0x0000000101fe4fb9 liblldb.3.7.0.dylib`ProcessKDP::~ProcessKDP(this=0x0000000109293200) + 25 at ProcessKDP.cpp:197
    frame #16: 0x0000000101feaa0c liblldb.3.7.0.dylib`std::__1::__shared_ptr_pointer<ProcessKDP*, std::__1::default_delete<ProcessKDP>, std::__1::allocator<ProcessKDP> >::__on_zero_shared() [inlined] std::__1::default_delete<ProcessKDP>::operator(this=0x000000010aa626a8, this=0x000000010aa626a8, this=0x000000010aa626a8, this=0x000000010aa626a8, this=0x000000010aa626a8, __ptr=0x0000000109293200)(ProcessKDP*) const + 156 at memory:2426
    frame #17: 0x0000000101fea9e1 liblldb.3.7.0.dylib`std::__1::__shared_ptr_pointer<ProcessKDP*, std::__1::default_delete<ProcessKDP>, std::__1::allocator<ProcessKDP> >::__on_zero_shared(this=0x000000010aa62690) + 113 at memory:3669
    frame #18: 0x00007fff9797e8a6 libc++.1.dylib`std::__1::__shared_weak_count::__release_shared() + 44
    frame #19: 0x000000010002d2df liblldb.3.7.0.dylib`std::__1::shared_ptr<lldb_private::Process>::~shared_ptr(this=0x00007fff5fbfa948) + 47 at memory:4448
    frame #20: 0x000000010002bf15 liblldb.3.7.0.dylib`std::__1::shared_ptr<lldb_private::Process>::~shared_ptr(this=0x00007fff5fbfa948) + 21 at memory:4446
    frame #21: 0x0000000101bcb7fa liblldb.3.7.0.dylib`lldb_private::Process::FindPlugin(lldb_private::Target&, char const*, lldb_private::Listener&, lldb_private::FileSpec const*) [inlined] std::__1::shared_ptr<lldb_private::Process>::reset(this=0x00007fff5fbfab88) + 2378 at memory:4577
    frame #22: 0x0000000101bcb6de liblldb.3.7.0.dylib`lldb_private::Process::FindPlugin(target=0x0000000109820e00, plugin_name=0x0000000000000000, listener=0x000000010903d278, crash_file_path=0x0000000000000000) + 2094 at Process.cpp:679
    frame #23: 0x0000000101c3b56e liblldb.3.7.0.dylib`lldb_private::Target::CreateProcess(this=0x0000000109820e00, listener=0x000000010903d278, plugin_name=0x0000000000000000, crash_file=0x0000000000000000) + 142 at Target.cpp:206
    frame #24: 0x0000000101e8473d liblldb.3.7.0.dylib`PlatformPOSIX::Attach(this=0x000000010881b640, attach_info=0x00007fff5fbfb270, debugger=0x000000010903d000, target=0x0000000109820e00, error=0x00007fff5fbfbd68) + 1325 at PlatformPOSIX.cpp:785
    frame #25: 0x0000000101bbc204 liblldb.3.7.0.dylib`lldb_private::Platform::DebugProcess(this=0x000000010881b640, launch_info=0x00000001088abc28, debugger=0x000000010903d000, target=0x0000000109820e00, error=0x00007fff5fbfbd68) + 644 at Platform.cpp:1289
    frame #26: 0x0000000101e84fe4 liblldb.3.7.0.dylib`PlatformPOSIX::DebugProcess(this=0x000000010881b640, launch_info=0x00000001088abc28, debugger=0x000000010903d000, target=0x0000000109820e00, error=0x00007fff5fbfbd68) + 244 at PlatformPOSIX.cpp:827
    frame #27: 0x0000000101c4bae1 liblldb.3.7.0.dylib`lldb_private::Target::Launch(this=0x0000000109820e00, launch_info=0x00000001088abc28, stream=0x00007fff5fbfbd88) + 1953 at Target.cpp:2560
    frame #28: 0x00000001015ba7dd liblldb.3.7.0.dylib`CommandObjectProcessLaunch::DoExecute(this=0x00000001088abab0, launch_args=0x00007fff5fbfc038, result=0x00007fff5fbfed20) + 1357 at CommandObjectProcess.cpp:263
    frame #29: 0x0000000101a06e99 liblldb.3.7.0.dylib`lldb_private::CommandObjectParsed::Execute(this=0x00000001088abab0, args_string=0x00007fff5fbfc891, result=0x00007fff5fbfed20) + 537 at CommandObject.cpp:1081
    frame #30: 0x00000001019dd82e liblldb.3.7.0.dylib`lldb_private::CommandInterpreter::HandleCommand(this=0x00000001088a11e0, command_line=0x00007fff5fbfeec9, lazy_add_to_history=eLazyBoolCalculate, result=0x00007fff5fbfed20, override_context=0x0000000000000000, repeat_on_empty_command=true, no_context_switching=false) + 23358 at CommandInterpreter.cpp:1966
    frame #31: 0x00000001019e7409 liblldb.3.7.0.dylib`lldb_private::CommandInterpreter::IOHandlerInputComplete(this=0x00000001088a11e0, io_handler=0x0000000108d03370, line=0x00007fff5fbfeec8) + 1161 at CommandInterpreter.cpp:3151
    frame #32: 0x00000001019e7ac7 liblldb.3.7.0.dylib`non-virtual thunk to lldb_private::CommandInterpreter::IOHandlerInputComplete(this=0x00000001088a12b0, io_handler=0x0000000108d03370, line=0x00007fff5fbfeec8) + 55 at CommandInterpreter.cpp:3230
    frame #33: 0x00000001017d8a67 liblldb.3.7.0.dylib`lldb_private::IOHandlerEditline::Run(this=0x0000000108d03370) + 1575 at IOHandler.cpp:729
    frame #34: 0x000000010179266e liblldb.3.7.0.dylib`lldb_private::Debugger::ExecuteIOHandlers(this=0x000000010903d000) + 142 at Debugger.cpp:920
    frame #35: 0x00000001019e8567 liblldb.3.7.0.dylib`lldb_private::CommandInterpreter::RunCommandInterpreter(this=0x00000001088a11e0, auto_handle_events=true, spawn_thread=false, options=0x00007fff5fbff210) + 199 at CommandInterpreter.cpp:3388
    frame #36: 0x0000000100048809 liblldb.3.7.0.dylib`lldb::SBDebugger::RunCommandInterpreter(this=0x00007fff5fbff9f8, auto_handle_events=true, spawn_thread=false) + 137 at SBDebugger.cpp:975
    frame #37: 0x0000000100008abe lldb`Driver::MainLoop(this=0x00007fff5fbff9d8) + 5502 at Driver.cpp:1152
    frame #38: 0x0000000100009077 lldb`main(argc=2, argv=0x00007fff5fbffb20, envp=0x00007fff5fbffb38) + 359 at Driver.cpp:1252
    frame #39: 0x00007fff94f3b5c9 libdyld.dylib`start + 1
(lldb) quit
Quitting LLDB will kill one or more processes. Do you really want to proceed: [Y/n] y

Thanks,
Ilia

[clayborg]

Ah yes, do leave the switch statement in then...

source/Target/Process.cpp

Process::GetGlobalProperties()
     static ProcessPropertiesSP g_settings_sp;
     if (!g_settings_sp)
         g_settings_sp.reset (new ProcessProperties (NULL));
     return g_settings_sp;
 }
 
 void
 Process::Finalize()
 {
-    switch (GetPrivateState())
-    {
-        case eStateConnected:
-        case eStateAttaching:
-        case eStateLaunching:
-        case eStateStopped:
-        case eStateRunning:
-        case eStateStepping:
-        case eStateCrashed:
-        case eStateSuspended:
-            if (GetShouldDetach())
-            {
-                // FIXME: This will have to be a process setting:
-                bool keep_stopped = false;
-                Detach(keep_stopped);
-            }
-            else
-                Destroy();
-            break;
-            
-        case eStateInvalid:
-        case eStateUnloaded:
-        case eStateDetached:
-        case eStateExited:
-            break;
-    }
+    // Destroy this process
 
     // Clear our broadcaster before we proceed with destroying
     Broadcaster::Clear();
 
     // Do any cleanup needed prior to being destructed... Subclasses
     // that override this method should call this superclass method as well.
     
     // We need to destroy the loader before the derived Process class gets destroyed

 Error
 Process::Destroy ()
 {
     
     // Tell ourselves we are in the process of destroying the process, so that we don't do any unnecessary work
     // that might hinder the destruction.  Remember to set this back to false when we are done.  That way if the attempt
     // failed and the process stays around for some reason it won't be in a confused state.
     
+    if (GetShouldDetach())
+    {
+        // FIXME: This will have to be a process setting:
+        bool keep_stopped = false;
+        Detach(keep_stopped);
+    }
+
     m_destroy_in_process = true;
 
     Error error (WillDestroy());
     if (error.Success())
     {
         EventSP exit_event_sp;
         if (DestroyRequiresHalt())
         {

tools/lldb-mi/MICmdCmdMiscellanous.cpp

 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmdCmdGdbExit::Execute(void)
 {
     CMICmnLLDBDebugger::Instance().GetDriver().SetExitApplicationFlag(true);
-    const lldb::SBError sbErr = m_rLLDBDebugSessionInfo.GetProcess().Detach();
+    const lldb::SBError sbErr = m_rLLDBDebugSessionInfo.GetProcess().Destroy();
     // Do not check for sbErr.Fail() here, m_lldbProcess is likely !IsValid()
 
     return MIstatus::success;
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: The invoker requires this function. The command prepares a MI Record Result
 //          for the work carried out in the Execute().
 // Type:    Overridden.