This is an archive of the discontinued LLVM Phabricator instance.

Synchronize PlatformNetBSD with Linux
ClosedPublic

Authored by krytarowski on Jan 29 2017, 3:16 PM.

Details

Summary

Update the code to the new world code.

These changes are needed for remote process plugin.

Sponsored by <The NetBSD Foundation>

Diff Detail

Repository
rL LLVM

Event Timeline

krytarowski created this revision.Jan 29 2017, 3:16 PM
clayborg accepted this revision.Jan 30 2017, 9:27 AM
This revision is now accepted and ready to land.Jan 30 2017, 9:27 AM
labath requested changes to this revision.Jan 30 2017, 6:21 PM

How much of the code is now actually different between the two classes? If the only changes are of the s/linux/netbsd type, then we should just create a new base class for the two, and pass the platform name in as a parameter.

This revision now requires changes to proceed.Jan 30 2017, 6:21 PM

Diff

diff -u source/Plugins/Platform/Linux/PlatformLinux.cpp source/Plugins/Platform/NetBSD/PlatformNetBSD.cpp

Mostly everything is the same for now.

--- source/Plugins/Platform/Linux/PlatformLinux.cpp	2016-12-19 17:48:18.156356172 +0100
+++ source/Plugins/Platform/NetBSD/PlatformNetBSD.cpp	2017-01-31 16:48:56.313379692 +0100
@@ -1,4 +1,4 @@
-//===-- PlatformLinux.cpp ---------------------------------------*- C++ -*-===//
+//===-- PlatformNetBSD.cpp -------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -7,7 +7,7 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "PlatformLinux.h"
+#include "PlatformNetBSD.h"
 #include "lldb/Host/Config.h"
 
 // C Includes
@@ -36,27 +36,27 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 
-// Define these constants from Linux mman.h for use when targeting
+// Define these constants from NetBSD mman.h for use when targeting
 // remote linux systems even when host has different values.
-#define MAP_PRIVATE 2
-#define MAP_ANON 0x20
+#define MAP_PRIVATE 0x0002
+#define MAP_ANON 0x1000
 
 using namespace lldb;
 using namespace lldb_private;
-using namespace lldb_private::platform_linux;
+using namespace lldb_private::platform_netbsd;
 
 static uint32_t g_initialize_count = 0;
 
 //------------------------------------------------------------------
