Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MinGW: improve compatibility with LLD #70852

Closed
wants to merge 1 commit into from

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Apr 6, 2020

LLD unlike LD doesn't implement linker script support for MinGW and this has so low priority it may be never implemented. However they both support MSVC like .def files.
Once this and rust-lang/cargo#6875 reach beta and bootstrap compiler is updated using LLD as the linker will work fine.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 6, 2020
@mati865
Copy link
Contributor Author

mati865 commented Apr 6, 2020

Updated description, it was PEBCAK and this PR doesn't help with #50176

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 6, 2020
@mati865 mati865 force-pushed the mingw-lld branch 2 times, most recently from a0cf890 to 809aa16 Compare April 7, 2020 10:21
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 7, 2020

📌 Commit 809aa16c40d962c7a5090223a5ce98cd57ee233e has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 7, 2020
This greatly improves compatibility with LLD which is not going to support linker scripts anytime soon
@mati865
Copy link
Contributor Author

mati865 commented Apr 10, 2020

Just noticed that the commit message was malformed, sorry for the trouble.

@mati865
Copy link
Contributor Author

mati865 commented Apr 11, 2020

@petrochenkov could you r+ it again? ^

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 11, 2020

📌 Commit 44e602f has been approved by petrochenkov

@bors
Copy link
Contributor

bors commented Apr 12, 2020

⌛ Testing commit 44e602f with merge 54fd296879f8ec8875d2aaf5735bc395c701ad83...

@bors
Copy link
Contributor

bors commented Apr 12, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 12, 2020
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2020
@mati865
Copy link
Contributor Author

mati865 commented Apr 12, 2020

Some symbols are not exported in the import libs. This was not visible before #70937 because LD could not find import lib (wrong extension). Good thing #70937 has landed before this PR.

This gonna be fun...

@mati865
Copy link
Contributor Author

mati865 commented Apr 13, 2020

It's much worse than I thought. I'll see if I can implement version-script support for LLD.

@mati865 mati865 closed this Apr 13, 2020
@petrochenkov
Copy link
Contributor

@mati865

It's much worse than I thought.

😆
What exactly? ld doesn't really support .def?

@mati865
Copy link
Contributor Author

mati865 commented Apr 13, 2020

It does support it at least to some extent. Either the .def file needs some more work on it or it's just bugged.

Regressions I have found:

  • DLLs have additional dependencies:
    • symbols exported via .def (bad):
      $ ntldd cci_const_block.dll
              std-979e3b2f81b3e5a5.dll => D:\Projekty\rust\build\x86_64-pc-windows-gnu\stage2\bin\std-979e3b2f81b3e5a5.dll (0x0000000000ef0000)
              KERNEL32.dll => C:\WINDOWS\SYSTEM32\KERNEL32.dll (0x00000000016f0000)
              msvcrt.dll => C:\WINDOWS\SYSTEM32\msvcrt.dll (0x00000000006e0000)
      
    • symbols exported via version-script (good):
      $ ntldd cci_const_block.dll
              KERNEL32.dll => C:\WINDOWS\SYSTEM32\KERNEL32.dll (0x0000000000eb0000)
              msvcrt.dll => C:\WINDOWS\SYSTEM32\msvcrt.dll (0x0000000000eb0000)
      
  • linking to DLLs via implibs causes segfaults (direct linking to DLL):
    • .def (bad):
      $ nm libcci_const_block.dll.a | grep BLOCK_FN_DEF
      0000000000000000 I __imp__ZN15cci_const_block12BLOCK_FN_DEF17h2bbe18e6734ce46cE
      0000000000000000 T _ZN15cci_const_block12BLOCK_FN_DEF17h2bbe18e6734ce46cE
      
    • version-script (good):
      $ nm const-block-cross-crate-fn/auxiliary/libcci_const_block.dll.a | grep BLOCK_FN_DEF
      0000000000000000 I __imp__ZN15cci_const_block12BLOCK_FN_DEF17h2bbe18e6734ce46cE
      0000000000000000 I __nm__ZN15cci_const_block12BLOCK_FN_DEF17h2bbe18e6734ce46cE
      
    • the DLL:
      $ nm cci_const_block.dll | grep BLOCK_FN_DEF
      0000000067cc5000 R _ZN15cci_const_block12BLOCK_FN_DEF17h2bbe18e6734ce46cE
      0000000067cc13b0 t _ZN15cci_const_block12BLOCK_FN_DEF3foo17h3d71b48312ebbcc6E
      

@mati865
Copy link
Contributor Author

mati865 commented May 9, 2020

Another attempt at breaking improving MinGW: #72049.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants