[Host] Add LibraryLoader abstraction around dlopen/LoadLibrary

[JDevlieghere]

Summary

Since there's no dlopen on Windows, we need a cross platform abstraction. The LibraryLoader is exactly that.

I've kept it as simple as possible for now.

As per usual I don't have access to a Windows machine. If anybody could test this before we check this in (looks at @zturner and @stella.stamenova) that would be great! ;-)

[amccarth]

The Windows code looks correct to me, but I've added a couple inline questions/comments.

I can patch it in and test it for you tomorrow morning if nobody else has done it by then. I think zturner's still on vacation for another day or two.

lldb/include/lldb/Host/LibraryLoader.h

18

I'm curious why anyone would choose close = false

18

Why const char * here when the GetSymbol method takes a StringRef?

21

Probably should be a const method.

This will work on Windows, since we know this handle is from LoadLibrary. Some other Windows handles can signal an error with INVALID_HANDLE_VALUE rather than a null pointer, but LoadLibrary doesn't. If you wanted to be extra careful, you could delegate the validity check to the concrete type, but that doesn't seem necessary and it looks like we could change it later without affecting the interface.

lldb/source/Host/windows/LibraryLoader.cpp

18

Since you're passing a char string to LoadLibrary, you might want to explicitly call LoadLibraryA (note the -A suffix).

I know that we generally don't support Unicode well in filenames throughout the LLVM system. If you know the encoded string is UTF-8, you could convert it to UTF-16 and call LoadLibraryW. If you leave it to LoadLibraryA, it'll use the user's current default code page, which almost certainly is not UTF-8.

[jingham]

It isn't safe to pass "data()" of a StringRef to dlsym.

Also, it's a little weird that the library name is a "const char *" but the symbol name is a StringRef?

[JDevlieghere]

Thanks for the review, Adrian!

lldb/include/lldb/Host/LibraryLoader.h

18

I changed it so I could pass a nullptr in the unittest.

18

I'm not sure about the semantics of "closing" the library. For the python plugin we call initialize once, so it's fine that the symbol is gone, as long as the library remains in memory.

[JDevlieghere]

In D59854#1443891, @jingham wrote:

It isn't safe to pass "data()" of a StringRef to dlsym.

Why?

[jingham]

In D59854#1443904, @JDevlieghere wrote:

In D59854#1443891, @jingham wrote:

It isn't safe to pass "data()" of a StringRef to dlsym.

Why?

StringRef's aren't guaranteed to be NULL terminated. Or rather, the Data that gets returned by the data() method is just the raw pointer in the StringRef, which isn't guaranteed to be NULL terminated.

[zturner]

In D59854#1443906, @jingham wrote:

In D59854#1443904, @JDevlieghere wrote:

In D59854#1443891, @jingham wrote:

It isn't safe to pass "data()" of a StringRef to dlsym.

Why?

StringRef's aren't guaranteed to be NULL terminated. Or rather, the Data that gets returned by the data() method is just the raw pointer in the StringRef, which isn't guaranteed to be NULL terminated.

While true, I don't think this is a good reason to make the interface accept a const char*, because that means that the interface is exposing an implementation detail. Instead, the interface should accept a StringRef, and the implementation can pass foo.str().c_str(). It might be slightly less efficient (almost to the point of minutia), but this isn't really performance sensitive code, as loading the library will dominate the call to malloc / memcpy.

[jingham]

In D59854#1443915, @zturner wrote:

In D59854#1443906, @jingham wrote:

In D59854#1443904, @JDevlieghere wrote:

In D59854#1443891, @jingham wrote:

It isn't safe to pass "data()" of a StringRef to dlsym.

Why?

StringRef's aren't guaranteed to be NULL terminated. Or rather, the Data that gets returned by the data() method is just the raw pointer in the StringRef, which isn't guaranteed to be NULL terminated.

While true, I don't think this is a good reason to make the interface accept a const char*, because that means that the interface is exposing an implementation detail. Instead, the interface should accept a StringRef, and the implementation can pass foo.str().c_str(). It might be slightly less efficient (almost to the point of minutia), but this isn't really performance sensitive code, as loading the library will dominate the call to malloc / memcpy.

I don't actually care one way or the other about using a StringRef here (though I still think it is odd to have the library be a char * and the symbol a StringRef...). I was only concerned that you can't pass the data() of a StringRef to anything that expects a c-string.

[JDevlieghere]

data() -> str().c_str()

[mgorny]

Is there a reason you can't reuse/extend DynamicLibrary from LLVMSupport?

[labath]

In D59854#1444108, @mgorny wrote:

Is there a reason you can't reuse/extend DynamicLibrary from LLVMSupport?

+1. It sounds like you don't need anything fancy here, and this functionality should already be available in llvm.

Also, this is not relevant here, but as I think I know where this is heading, I feel like I should point out that the API surface shared with the loaded library will need to be annotated with __declspec(dllimport/export) in order for things to work on windows. If you're going to be dlopening libpython (that's how I'd probably do it, but it wouldn't give us real "plugins"), then this should already be taken care of. However, if you want to dlopen some lldb custom library, then any stuff that crosses the dll boundary will probably need to be anotated (this may include llvm classes like StringRef etc.)...

[JDevlieghere]

Apparently Phabricator is again not importing e-mail replies. As I said per e-mail, I didn't know about the existence of DynamicLibrary in LLVM and only checked LLDB for prior art.

lldb/include/lldb/Host/LibraryLoader.h

