[Lldb-commits] [PATCH] D77287: Windows: add very basic support for `DoLoadImage`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D77287#1957472 <https://reviews.llvm.org/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 <https://godbolt.org/z/75sHMK>)

> 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 <https://github.com/llvm/llvm-project/blob/master/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp#L661> 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.


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

https://reviews.llvm.org/D77287