Remove dependency from Host to python

[labath]

Summary

The only reason python was used in the Host module was to compute the
python path. I resolve this the same way as D47384 did for clang, moving
the path computation into the python plugin and modifying SBHostOS class
to call into this module for ePathTypePythonDir.

[clayborg]

source/API/SBHostOS.cpp

48 ↗

(On Diff #151489)

Why is this here instead of inside of "HostInfo::GetLLDBPath(path_type, fspec);"? Does this mean internal code that calls:

HostInfo::GetLLDBPath(ePathTypePythonDir, fspec);

Won't work anymore? Also same for ePathTypeClangDir? Are we trying to solve a layering problems by hosing over internal clients?

[zturner]

The internal api has no guarantees as to its stability.

[clayborg]

In D48215#1133695, @zturner wrote:

The internal api has no guarantees as to its stability.

What do you mean by this? If we have an internal API that claims to give out paths, it should give out paths. What am I missing here?

[zturner]

It can assert when we pass the python or clang directory enumeration. We
could also remove the values from the internal enumeration but keep them in
the public enumeration. However, we can’t be forced into providing a stable
internal api just because we must provide a stable public api.

[labath]

Actually, my plan was to follow this up with a patch which removes usages of that enumeration from the internal api altogether (basically do a s/GetLLDBPath(ePathTypeXXX/GetXXXPath). That should make the internal api nicer, and provide a clean separation between the internal and public api (thereby taking pressure off of adding new enums to the public api just because we have a new kind of an internal path).

I was considering in which order to submit the patches. In the end I chose this one because it will make the second patch slightly smaller, but I can do it in the other order too...

[labath]

(pressed return too soon)

Would that address your reservations about this idea?

[clayborg]

It might be nice to start with the removal of HostInfo::GetLLDBPath(...) first and fixing up all call sites that use that API to use the new internally sanctioned versions. Then submit this patch?

[clayborg]

In D48215#1133725, @labath wrote:

(pressed return too soon)

Would that address your reservations about this idea?

yeah, just don't like the idea of HostInfo::GetLLDBPath() being non functional.

[labath]

Ok, I'll start with the other patch first.

[labath]

Rebase this patch on trunk. Now this patch is simply about moving GetPythonDir
from Host module to ScriptInterpreterPython.

lldb/trunk/include/lldb/Host/macosx/HostInfoMacOSX.h

public:
   static bool GetOSKernelDescription(std::string &s);
   static FileSpec GetProgramFileSpec();
 
 protected:
   static bool ComputeSupportExeDirectory(FileSpec &file_spec);
   static void ComputeHostArchitectureSupport(ArchSpec &arch_32,
                                              ArchSpec &arch_64);
   static bool ComputeHeaderDirectory(FileSpec &file_spec);
-  static bool ComputePythonDirectory(FileSpec &file_spec);
   static bool ComputeSystemPluginsDirectory(FileSpec &file_spec);
   static bool ComputeUserPluginsDirectory(FileSpec &file_spec);
 };
 }
 
 #endif

lldb/trunk/include/lldb/Host/posix/HostInfoPosix.h

public:
   static bool GetEnvironmentVar(const std::string &var_name, std::string &var);
 
   static bool ComputePathRelativeToLibrary(FileSpec &file_spec,
                                            llvm::StringRef dir);
 
 protected:
   static bool ComputeSupportExeDirectory(FileSpec &file_spec);
   static bool ComputeHeaderDirectory(FileSpec &file_spec);
-  static bool ComputePythonDirectory(FileSpec &file_spec);
 };
 }
 
 #endif

lldb/trunk/include/lldb/Host/windows/HostInfoWindows.h

public:
   static bool GetOSBuildString(std::string &s);
   static bool GetOSKernelDescription(std::string &s);
   static bool GetHostname(std::string &s);
   static FileSpec GetProgramFileSpec();
   static FileSpec GetDefaultShell();
 
   static bool GetEnvironmentVar(const std::string &var_name, std::string &var);
 
