[PATCH] D151620: [clang-repl] Fix REPL_EXTERNAL_VISIBILITY and building libclang-cpp.dll for MinGW configurations

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: junaire, alvinhochun, mati865.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a subscriber: bd1976llvm.
Herald added a project: clang.

This fixes two issues that are observed after
5111286f06e1e10f24745007a45a830760f1790c <https://reviews.llvm.org/rG5111286f06e1e10f24745007a45a830760f1790c>:

For builds with GCC with LLVM_LINK_LLVM_DYLIB=ON, we previously got
build errors, as libclang-cpp.dll suddenly only contained the
functions that were marked dllexport via REPL_EXTERNAL_VISIBILITY,
instead of all symbols as expected.

For MinGW builds with Clang, building previously succeeded (as it
used either the __attribute__((visibility("default"))) annotation or
nothing at all), and the functions were exported from libclang-cpp.dll
if that was built, but the unit test failed (as neither of those cases
made the functions exported from an EXE).

Don't use the visibility attributes on MinGW targets for these purposes;
setting default visibility only makes a difference if building with
e.g. -fvisibility=hidden, but it doesn't make the symbols exported
from an EXE.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151620

Files:
  clang/include/clang/Interpreter/Value.h
  clang/tools/clang-shlib/CMakeLists.txt


Index: clang/tools/clang-shlib/CMakeLists.txt
===================================================================
--- clang/tools/clang-shlib/CMakeLists.txt
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -53,3 +53,11 @@
 if (NOT APPLE AND NOT MINGW)
   target_link_options(clang-cpp PRIVATE LINKER:-Bsymbolic-functions)
 endif()
+if (MINGW OR CYGWIN)
+  # The clang-cpp DLL is supposed to export all symbols (except for ones
+  # that are explicitly hidden). Normally, this is what happens anyway, but
+  # if there are symbols that are marked explicitly as dllexport, we'd only
+  # export them and nothing else. Therefore, add --export-all-symbols to
+  # make sure we export all symbols despite potential dllexports.
+  target_link_options(clang-cpp PRIVATE LINKER:--export-all-symbols)
+endif()
Index: clang/include/clang/Interpreter/Value.h
===================================================================
--- clang/include/clang/Interpreter/Value.h
+++ clang/include/clang/Interpreter/Value.h
@@ -52,18 +52,24 @@
 class Interpreter;
 class QualType;
 
-#if __has_attribute(visibility) &&                                             \
-    (!(defined(_WIN32) || defined(__CYGWIN__)) ||                              \
-     (defined(__MINGW32__) && defined(__clang__)))
+#if defined(_WIN32)
+// REPL_EXTERNAL_VISIBILITY are symbols that we need to be able to locate
+// at runtime. On Windows, this requires them to be exported from any of the
+// modules loaded at runtime. Marking them as dllexport achieves this; both
+// for DLLs (that normally export symbols as part of their interface) and for
+// EXEs (that normally don't export anything).
+// For a build with libclang-cpp.dll, this doesn't make any difference - the
+// functions would have been exported anyway. But for cases when these are
+// statically linked into an EXE, it makes sure that they're exported.
+#define REPL_EXTERNAL_VISIBILITY __declspec(dllexport)
+#elif __has_attribute(visibility)
 #if defined(LLVM_BUILD_LLVM_DYLIB) || defined(LLVM_BUILD_SHARED_LIBS)
 #define REPL_EXTERNAL_VISIBILITY __attribute__((visibility("default")))
 #else
 #define REPL_EXTERNAL_VISIBILITY
 #endif
 #else
-#if defined(_WIN32)
-#define REPL_EXTERNAL_VISIBILITY __declspec(dllexport)
-#endif
+#define REPL_EXTERNAL_VISIBILITY
 #endif
 
 #define REPL_BUILTIN_TYPES                                                     \


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D151620.526293.patch
Type: text/x-patch
Size: 2382 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230527/f96a8368/attachment.bin>