[lldb] Make Process and subclass constructors protected

[mgorny]

Summary

Make constructors of the Process and its subclasses class protected,
to prevent accidentally constructing Process on stack when it could be
afterwards accessed via a shared_ptr (since it uses
std::enable_shared_from_this<>).

The only place where a stack allocation was used were unittests,
and fixing them via declaring an explicit public constructor
in the respective mock classes is trivial.

Sponsored by: The FreeBSD Foundation

[labath]

lldb/include/lldb/Target/Process.h

478–487

Just move this to the existing protected section on line 2500 (boy is this class big). Too many sections makes it hard to find what's the visibility of something.

lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h

38–39

same here

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

51–52

and here

lldb/source/Plugins/Process/scripted/ScriptedProcess.h

52–53

and here

lldb/unittests/Expression/DWARFExpressionTest.cpp

331–333

huh, I did not expect this to be necessary. Inherited constructors are weird...

[mgorny]

lldb/include/lldb/Target/Process.h

478–487

Will do. Wasn't sure whether this was desirable or not.

lldb/unittests/Expression/DWARFExpressionTest.cpp

331–333

Yes, they are. I've searched a bit and couldn't find a way to make using change their visibility.

lldb/include/lldb/Target/PostMortemProcess.h

 /// \class PostMortemProcess
 /// Base class for all processes that don't represent a live process, such as
 /// coredumps or processes traced in the past.
 ///
 /// \a lldb_private::Process virtual functions overrides that are common
 /// between these kinds of processes can have default implementations in this
 /// class.
 class PostMortemProcess : public Process {
-public:
   using Process::Process;
 
+public:
   bool IsLiveDebugSession() const override { return false; }
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_POSTMORTEMPROCESS_H

lldb/include/lldb/Target/Process.h

bool m_restarted = false; // For "eStateStopped" events, this is true if the
                               // target was automatically restarted.
     int m_update_state = 0;
     bool m_interrupted = false;
 
     ProcessEventData(const ProcessEventData &) = delete;
     const ProcessEventData &operator=(const ProcessEventData &) = delete;
   };
 