+//===-- LibraryLoader.h -----------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef liblldb_Host_LibraryLoader_h
+#define liblldb_Host_LibraryLoader_h
+
+#include "llvm/ADT/StringRef.h"
+
+namespace lldb_private {
+class LibraryLoader {
+public:
+  LibraryLoader(llvm::StringRef library)
+      : LibraryLoader(library.str().c_str()) {}
+  ~LibraryLoader();
+
+  operator bool() const { return m_handle != nullptr; }
+
+  template <typename T> T *GetSymbol(llvm::StringRef symbol) {
+    return reinterpret_cast<T *>(GetSymbolImpl(symbol));
+  }
+
+protected:
+  LibraryLoader(const char *library);
+
+private:
+  void *GetSymbolImpl(llvm::StringRef symbol);
+  void *m_handle;
+};
+} // namespace lldb_private
+
+#endif

lldb/source/Host/CMakeLists.txt

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/LibraryLoader.cpp
     windows/LockFileWindows.cpp
     windows/PipeWindows.cpp
     windows/ProcessLauncherWindows.cpp
     windows/ProcessRunLock.cpp
     windows/Windows.cpp
     )
 else()
   add_host_subdirectory(posix
     posix/DomainSocket.cpp
     posix/FileSystem.cpp
     posix/HostInfoPosix.cpp
     posix/HostProcessPosix.cpp
     posix/HostThreadPosix.cpp
+    posix/LibraryLoader.cpp
     posix/LockFilePosix.cpp
     posix/PipePosix.cpp
     posix/ProcessLauncherPosixFork.cpp
     )
 
   if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
     include_directories(SYSTEM ${LIBXML2_INCLUDE_DIR})
     add_subdirectory(macosx/objcxx)
     set(LLDBObjCLibs lldbHostMacOSXObjCXX)
     add_host_subdirectory(macosx
       macosx/cfcpp/CFCBundle.cpp
       macosx/cfcpp/CFCData.cpp
       macosx/cfcpp/CFCMutableArray.cpp
       macosx/cfcpp/CFCMutableDictionary.cpp
       macosx/cfcpp/CFCMutableSet.cpp
       macosx/cfcpp/CFCString.cpp
       )
     if(IOS)
-      set_property(SOURCE macosx/Host.mm APPEND PROPERTY 
+      set_property(SOURCE macosx/Host.mm APPEND PROPERTY
                COMPILE_DEFINITIONS "NO_XPC_SERVICES=1")
     endif()
 
 
   elseif (CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
     add_host_subdirectory(linux
       linux/AbstractSocket.cpp
       linux/Host.cpp

lldb/source/Host/posix/LibraryLoader.cpp

+//===-- LibraryLoader.cpp ---------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Host/LibraryLoader.h"
+#include <dlfcn.h>
+
+using namespace lldb_private;
+using namespace llvm;
+
+LibraryLoader::LibraryLoader(const char *library) : m_handle(nullptr) {
+  m_handle = dlopen(library, RTLD_LAZY);
+}
+
+LibraryLoader::~LibraryLoader() {
+  if (m_handle) {
+    dlclose(m_handle);
+    m_handle = nullptr;
+  }
+}
+
+void *LibraryLoader::GetSymbolImpl(llvm::StringRef symbol) {
+  if (!m_handle)
+    return nullptr;
+  return dlsym(m_handle, symbol.str().c_str());
+}

lldb/source/Host/windows/LibraryLoader.cpp

+//===-- LibraryLoader.cpp ---------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Host/LibraryLoader.h"
+#include <windows.h>
+
+using namespace lldb_private;
+using namespace llvm;
+
+LibraryLoader::LibraryLoader(const char *library) : m_handle(nullptr) {
+  m_handle = reinterpret_cast<void *>(LoadLibraryA(library));
+}
+
+LibraryLoader::~LibraryLoader() {
+  if (m_handle) {
+    FreeLibrary(m_handle);
+    m_handle = nullptr;
+  }
+}
+
+void *LibraryLoader::GetSymbolImpl(llvm::StringRef symbol) {
+  if (!m_handle)
+    return nullptr;
+  return GetProcAddress(reinterpret_cast<HMODULE>(m_handle),
+                        symbol.str().c_str());
+}

lldb/unittests/Host/CMakeLists.txt

 set (FILES
   FileActionTest.cpp
   FileSystemTest.cpp
   HostInfoTest.cpp
   HostTest.cpp
+  LibraryLoaderTest.cpp
   MainLoopTest.cpp
   NativeProcessProtocolTest.cpp
   ProcessLaunchInfoTest.cpp
   SocketAddressTest.cpp
   SocketTest.cpp
   TaskPoolTest.cpp
 )
 

lldb/unittests/Host/LibraryLoaderTest.cpp

+//===-- LibraryLoaderTest.cpp -----------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Host/LibraryLoader.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+typedef size_t strlen_t(const char *str);
+
+class TestLibraryLoader : public LibraryLoader {
+public:
+  TestLibraryLoader(const char *library) : LibraryLoader(library) {}
+};
+
+TEST(LibraryLoaderTest, OperatorBool) {
+  // Loading a bogus library fails.
+  TestLibraryLoader bogus("bogus");
+  EXPECT_FALSE(static_cast<bool>(bogus));
+
+  // Loading ourselves succeeds.
+  TestLibraryLoader self(nullptr);
+  EXPECT_TRUE(static_cast<bool>(self));
+}
+
+TEST(LibraryLoaderTest, GetSymbol) {
+  TestLibraryLoader loader(nullptr);
+  EXPECT_TRUE(static_cast<bool>(loader));
+
+  strlen_t *test = loader.GetSymbol<strlen_t>("strlen");
+
+  ASSERT_NE(nullptr, test);
+  EXPECT_EQ((size_t)4, test("test"));
+
+  strlen_t *bogus = loader.GetSymbol<strlen_t>("bogus");
+  EXPECT_EQ(nullptr, bogus);
+}