[Lldb-commits] [PATCH] Fix makefiles to build shared libraries (DLLs) for tests on Windows

[Zachary Turner]

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


================
Comment at: test/make/Makefile.rules:152-153
@@ -150,2 +151,4 @@
 		DYLIB_FILENAME = lib$(DYLIB_NAME).dylib
+  else ifeq "$(OS)" "Windows_NT"
+    DYLIB_FILENAME = $(DYLIB_NAME).dll
 	else
----------------
See my comments in test_common.h about having a -D COMPILING_TEST_DLL.  This seems like a good place to do it.

================
Comment at: test/make/Makefile.rules:190
@@ -186,3 +189,3 @@
 		CXXFLAGS += -fno-exceptions
 		CXXFLAGS += -include $(THIS_FILE_DIR)uncaught_exception.h
 		CXXFLAGS += -D_HAS_EXCEPTIONS=0
----------------
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
```

================
Comment at: test/make/Makefile.rules:411-417
@@ -407,1 +410,9 @@
 #----------------------------------------------------------------------
+ifeq "$(OS)" "Windows_NT"
+	SEMICOLON = &
+	QUOTE = "
+else
+	SEMICOLON = ;
+	QUOTE = '
+endif
+
----------------
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?

================
Comment at: test/make/test_common.h:3
@@ +2,3 @@
+// hook for dealing with platform-specifics.
+#if defined(_WIN32) || defined(_WIN64)
+    #define DLLEXPORT __declspec(dllexport)
----------------
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.


================
Comment at: test/make/test_common.h:4
@@ +3,3 @@
+#if defined(_WIN32) || defined(_WIN64)
+    #define DLLEXPORT __declspec(dllexport)
+#else
----------------
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.

http://reviews.llvm.org/D8277

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/