This is an archive of the discontinued LLVM Phabricator instance.

[lldb/test] Make TestLoadUnload compatible with windows
ClosedPublic

Authored by labath on Apr 7 2020, 10:41 AM.

Details

Summary

This patch introduces a header "dylib.h" which can be used in tests to
handle shared libraries semi-portably. The shared library APIs on
windows and posix systems look very different, but their underlying
functionality is relatively similar, so the mapping is not difficult.

It also introduces two new macros to wrap the functinality necessary to
export/import function across the dll boundary on windows. Previously we
had the LLDB_TEST_API macro for this purpose, which automagically
changed meaning depending on whether we were building the shared library
or the executable. While convenient for simple cases, this approach was
not sufficient for the more complicated setups where one deals with
multiple shared libraries.

Lastly it rewrites TestLoadUnload, to make use of the new APIs. The
trickiest aspect there is the handling of DYLD_LIBRARY_PATH on macos --
previously setting this variable was not needed as the test used
@executable_path-relative dlopens, but the new generic api does not
support that. Other systems do not support such dlopens either so the
test already contained support for setting the appropriate path
variable, and this patch just makes that logic more generic. In doesn't
seem that the purpose of this test was to exercise @executable_path
imports, so this should not be a problem.

These changes are sufficient to make some of the TestLoadUnload tests
pass on windows. Two other tests will start to pass once D77287 lands.

Diff Detail

Event Timeline

labath created this revision.Apr 7 2020, 10:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2020, 10:41 AM
labath updated this revision to Diff 255727.Apr 7 2020, 10:46 AM

include some changes I forgot (they're the ones that need cleaning up the most)

Harbormaster failed remote builds in B52180: Diff 255727!
jingham added inline comments.Apr 7 2020, 11:51 AM
lldb/test/API/functionalities/load_unload/TestLoadUnload.py
414

You could also do this by making an SBLaunchInfo, adding the environment variable to it, and then you could use lldbutil.run_to_name_breakpoint. That would remove some of the boilerplate below.

I like where this is going.

lldb/packages/Python/lldbsuite/test/make/dylib.h
51

This leaks the buffer allocated for the message. I guess we don't expect this to happen often, so maybe that's not a big deal, and it's a terrible API to have to deal with.

lldb/test/API/functionalities/load_unload/TestLoadUnload.py
209

The env_cmd_string is going to eliminate ALL of the environment variables for the target except the one(s) that it explicitly sets. Is that what you intended?

The lldb help says:

Warning: The 'set' command re-sets the entire array or dictionary. If you just want to add, remove or update individual values (or add something to the end), use one of the other settings sub-commands: append, replace, insert-before or insert-after.

labath marked 3 inline comments as done.Apr 8 2020, 12:53 AM
labath added inline comments.
lldb/packages/Python/lldbsuite/test/make/dylib.h
51

yeah, that's just the test code and it's only purpose is to give some indication for when things go wrong. I've contemplated returning std::string here, but then I figured we may want to use this from some C code too and it may be better to just stick to C.

lldb/test/API/functionalities/load_unload/TestLoadUnload.py
209

That's not as bad as it sounds because target.env-vars (as of last week, anyway) will only contain the variables set by the user, and the host environment will be applied separately.

Nonetheless, using append does seem like a better choice here.

414

run_to_name_breakpoint does not seem like a completely good fit here as the test has assertions about numbers of breakpoint locations being found before running the process.

However, I think I've found an even better solution -- the test actually already contains logic for setting these paths, but it was hidden behind an if(!darwin) as it was not needed due to the use of @executable_path in dlopen calls. Now I've just removed the if and made that code slightly more generic.

labath updated this revision to Diff 255916.Apr 8 2020, 12:54 AM

Cleaning up the patch. It should be ready now, I'm just going to give it one more spin on windows.

labath edited the summary of this revision. (Show Details)Apr 8 2020, 12:55 AM
labath updated this revision to Diff 255921.Apr 8 2020, 1:08 AM
labath edited the summary of this revision. (Show Details)

Remove bogus runCmd. Everything should check out now.

labath retitled this revision from [WIP][lldb/test] Make TestLoadUnload compatible with windows to [lldb/test] Make TestLoadUnload compatible with windows.Apr 8 2020, 1:08 AM
amccarth added inline comments.Apr 8 2020, 4:39 PM
lldb/packages/Python/lldbsuite/test/make/Makefile.rules
477

I'm no expert in makefile syntax (especially not for Unix-y versions), but this looks weird, with the leading comma and the matching parenthesis for $(filter sitting near the end of the expression. Is this right?

labath marked 4 inline comments as done.Apr 9 2020, 12:02 AM
labath added inline comments.
lldb/packages/Python/lldbsuite/test/make/Makefile.rules
477

Yes, it is.

The "leading" comma introduces a empty string as the first argument to ifeq. Then the $filter thingy will turn $OS into an empty string iff it matches one of the strings it got as an argument (technically it will just remove any subwords which match those strings, but since $OS is just a single word, the effect is the same).
So this is a very convoluted way of writing $(OS) in [NetBSD, Windows_NT] but:
a) it is consistent with other usages in these makefiles
b) I don't actually know of a better way to write that. If you do, let me know.

amccarth accepted this revision.Apr 9 2020, 9:28 AM

Well, it looks to go _me_.

This revision is now accepted and ready to land.Apr 9 2020, 9:28 AM

@labath - can this get merged so that I can rebase and get D77287 merged?

This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.