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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai 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)
----------------
bhelyer wrote:
> 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.
> 
I think it makes sense to have it default to off and then we could request the Windows packaging to turn this option on. @hans does that sound okay?


================
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)
----------------
bhelyer wrote:
> 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.
It's on by default (pending our discussion above), but someone could override it themselves and then the comment wouldn't hold, right?


================
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)
+
----------------
bhelyer wrote:
> 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.
Cool. I think `llvm-nm -g` should be equivalent to `dumpbin /linkermember:1`.


https://reviews.llvm.org/D35077