Extract attach core logic from SBTarget::Attach* methods into unified SBTarget::AttachToProcess and make it work with platform for remote attach purposes.

[ovyalov]

Summary

SBTarget::Attach, SBTarget::AttachToProcessWithID and SBTarget::AttachToProcessWithName use the same core logic - extracted it into new static method SBTarget::AttachToProcess.
Made SBTarget::AttachToProcess to support remote attach if a platform is available and can debug a process - this makes TestHelloWorld to pass in case of remote platform on Linux (test is passing when test suite and platform are running on the same machine, fully distributed test run requires further investigation).

[clayborg]

I can't login to comment, I am home sick today, but I would rather not add anything extra to the SB header files. Can you move this static function into lldb_private::Target instead? We really should try to keep the SB layer free from any distractions.

[ovyalov]

Thanks for suggestion - added Target::Attach method and wired it with SBTarget::Attach* methods and CommandObjectProcessAttach.
Please take another look.

[ovyalov]

Friendly ping.

[clayborg]

Looks good.

[ovyalov]

Committed as http://reviews.llvm.org/rL228757

include/lldb/Target/Target.h

IsValid()
         return m_valid;
     }
 
     void
     Destroy();
     
     Error
     Launch (ProcessLaunchInfo &launch_info,
-            Stream *stream); // Optional stream to receive first stop info
+            Stream *stream);  // Optional stream to receive first stop info
 
+    Error
+    Attach (ProcessAttachInfo &attach_info,
+            Stream *stream);  // Optional stream to receive first stop info
+
     //------------------------------------------------------------------
     // This part handles the breakpoints.
     //------------------------------------------------------------------
 
     BreakpointList &
     GetBreakpointList(bool internal = false);
 
     const BreakpointList &

source/API/SBTarget.cpp

 }
 
 void
 SBAttachInfo::SetListener (SBListener &listener)
 {
     m_opaque_sp->SetListener(listener.GetSP());
 }
 
+namespace {
+
+Error
+AttachToProcess (ProcessAttachInfo &attach_info, Target &target)
+{
+    Mutex::Locker api_locker (target.GetAPIMutex ());
+
+    auto process_sp = target.GetProcessSP ();
+    if (process_sp)
+    {
+        const auto state = process_sp->GetState ();
+        if (process_sp->IsAlive () && state == eStateConnected)
+        {
+            // If we are already connected, then we have already specified the
+            // listener, so if a valid listener is supplied, we need to error out
+            // to let the client know.
+            if (attach_info.GetListener ())
+                return Error ("process is connected and already has a listener, pass empty listener");
+        }
+    }
+
+    return target.Attach (attach_info, nullptr);
+}
+
+}  // namespace
+
 //----------------------------------------------------------------------
 // SBTarget constructor
 //----------------------------------------------------------------------
 SBTarget::SBTarget () :
     m_opaque_sp ()
 {
 }
 

 }
 
 lldb::SBProcess
 SBTarget::Attach (SBAttachInfo &sb_attach_info, SBError& error)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_API));
 
     SBProcess sb_process;
