Windows: support `DoLoadImage`

[compnerd]

Summary

This implements DoLoadImage and UnloadImage in the Windows platform
plugin modelled after the POSIX platform plugin. This was previously
unimplemented and resulted in a difficult to decipher error without any
logging.

This implementation is intended to support enables the use of LLDB's
Swift REPL on Windows.

Paths which are added to the library search path are persistent and
applied to all subsequent loads. This can be adjusted in the future by
storing all the cookies and restoring the path prior to returning from
the helper. However, the dynamic path count makes this a bit more
challenging.

[labath]

We have some existing tests (functionalities/load_using_paths/TestLoadUsingPaths.py) for this functionality, but they are are all skipped on windows. Can you check if they work now, or create a new test for the subset of functionality which does work?

Ideally, the user input would be converted from UTF-8 to UTF-16.

Would it be possible to do the conversion within lldb (llvm::ConvertUTF8toWide), and pass that as a prebaked string?

[compnerd]

Thanks for the hint about the string conversion, however, I think that it's going to complicate things as the string is going to be a mixture of UTF-8 and UTF-16 content.

As to the testing, functionalities/load_using_paths/TestLoadUsingPaths.py is not applicable to Windows. In fact, I don't really see a good way to really test much of this outside the context of the swift REPL which forces the loading of a DLL which is in fact how I discovered this. If there is an easy way to ensure that the dll that is needed is in the user's PATH, then I suppose creating an empty dll and loading that is theoretically possible, but that too can have a lot of flakiness due to dependencies to build and all.

[labath]

In D77287#1957472, @compnerd wrote:

Thanks for the hint about the string conversion, however, I think that it's going to complicate things as the string is going to be a mixture of UTF-8 and UTF-16 content.

That's true. Ok, plan B: have clang do the conversion for us. If you form the expression like printf("LoadLibraryW(L\"%s\")", path_in_utf8) then the path will get transformed to utf-16 by the compiler. However the path should also get sanitized/escaped into a form suitable for passing to the C compiler. (godbolt link)

As to the testing, functionalities/load_using_paths/TestLoadUsingPaths.py is not applicable to Windows. In fact, I don't really see a good way to really test much of this outside the context of the swift REPL which forces the loading of a DLL which is in fact how I discovered this. If there is an easy way to ensure that the dll that is needed is in the user's PATH, then I suppose creating an empty dll and loading that is theoretically possible, but that too can have a lot of flakiness due to dependencies to build and all.

Ok, so your patch does not implement that functionality. It does not sound like there's a fundamental limitation there, as you could do the same thing as what the posix code does ( == iterate over the path), but that's fine -- you don't need to implement everything straight away. In that case it would be good to check that the paths argument is empty and return an error instead of attempting to load a random library.

However, I believe you are implementing sufficient functionality for the SBProcess::LoadImage (aka "process load" in CLI) command. So, maybe you could take a look at API/functionalities/load_unload/TestLoadUnload.py and see if anything there can be enabled ? Or (if the test contains too many posix specifics), you could could create a windows version of that test doing something similar.

[compnerd]

Okay, thanks to some help from @JDevlieghere I was able to get a test going for this. I think that the basic test is sufficient for this. I think that the path sanitizing and conversion should be a subsequent change.

[JDevlieghere]

lldb/test/Shell/Process/Windows/process_load.cpp

3 ↗