-protected:
-  static bool ComputePythonDirectory(FileSpec &file_spec);
-
 private:
   static FileSpec m_program_filespec;
 };
 }
 
 #endif

lldb/trunk/source/API/SBHostOS.cpp

 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/HostNativeThread.h"
 #include "lldb/Host/HostThread.h"
 #include "lldb/Host/ThreadLauncher.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangHost.h"
+#include "Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h"
 
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/Path.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 SBFileSpec SBHostOS::GetProgramFileSpec() {
case ePathTypeLLDBShlibDir:
     break;
   case ePathTypeSupportExecutableDir:
     fspec = HostInfo::GetSupportExeDir();
     break;
   case ePathTypeHeaderDir:
     fspec = HostInfo::GetHeaderDir();
     break;
   case ePathTypePythonDir:
-    fspec = HostInfo::GetPythonDir();
+    fspec = ScriptInterpreterPython::GetPythonDir();
     break;
   case ePathTypeLLDBSystemPlugins:
     fspec = HostInfo::GetSystemPluginDir();
     break;
   case ePathTypeLLDBUserPlugins:
     fspec = HostInfo::GetUserPluginDir();
     break;
   case ePathTypeLLDBTempSystemDir:

lldb/trunk/source/Host/CMakeLists.txt

add_host_subdirectory(common
   common/TaskPool.cpp
   common/TCPSocket.cpp
   common/Terminal.cpp
   common/ThreadLauncher.cpp
   common/XML.cpp
   common/UDPSocket.cpp
   )
 
-# Keep track of whether we want to provide a define for the
-# Python's architecture-specific lib path (i.e. where a
-# Python lldb module would go).
-set (get_python_libdir 0)
-
 if (NOT LLDB_DISABLE_LIBEDIT)
   add_host_subdirectory(common
     common/Editline.cpp
     )
 endif()
 
 add_host_subdirectory(posix
   posix/ConnectionFileDescriptorPosix.cpp
   )
 
-if(NOT LLDB_DISABLE_PYTHON)
-  list(APPEND LLDB_PLUGINS lldbPluginScriptInterpreterPython)
-endif()
-
 if (CMAKE_SYSTEM_NAME MATCHES "Windows")
   add_host_subdirectory(windows
     windows/ConnectionGenericFileWindows.cpp
     windows/EditLineWin.cpp
     windows/FileSystem.cpp
     windows/Host.cpp
     windows/HostInfoWindows.cpp
     windows/HostProcessWindows.cpp
     windows/HostThreadWindows.cpp
     windows/LockFileWindows.cpp
     windows/PipeWindows.cpp
     windows/ProcessLauncherWindows.cpp
     windows/ProcessRunLock.cpp
     windows/Windows.cpp
     )
 else()
-  if (NOT LLDB_DISABLE_PYTHON)
-    # We'll grab the arch-specific python libdir on POSIX systems.
-    set (get_python_libdir 1)
-  endif()
-
   add_host_subdirectory(posix
     posix/DomainSocket.cpp
     posix/FileSystem.cpp
     posix/HostInfoPosix.cpp
     posix/HostProcessPosix.cpp
     posix/HostThreadPosix.cpp
     posix/LockFilePosix.cpp
     posix/PipePosix.cpp
else()
   elseif (CMAKE_SYSTEM_NAME MATCHES "OpenBSD")
     add_host_subdirectory(openbsd
       openbsd/Host.cpp
       openbsd/HostInfoOpenBSD.cpp
       )
   endif()
 endif()
 
-if (${get_python_libdir})
-  # Call a python script to gather the arch-specific libdir for
-  # modules like the lldb module.
-  execute_process(
-    COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/../../scripts/get_relative_lib_dir.py
-    RESULT_VARIABLE get_libdir_status
-    OUTPUT_VARIABLE relative_libdir
-    )
-  if (get_libdir_status EQUAL 0)
-    add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${relative_libdir}")
-  endif()
-endif()
-
 set(EXTRA_LIBS)
 if (CMAKE_SYSTEM_NAME MATCHES "NetBSD")
   list(APPEND EXTRA_LIBS kvm)
 endif ()
 if (APPLE)
   list(APPEND EXTRA_LIBS xml2)
 else ()
   if (LIBXML2_FOUND)

 add_lldb_library(lldbHost
   ${HOST_SOURCES}
 
   LINK_LIBS
     lldbCore
     lldbSymbol
     lldbTarget
     lldbUtility
-    ${LLDB_PLUGINS}
     ${EXTRA_LIBS}
     ${LLDBObjCLibs}
 
   LINK_COMPONENTS
     Object
     Support
   )

lldb/trunk/source/Host/common/HostInfoBase.cpp

struct HostInfoBaseFields {
   std::string m_host_triple;
 
   ArchSpec m_host_arch_32;
   ArchSpec m_host_arch_64;
 
   FileSpec m_lldb_so_dir;
   FileSpec m_lldb_support_exe_dir;
   FileSpec m_lldb_headers_dir;
-  FileSpec m_lldb_python_dir;
   FileSpec m_lldb_clang_resource_dir;
   FileSpec m_lldb_system_plugin_dir;
   FileSpec m_lldb_user_plugin_dir;
   FileSpec m_lldb_process_tmp_dir;
   FileSpec m_lldb_global_tmp_dir;
 };
 
 HostInfoBaseFields *g_fields = nullptr;
FileSpec HostInfoBase::GetHeaderDir() {
   llvm::call_once(g_once_flag, []() {
     success = HostInfo::ComputeHeaderDirectory(g_fields->m_lldb_headers_dir);
     Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
     LLDB_LOG(log, "header dir -> `{0}`", g_fields->m_lldb_headers_dir);
   });
   return success ? g_fields->m_lldb_headers_dir : FileSpec();
 }
 
-FileSpec HostInfoBase::GetPythonDir() {
-  static llvm::once_flag g_once_flag;
-  static bool success = false;
-  llvm::call_once(g_once_flag, []() {
-    success = HostInfo::ComputePythonDirectory(g_fields->m_lldb_python_dir);
-    Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
-    LLDB_LOG(log, "python dir -> `{0}`", g_fields->m_lldb_python_dir);
-  });
-  return success ? g_fields->m_lldb_python_dir : FileSpec();
-}
-
 FileSpec HostInfoBase::GetSystemPluginDir() {
   static llvm::once_flag g_once_flag;
   static bool success = false;
   llvm::call_once(g_once_flag, []() {
     success = HostInfo::ComputeSystemPluginsDirectory(
         g_fields->m_lldb_system_plugin_dir);
     Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
     LLDB_LOG(log, "system plugin dir -> `{0}`",

lldb/trunk/source/Host/macosx/objcxx/HostInfoMacOSX.mm

 //===-- HostInfoMacOSX.mm ---------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
-#if !defined(LLDB_DISABLE_PYTHON)
-#include "Plugins/ScriptInterpreter/Python/lldb-python.h"
-#endif
-
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/macosx/HostInfoMacOSX.h"
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/SafeMachO.h"
 
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/FileSystem.h"
if (framework_pos != std::string::npos) {
     raw_path.resize(framework_pos);
     raw_path.append("/Headers");
   }
   file_spec.GetDirectory().SetString(
       llvm::StringRef(raw_path.c_str(), raw_path.size()));
   return true;
 }
 
-bool HostInfoMacOSX::ComputePythonDirectory(FileSpec &file_spec) {
-#ifndef LLDB_DISABLE_PYTHON
-  FileSpec lldb_file_spec = GetShlibDir();
-  if (!lldb_file_spec)
-    return false;
-
-  std::string raw_path = lldb_file_spec.GetPath();
-
-  size_t framework_pos = raw_path.find("LLDB.framework");
-  if (framework_pos != std::string::npos) {
-    framework_pos += strlen("LLDB.framework");
-    raw_path.resize(framework_pos);
-    raw_path.append("/Resources/Python");
-  } else {
-    llvm::SmallString<256> python_version_dir;
-    llvm::raw_svector_ostream os(python_version_dir);
-    os << "/python" << PY_MAJOR_VERSION << '.' << PY_MINOR_VERSION
-       << "/site-packages";
-
-    // We may get our string truncated. Should we protect this with an assert?
-    raw_path.append(python_version_dir.c_str());
-  }
-  file_spec.GetDirectory().SetString(
-      llvm::StringRef(raw_path.c_str(), raw_path.size()));
-  return true;
-#else
-  return false;
-#endif
-}
-
 bool HostInfoMacOSX::ComputeSystemPluginsDirectory(FileSpec &file_spec) {
   FileSpec lldb_file_spec = GetShlibDir();
   if (!lldb_file_spec)
     return false;
 
   std::string raw_path = lldb_file_spec.GetPath();
 
   size_t framework_pos = raw_path.find("LLDB.framework");

lldb/trunk/source/Host/posix/HostInfoPosix.cpp

 //===-- HostInfoPosix.cpp ---------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
-#if !defined(LLDB_DISABLE_PYTHON)
-#include "Plugins/ScriptInterpreter/Python/lldb-python.h"
-#endif
-
 #include "lldb/Host/posix/HostInfoPosix.h"
 #include "lldb/Utility/Log.h"
 
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 

 }
 
 bool HostInfoPosix::ComputeHeaderDirectory(FileSpec &file_spec) {
   FileSpec temp_file("/opt/local/include/lldb", false);
   file_spec.GetDirectory().SetCString(temp_file.GetPath().c_str());
   return true;
 }
 
-bool HostInfoPosix::ComputePythonDirectory(FileSpec &file_spec) {
-#ifndef LLDB_DISABLE_PYTHON
-  FileSpec lldb_file_spec = GetShlibDir();
-  if (!lldb_file_spec)
-    return false;
-
-  char raw_path[PATH_MAX];
-  lldb_file_spec.GetPath(raw_path, sizeof(raw_path));
-
-#if defined(LLDB_PYTHON_RELATIVE_LIBDIR)
-  // Build the path by backing out of the lib dir, then building with whatever
-  // the real python interpreter uses.  (e.g. lib for most, lib64 on RHEL
-  // x86_64).
-  char python_path[PATH_MAX];
-  ::snprintf(python_path, sizeof(python_path), "%s/../%s", raw_path,
-             LLDB_PYTHON_RELATIVE_LIBDIR);
-
-  char final_path[PATH_MAX];
-  realpath(python_path, final_path);
-  file_spec.GetDirectory().SetCString(final_path);
-
-  return true;
-#else
-  llvm::SmallString<256> python_version_dir;
-  llvm::raw_svector_ostream os(python_version_dir);
-  os << "/python" << PY_MAJOR_VERSION << '.' << PY_MINOR_VERSION
-     << "/site-packages";
-
-  // We may get our string truncated. Should we protect this with an assert?
-  ::strncat(raw_path, python_version_dir.c_str(),
-            sizeof(raw_path) - strlen(raw_path) - 1);
-
-  file_spec.GetDirectory().SetCString(raw_path);
-  return true;
-#endif
-#else
-  return false;
-#endif
-}
-
 bool HostInfoPosix::GetEnvironmentVar(const std::string &var_name,
                                       std::string &var) {
   if (const char *pvar = ::getenv(var_name.c_str())) {
     var = std::string(pvar);
     return true;
   }
   return false;
 }

lldb/trunk/source/Host/windows/HostInfoWindows.cpp

 }
 
 FileSpec HostInfoWindows::GetDefaultShell() {
   std::string shell;
   GetEnvironmentVar("ComSpec", shell);
   return FileSpec(shell, false);
 }
 
-bool HostInfoWindows::ComputePythonDirectory(FileSpec &file_spec) {
-  FileSpec lldb_file_spec = GetShlibDir();
-  if (!lldb_file_spec)
-    return false;
-  llvm::SmallString<64> path(lldb_file_spec.GetDirectory().AsCString());
-  llvm::sys::path::remove_filename(path);
-  llvm::sys::path::append(path, "lib", "site-packages");
-  std::replace(path.begin(), path.end(), '\\', '/');
-  file_spec.GetDirectory().SetString(path.c_str());
-  return true;
-}
-
 bool HostInfoWindows::GetEnvironmentVar(const std::string &var_name,
                                         std::string &var) {
   std::wstring wvar_name;
   if (!llvm::ConvertUTF8toWide(var_name, wvar_name))
     return false;
 
   if (const wchar_t *wvar = _wgetenv(wvar_name.c_str()))
     return llvm::convertWideToUTF8(wvar, var);
   return false;
 }

lldb/trunk/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt

+if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows" AND NOT LLDB_DISABLE_PYTHON)
+  # Call a python script to gather the arch-specific libdir for
+  # modules like the lldb module.
+  execute_process(
+    COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/../../../../scripts/get_relative_lib_dir.py
+    RESULT_VARIABLE get_libdir_status
+    OUTPUT_VARIABLE relative_libdir
+    )
+  if (get_libdir_status EQUAL 0)
+    add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${relative_libdir}")
+  endif()
+endif()
+
 add_lldb_library(lldbPluginScriptInterpreterPython PLUGIN
   PythonDataObjects.cpp
   PythonExceptionState.cpp
   ScriptInterpreterPython.cpp
 
   LINK_LIBS
     lldbBreakpoint
     lldbCore
     lldbDataFormatters
     lldbHost
     lldbInterpreter
     lldbTarget
   LINK_COMPONENTS
     Support
   )

lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h

#endif
 
   static lldb::ScriptInterpreterSP
   CreateInstance(CommandInterpreter &interpreter);
 
   static lldb_private::ConstString GetPluginNameStatic();
 
   static const char *GetPluginDescriptionStatic();
 
+  static FileSpec GetPythonDir();
+
   //------------------------------------------------------------------
   // PluginInterface protocol
   //------------------------------------------------------------------
   lldb_private::ConstString GetPluginName() override;
 
   uint32_t GetPluginVersion() override;
 
   class Locker : public ScriptInterpreterLocker {
public:
 
     ~SynchronicityHandler();
   };
 
   enum class AddLocation { Beginning, End };
 
   static void AddToSysPath(AddLocation location, std::string path);
 
+  static void ComputePythonDirForApple(llvm::SmallVectorImpl<char> &path);
+  static void ComputePythonDirForPosix(llvm::SmallVectorImpl<char> &path);
+  static void ComputePythonDirForWindows(llvm::SmallVectorImpl<char> &path);
+
   bool EnterSession(uint16_t on_entry_flags, FILE *in, FILE *out, FILE *err);
 
   void LeaveSession();
 
   void SaveTerminalState(int fd);
 
   void RestoreTerminalState();
 

lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

lldb_private::ConstString ScriptInterpreterPython::GetPluginNameStatic() {
   static ConstString g_name("script-python");
   return g_name;
 }
 
 const char *ScriptInterpreterPython::GetPluginDescriptionStatic() {
   return "Embedded Python interpreter";
 }
 
+void ScriptInterpreterPython::ComputePythonDirForApple(
+    llvm::SmallVectorImpl<char> &path) {
+  auto style = llvm::sys::path::Style::posix;
+
+  llvm::StringRef path_ref(path.begin(), path.size());
+  auto rbegin = llvm::sys::path::rbegin(path_ref, style);
+  auto rend = llvm::sys::path::rend(path_ref);
+  auto framework = std::find(rbegin, rend, "LLDB.framework");
+  if (framework == rend) {
+    ComputePythonDirForPosix(path);
+    return;
+  }
+  path.resize(framework - rend);
+  llvm::sys::path::append(path, style, "LLDB.framework", "Resources", "Python");
+}
+
+void ScriptInterpreterPython::ComputePythonDirForPosix(
+    llvm::SmallVectorImpl<char> &path) {
+  auto style = llvm::sys::path::Style::posix;
+#if defined(LLDB_PYTHON_RELATIVE_LIBDIR)
+  // Build the path by backing out of the lib dir, then building with whatever
+  // the real python interpreter uses.  (e.g. lib for most, lib64 on RHEL
+  // x86_64).
+  llvm::sys::path::remove_filename(path, style);
+  llvm::sys::path::append(path, style, LLDB_PYTHON_RELATIVE_LIBDIR);
+#else
+  llvm::sys::path::append(path, style,
+                          "python" + llvm::Twine(PY_MAJOR_VERSION) + "." +
+                              llvm::Twine(PY_MINOR_VERSION),
+                          "site-packages");
+#endif
+}
+
+void ScriptInterpreterPython::ComputePythonDirForWindows(
+    llvm::SmallVectorImpl<char> &path) {
+  auto style = llvm::sys::path::Style::windows;
+  llvm::sys::path::remove_filename(path, style);
+  llvm::sys::path::append(path, style, "lib", "site-packages");
+
+  // This will be injected directly through FileSpec.GetDirectory().SetString(),
+  // so we need to normalize manually.
+  std::replace(path.begin(), path.end(), '\\', '/');
+}
+
+FileSpec ScriptInterpreterPython::GetPythonDir() {
+  static FileSpec g_spec = []() {
+    FileSpec spec = HostInfo::GetShlibDir();
+    if (!spec)
+      return FileSpec();
+    llvm::SmallString<64> path;
+    spec.GetPath(path);
+
+#if defined(__APPLE__)
+    ComputePythonDirForApple(path);
+#elif defined(_WIN32)
+    ComputePythonDirForWindows(path);
+#else
+    ComputePythonDirForPosix(path);
+#endif
+    spec.GetDirectory().SetString(path);
+    return spec;
+  }();
+  return g_spec;
+}
+
 lldb_private::ConstString ScriptInterpreterPython::GetPluginName() {
   return GetPluginNameStatic();
 }
 
 uint32_t ScriptInterpreterPython::GetPluginVersion() { return 1; }
 
 void ScriptInterpreterPython::IOHandlerActivated(IOHandler &io_handler) {
   const char *instructions = nullptr;
void ScriptInterpreterPython::InitializePrivate() {
 
   PyRun_SimpleString("import sys");
   AddToSysPath(AddLocation::End, ".");
 
   // Don't denormalize paths when calling file_spec.GetPath().  On platforms
   // that use a backslash as the path separator, this will result in executing
   // python code containing paths with unescaped backslashes.  But Python also
   // accepts forward slashes, so to make life easier we just use that.
-  if (FileSpec file_spec = HostInfo::GetPythonDir())
+  if (FileSpec file_spec = GetPythonDir())
     AddToSysPath(AddLocation::Beginning, file_spec.GetPath(false));
   if (FileSpec file_spec = HostInfo::GetShlibDir())
     AddToSysPath(AddLocation::Beginning, file_spec.GetPath(false));
 
   PyRun_SimpleString("sys.dont_write_bytecode = 1; import "
                      "lldb.embedded_interpreter; from "
                      "lldb.embedded_interpreter import run_python_interpreter; "
                      "from lldb.embedded_interpreter import run_one_line");