-    ProcessSP process_sp;
     TargetSP target_sp(GetSP());
 
     if (log)
         log->Printf ("SBTarget(%p)::Attach (sb_attach_info, error)...",
                      static_cast<void*>(target_sp.get()));
 
     if (target_sp)
     {
-        Mutex::Locker api_locker (target_sp->GetAPIMutex());
-
-        StateType state = eStateInvalid;
-        process_sp = target_sp->GetProcessSP();
-        if (process_sp)
-        {
-            state = process_sp->GetState();
-
-            if (process_sp->IsAlive() && state != eStateConnected)
-            {
-                if (state == eStateAttaching)
-                    error.SetErrorString ("process attach is in progress");
-                else
-                    error.SetErrorString ("a process is already being debugged");
-                if (log)
-                    log->Printf ("SBTarget(%p)::Attach (...) => error %s",
-                                 static_cast<void*>(target_sp.get()),
-                                 error.GetCString());
-                return sb_process;
-            }
-        }
-
-        if (state != eStateConnected)
-            process_sp = target_sp->CreateProcess (target_sp->GetDebugger().GetListener(), NULL, NULL);
-
-        if (process_sp)
-        {
-            ProcessAttachInfo &attach_info = sb_attach_info.ref();
-            if (attach_info.ProcessIDIsValid() && !attach_info.UserIDIsValid())
-            {
-                PlatformSP platform_sp = target_sp->GetPlatform();
-                // See if we can pre-verify if a process exists or not
-                if (platform_sp && platform_sp->IsConnected())
-                {
-                    lldb::pid_t attach_pid = attach_info.GetProcessID();
-                    ProcessInstanceInfo instance_info;
-                    if (platform_sp->GetProcessInfo(attach_pid, instance_info))
-                    {
-                        attach_info.SetUserID(instance_info.GetEffectiveUserID());
-                    }
-                    else
-                    {
-                        error.ref().SetErrorStringWithFormat("no process found with process ID %" PRIu64, attach_pid);
-                        if (log)
-                        {
-                            log->Printf ("SBTarget(%p)::Attach (...) => error %s",
-                                         static_cast<void*>(target_sp.get()), error.GetCString());
-                        }
-                        return sb_process;
-                    }
-                }
-            }
-            error.SetError (process_sp->Attach (attach_info));
-            if (error.Success())
-            {
-                sb_process.SetSP (process_sp);
-                // If we are doing synchronous mode, then wait for the
-                // process to stop!
-                if (target_sp->GetDebugger().GetAsyncExecution () == false)
-                    process_sp->WaitForProcessToStop (NULL);
-            }
-        }
-        else
-        {
-            error.SetErrorString ("unable to create lldb_private::Process");
-        }
+        error.SetError(AttachToProcess(attach_info, *target_sp));
+            sb_process.SetSP(target_sp->GetProcessSP());
     }
     else
     {
         error.SetErrorString ("SBTarget is invalid");
     }
 
     if (log)
         log->Printf ("SBTarget(%p)::Attach (...) => SBProcess(%p)",
                      static_cast<void*>(target_sp.get()),
-                     static_cast<void*>(process_sp.get()));
+                     static_cast<void*>(sb_process.GetSP().get()));
 
     return sb_process;
 }
 
 
 #if defined(__APPLE__)
 
 lldb::SBProcess
(
     SBListener &listener, 
     lldb::pid_t pid,// The process ID to attach to
     SBError& error  // An error explaining what went wrong if attach fails
 )
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_API));
 
     SBProcess sb_process;
-    ProcessSP process_sp;
     TargetSP target_sp(GetSP());
 
     if (log)
-        log->Printf ("SBTarget(%p)::AttachToProcessWithID (listener, pid=%" PRId64 ", error)...",
-                     static_cast<void*>(target_sp.get()), pid);
+        log->Printf ("SBTarget(%p)::%s (listener, pid=%" PRId64 ", error)...",
+                     static_cast<void*>(target_sp.get()),
+                     __FUNCTION__,
+                     pid);
 
     if (target_sp)
     {
-        Mutex::Locker api_locker (target_sp->GetAPIMutex());
-
-        StateType state = eStateInvalid;
-        process_sp = target_sp->GetProcessSP();
-        if (process_sp)
-        {
-            state = process_sp->GetState();
-
-            if (process_sp->IsAlive() && state != eStateConnected)
-            {
-                if (state == eStateAttaching)
-                    error.SetErrorString ("process attach is in progress");
-                else
-                    error.SetErrorString ("a process is already being debugged");
-                return sb_process;
-            }
-        }
-
-        if (state == eStateConnected)
-        {
-            // If we are already connected, then we have already specified the
-            // listener, so if a valid listener is supplied, we need to error out
-            // to let the client know.
-            if (listener.IsValid())
-            {
-                error.SetErrorString ("process is connected and already has a listener, pass empty listener");
-                return sb_process;
-            }
-        }
-        else
-        {
-            if (listener.IsValid())
-                process_sp = target_sp->CreateProcess (listener.ref(), NULL, NULL);
-            else
-                process_sp = target_sp->CreateProcess (target_sp->GetDebugger().GetListener(), NULL, NULL);
-        }
-        if (process_sp)
-        {
-            sb_process.SetSP (process_sp);
-
-            ProcessAttachInfo attach_info;
-            attach_info.SetProcessID (pid);
+        if (listener.IsValid())
+            attach_info.SetListener(listener.GetSP());
 
-            PlatformSP platform_sp = target_sp->GetPlatform();
-            ProcessInstanceInfo instance_info;
-            if (platform_sp->GetProcessInfo(pid, instance_info))
-            {
-                attach_info.SetUserID(instance_info.GetEffectiveUserID());
-            }
-            error.SetError (process_sp->Attach (attach_info));            
-            if (error.Success())
-            {
-                // If we are doing synchronous mode, then wait for the
-                // process to stop!
-                if (target_sp->GetDebugger().GetAsyncExecution () == false)
-                    process_sp->WaitForProcessToStop (NULL);
-            }
-        }
-        else
-        {
-            error.SetErrorString ("unable to create lldb_private::Process");
-        }
+        if (target_sp->GetPlatform ()->GetProcessInfo (pid, instance_info))
+            attach_info.SetUserID (instance_info.GetEffectiveUserID ());
+
+        error.SetError (AttachToProcess (attach_info, *target_sp));
+        if (error.Success ())
+            sb_process.SetSP (target_sp->GetProcessSP ());
     }
     else
