Page MenuHomePhabricator

Windows: add very basic support for `DoLoadImage`
Needs ReviewPublic

Authored by compnerd on Apr 1 2020, 10:06 PM.

Details

Summary

Add some very basic support for DoLoadImage and UnloadImage for Windows. This was previously not implemented and would result in a failure at runtime that is hard to detect.

This implementation is extremely limited but serves as a starting point for proper support for loading a library. Ideally, the user input would be converted from UTF-8 to UTF-16. This requires additional heap allocations and conversion logic. Error recovery there requires additional allocations both from the local heap and the global heap.

This support enables the use of LLDB's Swift REPL on Windows.

Diff Detail

Event Timeline

compnerd created this revision.Apr 1 2020, 10:06 PM
labath added a subscriber: labath.Apr 2 2020, 2:40 AM

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 added a comment.EditedApr 2 2020, 9:24 AM

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.

compnerd updated this revision to Diff 254631.Apr 2 2020, 3:14 PM

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 added a comment.EditedApr 3 2020, 12:12 PM

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.

compnerd updated this revision to Diff 254883.Apr 3 2020, 12:16 PM
JDevlieghere added inline comments.Apr 3 2020, 12:20 PM
lldb/test/Shell/Process/Windows/process_load.cpp
4

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 marked an inline comment as done.Apr 3 2020, 12:33 PM
compnerd added inline comments.
lldb/test/Shell/Process/Windows/process_load.cpp
4

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

labath added a comment.Apr 6 2020, 1:50 AM

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.

This comment was removed by compnerd.

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 updated this revision to Diff 255580.Apr 6 2020, 9:09 PM
compnerd updated this revision to Diff 255581.Apr 6 2020, 9:18 PM