WIP: Dynamic linking support for Windows (Part 1/2) [skip ci]
Needs RevisionPublic

Authored by Phyx on Oct 13 2016, 5:57 PM.

Details

Summary

This patch is the first of 2 patches to enable dynamic support on
Windows. This patch should be committed on it's own. Second patch
focuses more on distribution.

This introduces an auto-splitting functions for DLLs. The way the
split is done, no extra handling is required for the rest of the
toolchain. So it's all confined to the shell script that does the work.

Secondly, this script corrects the way symbols were linked. There weren't
enough symbols to go above the threshold. If I measure amount of symbols
in the input object files going into the link and the amount coming out,
the difference is huge.

Looking at it further this is because of two things. We never explicitly
use __declspec, we just change the names of the functions to match the
conventions that __declspec would use.
This is fine, but it means that binutil's default of --export-all-symbols
is still enabled. Which means, we'll re-export any symbol we link from
archives as well.

In fact I got rid of dll-split all together and allow all symbols to go
into the same dll and we end up with:

$ nm -g "R:\ghc\libHSghc-8.1-ghc8.1.20160617.dll" | wc -l
49610

This down from ~240,000 (mingwex and mingw32 are huge for instance).

These two changes should solve the problem once and for all. There is one
caveat, the RTS packages can never be split. The reason is there is a mutual
dependencies between Base and RTS. The cycle is broken at the compilation of
Base, but the build system has no idea about the relationship between these
two, so it assumes RTS is one package and "guesses" the final RTS dll name.
This isn't easy to change, however the RTS doesn't have a ton of symbols,
but do keep an eye out for this:

Number of symbols in object files for
rts/dist/build/libHSrts-ghc8.1.20161013.dll: 1273
DLL rts/dist/build/libHSrts-ghc8.1.20161013.dll OK, no need to split

This cycle is also fragile, since ghc-prim is also required before it is built.
So you might get strange link errors because a new closure is needed by the rts
that hasn't been manually added. In that case they should be added to the def
files in rts/win32.

The major limitation in this version is that only the threaded rts can be used.
Due to the fact that we can't link with unknown symbols on Windows in the final
link. This will be addressed next.

More details of how the linking works here
https://ghc.haskell.org/trac/ghc/wiki/WindowsDynamicLinking

Test Plan

make && inplace/bin/ghc-stage2 --version.

./validate will only pass dynamic tests compiled with -threaded.
on all others it will segfault because it will load multiple RTS version.
This will be addressed by patch 2.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
trac-5987-fix-dynamic-library-loading-lib-march
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 14920
Build 24256: arc lint + arc unit
Phyx retitled this revision from to Dynamic linking support for Windows (Part 1/4).Oct 13 2016, 5:57 PM
Phyx updated this object.
Phyx edited the test plan for this revision. (Show Details)
Phyx updated the Trac tickets for this revision.
Phyx added a subscriber: Restricted Project.
Phyx updated this object.Oct 13 2016, 6:21 PM
Phyx updated this revision to Diff 9011.Oct 15 2016, 5:18 AM
  • T5987: remove redundant code.

Thanks @Phyx! This looks fantastic. I've left a number of comments inline but nothing fundamental.

When you update this Diff could you upload to the staging area as well so we can see how CI fares on this.

compiler/main/Manifest.hs
8

I'm actually really not sure who technically ow

21

Can you add an explicit import list for SysTools since it constitutes an import loop. Explicit import lists make it easier to disentangle loops later.

70

Is this choice for uiAccess always going to be safe? I did a quick search and it seems like it's only relevant to remote desktop-type applications, but so I suppose we can cross this bridge once someone needs it.

130

Too many dots. <.> already adds a period; drop it from ".dll".

Also, for future reference @duncan would like for Cabal and friends to have more control over the library names, so we'll need to revisit this construction at some point.

136

Is this fromJust safe?

139

Wouldn't it be easier to just look at Data.Version.versionBranch instead of showing the version and then parsing out its components?