-    {
         error.SetErrorString ("SBTarget is invalid");
-    }
 
     if (log)
-        log->Printf ("SBTarget(%p)::AttachToProcessWithID (...) => SBProcess(%p)",
-                     static_cast<void*>(target_sp.get()),
-                     static_cast<void*>(process_sp.get()));
+        log->Printf ("SBTarget(%p)::%s (...) => SBProcess(%p)",
+                     static_cast<void*>(target_sp.get ()),
+                     __FUNCTION__,
+                     static_cast<void*>(sb_process.GetSP().get ()));
     return sb_process;
 }
 
 lldb::SBProcess
 SBTarget::AttachToProcessWithName 
 (
     SBListener &listener, 
     const char *name,   // basename of process to attach to
     bool wait_for,      // if true wait for a new instance of "name" to be launched
     SBError& error      // An error explaining what went wrong if attach fails
 )
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_API));
 
     SBProcess sb_process;
-    ProcessSP process_sp;
     TargetSP target_sp(GetSP());
 
     if (log)
-        log->Printf ("SBTarget(%p)::AttachToProcessWithName (listener, name=%s, wait_for=%s, error)...",
-                     static_cast<void*>(target_sp.get()), name,
+        log->Printf ("SBTarget(%p)::%s (listener, name=%s, wait_for=%s, error)...",
+                     static_cast<void*>(target_sp.get()),
+                     __FUNCTION__,
+                     name,
                      wait_for ? "true" : "false");
 
     if (name && target_sp)
     {
-        Mutex::Locker api_locker (target_sp->GetAPIMutex());
-
-        StateType state = eStateInvalid;
-        process_sp = target_sp->GetProcessSP();
-        if (process_sp)
-        {
-            state = process_sp->GetState();
-
-            if (process_sp->IsAlive() && state != eStateConnected)
-            {
-                if (state == eStateAttaching)
-                    error.SetErrorString ("process attach is in progress");
-                else
-                    error.SetErrorString ("a process is already being debugged");
-                return sb_process;
-            }
-        }
-
-        if (state == eStateConnected)
-        {
-            // If we are already connected, then we have already specified the
-            // listener, so if a valid listener is supplied, we need to error out
-            // to let the client know.
-            if (listener.IsValid())
-            {
-                error.SetErrorString ("process is connected and already has a listener, pass empty listener");
-                return sb_process;
-            }
-        }
-        else
-        {
-            if (listener.IsValid())
-                process_sp = target_sp->CreateProcess (listener.ref(), NULL, NULL);
-            else
-                process_sp = target_sp->CreateProcess (target_sp->GetDebugger().GetListener(), NULL, NULL);
-        }
-
-        if (process_sp)
-        {
-            sb_process.SetSP (process_sp);
-            ProcessAttachInfo attach_info;
-            attach_info.GetExecutableFile().SetFile(name, false);
-            attach_info.SetWaitForLaunch(wait_for);
-            error.SetError (process_sp->Attach (attach_info));
-            if (error.Success())
-            {
-                // If we are doing synchronous mode, then wait for the
-                // process to stop!
-                if (target_sp->GetDebugger().GetAsyncExecution () == false)
-                    process_sp->WaitForProcessToStop (NULL);
-            }
-        }
-        else
-        {
-            error.SetErrorString ("unable to create lldb_private::Process");
-        }
+        if (listener.IsValid())
+            attach_info.SetListener(listener.GetSP());
+
+        error.SetError (AttachToProcess (attach_info, *target_sp));
+        if (error.Success ())
+            sb_process.SetSP (target_sp->GetProcessSP ());
     }
     else
-    {
         error.SetErrorString ("SBTarget is invalid");
-    }
 
     if (log)
-        log->Printf ("SBTarget(%p)::AttachToPorcessWithName (...) => SBProcess(%p)",
+        log->Printf ("SBTarget(%p)::%s (...) => SBProcess(%p)",
                      static_cast<void*>(target_sp.get()),
-                     static_cast<void*>(process_sp.get()));
+                     __FUNCTION__,
+                     static_cast<void*>(sb_process.GetSP().get()));
     return sb_process;
 }
 
 lldb::SBProcess
 SBTarget::ConnectRemote
 (
     SBListener &listener,
     const char *url,
if (target_sp)
             abi_sp = process_sp->GetABI();
         else
             abi_sp = ABI::FindPlugin(target_sp->GetArchitecture());
         if (abi_sp)
             return abi_sp->GetRedZoneSize();
     }
     return 0;
 }
