Page MenuHomePhabricator

Fix makefiles to build shared libraries (DLLs) for tests on Windows
ClosedPublic

Authored by amccarth on Mar 11 2015, 4:49 PM.

Details

Summary

Abstracted away some POSIX-isms that caused make to issue invalid commands on Windows. Added a new force-include for the test programs (C and C++) so that we can use platform-specific macros. I left uncaught_exception.h as is, since it's specific to C++.

TestSharedLib now builds and cleans up on Windows, though the test still fails some of the expectations. I want to deal with that later.

Diff Detail

Event Timeline

amccarth updated this revision to Diff 21791.Mar 11 2015, 4:49 PM
amccarth retitled this revision from to Fix makefiles to build shared libraries (DLLs) for tests on Windows.
amccarth updated this object.
amccarth edited the test plan for this revision. (Show Details)
amccarth added a subscriber: Unknown Object (MLST).

Looks like the right general idea. I have a few specific comments, so after you address those I think it's good to go.

test/make/Makefile.rules
152–153

See my comments in test_common.h about having a -D COMPILING_TEST_DLL. This seems like a good place to do it.

189–190

I feel like we should get rid of this and sink the logic into test_common.h. Mainly because force includes are gross, so it would be nice if there were just one force include for all platforms so it's more easy to audit what goes into them, and more straightforward for people modifying it so they don't accidentally introduce an order-dependency on what order the force includes happen.

The dummy definition of the function could be wrapped in something like:

#if defined(__cplusplus) && defined(_MSC_VER) && (_HAS_EXCEPTIONS==0)
// Declare the function here.
#endif
412–418

What is all this stuff for? Make isn't my thing, so I'm not sure why this is needed. Is it necessary for the shlib stuff?

test/make/test_common.h
4

I think this is another Microsoft specific thing, so it shouldn't (?) apply to people running tests on MinGW. So probably #if defined(_MSC_VER) is better.

5

Can we call the macro something like LLDB_TEST_EXPORT? Makes it clearer that this is something that's part of the test suite.

Also, I think we will need to be a little bit fancier. Only the DLL needs to see declspec(dllexport). The executable should see declspec(dllimport) or it won't be able to bind to the exports at runtime. So I think we need something else inside this conditional like

#if defined(COMPILING_TEST_DLL)
#define LLDB_TEST_EXPORT __declspec(dllexport)
#else
#define LLDB_TEST_EXPORT __declspec(dllimport)
#endif

The non windows codepath is fine, it can always just define it to be nothing. Then the makefile needs to do something to figure oout that for compiling the shared library, it needs to #define COMPILING_TEST_DLL. I commented earlier up about what looks like might be a good place.

amccarth added inline comments.Mar 12 2015, 4:01 PM
test/make/Makefile.rules
152–153

It's trickier than that, because if I add it to CFLAGS here, it'll apply to everything, not just the dynamic library objects. Instead, I've used Target-specific Variables down where it builds the dynamic library.

Target-specific Variable Values: http://www.gnu.org/software/make/manual/html_node/Target_002dspecific.html

189–190

Agreed and done.

412–418

These commands fire on almost every make command, and they were causing errors on Windows because the syntax wasn't right for CMD. In some cases, the errors prevented the shared library from being built.

test/make/test_common.h
4

Actually, it looks like MinGW does use dllimport/dllexport: http://www.mingw.org/wiki/sampledll

5

Can we call the macro something like LLDB_TEST_EXPORT?

Yes. I chose LLDB_TEST_API, since it's used for both import and export. I've seen other projects that use API as part of the macro name for this.

Only the DLL needs to see declspec(dllexport). The executable should see declspec(dllimport) or it won't be able to bind to the exports at runtime.

I was expecting to need to do that, but the need never arose in practice, so I defaulted to making the smallest possible change. It's a good idea nonetheless, so now I've gone ahead and implemented it.

amccarth updated this revision to Diff 21884.Mar 12 2015, 4:04 PM

I'm still testing, and I've got to go through the other tests that build dynamic libraries in order to add the LLDB_TEST_API macro to the exported functions.

zturner accepted this revision.Mar 12 2015, 4:16 PM
zturner added a reviewer: zturner.

Looks good, after the remaining changes I'll commit

test/make/Makefile.rules
378

I think this should be called something like COMPILING_LLDB_TEST_DLL or COMPILING_TEST_DLL. Or anything as long as it has "DLL" in it. Because an EXE is technically an "LLDB Test" also, but it wouldn't have this definition.

413

I'd probably change the name from SEMICOLON to something like JOIN_COMMAND. It's a little confusing to see SEMICOLON = &.

test/make/test_common.h
20

I know you were just copying over my code, but my code was actually wrong here :) __uncaught_exception() should return bool. Can you change this to an inline bool which returns true? https://msdn.microsoft.com/en-us/library/ff770584.aspx?f=255&MSPPError=-2147217396 This is probably only of theoretical value, but we might as well.

This revision is now accepted and ready to land.Mar 12 2015, 4:16 PM
amccarth added inline comments.Mar 13 2015, 1:52 PM
test/make/Makefile.rules
378

Done.

413

Agreed and done.

test/make/test_common.h
20

I've changed this to #include <eh.h> and rewritten the comment to describe the better understand we arrived at last night.

amccarth updated this revision to Diff 21952.Mar 13 2015, 1:56 PM
amccarth edited edge metadata.

I had to fix a little Python as well. There may be more to do, but we now build shared libs on Windows, the cleanup is a little more reliable, and some tests that were failing in the build are now actually building and failing in the expectations.

looks good, i'll commit later. Thanks!