compiler/main/Packages.hs
1891

Perhaps you could fix this truncated comment while you are at it?

compiler/main/SysTools.hs
56

I suspect SysTools could use some breaking up. It has a mix of low-level utilities and logic-heavy linking/compilation tool helpers. That being said, it's unclear whether this would help you Manifest loop.

docs/users_guide/8.2.1-notes.rst
59

Yay! Thanks for the release notes.

63

We should talk about what we need to do to produce these.

docs/users_guide/shared_libs.rst
239

Could we be more specific about where to look on MSDN? It's a rather large site ;)

244

Perhaps use the .. note:: directive.

255

I think you need two colons here. e.g.,

.. code-block:: xml
driver/utils/dynwrapper.c
83–87

mmmmm. cookies.

143

I'm a little lost here. Why are we skipping the first entry? Is the API being referred to here LoadLibraryExW?

includes/Rts.h
221

What is the preferred order here? In the hunk above the DLL_IMPORT_RTS appeared before the return type; here it appears after.

rts/ghc.mk
216

Take note, @snowleopard.

rules/build-package-way.mk
99

Is this a to-do or a comment?

testsuite/mk/test.mk
37–39

What is the reason for this change? I believe -dno-debug-output is already passed explicitly by many tests.

utils/mkUserGuidePart/Options/Linking.hs
140

Use,

:ghc-flag:`-shared`

So the reader can easily jump to definition.

148

Again, use ghc-flag here.

bgamari requested changes to this revision.Oct 18 2016, 8:31 PM
This revision now requires changes to proceed.Oct 18 2016, 8:31 PM
Phyx added a comment.Oct 20 2016, 2:58 PM

Thanks @Phyx! This looks fantastic. I've left a number of comments inline but nothing fundamental.

When you update this Diff could you upload to the staging area as well so we can see how CI fares on this.

Thanks for the review! Yeah, the staging area was skipped due to a timeout (I didn't notice it was asking for my ssh key). and couldn't figure out how to push it to staging without making changes :)

I'll take care of the changed this weekend.

compiler/main/Manifest.hs
8

your comment ended abruptly :) I assume you meant owns.

Fair enough, I just copied it from a random other file. perhaps I should just say "GHC Team" or something?

70

