-
Couldn't load subscription status.
- Fork 15k
[lldb][windows] print an error if python.dll is not in the DLL search path #164893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[lldb][windows] print an error if python.dll is not in the DLL search path #164893
Conversation
|
@llvm/pr-subscribers-lldb Author: Charles Zablit (charles-zablit) ChangesThis is a follow up to #162509. Using the Before #162509, when invoked from Powershell, lldb would silently crash (no error message/crash report). After #162509, it crashes without any indications that the root cause is the missing python.dll. With this patch, we print the error before crashing. Full diff: https://github.com/llvm/llvm-project/pull/164893.diff 3 Files Affected:
diff --git a/lldb/CMakeLists.txt b/lldb/CMakeLists.txt
index e3b72e94d4beb..7c85c6fa8b825 100644
--- a/lldb/CMakeLists.txt
+++ b/lldb/CMakeLists.txt
@@ -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_SHARED_LIBRARY_NAME
+ "Filename of Python's shared library")
foreach(var LLDB_PYTHON_RELATIVE_PATH LLDB_PYTHON_EXE_RELATIVE_PATH LLDB_PYTHON_EXT_SUFFIX)
if(NOT DEFINED ${var} AND NOT CMAKE_CROSSCOMPILING)
@@ -87,6 +89,8 @@ if (LLDB_ENABLE_PYTHON)
set(LLDB_PYTHON_EXT_SUFFIX "_d${LLDB_PYTHON_EXT_SUFFIX}")
endif()
endif()
+ set(LLDB_PYTHON_SHARED_LIBRARY_FILENAME
+ "python${Python3_VERSION_MAJOR}${Python3_VERSION_MINOR}${CMAKE_SHARED_LIBRARY_SUFFIX}")
endif ()
if (LLDB_ENABLE_LUA)
diff --git a/lldb/tools/driver/CMakeLists.txt b/lldb/tools/driver/CMakeLists.txt
index 67956af7fe3fb..467ca9f91b3c1 100644
--- a/lldb/tools/driver/CMakeLists.txt
+++ b/lldb/tools/driver/CMakeLists.txt
@@ -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}")
+endif()
if(LLDB_BUILD_FRAMEWORK)
# In the build-tree, we know the exact path to the framework directory.
diff --git a/lldb/tools/driver/Driver.cpp b/lldb/tools/driver/Driver.cpp
index ba0041111045b..96157525f3703 100644
--- a/lldb/tools/driver/Driver.cpp
+++ b/lldb/tools/driver/Driver.cpp
@@ -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).
@@ -447,32 +448,51 @@ 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_SHARED_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_SHARED_LIBRARY_FILENAME), nullptr,
+ MAX_PATH, foundPath, nullptr);
+#undef WIDEN2
+#undef WIDEN
+
+ return result > 0;
}
#endif
+#endif
std::string EscapeString(std::string arg) {
std::string::size_type pos = 0;
@@ -776,8 +796,13 @@ int main(int argc, char const *argv[]) {
"~/Library/Logs/DiagnosticReports/.\n");
#endif
-#if defined(_WIN32) && defined(LLDB_PYTHON_DLL_RELATIVE_PATH)
- AddPythonDLLToSearchPath();
+#if defined(_WIN32) && defined(LLDB_PYTHON_SHARED_LIBRARY_FILENAME)
+ if (!IsPythonDLLInPath())
+#ifdef LLDB_PYTHON_DLL_RELATIVE_PATH
+ if (!AddPythonDLLToSearchPath())
+#endif
+ llvm::errs() << "error: unable to find "
+ << LLDB_PYTHON_SHARED_LIBRARY_FILENAME << ".\n";
#endif
// Parse arguments.
|
lldb/CMakeLists.txt
Outdated
| endif() | ||
| endif() | ||
| set(LLDB_PYTHON_SHARED_LIBRARY_FILENAME | ||
| "python${Python3_VERSION_MAJOR}${Python3_VERSION_MINOR}${CMAKE_SHARED_LIBRARY_SUFFIX}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may wish to include a cmake variable for the prefix as well; on mingw, it's libpython3.12.dll - I'm sure that there's a cmake variable that expands to lib for mingw but nothing for msvc like environments.
Also; if i understand this correctly, this variable gets set automatically, but is it possible to override this string through cmake parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched to using Python3_RUNTIME_LIBRARY which points directly to the correct dll. Hopefully that also works in mingw and other environment.
Also; if i understand this correctly, this variable gets set automatically, but is it possible to override this string through cmake parameters?
From my understanding of Cmake, it will be overwritten by the set, so the answer is no. Do you think it would be a useful variable to expose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we pick the right name for all configurations, then no, we probably don't need to expose it. From a distributor freedom point of view, it maybe would be good to have the possibility to do that. But on the other hand, we can go with this first, and make it settable if someone later tells us they'd want to do that.
lldb/tools/driver/Driver.cpp
Outdated
| AddPythonDLLToSearchPath(); | ||
| #if defined(_WIN32) && defined(LLDB_PYTHON_SHARED_LIBRARY_FILENAME) | ||
| if (!IsPythonDLLInPath()) | ||
| #ifdef LLDB_PYTHON_DLL_RELATIVE_PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition nesting doesn't seem right. If we have LLDB_PYTHON_DLL_RELATIVE_PATH defined, but we don't have LLDB_PYTHON_SHARED_LIBRARY_FILENAME, then we no longer will call AddPythonDLLToSearchPath at all - which we did before. Not sure what the neatest form of this is; maybe an #elif defined(LLDB_PYTHON_DLL_RELATIVE_PATH) AddPythonDLLToSearchPath(); #endif?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial assumption was that LLDB_PYTHON_SHARED_LIBRARY_FILENAME would always be set if LLDB_PYTHON_DLL_RELATIVE_PATH is set, because it's set if we are linking against Python.
To resolve this I've extracted the logic into a function which made it easier by using return to control the execution flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, practically speaking (as that variable is set automatically by cmake), it should always be defined. But it might be good to have the code robust for all potential combinations anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, practically speaking (as that variable is set automatically by cmake), it should always be defined. But it might be good to have the code robust for all potential combinations anyway?
I agree, the new logic allows for either of them (or both) to be defined.
I agree, I think this is the better call. As mentioned above, the current guesstimate of the python DLL name would be wrong on mingw, so I'd rather have a spurious error message than exiting altogether. (Also, I wonder if there's some way one can learn the actual python library name through the CMake FindPython scripts, rather than having to guess it; guessing it seems brittle. E.g. if we manage to switch over to the Python stable API, it can be just |
I ended up using |
According to the documentation of |
Ok, that sounds reasonable - especially as long as that is what we actually link against right now. |
lldb/tools/driver/Driver.cpp
Outdated
| return; | ||
| #ifdef LLDB_PYTHON_DLL_RELATIVE_PATH | ||
| if (AddPythonDLLToSearchPath()) | ||
| return |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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().
lldb/tools/driver/CMakeLists.txt
Outdated
| 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}") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks 👍
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it, thanks 👍
|
My understanding is that this is a variable we set ourselves, rather than one that CMake resolves:
|
I'm confused as to where this variable comes from in the first place, because it's not documented and now that I did a clean build, it's gone. I'm still trying to find a proper way of getting the dll name, there does not seem to be any variable that return the path/name. |
|
That's where it's set: https://gitlab.kitware.com/cmake/cmake/blob/master/Modules/FindPython/Support.cmake#L3865 |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@mstorsjo my understanding is that I see 2 solutions:
I prefer the first solution because it defers the logic to CMake, but you are saying that |
|
Pushing this into CMake feels like the right thing IMO. I think that we should be able to do something more aggressive if we want. We could query the |
|
Using |
|
First replying to the older messages:
I don't see alternative 1 as an option, as you yourself also said that it wasn't set for you after doing a clean build? Anyway, the current suggestion of using It's probably relevant to say, that in my cross compiling setup, I'm not letting |
|
Anyway, so with my setup of manually setting I did check the original approach, of guessing the library name, and that does seem to work to the point of getting the version number right and all. It does need adding I.e. this snippet seems to work for me: But I don't mind keeping the current suggestion, as it won't produce any spurious runtime warnings in LLDB anyway. It would be nice to check if the target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but I think that merging the two paths for the relative and regular behavior is desirable before merging.
lldb/tools/driver/Driver.cpp
Outdated
| return; | ||
| #endif | ||
| llvm::errs() << "error: unable to find " | ||
| << LLDB_PYTHON_RUNTIME_LIBRARY_FILENAME << ".\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we quote the library name in the style of literals in diagnostics (e.g. 'python311.dll').
| #elif defined(LLDB_PYTHON_DLL_RELATIVE_PATH) | ||
| if (!AddPythonDLLToSearchPath()) | ||
| llvm::errs() << "error: unable to find the Python runtime library.\n"; | ||
| #endif |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
I added a check to make sure the target is defined before trying to access its properties. This should remove the CMake warning. |
Thanks! Yes, the warning seems to be gone for me now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking mostly good, but another question about the warnings in the current form.
Other than that, this looks good to me now, if you've verified that it actually does work as expected in the final form.
| #ifdef LLDB_PYTHON_DLL_RELATIVE_PATH | ||
| if (AddPythonDLLToSearchPath()) | ||
| return; | ||
| #endif |
There was a problem hiding this comment.
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.
| #elif defined(LLDB_PYTHON_DLL_RELATIVE_PATH) | ||
| if (!AddPythonDLLToSearchPath()) | ||
| llvm::errs() << "error: unable to find the Python runtime library.\n"; | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused - why do we need the AddPythonDLLToSearchPath twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to be sure to check that python.dll is in the Path and/or try to add it to the Path whether LLDB_PYTHON_RUNTIME_LIBRARY_FILENAME and LLDB_PYTHON_DLL_RELATIVE_PATH are both set or only one or the other.
If only LLDB_PYTHON_RUNTIME_LIBRARY_FILENAME is set:
- Check if
python.dllis in the Path - Print a warning if it's not.
If only LLDB_PYTHON_DLL_RELATIVE_PATH is set:
- Try to add
python.dllto the Path
If both are set:
- Check if
python.dllis in the Path. - If it's not try to add it.
- Check again if it's in the Path (not implemented yet).
- Print a warning if it's not.

This is a follow up to #162509.
Using the
SearchPathWAPI, we can ensure that the correct version of Python is installed beforeliblldbis loaded (andpython.dllsubsequently). If it's not, we try to add it to the search path with the methods introduced in #162509. If that fails or if that method is#ifdef'd out, we print an error which will appear before lldb crashes due to the missing dll.Before #162509, when invoked from Powershell, lldb would silently crash (no error message/crash report). After #162509, it crashes without any indications that the root cause is the missing python.dll. With this patch, we print the error before crashing.