[PATCH] D35077: [RFC] Build LLVM-C.dll on MSVC that exports only the C API

Shoaib Meenai via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 16 23:35:13 PDT 2018


smeenai added a comment.

Sorry this has been sitting for so long. I have some questions, and I'll also try to play around with this locally because I don't fully understand some parts.



================
Comment at: CMakeLists.txt:545
+if(MSVC)
+  # Defaults to on.
+  option(LLVM_BUILD_LLVM_C_DYLIB "Build LLVM-C.dll (Windows only)" ON)
----------------
Comment is unnecessary as-is. Why does this default to on though, given that the corresponding Darwin option defaults to off?


================
Comment at: CMakeLists.txt:551
 set(LLVM_BUILD_LLVM_DYLIB_default OFF)
-if(LLVM_LINK_LLVM_DYLIB OR LLVM_BUILD_LLVM_C_DYLIB)
+if(LLVM_BUILD_LLVM_C_DYLIB AND NOT MSVC OR LLVM_LINK_LLVM_DYLIB)
   set(LLVM_BUILD_LLVM_DYLIB_default ON)
----------------
CMake has the same precedence for AND and OR (unlike most programming languages), so I always prefer adding parentheses to make the precedence clear. (That would also remove the need for reordering the parts of the OR here.)


================
Comment at: tools/CMakeLists.txt:21
+# Don't build the shlib if not requested,
+# always build on windows.
+if(NOT LLVM_BUILD_LLVM_DYLIB AND NOT LLVM_BUILD_LLVM_C_DYLIB)
----------------
How does this ensure we'll always build on Windows?


================
Comment at: tools/llvm-shlib/CMakeLists.txt:133
+  if(CMAKE_SIZEOF_VOID_P EQUAL 8)
+    set(GEN_ARCH x64)
+  else()
----------------
This is used for user label prefix purposes, so it's better to just pass the user label prefix (or whether there is one) to the script. In particular, arm and arm64 also don't have a user label prefix; it's only x86 that does.

You can either preprocess a simple program for the target and see what the `__USER_LABEL_PREFIX__` macro expands to, or you can assume that x86 means the user label prefix is `_` and any other architecture means no user label prefix. I'd be fine with either one.


================
Comment at: tools/llvm-shlib/gen-msvc-exports.py:64
+                with open(dumpout, 'w+t') as dumpout_f:
+                    check_call(['dumpbin', '/linkermember:1', lib], stdout=dumpout_f)
+
----------------
Can we use one of llvm-nm, llvm-objdump, or llvm-readobj instead? It would allow building this when cross-compiling and would remove the dependency on the VS command prompt (although you'd probably want to take the path to the LLVM program as an argument).


https://reviews.llvm.org/D35077





More information about the llvm-commits mailing list