[Lldb-commits] [PATCH] D59854: [Host] Add LibraryLoader abstraction around dlopen/LoadLibrary

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

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.



================
Comment at: lldb/include/lldb/Host/LibraryLoader.h:17
+public:
+  LibraryLoader(const char *library, bool close = true);
+  ~LibraryLoader();
----------------
I'm curious why anyone would choose `close = false`


================
Comment at: lldb/include/lldb/Host/LibraryLoader.h:17
+public:
+  LibraryLoader(const char *library, bool close = true);
+  ~LibraryLoader();
----------------
amccarth wrote:
> I'm curious why anyone would choose `close = false`
Why `const char *` here when the GetSymbol method takes a StringRef?


================
Comment at: lldb/include/lldb/Host/LibraryLoader.h:20
+
+  operator bool() { return m_handle != nullptr; }
+
----------------
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.


================
Comment at: lldb/source/Host/windows/LibraryLoader.cpp:17
+    : m_handle(nullptr), m_close(close) {
+  m_handle = reinterpret_cast<void *>(LoadLibrary(library));
+}
----------------
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.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59854/new/

https://reviews.llvm.org/D59854