-/// Code to handle the PlatformLinux settings
+/// Code to handle the PlatformNetBSD settings
 //------------------------------------------------------------------
 
 namespace {
-class PlatformLinuxProperties : public Properties {
+class PlatformNetBSDProperties : public Properties {
 public:
-  PlatformLinuxProperties();
+  PlatformNetBSDProperties();
 
-  ~PlatformLinuxProperties() override = default;
+  ~PlatformNetBSDProperties() override = default;
 
   static ConstString &GetSettingName();
 
@@ -64,49 +64,49 @@
   static const PropertyDefinition *GetStaticPropertyDefinitions();
 };
 
-typedef std::shared_ptr<PlatformLinuxProperties> PlatformLinuxPropertiesSP;
+typedef std::shared_ptr<PlatformNetBSDProperties> PlatformNetBSDPropertiesSP;
 
 } // anonymous namespace
 
-PlatformLinuxProperties::PlatformLinuxProperties() : Properties() {
+PlatformNetBSDProperties::PlatformNetBSDProperties() : Properties() {
   m_collection_sp.reset(new OptionValueProperties(GetSettingName()));
   m_collection_sp->Initialize(GetStaticPropertyDefinitions());
 }
 
-ConstString &PlatformLinuxProperties::GetSettingName() {
-  static ConstString g_setting_name("linux");
+ConstString &PlatformNetBSDProperties::GetSettingName() {
+  static ConstString g_setting_name("netbsd");
   return g_setting_name;
 }
 
 const PropertyDefinition *
-PlatformLinuxProperties::GetStaticPropertyDefinitions() {
+PlatformNetBSDProperties::GetStaticPropertyDefinitions() {
   static PropertyDefinition g_properties[] = {
       {NULL, OptionValue::eTypeInvalid, false, 0, NULL, NULL, NULL}};
 
   return g_properties;
 }
 
-static const PlatformLinuxPropertiesSP &GetGlobalProperties() {
-  static PlatformLinuxPropertiesSP g_settings_sp;
+static const PlatformNetBSDPropertiesSP &GetGlobalProperties() {
+  static PlatformNetBSDPropertiesSP g_settings_sp;
   if (!g_settings_sp)
-    g_settings_sp.reset(new PlatformLinuxProperties());
+    g_settings_sp.reset(new PlatformNetBSDProperties());
   return g_settings_sp;
 }
 
-void PlatformLinux::DebuggerInitialize(Debugger &debugger) {
+void PlatformNetBSD::DebuggerInitialize(Debugger &debugger) {
   if (!PluginManager::GetSettingForPlatformPlugin(
-          debugger, PlatformLinuxProperties::GetSettingName())) {
+          debugger, PlatformNetBSDProperties::GetSettingName())) {
     const bool is_global_setting = true;
     PluginManager::CreateSettingForPlatformPlugin(
         debugger, GetGlobalProperties()->GetValueProperties(),
-        ConstString("Properties for the PlatformLinux plug-in."),
+        ConstString("Properties for the PlatformNetBSD plug-in."),
         is_global_setting);
   }
 }
 
 //------------------------------------------------------------------
 
-PlatformSP PlatformLinux::CreateInstance(bool force, const ArchSpec *arch) {
+PlatformSP PlatformNetBSD::CreateInstance(bool force, const ArchSpec *arch) {
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM));
   if (log) {
     const char *arch_name;
@@ -118,7 +118,7 @@
     const char *triple_cstr =
         arch ? arch->GetTriple().getTriple().c_str() : "<null>";
 
-    log->Printf("PlatformLinux::%s(force=%s, arch={%s,%s})", __FUNCTION__,
+    log->Printf("PlatformNetBSD::%s(force=%s, arch={%s,%s})", __FUNCTION__,
                 force ? "true" : "false", arch_name, triple_cstr);
   }
 
@@ -126,18 +126,10 @@
   if (create == false && arch && arch->IsValid()) {
     const llvm::Triple &triple = arch->GetTriple();
     switch (triple.getOS()) {
-    case llvm::Triple::Linux:
+    case llvm::Triple::NetBSD:
       create = true;
       break;
 
-#if defined(__linux__)
-    // Only accept "unknown" for the OS if the host is linux and
-    // it "unknown" wasn't specified (it was just returned because it
-    // was NOT specified)
-    case llvm::Triple::OSType::UnknownOS:
-      create = !arch->TripleOSWasSpecified();
-      break;
-#endif
     default:
       break;
     }
@@ -145,67 +137,67 @@
 
   if (create) {
     if (log)
-      log->Printf("PlatformLinux::%s() creating remote-linux platform",
+      log->Printf("PlatformNetBSD::%s() creating remote-netbsd platform",
                   __FUNCTION__);
-    return PlatformSP(new PlatformLinux(false));
+    return PlatformSP(new PlatformNetBSD(false));
   }
 
   if (log)
     log->Printf(
-        "PlatformLinux::%s() aborting creation of remote-linux platform",
+        "PlatformNetBSD::%s() aborting creation of remote-netbsd platform",
         __FUNCTION__);
 
   return PlatformSP();
 }
 
-ConstString PlatformLinux::GetPluginNameStatic(bool is_host) {
+ConstString PlatformNetBSD::GetPluginNameStatic(bool is_host) {
   if (is_host) {
     static ConstString g_host_name(Platform::GetHostPlatformName());
     return g_host_name;
   } else {
-    static ConstString g_remote_name("remote-linux");
+    static ConstString g_remote_name("remote-netbsd");
     return g_remote_name;
   }
 }
 
-const char *PlatformLinux::GetPluginDescriptionStatic(bool is_host) {
+const char *PlatformNetBSD::GetPluginDescriptionStatic(bool is_host) {
   if (is_host)
-    return "Local Linux user platform plug-in.";
+    return "Local NetBSD user platform plug-in.";
   else
-    return "Remote Linux user platform plug-in.";
+    return "Remote NetBSD user platform plug-in.";
 }
 
-ConstString PlatformLinux::GetPluginName() {
+ConstString PlatformNetBSD::GetPluginName() {
   return GetPluginNameStatic(IsHost());
 }
 
-void PlatformLinux::Initialize() {
+void PlatformNetBSD::Initialize() {
   PlatformPOSIX::Initialize();
 
   if (g_initialize_count++ == 0) {
-#if defined(__linux__) && !defined(__ANDROID__)
-    PlatformSP default_platform_sp(new PlatformLinux(true));
+#if defined(__NetBSD__)
+    PlatformSP default_platform_sp(new PlatformNetBSD(true));
     default_platform_sp->SetSystemArchitecture(HostInfo::GetArchitecture());
     Platform::SetHostPlatform(default_platform_sp);
 #endif
     PluginManager::RegisterPlugin(
-        PlatformLinux::GetPluginNameStatic(false),
-        PlatformLinux::GetPluginDescriptionStatic(false),
-        PlatformLinux::CreateInstance, PlatformLinux::DebuggerInitialize);
+        PlatformNetBSD::GetPluginNameStatic(false),
+        PlatformNetBSD::GetPluginDescriptionStatic(false),
+        PlatformNetBSD::CreateInstance, PlatformNetBSD::DebuggerInitialize);
   }
 }
 
-void PlatformLinux::Terminate() {
+void PlatformNetBSD::Terminate() {
   if (g_initialize_count > 0) {
     if (--g_initialize_count == 0) {
-      PluginManager::UnregisterPlugin(PlatformLinux::CreateInstance);
+      PluginManager::UnregisterPlugin(PlatformNetBSD::CreateInstance);
     }
   }
 
   PlatformPOSIX::Terminate();
 }
 
-Error PlatformLinux::ResolveExecutable(
+Error PlatformNetBSD::ResolveExecutable(
     const ModuleSpec &ms, lldb::ModuleSP &exe_module_sp,
     const FileSpecList *module_search_paths_ptr) {
   Error error;
@@ -330,7 +322,7 @@
   return error;
 }
 
-Error PlatformLinux::GetFileWithUUID(const FileSpec &platform_file,
+Error PlatformNetBSD::GetFileWithUUID(const FileSpec &platform_file,
                                      const UUID *uuid_ptr,
                                      FileSpec &local_file) {
   if (IsRemote()) {
@@ -347,7 +339,7 @@
 //------------------------------------------------------------------
 /// Default Constructor
 //------------------------------------------------------------------
-PlatformLinux::PlatformLinux(bool is_host)
+PlatformNetBSD::PlatformNetBSD(bool is_host)
     : PlatformPOSIX(is_host) // This is the local host platform
 {}
 
@@ -357,9 +349,9 @@
 /// The destructor is virtual since this class is designed to be
 /// inherited from by the plug-in instance.
 //------------------------------------------------------------------
-PlatformLinux::~PlatformLinux() = default;
+PlatformNetBSD::~PlatformNetBSD() = default;
 
-bool PlatformLinux::GetProcessInfo(lldb::pid_t pid,
+bool PlatformNetBSD::GetProcessInfo(lldb::pid_t pid,
                                    ProcessInstanceInfo &process_info) {
   bool success = false;
   if (IsHost()) {
@@ -372,8 +364,8 @@
 }
 
 uint32_t
-PlatformLinux::FindProcesses(const ProcessInstanceInfoMatch &match_info,
-                             ProcessInstanceInfoList &process_infos) {
+PlatformNetBSD::FindProcesses(const ProcessInstanceInfoMatch &match_info,
+                              ProcessInstanceInfoList &process_infos) {
   uint32_t match_count = 0;
   if (IsHost()) {
     // Let the base class figure out the host details
@@ -387,11 +379,11 @@
   return match_count;
 }
 
-bool PlatformLinux::GetSupportedArchitectureAtIndex(uint32_t idx,
-                                                    ArchSpec &arch) {
+bool PlatformNetBSD::GetSupportedArchitectureAtIndex(uint32_t idx,
+                                                     ArchSpec &arch) {
   if (IsHost()) {
     ArchSpec hostArch = HostInfo::GetArchitecture(HostInfo::eArchKindDefault);
-    if (hostArch.GetTriple().isOSLinux()) {
+    if (hostArch.GetTriple().isOSNetBSD()) {
       if (idx == 0) {
         arch = hostArch;
         return arch.IsValid();
@@ -408,8 +400,8 @@
       return m_remote_platform_sp->GetSupportedArchitectureAtIndex(idx, arch);
 
     llvm::Triple triple;
-    // Set the OS to linux
-    triple.setOS(llvm::Triple::Linux);
+    // Set the OS to NetBSD
+    triple.setOS(llvm::Triple::NetBSD);
     // Set the architecture
     switch (idx) {
     case 0:
@@ -418,30 +410,6 @@
     case 1:
       triple.setArchName("i386");
       break;
-    case 2:
-      triple.setArchName("arm");
-      break;
-    case 3:
-      triple.setArchName("aarch64");
-      break;
-    case 4:
-      triple.setArchName("mips64");
-      break;
-    case 5:
-      triple.setArchName("hexagon");
-      break;
-    case 6:
-      triple.setArchName("mips");
-      break;
-    case 7:
-      triple.setArchName("mips64el");
-      break;
-    case 8:
-      triple.setArchName("mipsel");
-      break;
-    case 9:
-      triple.setArchName("s390x");
-      break;
     default:
       return false;
     }
@@ -461,12 +429,12 @@
   return false;
 }
 
-void PlatformLinux::GetStatus(Stream &strm) {
+void PlatformNetBSD::GetStatus(Stream &strm) {
   Platform::GetStatus(strm);
 
 #ifndef LLDB_DISABLE_POSIX
   // Display local kernel information only when we are running in host mode.
-  // Otherwise, we would end up printing non-Linux information (when running
+  // Otherwise, we would end up printing non-NetBSD information (when running
   // on Mac OS for example).
   if (IsHost()) {
     struct utsname un;
@@ -482,7 +450,7 @@
 }
 
 int32_t
-PlatformLinux::GetResumeCountForLaunchInfo(ProcessLaunchInfo &launch_info) {
+PlatformNetBSD::GetResumeCountForLaunchInfo(ProcessLaunchInfo &launch_info) {
   int32_t resume_count = 0;
 
   // Always resume past the initial stop when we use eLaunchFlagDebug
@@ -516,7 +484,7 @@
   return resume_count;
 }
 
-bool PlatformLinux::CanDebugProcess() {
+bool PlatformNetBSD::CanDebugProcess() {
   if (IsHost()) {
     return true;
   } else {
@@ -525,19 +493,19 @@
   }
 }
 
-// For local debugging, Linux will override the debug logic to use llgs-launch
+// For local debugging, NetBSD will override the debug logic to use llgs-launch
 // rather than
 // lldb-launch, llgs-attach.  This differs from current lldb-launch,
 // debugserver-attach
 // approach on MacOSX.
 lldb::ProcessSP
-PlatformLinux::DebugProcess(ProcessLaunchInfo &launch_info, Debugger &debugger,
+PlatformNetBSD::DebugProcess(ProcessLaunchInfo &launch_info, Debugger &debugger,
                             Target *target, // Can be NULL, if NULL create a new
                                             // target, else use existing one
                             Error &error) {
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM));
   if (log)
-    log->Printf("PlatformLinux::%s entered (target %p)", __FUNCTION__,
+    log->Printf("PlatformNetBSD::%s entered (target %p)", __FUNCTION__,
                 static_cast<void *>(target));
 
   // If we're a remote host, use standard behavior from parent class.
@@ -562,14 +530,14 @@
   // Ensure we have a target.
   if (target == nullptr) {
     if (log)
-      log->Printf("PlatformLinux::%s creating new target", __FUNCTION__);
+      log->Printf("PlatformNetBSD::%s creating new target", __FUNCTION__);
 
     TargetSP new_target_sp;
     error = debugger.GetTargetList().CreateTarget(debugger, "", "", false,
                                                   nullptr, new_target_sp);
     if (error.Fail()) {
       if (log)
-        log->Printf("PlatformLinux::%s failed to create new target: %s",
+        log->Printf("PlatformNetBSD::%s failed to create new target: %s",
                     __FUNCTION__, error.AsCString());
       return process_sp;
     }
@@ -578,13 +546,13 @@
     if (!target) {
       error.SetErrorString("CreateTarget() returned nullptr");
       if (log)
-        log->Printf("PlatformLinux::%s failed: %s", __FUNCTION__,
+        log->Printf("PlatformNetBSD::%s failed: %s", __FUNCTION__,
                     error.AsCString());
       return process_sp;
     }
   } else {
     if (log)
-      log->Printf("PlatformLinux::%s using provided target", __FUNCTION__);
+      log->Printf("PlatformNetBSD::%s using provided target", __FUNCTION__);
   }
 
   // Mark target as currently selected target.
@@ -593,7 +561,7 @@
   // Now create the gdb-remote process.
   if (log)
     log->Printf(
-        "PlatformLinux::%s having target create process with gdb-remote plugin",
+        "PlatformNetBSD::%s having target create process with gdb-remote plugin",
         __FUNCTION__);
   process_sp = target->CreateProcess(
       launch_info.GetListenerForProcess(debugger), "gdb-remote", nullptr);
@@ -601,12 +569,12 @@
   if (!process_sp) {
     error.SetErrorString("CreateProcess() failed for gdb-remote process");
     if (log)
-      log->Printf("PlatformLinux::%s failed: %s", __FUNCTION__,
+      log->Printf("PlatformNetBSD::%s failed: %s", __FUNCTION__,
                   error.AsCString());
     return process_sp;
   } else {
     if (log)
-      log->Printf("PlatformLinux::%s successfully created process",
+      log->Printf("PlatformNetBSD::%s successfully created process",
                   __FUNCTION__);
   }
 
@@ -614,10 +582,10 @@
   ListenerSP listener_sp;
   if (!launch_info.GetHijackListener()) {
     if (log)
-      log->Printf("PlatformLinux::%s setting up hijacker", __FUNCTION__);
+      log->Printf("PlatformNetBSD::%s setting up hijacker", __FUNCTION__);
 
     listener_sp =
-        Listener::MakeListener("lldb.PlatformLinux.DebugProcess.hijack");
+        Listener::MakeListener("lldb.PlatformNetBSD.DebugProcess.hijack");
     launch_info.SetHijackListener(listener_sp);
     process_sp->HijackProcessEvents(listener_sp);
   }
@@ -625,7 +593,7 @@
   // Log file actions.
   if (log) {
     log->Printf(
-        "PlatformLinux::%s launching process with the following file actions:",
+        "PlatformNetBSD::%s launching process with the following file actions:",
         __FUNCTION__);
 
     StreamString stream;
@@ -648,11 +616,11 @@
 
       if (state == eStateStopped) {
         if (log)
-          log->Printf("PlatformLinux::%s pid %" PRIu64 " state %s\n",
+          log->Printf("PlatformNetBSD::%s pid %" PRIu64 " state %s\n",
                       __FUNCTION__, process_sp->GetID(), StateAsCString(state));
       } else {
         if (log)
-          log->Printf("PlatformLinux::%s pid %" PRIu64
+          log->Printf("PlatformNetBSD::%s pid %" PRIu64
                       " state is not stopped - %s\n",
                       __FUNCTION__, process_sp->GetID(), StateAsCString(state));
       }
@@ -664,18 +632,18 @@
     if (pty_fd != lldb_utility::PseudoTerminal::invalid_fd) {
       process_sp->SetSTDIOFileDescriptor(pty_fd);
       if (log)
-        log->Printf("PlatformLinux::%s pid %" PRIu64
+        log->Printf("PlatformNetBSD::%s pid %" PRIu64
                     " hooked up STDIO pty to process",
                     __FUNCTION__, process_sp->GetID());
     } else {
       if (log)
-        log->Printf("PlatformLinux::%s pid %" PRIu64
+        log->Printf("PlatformNetBSD::%s pid %" PRIu64
                     " not using process STDIO pty",
                     __FUNCTION__, process_sp->GetID());
     }
   } else {
     if (log)
-      log->Printf("PlatformLinux::%s process launch failed: %s", __FUNCTION__,
+      log->Printf("PlatformNetBSD::%s process launch failed: %s", __FUNCTION__,
                   error.AsCString());
     // FIXME figure out appropriate cleanup here.  Do we delete the target? Do
     // we delete the process?  Does our caller do that?
@@ -684,30 +652,22 @@
   return process_sp;
 }
 
-void PlatformLinux::CalculateTrapHandlerSymbolNames() {
+void PlatformNetBSD::CalculateTrapHandlerSymbolNames() {
   m_trap_handlers.push_back(ConstString("_sigtramp"));
 }
 
-uint64_t PlatformLinux::ConvertMmapFlagsToPlatform(const ArchSpec &arch,
+uint64_t PlatformNetBSD::ConvertMmapFlagsToPlatform(const ArchSpec &arch,
                                                    unsigned flags) {
   uint64_t flags_platform = 0;
-  uint64_t map_anon = MAP_ANON;
-
-  // To get correct flags for MIPS Architecture
-  if (arch.GetTriple().getArch() == llvm::Triple::mips64 ||
-      arch.GetTriple().getArch() == llvm::Triple::mips64el ||
-      arch.GetTriple().getArch() == llvm::Triple::mips ||
-      arch.GetTriple().getArch() == llvm::Triple::mipsel)
-    map_anon = 0x800;
 
   if (flags & eMmapFlagsPrivate)
     flags_platform |= MAP_PRIVATE;
   if (flags & eMmapFlagsAnon)
-    flags_platform |= map_anon;
+    flags_platform |= MAP_ANON;
   return flags_platform;
 }
 
-ConstString PlatformLinux::GetFullNameForDylib(ConstString basename) {
+ConstString PlatformNetBSD::GetFullNameForDylib(ConstString basename) {
   if (basename.IsEmpty())
     return basename;

and

--- source/Plugins/Platform/Linux/PlatformLinux.h	2016-09-10 20:16:06.905683528 +0200
+++ source/Plugins/Platform/NetBSD/PlatformNetBSD.h	2017-01-31 16:48:56.332198593 +0100
@@ -1,4 +1,4 @@
-//===-- PlatformLinux.h -----------------------------------------*- C++ -*-===//
+//===-- PlatformNetBSD.h ----------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -7,8 +7,8 @@
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef liblldb_PlatformLinux_h_
-#define liblldb_PlatformLinux_h_
+#ifndef liblldb_PlatformNetBSD_h_
+#define liblldb_PlatformNetBSD_h_
 
 // C Includes
 // C++ Includes
@@ -17,13 +17,13 @@
 #include "Plugins/Platform/POSIX/PlatformPOSIX.h"
 
 namespace lldb_private {
-namespace platform_linux {
+namespace platform_netbsd {
 
-class PlatformLinux : public PlatformPOSIX {
+class PlatformNetBSD : public PlatformPOSIX {
 public:
-  PlatformLinux(bool is_host);
+  PlatformNetBSD(bool is_host);
 
-  ~PlatformLinux() override;
+  ~PlatformNetBSD() override;
 
   static void DebuggerInitialize(Debugger &debugger);
 
@@ -83,10 +83,10 @@
   ConstString GetFullNameForDylib(ConstString basename) override;
 
 private:
-  DISALLOW_COPY_AND_ASSIGN(PlatformLinux);
+  DISALLOW_COPY_AND_ASSIGN(PlatformNetBSD);
 };
 
-} // namespace platform_linux
+} // namespace platform_netbsd
 } // namespace lldb_private
 
-#endif // liblldb_PlatformLinux_h_
+#endif // liblldb_PlatformNetBSD_h_

How would you like to enhance it? How to name the base class, where to put it? Reuse C++ class inheritance?

After looking at this closer, I don't think we will even need a separate class to achieve deduplication. The code is so heavily duplicated (see comments on the first three non-boilerplate functions, I did not look at the rest yet, but I expect the situation to be the same) already that we can just push the definitions down the existing class hierarchy and achieve massive savings that way. After we're done with that, I think we'll find that the PlatformLinux class will be so small that your current s/linux/netbsd/ approach will no longer be a problem.

If @clayborg agrees with this approach, I can help you prepare the ground for that.

source/Plugins/Platform/NetBSD/PlatformNetBSD.cpp
136–137

PlatformLinux, PlatformFreeBSD and PlatformWindows have copies of this code already. Proposal:

  • put this code into Platform::ResolveExecutable
  • remove other identical copies
136–137

Every platform except PlatformDarwin already defines this function to the exact same code. Proposal:

  • put this code into Platform::GetFileWithUUID
  • remove other definitions with the same code
143–144

Every platform already defines this function to the same code. Proposal:

  • move this code into Platform::GetProcessInfo
  • remove other (*all*) identical definitions

After looking at this closer, I don't think we will even need a separate class to achieve deduplication. The code is so heavily duplicated (see comments on the first three non-boilerplate functions, I did not look at the rest yet, but I expect the situation to be the same) already that we can just push the definitions down the existing class hierarchy and achieve massive savings that way. After we're done with that, I think we'll find that the PlatformLinux class will be so small that your current s/linux/netbsd/ approach will no longer be a problem.

If @clayborg agrees with this approach, I can help you prepare the ground for that.

I'm looking forward to this approach. In the past NetBSD code was almost exact copy of the FreeBSD one. Once we will deduplicate the code, FreeBSD will catch up.

I need to finish three patches:

  • llvm::call_once,
  • auxv reading,
  • six.py conflict removal

and I will join here.

After that, I will switch the PT_WATCHPOINT* interface to PT_GETDBREGS and PT_SETDBREGS -- and in the end, add dbregs support in NetBSD's userdata. Later on, I will need to finish few tasks on the NetBSD side (thread suspend/resume; detect simple false positives in the check-lldb target).

krytarowski updated this revision to Diff 87102.Feb 4 2017, 1:05 PM
krytarowski edited edge metadata.

Sync this patch with SVN 294071.

It is to be used at least for a reference in further code sharing.

labath accepted this revision.Feb 4 2017, 5:17 PM

Just sync once more please and I think we're done. We've removed enough code to offset the remaining bit of duplication here. DebugProcess is the only remaining candidate for merging, but we don't have a good class for it now. Maybe when Freebsd switches to lldb-server we can put it in processposix.

This revision is now accepted and ready to land.Feb 4 2017, 5:17 PM

I'm syncing the code to get the changes from SVN revision 294114 "Clean up PlatformLinux code"

krytarowski updated this revision to Diff 87126.Feb 4 2017, 6:14 PM

Catch up after r294114 - Clean up PlatformLinux code

OK to land it? FreeBSD can reuse almost exact the same code. I'm not sure if it's compatible with their Process Plugin.

krytarowski closed this revision.Feb 5 2017, 1:43 PM