Page MenuHomePhabricator

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

Authored by Wallbraker on Jul 6 2017, 11:15 AM.

Details

Summary

Hello!

This commit adds a LLVM-C target that is always built on MSVC. A big fat warning, this is my first cmake code ever so there is a fair bit of I-have-no-idea-what-I'm-doing going on here. Which is also why I placed it outside of llvm-shlib as I was afraid of breaking things of other people. Secondly llvm-shlib builds a LLVM.so which exports all symbols and then does a thin library that points to it, but on Windows we do not build a LLVM.dll so that would have complicated the code more.

The patch includes a python script that calls dumpbin.exe to get all of the symbols from the built libraries. It then grabs all the symbols starting with LLVM and generates the export file from those. The export file is then used to create the library just like the LLVM-C that is built on darwin.

Improvements that I need help with, to follow up this review.

  • Get cmake to make sure that dumpbin.exe is on the path and wire the full path to the script.
  • Use LLVM-C.dll when building llvm-c-test so we can verify that the symbols are exported.
  • Bundle the LLVM-C.dll with the windows installer.

Why do this? I'm building a language frontend which is self-hosting, and on windows because of various tooling issues we have a problem of consuming the LLVM*.lib directly on windows. Me and the users of my projects using LLVM would be greatly helped by having LLVM-C.dll built and shipped by the Windows installer. Not only does LLVM takes forever to build, you have to run a extra python script in order to get the final DLL.

Any comments, thoughts or help is greatly appreciated.

Cheers, Jakob.

Patch by: Wallbraker (Jakob Bornecrantz)

Diff Detail

Repository
rL LLVM

Event Timeline

Wallbraker created this revision.Jul 6 2017, 11:15 AM
smeenai added a subscriber: smeenai.

(adding some Windows and cmake people)

chapuni added a subscriber: chapuni.Jul 6 2017, 5:25 PM
beanz edited edge metadata.Jul 18 2017, 1:41 PM

I don't think this should be added as a new tool. We support generating LLVM-C.dylib on Darwin via the tools/llvm-shlib tool. It would be better if this were added there instead of completely separate.

I don't think this should be added as a new tool. We support generating LLVM-C.dylib on Darwin via the tools/llvm-shlib tool. It would be better if this were added there instead of completely separate.

Thank you for your comment, I respun this patch so that it does not add a new tool but reuse the llvm-shlib tool.

