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

Mon Apr 6 09:12:06 PDT 2020

compnerd added a comment.

In D77287#1963242 <https://reviews.llvm.org/D77287#1963242>, @labath wrote:

> In D77287#1960390 <https://reviews.llvm.org/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.

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

https://reviews.llvm.org/D77287