(On Diff #254883)

We should probably have a lit.local.cfg in the Windows directory with

if 'system-windows' not in config.available_features:
  config.unsupported = True

[compnerd]

lldb/test/Shell/Process/Windows/process_load.cpp

3 ↗

(On Diff #254883)

I think that's a good idea, but, should be a separate change - it isn't related to the load/unload functionality.

[labath]

In D77287#1960390, @compnerd wrote:

I think that the basic test is sufficient for this.

That test does not seem to be exercising the "unload" part of this patch. It would also be nice to run some basic command like "image list" to verify that the loaded binary is indeed listed.

(I don't really have a hard objection to this being a lit test, but it does sound to me like at that point, this will be reimplementing TestLoadUnload.py)

I think that the path sanitizing and conversion should be a subsequent change.

Why is that? The need for sanitation is a direct consequence of how you've chosen to implement this patch -- the posix version of this function does not do sanitation, but it does not need to, as it does not embed the library name into the compiled expression. I can see how the support for wide strings might be considered a separate feature, but given that supporting that is a matter of adding a single L to the compiled expression, I don't see a problem with including that here.

[compnerd]

This comment was removed by compnerd.

[compnerd]

In D77287#1963242, @labath wrote:

In D77287#1960390, @compnerd wrote:

I think that the basic test is sufficient for this.

That test does not seem to be exercising the "unload" part of this patch. It would also be nice to run some basic command like "image list" to verify that the loaded binary is indeed listed.

(I don't really have a hard objection to this being a lit test, but it does sound to me like at that point, this will be reimplementing TestLoadUnload.py)

I think that the path sanitizing and conversion should be a subsequent change.

Why is that? The need for sanitation is a direct consequence of how you've chosen to implement this patch -- the posix version of this function does not do sanitation, but it does not need to, as it does not embed the library name into the compiled expression. I can see how the support for wide strings might be considered a separate feature, but given that supporting that is a matter of adding a single L to the compiled expression, I don't see a problem with including that here.

Actually, it changes the APIs used and the path that this goes down on the Windows side, so it has a much broader impact than it appears.

[compnerd]

This is a more complete implementation that allows for error reporting and use of UCS-2 as that is needed for the search path alteration.

[compnerd]

Fix build rules

[compnerd]

Add missing null-terminators.

[JDevlieghere]

I can't speak to the correctness of the Windows parts, but all the utility function stuff looks sane to me. LGTM if Pavel has no outstanding objections.

lldb/source/Plugins/Platform/Windows/PlatformWindows.h

52–56

I know it's an override, but what a horrible interface...

lldb/test/Shell/Process/Windows/process_load.cpp

3 ↗

(On Diff #254883)

Fair enough

[compnerd]

lldb/source/Plugins/Platform/Windows/PlatformWindows.h

52–56

I completely agree.

lldb/test/Shell/Process/Windows/process_load.cpp

3 ↗

(On Diff #254883)

I already pushed that change, so it's already taken care of, but it is not part of the set of changes :)

[thakis]

lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp

529

../../lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp:397:62: warning: format specifies type 'unsigned long' but the argument has type 'uint64_t' (aka 'unsigned long long') [-Wformat]
    error.SetErrorStringWithFormat("LoadLibrary Error: %lu", error_code);
                                                       ~~~   ^~~~~~~~~~
                                                       %llu

[compnerd]

Thanks @thakis - 906e60b9f923464cba0f71a9205846550752162f should have addressed that from a few days ago (sorry about not mentioning that here)

lldb/source/Plugins/Platform/Windows/PlatformWindows.h

public:
   const char *GetDescription() override {
     return GetPluginDescriptionStatic(IsHost());
   }
 
   lldb_private::Status ConnectRemote(lldb_private::Args &args) override;
 
   lldb_private::Status DisconnectRemote() override;
 
+  uint32_t DoLoadImage(lldb_private::Process *process,
+                       const lldb_private::FileSpec &remote_file,
+                       const std::vector<std::string> *paths,
+                       lldb_private::Status &error,
+                       lldb_private::FileSpec *loaded_path) override;
+
+  lldb_private::Status UnloadImage(lldb_private::Process *process,
+                                   uint32_t image_token) override;
+
   lldb::ProcessSP DebugProcess(lldb_private::ProcessLaunchInfo &launch_info,
                                lldb_private::Debugger &debugger,
                                lldb_private::Target *target,
                                lldb_private::Status &error) override;
 
   lldb::ProcessSP Attach(lldb_private::ProcessAttachInfo &attach_info,
                          lldb_private::Debugger &debugger,
                          lldb_private::Target *target,
                          lldb_private::Status &error) override;
 
   bool GetSupportedArchitectureAtIndex(uint32_t idx,
                                        lldb_private::ArchSpec &arch) override;
 
   void GetStatus(lldb_private::Stream &strm) override;
 
   bool CanDebugProcess() override;
 
   // FIXME not sure what the _sigtramp equivalent would be on this platform
   void CalculateTrapHandlerSymbolNames() override {}
 
   ConstString GetFullNameForDylib(ConstString basename) override;
 
 private:
   DISALLOW_COPY_AND_ASSIGN(PlatformWindows);
+
+  lldb_private::Status EvaluateLoaderExpression(lldb_private::Process *process,
+                                                const char *expression,
+                                                lldb::ValueObjectSP &value);
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_PlatformWindows_h_

lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp

 #endif
 
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/BreakpointSite.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Core/ValueObject.h"
+#include "lldb/Expression/UserExpression.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/Status.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 static uint32_t g_initialize_count = 0;
 
if (IsHost()) {
     if (m_remote_platform_sp)
       error = m_remote_platform_sp->DisconnectRemote();
     else
       error.SetErrorString("the platform is not currently connected");
   }
   return error;
 }
 
+Status PlatformWindows::EvaluateLoaderExpression(Process *process,
+                                                 const char *expression,
+                                                 ValueObjectSP &value) {
+  // FIXME(compnerd) `-fdeclspec` is not passed to the clang instance?
+  static const char kLoaderDecls[] =
+      R"(
+          // libloaderapi.h
+
+          // WINBASEAPI BOOL WINAPI FreeModule(HMODULE);
+          extern "C" /* __declspec(dllimport) */ BOOL __stdcall FreeModule(void *hLibModule);
+
+          // WINBASEAPI HMODULE WINAPI LoadLibraryA(LPCSTR);
+          extern "C" /* __declspec(dllimport) */ void * __stdcall LoadLibraryA(const char *);
+        )";
+
+  if (DynamicLoader *loader = process->GetDynamicLoader()) {
+    Status result = loader->CanLoadImage();
+    if (result.Fail())
+      return result;
+  }
+
+  ThreadSP thread = process->GetThreadList().GetExpressionExecutionThread();
+  if (!thread)
+    return Status("selected thread is invalid");
+
+  StackFrameSP frame = thread->GetStackFrameAtIndex(0);
+  if (!frame)
+    return Status("frame 0 is invalid");
+
+  ExecutionContext context;
+  frame->CalculateExecutionContext(context);
+
+  EvaluateExpressionOptions options;
+  options.SetUnwindOnError(true);
+  options.SetIgnoreBreakpoints(true);
+  options.SetExecutionPolicy(eExecutionPolicyAlways);
+  options.SetLanguage(eLanguageTypeC_plus_plus);
+  // LoadLibrary{A,W}/FreeLibrary cannot raise exceptions which we can handle.
+  // They may potentially throw SEH exceptions which we do not know how to
+  // handle currently.
+  options.SetTrapExceptions(false);
+  options.SetTimeout(process->GetUtilityExpressionTimeout());
+
+  Status error;
+  ExpressionResults result = UserExpression::Evaluate(
+      context, options, expression, kLoaderDecls, value, error);
+  if (result != eExpressionCompleted)
+    return error;
+
+  if (value->GetError().Fail())
+    return value->GetError();
+
+  return Status();
+}
+
+uint32_t PlatformWindows::DoLoadImage(Process *process,
+                                      const FileSpec &remote_file,
+                                      const std::vector<std::string> *paths,
+                                      Status &error, FileSpec *loaded_path) {
+  if (loaded_path)
+    loaded_path->Clear();
+
+  StreamString expression;
+  expression.Printf("LoadLibraryA(\"%s\")", remote_file.GetPath().c_str());
+
+  ValueObjectSP value;
+  Status result =
+      EvaluateLoaderExpression(process, expression.GetData(), value);
+  if (result.Fail())
+    return LLDB_INVALID_IMAGE_TOKEN;
+
+  Scalar scalar;
+  if (value->ResolveValue(scalar)) {
+    lldb::addr_t address = scalar.ULongLong();
+    if (address == 0)
+      return LLDB_INVALID_IMAGE_TOKEN;
+    return process->AddImageToken(address);
+  }
+  return LLDB_INVALID_IMAGE_TOKEN;
+}
+
+Status PlatformWindows::UnloadImage(Process *process, uint32_t image_token) {
+  const addr_t address = process->GetImagePtrFromToken(image_token);
+  if (address == LLDB_INVALID_ADDRESS)
+    return Status("invalid image token");
+
+  StreamString expression;
+  expression.Printf("FreeLibrary((HMODULE)0x%" PRIx64 ")", address);
+
+  ValueObjectSP value;
+  Status result =
+      EvaluateLoaderExpression(process, expression.GetData(), value);
+  if (result.Fail())
+    return result;
+
+  Scalar scalar;
+  if (value->ResolveValue(scalar)) {
+    if (scalar.UInt(1))
+      return Status("expression failed: \"%s\"", expression.GetData());
+    process->ResetImageToken(image_token);
+  }
+
+  return Status();
+}
+
 ProcessSP PlatformWindows::DebugProcess(ProcessLaunchInfo &launch_info,
                                         Debugger &debugger, Target *target,
                                         Status &error) {
   // Windows has special considerations that must be followed when launching or
   // attaching to a process.  The key requirement is that when launching or
   // attaching to a process, you must do it from the same the thread that will
   // go into a permanent loop which will then receive debug events from the
   // process.  In particular, this means we can't use any of LLDB's generic

 }
 
 bool PlatformWindows::CanDebugProcess() { return true; }
 
 ConstString PlatformWindows::GetFullNameForDylib(ConstString basename) {
   if (basename.IsEmpty())
     return basename;
 
   StreamString stream;
   stream.Printf("%s.dll", basename.GetCString());
   return ConstString(stream.GetString());
 }