-  /// Construct with a shared pointer to a target, and the Process listener.
-  /// Uses the Host UnixSignalsSP by default.
-  Process(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
-
-  /// Construct with a shared pointer to a target, the Process listener, and
-  /// the appropriate UnixSignalsSP for the process.
-  Process(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
-          const lldb::UnixSignalsSP &unix_signals_sp);
-
   /// Destructor.
   ///
   /// The destructor is virtual since this class is designed to be inherited
   /// from by the plug-in instance.
   ~Process() override;
 
   static void SettingsInitialize();
 
   static void SettingsTerminate();
 
   static ProcessProperties &GetGlobalProperties();
 
   /// Find a Process plug-in that can debug \a module using the currently
   /// selected architecture.
   ///
   /// Scans all loaded plug-in interfaces that implement versions of the
   /// Process plug-in interface and returns the first instance that can debug
   /// the file.
void PruneThreadPlans();
   /// \return
   ///     The plugin if one is available for the specified feature;
   ///     otherwise, returns an empty shared pointer.
   lldb::StructuredDataPluginSP
   GetStructuredDataPlugin(ConstString type_name) const;
 
 protected:
   friend class Trace;
+
+  /// Construct with a shared pointer to a target, and the Process listener.
+  /// Uses the Host UnixSignalsSP by default.
+  Process(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
+
+  /// Construct with a shared pointer to a target, the Process listener, and
+  /// the appropriate UnixSignalsSP for the process.
+  Process(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
+          const lldb::UnixSignalsSP &unix_signals_sp);
+
   ///  Get the processor tracing type supported for this process.
   ///  Responses might be different depending on the architecture and
   ///  capabilities of the underlying OS.
   ///
   ///  \return
   ///     The supported trace type or an \a llvm::Error if tracing is
   ///     not supported for the inferior.
   virtual llvm::Expected<TraceSupportedResponse> TraceSupported();

lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h

public:
 
   static void Initialize();
 
   static void Terminate();
 
   static llvm::StringRef GetPluginNameStatic() { return "windows"; }
 
   static llvm::StringRef GetPluginDescriptionStatic();
 
-  // Constructors and destructors
-  ProcessWindows(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
-
   ~ProcessWindows();
 
   size_t GetSTDOUT(char *buf, size_t buf_size, Status &error) override;
   size_t GetSTDERR(char *buf, size_t buf_size, Status &error) override;
   size_t PutSTDIN(const char *buf, size_t buf_size, Status &error) override;
 
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
   Status EnableBreakpointSite(BreakpointSite *bp_site) override;
public:
   void OnDebuggerError(const Status &error, uint32_t type) override;
 
   Status GetWatchpointSupportInfo(uint32_t &num) override;
   Status GetWatchpointSupportInfo(uint32_t &num, bool &after) override;
   Status EnableWatchpoint(Watchpoint *wp, bool notify = true) override;
   Status DisableWatchpoint(Watchpoint *wp, bool notify = true) override;
 
 protected:
+  ProcessWindows(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
+
   Status DoGetMemoryRegionInfo(lldb::addr_t vm_addr,
                                MemoryRegionInfo &info) override;
 
 private:
   struct WatchpointInfo {
     uint32_t slot_id;
     lldb::addr_t address;
     uint32_t size;

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

 class Loader;
 }
 namespace process_gdb_remote {
 
 class ThreadGDBRemote;
 
 class ProcessGDBRemote : public Process,
                          private GDBRemoteClientBase::ContinueDelegate {
 public:
-  ProcessGDBRemote(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
-
   ~ProcessGDBRemote() override;
 
   static lldb::ProcessSP CreateInstance(lldb::TargetSP target_sp,
                                         lldb::ListenerSP listener_sp,
                                         const FileSpec *crash_file_path,
                                         bool can_connect);
 
   static void Initialize();
 
public:
 
   llvm::Expected<bool> SaveCore(llvm::StringRef outfile) override;
 
 protected:
   friend class ThreadGDBRemote;
   friend class GDBRemoteCommunicationClient;
   friend class GDBRemoteRegisterContext;
 
+  ProcessGDBRemote(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
+
   bool SupportsMemoryTagging() override;
 
   /// Broadcaster event bits definitions.
   enum {
     eBroadcastBitAsyncContinue = (1 << 0),
     eBroadcastBitAsyncThreadShouldExit = (1 << 1),
     eBroadcastBitAsyncThreadDidExit = (1 << 2)
   };

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

 llvm::StringRef ProcessGDBRemote::GetPluginDescriptionStatic() {
   return "GDB Remote protocol based debugging plug-in.";
 }
 
 void ProcessGDBRemote::Terminate() {
   PluginManager::UnregisterPlugin(ProcessGDBRemote::CreateInstance);
 }
 
-lldb::ProcessSP
-ProcessGDBRemote::CreateInstance(lldb::TargetSP target_sp,
-                                 ListenerSP listener_sp,
-                                 const FileSpec *crash_file_path,
-                                 bool can_connect) {
+lldb::ProcessSP ProcessGDBRemote::CreateInstance(
+    lldb::TargetSP target_sp, ListenerSP listener_sp,
+    const FileSpec *crash_file_path, bool can_connect) {
   lldb::ProcessSP process_sp;
   if (crash_file_path == nullptr)
-    process_sp = std::make_shared<ProcessGDBRemote>(target_sp, listener_sp);
+    process_sp = std::shared_ptr<ProcessGDBRemote>(
+        new ProcessGDBRemote(target_sp, listener_sp));
   return process_sp;
 }
 
 std::chrono::seconds ProcessGDBRemote::GetPacketTimeout() {
   return std::chrono::seconds(GetGlobalPluginProperties().GetPacketTimeout());
 }
 
 ArchSpec ProcessGDBRemote::GetSystemArchitecture() {

lldb/source/Plugins/Process/scripted/ScriptedProcess.h

public:
 
   static void Initialize();
 
   static void Terminate();
 
   static llvm::StringRef GetPluginNameStatic() { return "ScriptedProcess"; }
 
   static llvm::StringRef GetPluginDescriptionStatic();
 
-  ScriptedProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
-                  const ScriptedProcess::ScriptedProcessInfo &launch_info,
-                  Status &error);
-
   ~ScriptedProcess() override;
 
   bool CanDebug(lldb::TargetSP target_sp,
                 bool plugin_specified_by_name) override;
 
   DynamicLoader *GetDynamicLoader() override { return nullptr; }
 
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
public:
   GetMemoryRegions(lldb_private::MemoryRegionInfos &region_list) override;
 
   bool GetProcessInfo(ProcessInstanceInfo &info) override;
 
   lldb_private::StructuredData::ObjectSP
   GetLoadedDynamicLibrariesInfos() override;
 
 protected:
+  ScriptedProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
+                  const ScriptedProcess::ScriptedProcessInfo &launch_info,
+                  Status &error);
+
   Status DoStop();
 
   void Clear();
 
   bool DoUpdateThreadList(ThreadList &old_thread_list,
                           ThreadList &new_thread_list) override;
 
   Status DoGetMemoryRegionInfo(lldb::addr_t load_addr,

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

lldb::ProcessSP ScriptedProcess::CreateInstance(lldb::TargetSP target_sp,
   if (!target_sp ||
       !IsScriptLanguageSupported(target_sp->GetDebugger().GetScriptLanguage()))
     return nullptr;
 
   Status error;
   ScriptedProcess::ScriptedProcessInfo scripted_process_info(
       target_sp->GetProcessLaunchInfo());
 
-  auto process_sp = std::make_shared<ScriptedProcess>(
-      target_sp, listener_sp, scripted_process_info, error);
+  auto process_sp = std::shared_ptr<ScriptedProcess>(new ScriptedProcess(
+      target_sp, listener_sp, scripted_process_info, error));
 
   if (error.Fail() || !process_sp || !process_sp->m_script_object_sp ||
       !process_sp->m_script_object_sp->IsValid()) {
     LLDB_LOGF(GetLog(LLDBLog::Process), "%s", error.AsCString());
     return nullptr;
   }
 
   return process_sp;

lldb/unittests/Expression/DWARFExpressionTest.cpp

EXPECT_THAT_EXPECTED(
       llvm::FailedWithMessage(
           "Unhandled opcode DW_OP_unknown_ff in DWARFExpression"));
 }
 
 TEST_F(DWARFExpressionMockProcessTest, DW_OP_deref) {
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit0, DW_OP_deref}), llvm::Failed());
 
   struct MockProcess : Process {
-    using Process::Process;
+    MockProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+        : Process(target_sp, listener_sp) {}
+
     llvm::StringRef GetPluginName() override { return "mock process"; }
     bool CanDebug(lldb::TargetSP target,
                   bool plugin_specified_by_name) override {
       return false;
     };
     Status DoDestroy() override { return {}; }
     void RefreshStateAfterStop() override {}
     bool DoUpdateThreadList(ThreadList &old_thread_list,

lldb/unittests/Process/ProcessEventDataTest.cpp

void TearDown() override {
     HostInfo::Terminate();
     FileSystem::Terminate();
     Reproducer::Terminate();
   }
 };
 
 class DummyProcess : public Process {
 public:
-  using Process::Process;
+  DummyProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+      : Process(target_sp, listener_sp) {}
 
   bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) override {
     return true;
   }
   Status DoDestroy() override { return {}; }
   void RefreshStateAfterStop() override {}
   size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
                       Status &error) override {

lldb/unittests/Target/ExecutionContextTest.cpp

void TearDown() override {
     HostInfo::Terminate();
     FileSystem::Terminate();
     Reproducer::Terminate();
   }
 };
 
 class DummyProcess : public Process {
 public:
-  using Process::Process;
+  DummyProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+      : Process(target_sp, listener_sp) {}
 
   bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) override {
     return true;
   }
   Status DoDestroy() override { return {}; }
   void RefreshStateAfterStop() override {}
   size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
                       Status &error) override {

lldb/unittests/Thread/ThreadTest.cpp

void TearDown() override {
     HostInfo::Terminate();
     FileSystem::Terminate();
     Reproducer::Terminate();
   }
 };
 
 class DummyProcess : public Process {
 public:
-  using Process::Process;
+  DummyProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+      : Process(target_sp, listener_sp) {}
 
   bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) override {
     return true;
   }
   Status DoDestroy() override { return {}; }
   void RefreshStateAfterStop() override {}
   size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
                       Status &error) override {