-

source/Commands/CommandObjectProcess.cpp

DoExecute (Args& command,
         }
         
         // Record the old executable module, we want to issue a warning if the process of attaching changed the
         // current executable (like somebody said "file foo" then attached to a PID whose executable was bar.)
          
         ModuleSP old_exec_module_sp = target->GetExecutableModule();
         ArchSpec old_arch_spec = target->GetArchitecture();
 
-        ProcessSP process_sp;
-        Error error;
         if (command.GetArgumentCount())
         {
             result.AppendErrorWithFormat("Invalid arguments for '%s'.\nUsage: %s\n", m_cmd_name.c_str(), m_cmd_syntax.c_str());
             result.SetStatus (eReturnStatusFailed);
             return false;
         }
 
         m_interpreter.UpdateExecutionContext(nullptr);
-        ListenerSP listener_sp (new Listener("lldb.CommandObjectProcessAttach.DoExecute.attach.hijack"));
-        m_options.attach_info.SetHijackListener(listener_sp);
-
-        // If no process info was specified, then use the target executable
-        // name as the process to attach to by default
-        if (!m_options.attach_info.ProcessInfoSpecified ())
-        {
-            if (old_exec_module_sp)
-                m_options.attach_info.GetExecutableFile().GetFilename() = old_exec_module_sp->GetPlatformFileSpec().GetFilename();
-
-            if (!m_options.attach_info.ProcessInfoSpecified ())
-            {
-                error.SetErrorString ("no process specified, create a target with a file, or specify the --pid or --name command option");
-            }
-        }
-
+        StreamString stream;
+        const auto error = target->Attach(m_options.attach_info, &stream);
         if (error.Success())
         {
-            if (state != eStateConnected && platform_sp != nullptr && platform_sp->CanDebugProcess())
-            {
-                target->SetPlatform(platform_sp);
-                process = platform_sp->Attach(m_options.attach_info, m_interpreter.GetDebugger(), target, error).get();
-            }
-            else
-            {
-                if (state != eStateConnected)
-                {
-                    const char *plugin_name = m_options.attach_info.GetProcessPluginName();
-                    process = target->CreateProcess (m_interpreter.GetDebugger().GetListener(), plugin_name, nullptr).get();
-                    if (process == nullptr)
-                        error.SetErrorStringWithFormat("failed to create process using plugin %s", plugin_name);
-                }
-                if (process)
-                {
-                    process->HijackProcessEvents(listener_sp.get());
-                    error = process->Attach(m_options.attach_info);
-                }
-            }
-        }
-
-        if (error.Success() && process != nullptr)
+            ProcessSP process_sp (target->GetProcessSP());
+            if (process_sp)
             {
-            result.SetStatus (eReturnStatusSuccessContinuingNoResult);
-            StreamString stream;
-            StateType state = process->WaitForProcessToStop (nullptr, nullptr, false, listener_sp.get(), &stream);
-
-            process->RestoreProcessEvents();
-            result.SetDidChangeProcessState (true);
-
                 if (stream.GetData())
                     result.AppendMessage(stream.GetData());
-
-            if (state == eStateStopped)
-            {
                 result.SetStatus (eReturnStatusSuccessFinishNoResult);
+                result.SetDidChangeProcessState (true);
             }
             else
             {
-                const char *exit_desc = process->GetExitDescription();
-                if (exit_desc)
-                    result.AppendErrorWithFormat ("attach failed: %s", exit_desc);
-                else
-                    result.AppendError ("attach failed: process did not stop (no such process or permission problem?)");
-                process->Destroy();
+                result.AppendError("no error returned from Target::Attach, and target has no process");
                 result.SetStatus (eReturnStatusFailed);
             }
         }
         else
         {
             result.AppendErrorWithFormat ("attach failed: %s\n", error.AsCString());
             result.SetStatus (eReturnStatusFailed);
         }

