Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lldb/cmake/modules/AddLLDB.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,12 @@ function(add_lldb_executable name)
)

target_link_libraries(${name} PRIVATE ${ARG_LINK_LIBS})
if(WIN32)
list(FIND ARG_LINK_LIBS liblldb LIBLLD_INDEX)
Copy link
Member

Choose a reason for hiding this comment

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

nit: missing a B -- LIBLLDB_INDEX

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!

if(NOT LIBLLD_INDEX EQUAL -1)
target_link_options(${name} PRIVATE "/DELAYLOAD:$<TARGET_FILE_BASE_NAME:liblldb>.dll")
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason for using $<TARGET_FILE_BASE_NAME:liblldb>.dll rather than the more straightforward $<TARGET_FILE_NAME:liblldb>?

This patch broke compilation for mingw targets, as these options are specific to the MSVC style build case - and for that case, the DLL name we'd like to pass here has a lib prefix, i.e. liblldb.dll, not lldb.dll. So instead of trying to manually reapply a prefix and add .dll at the end, it seems more straightforward to me to just use $<TARGET_FILE_NAME:liblldb> which should give the right string in both cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to be gated behind a if(MSVC) check in the downstream patch: swiftlang@af218c3. I think that with #162831 we will get a complete solution as if LLDB_PYTHON_DLL_RELATIVE_PATH is not set, the mechanism to add to the DLL search path will not even be compiled. If it's set, then it will only work for MSVC and mingw and there will be a warning if it's a different compiler.

which should give the right string in both cases?

I agree, this is much better 👍

endif()
endif()
if(CLANG_LINK_CLANG_DYLIB)
target_link_libraries(${name} PRIVATE clang-cpp)
else()
Expand Down
4 changes: 4 additions & 0 deletions lldb/tools/driver/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ add_dependencies(lldb
${tablegen_deps}
)

if(DEFINED LLDB_PYTHON_DLL_RELATIVE_PATH)
target_compile_definitions(lldb PRIVATE LLDB_PYTHON_DLL_RELATIVE_PATH="${LLDB_PYTHON_DLL_RELATIVE_PATH}")
endif()

if(LLDB_BUILD_FRAMEWORK)
# In the build-tree, we know the exact path to the framework directory.
# The installed framework can be in different locations.
Expand Down
52 changes: 52 additions & 0 deletions lldb/tools/driver/Driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,21 @@
#include "lldb/Host/MainLoop.h"
#include "lldb/Host/MainLoopBase.h"
#include "lldb/Utility/Status.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/ConvertUTF.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Format.h"
#include "llvm/Support/InitLLVM.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/WithColor.h"
#include "llvm/Support/raw_ostream.h"

#if _WIN32
Copy link
Member

Choose a reason for hiding this comment

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

Should this be an ifdef?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should, I'm surprised that it worked before.

#include "llvm/Support/Windows/WindowsSupport.h"
#endif

#include <algorithm>
#include <atomic>
#include <bitset>
Expand Down Expand Up @@ -426,6 +433,47 @@ SBError Driver::ProcessArgs(const opt::InputArgList &args, bool &exiting) {
return error;
}

#ifdef _WIN32
// Returns the full path to the lldb.exe executable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Returns the full path to the lldb.exe executable
// Returns the full path to the lldb.exe executable.

inline std::wstring GetPathToExecutableW() {
// Iterate until we reach the Windows max path length (32,767).
std::vector<WCHAR> buffer;
buffer.resize(MAX_PATH);
while (buffer.size() < 32767) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if MAX_PATH is 32767 isn't this condition always false?

Copy link
Member

Choose a reason for hiding this comment

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

There is a doubling on failure, so this shouldn't be always false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MAX_PATH is defined in the Windows API as 260. However, a lot of the Windows API functions can return Unicode paths that are up to 32,767 bytes long.

I have updated the comments to explain this better.

if (GetModuleFileNameW(NULL, buffer.data(), buffer.size()) < buffer.size())
return std::wstring(buffer.begin(), buffer.end());
buffer.resize(buffer.size() * 2);
}
return L"";
}

// 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() {
std::wstring modulePath = GetPathToExecutableW();
if (modulePath.empty()) {
WithColor::error() << "Unable to find python: " << GetLastError() << '\n';
Copy link
Collaborator

Choose a reason for hiding this comment

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

We typically with the word "error:" in the error color, and not the entire message.

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 by using llvm::errs() and changing the message to start with error:

return;
}

SmallVector<char, MAX_PATH> utf8Path;
if (sys::windows::UTF16ToUTF8(modulePath.c_str(), modulePath.length(),
utf8Path))
return;
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;

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

std::string EscapeString(std::string arg) {
std::string::size_type pos = 0;
while ((pos = arg.find_first_of("\"\\", pos)) != std::string::npos) {
Expand Down Expand Up @@ -728,6 +776,10 @@ int main(int argc, char const *argv[]) {
"~/Library/Logs/DiagnosticReports/.\n");
#endif

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

// Parse arguments.
LLDBOptTable T;
unsigned MissingArgIndex;
Expand Down
Loading