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

[Bernard Helyer via Phabricator via llvm-commits]

bhelyer added inline comments.


================
Comment at: CMakeLists.txt:545
+if(MSVC)
+  # Defaults to on.
+  option(LLVM_BUILD_LLVM_C_DYLIB "Build LLVM-C.dll (Windows only)" ON)
----------------
smeenai wrote:
> Comment is unnecessary as-is. Why does this default to on though, given that the corresponding Darwin option defaults to off?
The idea is that we would like the DLL included in the windows published on llvm.org, if that's out of scope for this changeset, we can switch it to OFF and take that up elsewhere.

Either way, I'll nix the comment.



================
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)
----------------
smeenai wrote:
> 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.)
Roger. I'm still learning this cmake stuff, and I'll have to play around with cmake -P to make sure I've got the way everything binds correct.


================
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)
----------------
smeenai wrote:
> How does this ensure we'll always build on Windows?
As LLVM_BUILD_LLVM_C_DYLIB is set to On on windows by default, this will always evaluate false on windows.


================
Comment at: tools/llvm-shlib/CMakeLists.txt:133
+  if(CMAKE_SIZEOF_VOID_P EQUAL 8)
+    set(GEN_ARCH x64)
+  else()
----------------
smeenai wrote:
> 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.
To be honest, this one is a bit out of depth for me as yet, but I'll be sure to poke Jakob about it.


================
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)
+
----------------
smeenai wrote:
> 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).
I'll have to test the results to make sure it actually works and then wire it up, but all the tools look like they can output the information we need from the .lib files, so this shouldn't be an issue.


https://reviews.llvm.org/D35077