Skip to content
Merged
3 changes: 3 additions & 0 deletions lldb/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ if (LLDB_ENABLE_PYTHON)
"Path to python interpreter exectuable, relative to python's install prefix")
set(cachestring_LLDB_PYTHON_EXT_SUFFIX
"Filename extension for native code python modules")
set(cachestring_LLDB_PYTHON_RUNTIME_LIBRARY_FILENAME
"Filename of Python's runtime library")

foreach(var LLDB_PYTHON_RELATIVE_PATH LLDB_PYTHON_EXE_RELATIVE_PATH LLDB_PYTHON_EXT_SUFFIX)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the cachestring_ above is to be used, we should include LLDB_PYTHON_RUNTIME_LIBRARY_FILENAME in this loop too (if that makes sense? not familiar with what this does...). Otherwise the cachestring_ variable isn't used at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it, thanks 👍

if(NOT DEFINED ${var} AND NOT CMAKE_CROSSCOMPILING)
Expand All @@ -87,6 +89,7 @@ if (LLDB_ENABLE_PYTHON)
set(LLDB_PYTHON_EXT_SUFFIX "_d${LLDB_PYTHON_EXT_SUFFIX}")
endif()
endif()
get_filename_component(LLDB_PYTHON_RUNTIME_LIBRARY_FILENAME Python3_RUNTIME_LIBRARY NAME)
endif ()

if (LLDB_ENABLE_LUA)
Expand Down
3 changes: 3 additions & 0 deletions lldb/tools/driver/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ add_dependencies(lldb
if(DEFINED LLDB_PYTHON_DLL_RELATIVE_PATH)
target_compile_definitions(lldb PRIVATE LLDB_PYTHON_DLL_RELATIVE_PATH="${LLDB_PYTHON_DLL_RELATIVE_PATH}")
endif()
if(DEFINED LLDB_PYTHON_SHARED_LIBRARY_FILENAME)
target_compile_definitions(lldb PRIVATE LLDB_PYTHON_SHARED_LIBRARY_FILENAME="${LLDB_PYTHON_SHARED_LIBRARY_FILENAME}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems broken/mismatched right now; this uses LLDB_PYTHON_SHARED_LIBRARY_FILENAME while the rest of CMake, and the C++ code, uses LLDB_PYTHON_RUNTIME_LIBRARY_FILENAME.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks 👍

endif()

if(LLDB_BUILD_FRAMEWORK)
# In the build-tree, we know the exact path to the framework directory.
Expand Down
57 changes: 47 additions & 10 deletions lldb/tools/driver/Driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,8 @@ SBError Driver::ProcessArgs(const opt::InputArgList &args, bool &exiting) {
return error;
}

#if defined(_WIN32) && defined(LLDB_PYTHON_DLL_RELATIVE_PATH)
#ifdef _WIN32
#ifdef LLDB_PYTHON_DLL_RELATIVE_PATH
/// Returns the full path to the lldb.exe executable.
inline std::wstring GetPathToExecutableW() {
// Iterate until we reach the Windows API maximum path length (32,767).
Expand All @@ -447,30 +448,66 @@ inline std::wstring GetPathToExecutableW() {
return L"";
}

/// Resolve the full path of the directory defined by
/// \brief Resolve the full path of the directory defined by
/// LLDB_PYTHON_DLL_RELATIVE_PATH. If it exists, add it to the list of DLL
/// search directories.
void AddPythonDLLToSearchPath() {
/// \return `true` if the library was added to the search path.
/// `false` otherwise.
bool AddPythonDLLToSearchPath() {
std::wstring modulePath = GetPathToExecutableW();
if (modulePath.empty()) {
llvm::errs() << "error: unable to find python.dll." << '\n';
return;
return false;
}

SmallVector<char, MAX_PATH> utf8Path;
if (sys::windows::UTF16ToUTF8(modulePath.c_str(), modulePath.length(),
utf8Path))
return;
return false;
sys::path::remove_filename(utf8Path);
sys::path::append(utf8Path, LLDB_PYTHON_DLL_RELATIVE_PATH);
sys::fs::make_absolute(utf8Path);

SmallVector<wchar_t, 1> widePath;
if (sys::windows::widenPath(utf8Path.data(), widePath))
return;
return false;

if (sys::fs::exists(utf8Path))
SetDllDirectoryW(widePath.data());
return SetDllDirectoryW(widePath.data());
return false;
}
#endif

#ifdef LLDB_PYTHON_RUNTIME_LIBRARY_FILENAME
/// Returns whether `python3x.dll` is in the DLL search path.
bool IsPythonDLLInPath() {
#define WIDEN2(x) L##x
#define WIDEN(x) WIDEN2(x)
WCHAR foundPath[MAX_PATH];
DWORD result =
SearchPathW(nullptr, WIDEN(LLDB_PYTHON_RUNTIME_LIBRARY_FILENAME), nullptr,
MAX_PATH, foundPath, nullptr);
#undef WIDEN2
#undef WIDEN

return result > 0;
}
#endif

void SetupPythonRuntimeLibrary() {
#ifdef LLDB_PYTHON_RUNTIME_LIBRARY_FILENAME
if (IsPythonDLLInPath())
return;
#ifdef LLDB_PYTHON_DLL_RELATIVE_PATH
if (AddPythonDLLToSearchPath())
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want a trailing ; here, or do you want this to expand to return (void expression);?

From the code logic, I think you want return; here, and reduce the indent level of the llvm::errs()<<... below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want a trailing ; here, or do you want this to expand to return (void expression);?

I should add a ;, that's an omission. That's probably why clang-format did that weird formatting form llvm::errs().

#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In an earlier revision of this PR, we did a second IsPythonDLLInPath() check after a successful AddPythonDLLToSearchPath() - even though we did add a path, we don't still really know if it does contain the DLL we want or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added another call to IsPythonDLLInPath after a successful AddPythonDLLToSearchPath 👍

llvm::errs() << "error: unable to find "
<< LLDB_PYTHON_RUNTIME_LIBRARY_FILENAME << ".\n";
return;
#elif defined(LLDB_PYTHON_DLL_RELATIVE_PATH)
if (!AddPythonDLLToSearchPath())
llvm::errs() << "error: unable to find the Python runtime library.\n";
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we can merge the two paths and just conditionally emit the diagnostic, the handling feels identical otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning for keeping the two paths separate is to make sure the installed Python's dll takes precedence over the one we might bundle in the installer (LLDB_PYTHON_DLL_RELATIVE_PATH).

}
#endif

Expand Down Expand Up @@ -776,8 +813,8 @@ int main(int argc, char const *argv[]) {
"~/Library/Logs/DiagnosticReports/.\n");
#endif

#if defined(_WIN32) && defined(LLDB_PYTHON_DLL_RELATIVE_PATH)
AddPythonDLLToSearchPath();
#ifdef _WIN32
SetupPythonRuntimeLibrary();
#endif

// Parse arguments.
Expand Down