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

[Adrian McCarthy]

================
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
----------------
zturner wrote:
> See my comments in test_common.h about having a -D COMPILING_TEST_DLL.  This seems like a good place to do it.
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

================
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
----------------
zturner wrote:
> 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
> ```
Agreed and done.

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

================
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)
----------------
zturner wrote:
> 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.
> 
Actually, it looks like MinGW does use dllimport/dllexport:  http://www.mingw.org/wiki/sampledll



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

http://reviews.llvm.org/D8277

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