It should be, (I hadn't actually looked at this till now, it was moved from the previous code in DriverPipeline). The default for the property is false so we can just omit it. It's intention is to declare which privilege access the application needs. e.g. Does it need to communicate with processes at a higher privilege (which often times UI automation processes need).

https://msdn.microsoft.com/en-us/library/ms742884(v=vs.110).aspx

I'll remove it.

136

Technically it should be, since if it doesn't find the library it doesn't exist and the binary would have failed linking.

But you're right, I should handle that case explicitly.

139

It would, but the function is also used on the version on line 114. Which is the version of the SxS library being compiled (for which we may not have a package version). In which case it gets a string from the commandline.

I just wanted one function to correct things so I work on the string representation so I can re-use it.

compiler/main/SysTools.hs
56

I tried to break out the linker stuff from SysTools but that turned out to be a massive operation, particularly weird is that one of linkBinary or linkLibrary is in SysTools and the other in DriverPipeline.

docs/users_guide/8.2.1-notes.rst
63

Yeah we should. I kept it so at least the compiler will just work from within a tarball release. So that stack also just keeps working.

But in both cases for user programs compiled with -dynamic we have to register the SxS assemblies..

Ideally the Haskell Platform installer will also automatically do this. but I'm still working out what needs to be done.

driver/utils/dynwrapper.c
83–87

om nom nom nom..

143

No, the API is AddDllDirectory, so I'm trying to detect if this was available, because if it is it means we probably added entries that should be searched.

The constant LOAD_LIBRARY_SEARCH_USER_DIRS while always defined, only has meaning when AddDllDirectory is available. So by default we assume it's not and if it is, we use the flag.

This is copied from Linker.c which has a backup method, but here I just didn't bother. It won't work on Vista without the required patch. The amount of code for a workaround wasn't worth it imho. It's been long enough that Vista without a service pack isn't support anymore so.

I can actually simplify this some more, just assume the flag is there and remove the check.

includes/Rts.h
221

You're right, I wasn't very consistent here, I will move them to the front of the return type to be consistent with the other macros.

rts/ghc.mk
211

Add missing parameter definitions here

rules/build-package-way.mk
99

A todo, will clarify, It's part of the distribution/packaging story, so deferring it until the last patch.

131–132

Update docs with missing parameter definition.

testsuite/mk/test.mk
37–39

Sorry, this is a rebase artifact that slipped passed. Will remove

snowleopard added inline comments.Oct 20 2016, 5:29 PM
rts/ghc.mk
216

Thanks! I should find a way to subscribe to all revisions that touch the build system...

rules/build-dll-win32.sh
1 ↗(On Diff #9011)

I'm a little puzzled here. This script is supposedly Windows only, yet it's a Shell script... this strikes me as a rather roundabout way of doing things. (And yes, we do already execute Shell scripts in the build system, such as get-win32-tarballs.sh -- another Windows-only oddity!)

This particular script is far from being trivial and could probably benefit from being written in an easier to maintain language like... Haskell? It would then be easier make it part of Hadrian.

I'm not insisting on a rewrite, but I'm scratching my head as to why we have another non-trivial piece of code written not in Haskell...

Phyx added inline comments.Oct 22 2016, 5:33 AM
rules/build-dll-win32.sh
1 ↗(On Diff #9011)

To be quite honest it's because the thought hadn't even cross my mind to do It in Haskell. It was originally part of the Makefiles, but make was driving me bananas so I moved It out.

The reason for a shell script is because it was just a lot easier than making a Haskell utility. I hadn't even thought about shake when I made this, if it makes your life easier I can rewrite it in Haskell sometime before the final version. Though I find Haskell a very heavy approach...

snowleopard added inline comments.Oct 22 2016, 5:49 AM
rules/build-dll-win32.sh
1 ↗(On Diff #9011)

It is easy to execute the script from Shake.

But... the problem is that we have all these scripts written in Perl, Shell, Python and what not, that ended up being a part of the build system at some point. They may indeed be lighter than Haskell equivalents, but they lead to perpetual maintenance and portability burden.

They also add unnecessary complexity to the build system itself, because we need to set up stringly-typed interfaces for calling them from Shake via command line arguments. Things are so much nicer when we can simply call a Haskell function!

Phyx added inline comments.Oct 22 2016, 6:59 AM
rules/build-dll-win32.sh
1 ↗(On Diff #9011)

I hadn't appreciated the fact before that for shake it would be function call. I'll double back to this once I finish the other 3 patches and rewrite it In Haskell.

snowleopard added inline comments.Oct 22 2016, 8:52 AM
rules/build-dll-win32.sh
1 ↗(On Diff #9011)

Thanks @Phyx! Sorry for creating more work for you :)

Phyx added a comment.Dec 1 2016, 1:13 PM

Note to self: when updating also add symbols from https://phabricator.haskell.org/D2751#inline-22695

Phyx added a comment.Jan 19 2017, 6:57 AM

hmm @bgamari After I finish the making the shell script a haskell util, I'm thinking of putting this patch alone up for committing but disabling it (early after the 8.4 freeze).

It has the benefit of killing dll-split which isn't needed (should make some people happy), but also the rebasing is horrible :| usually takes me a while to complete correctly...

Thoughts?

Phyx retitled this revision from Dynamic linking support for Windows (Part 1/4) to WIP: Dynamic linking support for Windows (Part 1/2) [ci skip].Feb 5 2017, 5:09 AM
Phyx updated this object.
Phyx edited the test plan for this revision. (Show Details)
Phyx retitled this revision from WIP: Dynamic linking support for Windows (Part 1/2) [ci skip] to WIP: Dynamic linking support for Windows (Part 1/2) [skip ci].Feb 6 2017, 11:41 AM
Phyx updated this revision to Diff 11168.Feb 15 2017, 9:54 PM
  • T5987: rebased.
  • T5987: fixing after rebase
  • T5987: Fixes after rebase.
  • T5987: reverted submodules.
  • DynLib: Fix ghc-cabal build errors and rts build error.
  • T5987: Reworking DLL building code.
  • T5987: Fix failure.
  • T5987: turn off lazy loading.
  • Dyn: remove assert code and other werr issues.
  • Disable CoreToStg asserts for DynWay for now.
  • Update submodules.
  • Shared: added gen-dll
  • Gen-Dll: Added better argument parsing
  • Calling Win32 args parser
  • GenDll: Fixed gen-dll I/O
  • Dll: Enable dynamic-too on Windows.
  • Fix some more rebase screwiness
  • Remove more dllsplit tstuff again
  • Fix execProc.
  • Disable CoreToStg warnings completely.
  • Fix -Werror
  • Correct ar script.
  • Dyn: disable import lib creation.
  • fix compile.
Phyx planned changes to this revision.Feb 15 2017, 10:01 PM

Just updating with current state. after rebasing I now get some random segfaults which need to be figured out. dlltool is also way too slow so a custom import lib writer needs to be written and will leverage that to write a custom loader to solve the remaining issue of how to correctly handle const data issue introduced by GHC's dynamic linking not having been designed to work cross platform.

Phyx added a comment.Mar 13 2017, 1:30 PM
This comment was removed by Phyx.
Phyx updated this revision to Diff 11908.Mar 29 2017, 1:37 AM
  • T5987: rebased.
  • T5987: fixing after rebase
  • T5987: Fixes after rebase.
  • T5987: reverted submodules.
  • DynLib: Fix ghc-cabal build errors and rts build error.
  • T5987: Reworking DLL building code.
  • T5987: Fix failure.
  • T5987: turn off lazy loading.
  • Dyn: remove assert code and other werr issues.
  • Disable CoreToStg asserts for DynWay for now.
  • Update submodules.
  • Shared: added gen-dll
  • Gen-Dll: Added better argument parsing
  • Calling Win32 args parser
  • GenDll: Fixed gen-dll I/O
  • Dll: Enable dynamic-too on Windows.
  • Fix some more rebase screwiness
  • Remove more dllsplit tstuff again
  • Fix execProc.
  • Disable CoreToStg warnings completely.
  • Fix -Werror
  • Correct ar script.
  • Dyn: disable import lib creation.
  • fix compile.
  • use lib instead.
  • Fix segfault.
  • Fix mri script.
  • Working using dll.a again
  • Dyn: Generate a DllMain by default unless -no-hs-main
Phyx added a comment.Mar 29 2017, 1:56 AM

Almost there...

Most hacks are gone now and it compiles on it's own:

$ strace inplace/bin/ghc-stage2.exe
create_child: \inplace\bin\ghc-stage2.exe
--- Process 408372 created
...
--- Process 408372 loaded \ghc\stage2\build\tmp\ghc-stage2.exe.dll at 0000000070040000
--- Process 408372 loaded \libraries\base\dist-install\build\libHSbase-4.10.0.0-ghc8.3.20170323.dll at 0000000066F40000
--- Process 408372 loaded \compiler\stage2\build\libHSghc-8.3-ghc8.3.20170323-pt2.dll at 00000000064D0000
--- Process 408372 loaded \compiler\stage2\build\libHSghc-8.3-ghc8.3.20170323-pt1.dll at 00000000028F0000
--- Process 408372 loaded \compiler\stage2\build\libHSghc-8.3-ghc8.3.20170323-pt3.dll at 000000000A0A0000
--- Process 408372 loaded \libraries\bytestring\dist-install\build\libHSbytestring-0.10.8.2-ghc8.3.20170323.dll at 000000006EC00000
--- Process 408372 loaded \libraries\containers\dist-install\build\libHScontainers-0.5.10.2-ghc8.3.20170323.dll at 0000000063F40000
--- Process 408372 loaded \libraries\directory\dist-install\build\libHSdirectory-1.3.0.2-ghc8.3.20170323.dll at 0000000066280000
--- Process 408372 loaded \libraries\filepath\dist-install\build\libHSfilepath-1.4.1.1-ghc8.3.20170323.dll at 0000000067F00000
--- Process 408372 loaded \libraries\ghc-boot-th\dist-install\build\libHSghc-boot-th-8.3-ghc8.3.20170323.dll at 0000000062600000
--- Process 408372 loaded \libraries\ghc-prim\dist-install\build\libHSghc-prim-0.5.0.0-ghc8.3.20170323.dll at 000000006AE40000
--- Process 408372 loaded \libraries\ghci\dist-install\build\libHSghci-8.3-ghc8.3.20170323.dll at 000000006A600000
--- Process 408372 loaded \libraries\haskeline\dist-install\build\libHShaskeline-0.7.3.1-ghc8.3.20170323.dll at 000000000D9C0000
--- Process 408372 loaded \libraries\integer-gmp\dist-install\build\libHSinteger-gmp-1.0.0.1-ghc8.3.20170323.dll at 0000000063940000
--- Process 408372 loaded \libraries\process\dist-install\build\libHSprocess-1.4.3.0-ghc8.3.20170323.dll at 0000000065E40000
--- Process 408372 loaded \libraries\time\dist-install\build\libHStime-1.8.0.1-ghc8.3.20170323.dll at 000000006E340000
--- Process 408372 loaded \libraries\transformers\dist-install\build\libHStransformers-0.5.2.0-ghc8.3.20170323.dll at 000000006F140000
--- Process 408372 loaded \libraries\Win32\dist-install\build\libHSWin32-2.5.2.0-ghc8.3.20170323.dll at 0000000071040000
--- Process 408372 loaded \rts\dist\build\libHSrts_thr-ghc8.3.20170323.dll at 0000000065740000
--- Process 408372 loaded \libraries\deepseq\dist-install\build\libHSdeepseq-1.4.3.0-ghc8.3.20170323.dll at 000000006E780000
--- Process 408372 loaded \rts\dist\build\libffi-6.dll at 000000006B740000
--- Process 408372 loaded \libraries\array\dist-install\build\libHSarray-0.5.1.2-ghc8.3.20170323.dll at 0000000068B80000
--- Process 408372 loaded \libraries\binary\dist-install\build\libHSbinary-0.8.4.1-ghc8.3.20170323.dll at 0000000065D40000
--- Process 408372 loaded \libraries\ghc-boot\dist-install\build\libHSghc-boot-8.3-ghc8.3.20170323.dll at 000000000DB70000
--- Process 408372 loaded \libraries\template-haskell\dist-install\build\libHStemplate-haskell-2.12.0.0-ghc8.3.20170323.dll at 000000000DC50000
--- Process 408372 loaded \libraries\pretty\dist-install\build\libHSpretty-1.1.3.3-ghc8.3.20170323.dll at 000000006CE00000
--- Process 408372 loaded \libraries\hoopl\dist-install\build\libHShoopl-3.10.2.2-ghc8.3.20170323.dll at 00000000686C0000
--- Process 408372 loaded \libraries\hpc\dist-install\build\libHShpc-0.6.0.3-ghc8.3.20170323.dll at 0000000062D00000s
bgamari requested changes to this revision.Mar 29 2017, 4:18 PM

Awesome work, @Phyx. Just let me know when I should have another look at this.

This revision now requires changes to proceed.Mar 29 2017, 4:18 PM
austin resigned from this revision.Nov 9 2017, 9:44 AM