[lldb] Unify default/hijack listener between Process{Attach,Launch}Info (NFC)

[mib]

Summary

This patch is a simple refactor that unifies the default and hijack
listener methods and attributes between ProcessAttachInfo and
ProcessLaunchInfo.

These 2 classes are both derived from the ProcessInfo base class so this
patch moves the listeners attributes and getter/setter methods to the
base class.

Signed-off-by: Med Ismail Bennani <[e-mail]>

[bulbazord]

Creating a ProcessAttachInfo from a ProcessLaunchInfo with this change means they'll have different listeners. Is that ProcessLaunchInfo kept around for any other reason? Or is it just made to create the ProcessAttachInfo? This seems like a reasonable move to me, but I'm not sure how LaunchInfo and AttachInfo might interact.

[mib]

In D148395#4270508, @bulbazord wrote:

Creating a ProcessAttachInfo from a ProcessLaunchInfo with this change means they'll have different listeners. Is that ProcessLaunchInfo kept around for any other reason? Or is it just made to create the ProcessAttachInfo? This seems like a reasonable move to me, but I'm not sure how LaunchInfo and AttachInfo might interact.

@bulbazord Not sure what you mean ... We need to convert to the ProcessLaunchInfo into a ProcessAttachInfo when the user ask use to launch a process but we end up asking the platform to do the launch after which we attach to the process. In both cases, we use the default listener (the debugger listener if the user didn't provide a custom listener).

[bulbazord]

In D148395#4285017, @mib wrote:

In D148395#4270508, @bulbazord wrote:

Creating a ProcessAttachInfo from a ProcessLaunchInfo with this change means they'll have different listeners. Is that ProcessLaunchInfo kept around for any other reason? Or is it just made to create the ProcessAttachInfo? This seems like a reasonable move to me, but I'm not sure how LaunchInfo and AttachInfo might interact.

@bulbazord Not sure what you mean ... We need to convert to the ProcessLaunchInfo into a ProcessAttachInfo when the user ask use to launch a process but we end up asking the platform to do the launch after which we attach to the process. In both cases, we use the default listener (the debugger listener if the user didn't provide a custom listener).

What I'm not sure about is that the ProcessAttachInfo constructor we're using takes a ProcessLaunchInfo as an argument and fills in its fields with it. It used to be that ProcessAttachInfo would get its Listener and HijackListener from the ProcessLaunchInfo passed in. Now, they could have different Listeners and HijackListeners. When we create a ProcessAttachInfo from a ProcessLaunchInfo, is it expected that these 2 things will diverge in that way? Do we use the ProcessLaunchInfo that we build the ProcessAttachInfo with in any other way?

[mib]

In D148395#4287673, @bulbazord wrote:

In D148395#4285017, @mib wrote:

In D148395#4270508, @bulbazord wrote:

Creating a ProcessAttachInfo from a ProcessLaunchInfo with this change means they'll have different listeners. Is that ProcessLaunchInfo kept around for any other reason? Or is it just made to create the ProcessAttachInfo? This seems like a reasonable move to me, but I'm not sure how LaunchInfo and AttachInfo might interact.

@bulbazord Not sure what you mean ... We need to convert to the ProcessLaunchInfo into a ProcessAttachInfo when the user ask use to launch a process but we end up asking the platform to do the launch after which we attach to the process. In both cases, we use the default listener (the debugger listener if the user didn't provide a custom listener).

What I'm not sure about is that the ProcessAttachInfo constructor we're using takes a ProcessLaunchInfo as an argument and fills in its fields with it. It used to be that ProcessAttachInfo would get its Listener and HijackListener from the ProcessLaunchInfo passed in. Now, they could have different Listeners and HijackListeners. When we create a ProcessAttachInfo from a ProcessLaunchInfo, is it expected that these 2 things will diverge in that way? Do we use the ProcessLaunchInfo that we build the ProcessAttachInfo with in any other way?

We had to do that previously because ProcessLaunchInfo and ProcessAttachInfo were not related (they were not derived from the same base class). With this patch, I hoisted the the m_listener_sp and m_hijack_listener_sp with their getters and setters into the ProcessInfo class from which both the ProcessAttachInfo and ProcessLaunchInfo are derived. So by calling the copy constructor for ProcessInfo (see inline comment), that should also copy the listener and hijack listener from the process launch info into the process attach info.

lldb/include/lldb/Target/Process.h

122

Because we moved m_listener_sp and m_hijack_listener_sp to ProcessInfo and since both Process{Attach,Launch}Info are derived from that class, this should also copy the listeners.

[bulbazord]

lldb/include/lldb/Target/Process.h

122

I don't think that's true? We're taking a different subclass and initializing from that, we have to be explicit about which fields are copied over, no? Maybe I'm missing something here.

Either way, I'm not sure it really matters unless the ProcessLaunchInfo and ProcessAttachInfo necessarily need to be in sync. Do they?

[bulbazord]

lldb/include/lldb/Target/Process.h

122

Wait, I'm being silly, you're doing operator= there. Okay, sorry for all the noise, I think I was just confused.

[bulbazord]

I don't have an issue here then. LGTM

lldb/include/lldb/Host/ProcessLaunchInfo.h

public:
   // process. Return true if the LaunchInfo has taken care of monitoring the
   // process, and false if the caller might want to monitor the process
   // themselves.
 
   bool MonitorProcess() const;
 
   PseudoTerminal &GetPTY() { return *m_pty; }
 
-  // Get and set the actual listener that will be used for the process events
-  lldb::ListenerSP GetListener() const { return m_listener_sp; }
-
-  void SetListener(const lldb::ListenerSP &listener_sp) {
-    m_listener_sp = listener_sp;
-  }
-
-  lldb::ListenerSP GetHijackListener() const { return m_hijack_listener_sp; }
-
-  void SetHijackListener(const lldb::ListenerSP &listener_sp) {
-    m_hijack_listener_sp = listener_sp;
-  }
-
   void SetLaunchEventData(const char *data) { m_event_data.assign(data); }
 
   const char *GetLaunchEventData() const { return m_event_data.c_str(); }
 
   void SetDetachOnError(bool enable);
 
   bool GetDetachOnError() const {
     return m_flags.Test(lldb::eLaunchFlagDetachOnError);
   }
 
 protected:
   FileSpec m_working_dir;
   std::string m_plugin_name;
   FileSpec m_shell;
   Flags m_flags; // Bitwise OR of bits from lldb::LaunchFlags
   std::vector<FileAction> m_file_actions; // File actions for any other files
   std::shared_ptr<PseudoTerminal> m_pty;
   uint32_t m_resume_count = 0; // How many times do we resume after launching
   Host::MonitorChildProcessCallback m_monitor_callback;
   std::string m_event_data; // A string passed to the plugin launch, having no
                             // meaning to the upper levels of lldb.
-  lldb::ListenerSP m_listener_sp;
-  lldb::ListenerSP m_hijack_listener_sp;
 };
 }
 
 #endif // LLDB_HOST_PROCESSLAUNCHINFO_H

lldb/include/lldb/Target/Process.h

 class ProcessAttachInfo : public ProcessInstanceInfo {
 public:
   ProcessAttachInfo() = default;
 
   ProcessAttachInfo(const ProcessLaunchInfo &launch_info)
       : m_resume_count(0), m_wait_for_launch(false), m_ignore_existing(true),
         m_continue_once_attached(false), m_detach_on_error(true),
         m_async(false) {
     ProcessInfo::operator=(launch_info);
     SetProcessPluginName(launch_info.GetProcessPluginName());
     SetResumeCount(launch_info.GetResumeCount());
-    SetListener(launch_info.GetListener());
-    SetHijackListener(launch_info.GetHijackListener());
     m_detach_on_error = launch_info.GetDetachOnError();
   }
 
   bool GetWaitForLaunch() const { return m_wait_for_launch; }
 
   void SetWaitForLaunch(bool b) { m_wait_for_launch = b; }
 
   bool GetAsync() const { return m_async; }
if (GetExecutableFile())
       return true;
     if (GetProcessID() != LLDB_INVALID_PROCESS_ID)
       return true;
     if (GetParentProcessID() != LLDB_INVALID_PROCESS_ID)
       return true;
     return false;
   }
 
-  lldb::ListenerSP GetHijackListener() const { return m_hijack_listener_sp; }
-
-  void SetHijackListener(const lldb::ListenerSP &listener_sp) {
-    m_hijack_listener_sp = listener_sp;
-  }
-
   bool GetDetachOnError() const { return m_detach_on_error; }
 
   void SetDetachOnError(bool enable) { m_detach_on_error = enable; }
 
-  // Get and set the actual listener that will be used for the process events
-  lldb::ListenerSP GetListener() const { return m_listener_sp; }
-
-  void SetListener(const lldb::ListenerSP &listener_sp) {
-    m_listener_sp = listener_sp;
-  }
-
   lldb::ListenerSP GetListenerForProcess(Debugger &debugger);
 
 protected:
-  lldb::ListenerSP m_listener_sp;
-  lldb::ListenerSP m_hijack_listener_sp;
   std::string m_plugin_name;
   uint32_t m_resume_count = 0; // How many times do we resume after launching
   bool m_wait_for_launch = false;
   bool m_ignore_existing = true;
   bool m_continue_once_attached = false; // Supports the use-case scenario of
                                          // immediately continuing the process
                                          // once attached.
   bool m_detach_on_error =

lldb/include/lldb/Utility/ProcessInfo.h

public:
   lldb::ScriptedMetadataSP GetScriptedMetadata() const {
     return m_scripted_metadata_sp;
   }
 
   void SetScriptedMetadata(lldb::ScriptedMetadataSP metadata_sp) {
     m_scripted_metadata_sp = metadata_sp;
   }
 
+  // Get and set the actual listener that will be used for the process events
+  lldb::ListenerSP GetListener() const { return m_listener_sp; }
+
+  void SetListener(const lldb::ListenerSP &listener_sp) {
+    m_listener_sp = listener_sp;
+  }
+
+  lldb::ListenerSP GetHijackListener() const { return m_hijack_listener_sp; }
+
+  void SetHijackListener(const lldb::ListenerSP &listener_sp) {
+    m_hijack_listener_sp = listener_sp;
+  }
+
 protected:
   FileSpec m_executable;
   std::string m_arg0; // argv[0] if supported. If empty, then use m_executable.
   // Not all process plug-ins support specifying an argv[0] that differs from
   // the resolved platform executable (which is in m_executable)
   Args m_arguments; // All program arguments except argv[0]
   Environment m_environment;
   uint32_t m_uid = UINT32_MAX;
   uint32_t m_gid = UINT32_MAX;
   ArchSpec m_arch;
   lldb::pid_t m_pid = LLDB_INVALID_PROCESS_ID;
   lldb::ScriptedMetadataSP m_scripted_metadata_sp = nullptr;
+  lldb::ListenerSP m_listener_sp = nullptr;
+  lldb::ListenerSP m_hijack_listener_sp = nullptr;
 };
 
 // ProcessInstanceInfo
 //
 // Describes an existing process and any discoverable information that pertains
 // to that process.
 class ProcessInstanceInfo : public ProcessInfo {
 public:

lldb/source/Host/common/ProcessLaunchInfo.cpp

 
 using namespace lldb;
 using namespace lldb_private;
 
 // ProcessLaunchInfo member functions
 
 ProcessLaunchInfo::ProcessLaunchInfo()
     : ProcessInfo(), m_working_dir(), m_plugin_name(), m_flags(0),
-      m_file_actions(), m_pty(new PseudoTerminal), m_monitor_callback(nullptr),
-      m_listener_sp(), m_hijack_listener_sp() {}
+      m_file_actions(), m_pty(new PseudoTerminal), m_monitor_callback(nullptr) {
+}
 
 ProcessLaunchInfo::ProcessLaunchInfo(const FileSpec &stdin_file_spec,
                                      const FileSpec &stdout_file_spec,
                                      const FileSpec &stderr_file_spec,
                                      const FileSpec &working_directory,
                                      uint32_t launch_flags)
     : ProcessInfo(), m_working_dir(), m_plugin_name(), m_flags(launch_flags),
       m_file_actions(), m_pty(new PseudoTerminal) {

lldb/source/Utility/ProcessInfo.cpp

 
 #include <climits>
 #include <optional>
 
 using namespace lldb;
 using namespace lldb_private;
 
 ProcessInfo::ProcessInfo()
-    : m_executable(), m_arguments(), m_environment(), m_arch() {}
+    : m_executable(), m_arguments(), m_environment(), m_arch(), m_listener_sp(),
+      m_hijack_listener_sp(), m_passthrough_listener_sp() {}
 
 ProcessInfo::ProcessInfo(const char *name, const ArchSpec &arch,
                          lldb::pid_t pid)
     : m_executable(name), m_arguments(), m_environment(), m_arch(arch),
-      m_pid(pid) {}
+      m_pid(pid), m_listener_sp(), m_hijack_listener_sp(),
+      m_passthrough_listener_sp() {}
 
 void ProcessInfo::Clear() {
   m_executable.Clear();
   m_arguments.Clear();
   m_environment.clear();
   m_uid = UINT32_MAX;
   m_gid = UINT32_MAX;
   m_arch.Clear();