[lldb][windows] delay loading liblldb.dll and manually add Python's DLL directory #11571
Conversation
lldb/cmake/modules/AddLLDB.cmake
Outdated
| ) | ||
|
|
||
| target_link_libraries(${name} PRIVATE ${ARG_LINK_LIBS}) | ||
| if(MSVC) |
There was a problem hiding this comment.
This should be WIN32 not MSVC otherwise this will regress if you use a different compiler.
lldb/tools/driver/Driver.cpp
Outdated
| // toolchain, therefore the directory might not exist. | ||
| void AddPythonDLLToSearchPath() { | ||
| wchar_t buffer[MAX_PATH]; | ||
| DWORD length = GetModuleFileNameW(NULL, buffer, MAX_PATH); |
There was a problem hiding this comment.
This is risky - the path is possibly a long path (i.e. >MAX_PATH). In such a case, LLDB will be unusable permanently. The path returned is the path used to load, so this is not something we can control.
It is likely best to use GetModuleHandleW and then GetFinalPathNameByHandleW to get the path.
There was a problem hiding this comment.
I was not able to use GetModuleHandleW with GetFinalPathNameByHandleW directly after, there had to be an intermiedary GetModuleFileNameW and it was back to the original problem.
I found this SO thread where others are struggling with this API. I settled with the grow the buffer until it fits solution.
charles-zablit
force-pushed
the
charles-zablit/windows/delay-python-loading
branch
from
af218c3 to
764df35
Compare
lldb/tools/driver/Driver.cpp
Outdated
| inline std::wstring GetPathToExecutableW() { | ||
| // Iterate until we max out the Windows max path length (256*2^7 > 32,767). | ||
| static const size_t maximumIterations = 7; | ||
| std::wstring buffer; |
There was a problem hiding this comment.
Why not use std::vector<WCHAR>?
lldb/tools/driver/Driver.cpp
Outdated
| DWORD length = GetModuleFileNameW(NULL, &buffer[0], bufferSize); | ||
| if (length < buffer.length()) { | ||
| buffer.resize(length); | ||
| return buffer; | ||
| } |
There was a problem hiding this comment.
This could then be re-written as:
if (GetModuleFileNameW(nullptr, buffer.data(), buffer.size()) < buffer.size())
return std::wstring(buffer.begin(), buffer.end());
There was a problem hiding this comment.
Fixed, this looks better, thanks!
lldb/tools/driver/Driver.cpp
Outdated
| void AddPythonDLLToSearchPath() { | ||
| std::wstring modulePath = GetPathToExecutableW(); | ||
| if (modulePath.empty()) { | ||
| WithColor::error() << "Failed to get module file path. Error: " |
There was a problem hiding this comment.
This error message doesn't really give the user much information. How about:
"Unable to find python: " << GetLastError() << '\n';
lldb/tools/driver/Driver.cpp
Outdated
| std::wstring modulePath = GetPathToExecutableW(); | ||
| if (modulePath.empty()) { | ||
| WithColor::error() << "Failed to get module file path. Error: " | ||
| << GetLastError() << "\n"; |
There was a problem hiding this comment.
| << GetLastError() << "\n"; | |
| << GetLastError() << '\n'; |
Note that "\n" would cause unnecessary buffering.
charles-zablit
force-pushed
the
charles-zablit/windows/delay-python-loading
branch
from
764df35 to
8807d68
Compare
| static const size_t maximumIterations = 7; | ||
| std::vector<WCHAR> buffer; | ||
| DWORD bufferSize = MAX_PATH; | ||
| for (size_t i = 0; i < maximumIterations; i++) { |
There was a problem hiding this comment.
I wonder if this is better written as:
| for (size_t i = 0; i < maximumIterations; i++) { | |
| for (size_t i = 0; buffer.size() < 32767; i++) { |
| // `Swift/Toolchains/x.x.x/usr/bin/lldb.exe`. | ||
| for (int i = 0; i < 5; i++) { | ||
| sys::path::remove_filename(pythonDir); | ||
| } |
There was a problem hiding this comment.
I wonder if we can make this a CMake configuration option for a relative path, and then upstream this.
|
Can we support this configuration upstream and add all the necessary support there? |
This patch delays the load of
python310.dllon Windows, by using MSVC's/DELAYLOADparameter.This allows the use of the
SetDllDirectoryWAPI to manually addC:/toolchain-install-dir/Python-3.10.1to the list of DLL search paths.This patch is part of 2 other PRs which aim at packaging the Embeddable version of Python with the toolchain's installer: