Add short library names support to Windows linker
ClosedPublic

Authored by Phyx on Oct 5 2015, 3:27 AM.

Details

Summary

Make Linker.hs try asking gcc for lib%s.dll as well, also changed tryGcc to pass -L to all components
by using -B instead. These two fix shortnames linking on windows.

Test Plan

./validate

re-enabled tests: ghcilink003, ghcilink006 and T3333
Added two tests: load_short_name and enabled T1407 on windows.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Phyx updated this revision to Diff 4427.Oct 5 2015, 3:27 AM
Phyx retitled this revision from to Add short library names support to Windows linker.
Phyx updated this object.
Phyx edited the test plan for this revision. (Show Details)
Phyx added reviewers: thomie, bgamari, austin.
Phyx updated the Trac tickets for this revision.
Phyx added a subscriber: Restricted Project.
Phyx added inline comments.Oct 5 2015, 3:36 AM
testsuite/tests/ghci/linking/win_dyn/all.T
9 ↗(On Diff #4427)

whoops... I should probably compile libA.dll in this test as well.. Anyway to do it from here? since the action below is ghci_script and not run_command with a $MAKE

Phyx updated this revision to Diff 4428.Oct 5 2015, 5:04 AM
Phyx edited edge metadata.
  • Shortname: Fixed the windows only tests to not run on linux
thomie requested changes to this revision.Oct 5 2015, 10:09 AM
thomie edited edge metadata.

Why -B instead of -L? Can you add a comment.

testsuite/tests/ghci/linking/win_dyn/all.T
10 ↗(On Diff #4428)

You can use the pre_cmd setup function. Something like this (also check existing examples):

pre_cmd('$MAKE -s --no-print-directory compile_A_B')

You just have to make sure no two tests write to the same path.

(You could also change load_short_name to a ghci_script test, giving it a ghci script similar to the one for ghci_short_name but without the :set -lA, and adding the setup function: extra_hc_opts('-lA'). But it's also fine the way it is now).

One other remark: shouldn't you be loading B instead of A, since B depends on A and that is what you want to test presumably.

This revision now requires changes to proceed.Oct 5 2015, 10:09 AM
Phyx updated this revision to Diff 4447.Oct 8 2015, 8:02 AM
Phyx edited edge metadata.
  • Shortname: Added explanation of -B over -L
  • Shortname: cleaned up tests and made T1407 be one test for both windows and nix
Phyx added a comment.EditedOct 8 2015, 8:04 AM

I've added a comment in the source. Not sure if by design or a bug but when using --print-file-name it doesn't seem to use the values of -L.

$ gcc --print-file-name=libA.dll -Lbin
libA.dll

$ gcc --print-file-name=libA.dll -Bbin
bin/libA.dll

And

$ gcc --print-file-name=libA.dll -LE:\Foo\Bin -print-search-dirs
...
libraries: =E:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/4.9.2/;... 


$ gcc --print-file-name=libA.dll -B"E:\Foo\Bin" -print-search-dirs
...
libraries: =E:/Foo/Binx86_64-w64-mingw32/4.9.2/;E:/Foo/Bin;...
testsuite/tests/ghci/linking/win_dyn/all.T
10 ↗(On Diff #4428)

Thanks, I've changed the tests, I've left the load_short_name as is because the script would be empty in this case.

I actually do meant to load A as I'm just fixing short names in this commit. Loading dependencies is still broken. I have removed B from here and will add it when I commit the fix for dependencies.

currently what this test is testing is if libraries with short names are found and loaded. But in order for that to work I need to have it in a folder that's away from the standard windows loader's path. Hence the extra folder. This is just so I know it gets handled by Linker.hs instead of Linker.c.

Phyx updated this revision to Diff 4448.Oct 8 2015, 8:16 AM
Phyx edited edge metadata.
  • Shortname: make sure libAS is cleaned on all platforms
Phyx updated this revision to Diff 4450.Oct 8 2015, 11:29 AM
Phyx edited edge metadata.
  • Shortname: fix tests on linux
Phyx updated this revision to Diff 4451.Oct 8 2015, 12:24 PM
Phyx edited edge metadata.
  • Shortname: fix tests on linux
thomie requested changes to this revision.Oct 9 2015, 2:10 AM
thomie edited edge metadata.

The problem seems to be that dirs in searchForLibUsingGcc doesn't contain the current directory (.).

And so gcc --print-file-name won't turn libA.so into ./libA.so or an absolute path, and searchForLibUsingGcc will return Nothing because the input path and the output path are the same.

$ ~/ghc-devel2/inplace/bin/ghc-stage2 --interactive -lAS -v
...
*** gcc:
/usr/bin/gcc -fno-stack-protector -DTABLES_NEXT_TO_CODE --print-file-name libAS.so
*** gcc:
/usr/bin/gcc -fno-stack-protector -DTABLES_NEXT_TO_CODE --print-file-name liblibAS.so
Loading object (dynamic) AS ... failed.
testsuite/tests/ghci/linking/dyn/Makefile
19

Changing this to gcc -shared A.c -o "bin_short/$(call DLL,A)" (note the forward slash), and this test also works on Linux. I tested it.

This revision now requires changes to proceed.Oct 9 2015, 2:10 AM
Phyx updated this revision to Diff 4458.Oct 9 2015, 1:33 PM
Phyx edited edge metadata.
  • Shortname: Enabled load_short_name on linux as well
Phyx marked an inline comment as done.Oct 9 2015, 1:34 PM

@thomie thanks! I'll set up a Linux vm this weekend so I can fix it myself next time :)

Phyx edited the test plan for this revision. (Show Details)Oct 9 2015, 3:31 PM
Phyx edited edge metadata.
thomie accepted this revision.Oct 9 2015, 5:33 PM
thomie edited edge metadata.
In D1310#37394, @Phyx wrote:

@thomie thanks! I'll set up a Linux vm this weekend so I can fix it myself next time :)

No worries.

This revision is now accepted and ready to land.Oct 9 2015, 5:33 PM
bgamari accepted this revision.Oct 10 2015, 7:29 AM
bgamari edited edge metadata.

Looks great! Thanks again @Phyx and @thomie!

This revision was automatically updated to reflect the committed changes.