It becomes a bit more ugly then because:

  1. The LLVM-C.dylib on Darwin is a re-export library that holds no code it just points to LLVM.dylib but only the LLVM C API functions are exported.
  2. I'm pretty sure we do not want to ship a LLVM.dll exporting all of the C++ functions (I could be wrong here and we want to) so we should not build it on Windows.
  3. So we only build LLVM-C.dll without using a re-export library (also don't know how to do that on Windows).
  4. The llvm-shlib directory is only built if LLVM.dylib is built, which we do not want to do on Windows, see 2. So had to jump through some hoops to get it to enter it.

Both patches are in my llvm repo on github https://github.com/Wallbraker/llvm

Thanks, Jakob.

compnerd edited edge metadata.Jul 22 2017, 12:44 PM

Forwarders on Windows are roughly the equivalent to a re-export library. They aren't exactly the same thing as you need to list all the interfaces instead of saying all interfaces from this library.

rnk removed a reviewer: rnk.Aug 8 2017, 1:49 PM
rnk added a subscriber: rnk.

I never found time to look at this, removing myself to better reflect reality. I guess my stance is that we should start picking and choosing C++ APIs to expose from an LLVM DSO that we ship on all platforms. This isn't really a step towards or away from that goal, so I don't have many feelings one way or another. If the owners of the existing llvm-shlib logic are OK with this CMake complexity and it's useful to someone, I'm OK with it.

beanz accepted this revision.Aug 9 2017, 10:26 AM

LGTM.

This revision is now accepted and ready to land.Aug 9 2017, 10:26 AM

Thanks for the review, I do not have commit access so somebody needs to commit this for me.

Cheers. Jakob.

Memnarch added a subscriber: Memnarch.EditedSep 5 2017, 12:58 PM

Thanks for the review, I do not have commit access so somebody needs to commit this for me.

Cheers. Jakob.

Just tried it and it worked like a charm, just one little issue(when building from VisualStudio):
CMAKE_BINARY_DIR will not include Debug/Release Subdirectory and therefore the pythonscript will complain about missing files. I just added my desired config to the path in the script and it worked.
The question is, is this fixable? Or did i do something wrong on my end?

EDIT: To avoid confusion, i do not have commit-access either

Please honor CMAKE_CFG_INTDIR. See also LLVM_*_OUTPUT_INTDIR.

Wallbraker updated this revision to Diff 114168.Sep 7 2017, 6:14 AM

I have updated the commit to use for locating the libs CMAKE_CFG_INTDIR and should fix the issue @Memnarch saw. I only have build tools installed so I don't know if I can test it under a Visual Studio like environment.

Cheers, Jakob.

I have updated the commit to use for locating the libs CMAKE_CFG_INTDIR and should fix the issue @Memnarch saw. I only have build tools installed so I don't know if I can test it under a Visual Studio like environment.

Cheers, Jakob.

Can you check your diff? Expected changes in "tools/llvm-shlib/CMakeLists.txt" only but all files are modified with other changes(compare the new diff with the previous one)

I have updated the commit to use for locating the libs CMAKE_CFG_INTDIR and should fix the issue @Memnarch saw. I only have build tools installed so I don't know if I can test it under a Visual Studio like environment.

Cheers, Jakob.

Can you check your diff? Expected changes in "tools/llvm-shlib/CMakeLists.txt" only but all files are modified with other changes(compare the new diff with the previous one)

Those are not my changes, I think they are a artifact from me uploading a diff manually and the fact that the code has changed from the last time I uploaded the diff (The LLVM docs tells me to upload the diff with full context). There are two lines that show up in a slightly brighter green, those are the only changes I have made.

I have updated the commit to use for locating the libs CMAKE_CFG_INTDIR and should fix the issue @Memnarch saw. I only have build tools installed so I don't know if I can test it under a Visual Studio like environment.

Cheers, Jakob.

Can you check your diff? Expected changes in "tools/llvm-shlib/CMakeLists.txt" only but all files are modified with other changes(compare the new diff with the previous one)

Those are not my changes, I think they are a artifact from me uploading a diff manually and the fact that the code has changed from the last time I uploaded the diff (The LLVM docs tells me to upload the diff with full context). There are two lines that show up in a slightly brighter green, those are the only changes I have made.

i suspected that. But for the purpose of this review, you should remove these changes

Wallbraker updated this revision to Diff 114185.Sep 7 2017, 8:30 AM

This should not have any other changes, I think. Problem is I do not know which revision the old one was based on.

That looks much better. When in doubt, you can always download the revisions from the most recent(or any other diff) to make those changes based on those.

That looks much better. When in doubt, you can always download the revisions from the most recent(or any other diff) to make those changes based on those.

Excellent, okay thanks. Have you had a chance to try this on Visual Studio, I'm interested in knowing if this fixes your issue?

That looks much better. When in doubt, you can always download the revisions from the most recent(or any other diff) to make those changes based on those.

Excellent, okay thanks. Have you had a chance to try this on Visual Studio, I'm interested in knowing if this fixes your issue?

Worked

Ping?

I don't have commit access so I need somebody to commit this for me, it has been like a month since I got the first LGTM on this review.

Maybe this is a candidate for http://llvmweekly.org/reviewcorner
Submit it and it'll hopefully show up on the next weekly!

Ping?

did you submit it to the reviewcorner?

No sorry, I moved and started a new job. And don't have access to the windows machine for a while.

I can update the patch to master (which is a requirement for reviewcorner), but not test it.

Cheers, Jakob.

Wallbraker updated this revision to Diff 119666.EditedOct 20 2017, 9:49 AM

This is a NFC update to the patch, bringing it up to master. As I wrote above I can't test it, but should work just fine.

If anybody can test it I can ask for inclusion in the next reviewcorner.

Cheers, Jakob.

This is a NFC update to the patch, bringing it up to master. As I wrote above I can't test it, but should work just fine.

If anybody can test it I can ask for inclusion in the next reviewcorner.

Cheers, Jakob.

Ok since nobody else responded while i was absent, i'll test it during the next days. Have some time on Thurday, so we can get this to the ReviewCorner ASAP. Just in case it got incompatible with HEAD, can you check in the meantime?

@compnerd @beanz @Wallbraker
Tested it on Master (CommitID: 0ba1cd85f0f79ad7f3831690a243ec7d9421bab7)
Seemed to run fine.

Thanks for testing again @Memnarch, I'm going to update this changeset soon and then try to get it included in LLVM Weekly.

Cheers, Jakob.

Wallbraker updated this revision to Diff 141358.Apr 6 2018, 8:45 AM

Updated the diff to master, haven't build tested yet.

Wallbraker edited the summary of this revision. (Show Details)Apr 6 2018, 9:01 AM
Wallbraker added reviewers: hans, smeenai.

Added people who seems to be doing stuff with cmake on windows.

Wallbraker updated this revision to Diff 154515.Jul 8 2018, 9:23 AM

Another update of the commit for llvm master.

I work with Jakob on the afore mentioned self hosting language frontend. Would like to see this get in, what else is needed? I just built the version that Jakob recently updated, and it produces a LLVM-C.dll and lib file with what looks like the right functions (when looked at with a dependency walker); not sure how best to test it beyond that. Let me know what needs to be done and I'll try my best to do it.

Upon further testing, I can verify that the generated DLL appears to work.

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.

CMakeLists.txt
545 ↗(On Diff #154515)

Comment is unnecessary as-is. Why does this default to on though, given that the corresponding Darwin option defaults to off?

551 ↗(On Diff #154515)

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.)

tools/CMakeLists.txt
21 ↗(On Diff #154515)

How does this ensure we'll always build on Windows?

tools/llvm-shlib/CMakeLists.txt
133 ↗(On Diff #154515)

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.

tools/llvm-shlib/gen-msvc-exports.py
64 ↗(On Diff #154515)

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).

bhelyer added inline comments.Jul 17 2018, 4:32 AM
CMakeLists.txt
545 ↗(On Diff #154515)

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.

551 ↗(On Diff #154515)

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.

tools/CMakeLists.txt
21 ↗(On Diff #154515)

As LLVM_BUILD_LLVM_C_DYLIB is set to On on windows by default, this will always evaluate false on windows.

tools/llvm-shlib/CMakeLists.txt
133 ↗(On Diff #154515)

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.

tools/llvm-shlib/gen-msvc-exports.py
64 ↗(On Diff #154515)

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.

smeenai added inline comments.Jul 17 2018, 2:20 PM
CMakeLists.txt
545 ↗(On Diff #154515)

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?

tools/CMakeLists.txt
21 ↗(On Diff #154515)

It's on by default (pending our discussion above), but someone could override it themselves and then the comment wouldn't hold, right?

tools/llvm-shlib/gen-msvc-exports.py
64 ↗(On Diff #154515)

Cool. I think llvm-nm -g should be equivalent to dumpbin /linkermember:1.

Wallbraker updated this revision to Diff 159294.Aug 6 2018, 6:39 AM

Updated with @bhelyer changes and updated to latest master.

Wallbraker marked 12 inline comments as done.Aug 6 2018, 6:41 AM

That should be all of the comments done.

Can this please please please be pushed ASAP so it can go in to the 7.0 release?

xbolva00 added a subscriber: xbolva00.EditedAug 7 2018, 8:52 AM

Can this please please please be pushed ASAP so it can go in to the 7.0 release?

ask @hans if he can merge it to 7.0 branch :)

xbolva00 edited the summary of this revision. (Show Details)Aug 7 2018, 8:54 AM
This revision was automatically updated to reflect the committed changes.
hans added a comment.Aug 7 2018, 10:27 AM

Can this please please please be pushed ASAP so it can go in to the 7.0 release?

ask @hans if he can merge it to 7.0 branch :)

I'd prefer not to merge it. Since this is a new feature that's been in the pipe for over a year, I don't think we should rush it in now. It would be better to let it land safely on trunk, get any kinks worked out and then have it be part of the next release.

We should also check whether this will become part of the installer automatically, you can see how its built in utils/release/build_llvm_package.bat, or if that needs some update.

Can this please please please be pushed ASAP so it can go in to the 7.0 release?

ask @hans if he can merge it to 7.0 branch :)

I'd prefer not to merge it. Since this is a new feature that's been in the pipe for over a year, I don't think we should rush it in now. It would be better to let it land safely on trunk, get any kinks worked out and then have it be part of the next release.

We should also check whether this will become part of the installer automatically, you can see how its built in utils/release/build_llvm_package.bat, or if that needs some update.

Ok that make sense!

@hans Right now it is not built by default, but I would be very happy to change that as that will give the change much more coverage. So either we would need to change build_llvm_package.bat to include that option or change the default.

Also you can break bat lines up similar to bash ones with ^ which might make the cmake command a bit more readable.