source/Target/Target.cpp

#define LAUNCH_SHELL_MESSAGE "\n'r' and 'run' are aliases that default to launching through a shell.\nTry launching without going through a shell by using 'process launch'."
     else
     {
         Error error2;
         error2.SetErrorStringWithFormat ("process launch failed: %s", error.AsCString());
         error = error2;
     }
     return error;
 }
+
+Error
+Target::Attach (ProcessAttachInfo &attach_info, Stream *stream)
+{
+    auto state = eStateInvalid;
+    auto process_sp = GetProcessSP ();
+    if (process_sp)
+    {
+        state = process_sp->GetState ();
+        if (process_sp->IsAlive () && state != eStateConnected)
+        {
+            if (state == eStateAttaching)
+                return Error ("process attach is in progress");
+            return Error ("a process is already being debugged");
+        }
+    }
+
+    ListenerSP hijack_listener_sp (new Listener ("lldb.Target.Attach.attach.hijack"));
+    attach_info.SetHijackListener (hijack_listener_sp);
+
+    const ModuleSP old_exec_module_sp = GetExecutableModule ();
+
+    // If no process info was specified, then use the target executable
+    // name as the process to attach to by default
+    if (!attach_info.ProcessInfoSpecified ())
+    {
+        if (old_exec_module_sp)
+            attach_info.GetExecutableFile ().GetFilename () = old_exec_module_sp->GetPlatformFileSpec ().GetFilename ();
+
+        if (!attach_info.ProcessInfoSpecified ())
+        {
+            return Error ("no process specified, create a target with a file, or specify the --pid or --name");
+        }
+    }
+
+    const auto platform_sp = GetDebugger ().GetPlatformList ().GetSelectedPlatform ();
+
+    Error error;
+    if (state != eStateConnected && platform_sp != nullptr && platform_sp->CanDebugProcess ())
+    {
+        SetPlatform (platform_sp);
+        process_sp = platform_sp->Attach (attach_info, GetDebugger (), this, error);
+    }
+    else
+    {
+        if (state != eStateConnected)
+        {
+            const char *plugin_name = attach_info.GetProcessPluginName ();
+            process_sp = CreateProcess (attach_info.GetListenerForProcess (GetDebugger ()), plugin_name, nullptr);
+            if (process_sp == nullptr)
+            {
+                error.SetErrorStringWithFormat ("failed to create process using plugin %s", (plugin_name) ? plugin_name : "null");
+                return error;
+            }
+        }
+        process_sp->HijackProcessEvents (hijack_listener_sp.get ());
+        error = process_sp->Attach (attach_info);
+    }
+
+    if (error.Success () && process_sp)
+    {
+        state = process_sp->WaitForProcessToStop (nullptr, nullptr, false, attach_info.GetHijackListener ().get (), stream);
+        process_sp->RestoreProcessEvents ();
+
+        if (state != eStateStopped)
+        {
+            const char *exit_desc = process_sp->GetExitDescription ();
+            if (exit_desc)
+                error.SetErrorStringWithFormat ("attach failed: %s", exit_desc);
+            else
+                error.SetErrorString ("attach failed: process did not stop (no such process or permission problem?)");
+            process_sp->Destroy ();
+        }
+    }
+    return error;
+}
+
 //--------------------------------------------------------------
 // Target::StopHook
 //--------------------------------------------------------------
 Target::StopHook::StopHook (lldb::TargetSP target_sp, lldb::user_id_t uid) :
         UserID (uid),
         m_target_sp (target_sp),
         m_commands (),
         m_specifier_sp (),