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

Fix for raw-dylib when linking with MinGW BFD #88801

Closed
wants to merge 4 commits into from

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Sep 9, 2021

Basically a proof of concept right now, needs more work/thoughts.

@rust-highfive
Copy link
Collaborator

r? @nagisa

(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 Sep 9, 2021
@ricobbe
Copy link
Contributor

ricobbe commented Sep 10, 2021

This approach is admirable in its simplicity, but unfortunately, I don't think it supports all of the cases specified in the RFC. To the best of my knowledge, there is no way to specify stdcall, fastcall, or vectorcall functions in a .DEF file, and stdcall in particular is required in order to link against the Windows API on 32-bit Intel.

@mati865
Copy link
Contributor Author

mati865 commented Sep 10, 2021

Right, I tend to forget i686 targets still do exist.

I think there is no problem with most of calling conventions as long as we properly save them into .def.
Consider this snippet:

// $ cat calling_conventions.c
__cdecl __declspec(dllexport) int cdeclSum(int a, int b)
{
  return a + b;
}

__stdcall __declspec(dllexport) int stdcallSum(int a, int b)
{
  return a + b;
}

__fastcall __declspec(dllexport) int fastcallSum(int a, int b)
{
  return a + b;
}

Compile, link and check the symbols:

$ gcc -shared calling_conventions.c -o calling_conventions.dll

$ nm calling_conventions.dll | rg Sum
64ec14fc T @fastcallSum@8
64ec14e0 T _cdeclSum
64ec14ed T _stdcallSum@8

So far so good, now .def file:

$ gendef calling_conventions.dll
 * [calling_conventions.dll] Found PE image

$ cat calling_conventions.def
;
; Definition file of calling_conventions.dll
; Automatic generated by gendef
; written by Kai Tietz 2008
;
LIBRARY "calling_conventions.dll"
EXPORTS
@fastcallSum@8
cdeclSum
stdcallSum@8@8

Now the import library:

$ dlltool -d calling_conventions.def -l calling_conventions.dll.a

$ nm calling_conventions.dll.a | rg Sum
00000000 I __imp__stdcallSum@8@8
00000000 T _stdcallSum@8@8
00000000 I __imp__cdeclSum
00000000 T _cdeclSum
00000000 T @fastcallSum@8
00000000 I __imp_@fastcallSum@8

I'm not an Windows expert but seems fine for me.


Now the hard part for GNU toolchain, __vectorcall. Let's add this snippet to the previous file:

__vectorcall __declspec(dllexport) int vectorcallSum(int a, int b)
{
  return a + b;
}

Let's build, link and see the symbols:

$ gcc -shared calling_conventions.c -o calling_conventions.dll
calling_conventions.c:16:1: warning: data definition has no type or storage class
   16 | __vectorcall __declspec(dllexport) int vectorcallSum(int a, int b)
      | ^~~~~~~~~~~~
calling_conventions.c:16:1: warning: type defaults to 'int' in declaration of '__vectorcall' [-Wimplicit-int]
calling_conventions.c:16:36: error: expected ',' or ';' before 'int'
   16 | __vectorcall __declspec(dllexport) int vectorcallSum(int a, int b)
      |                                    ^~~

# GCC didn't work, trying Clang
$ clang -shared calling_conventions.c -o calling_conventions.dll
D:/msys64/mingw32/bin/ld: cannot export vectorcallSum@@8: symbol not found
clang: error: linker command failed with exit code 1 (use -v to see invocation)

# Which failed because of BFD..., now LLD
$ clang -fuse-ld=lld -shared calling_conventions.c -o calling_conventions.dll

$ nm calling_conventions.dll | rg Sum
10001520 T @fastcallSum@8
100014e0 T _cdeclSum
10001500 T _stdcallSum@8
10001540 T vectorcallSum@@8

So Clang + LLD worked, let's see about GNU dlltool:

$ gendef calling_conventions.dll
 * [calling_conventions.dll] Found PE image

$ cat calling_conventions.def
;
; Definition file of calling_conventions.dll
; Automatic generated by gendef
; written by Kai Tietz 2008
;
LIBRARY "calling_conventions.dll"
EXPORTS
@fastcallSum@8
cdeclSum
stdcallSum@8@8
vectorcallSum@@8

$ dlltool -d calling_conventions.def -l calling_conventions.dll.a

$ nm calling_conventions.dll.a | rg Sum
00000000 I __imp__vectorcallSum@@8
00000000 T _vectorcallSum@@8
00000000 I __imp__stdcallSum@8@8
00000000 T _stdcallSum@8@8
00000000 I __imp__cdeclSum
00000000 T _cdeclSum
00000000 T @fastcallSum@8
00000000 I __imp_@fastcallSum@8

It worked I guess.
Does this sound right?

@mati865
Copy link
Contributor Author

mati865 commented Sep 11, 2021

So this has almost worked, the main raw-dylib test passes on i686, alt calling convections not so much:

--- output.txt  2021-08-01 14:42:35.185553200 +0200
+++ /d/Projekty/rust/build/i686-pc-windows-gnu/test/run-make/raw-dylib-alt-calling-convention/raw-dylib-alt-calling-convention/output.txt       2021-09-11 01:51:42.126128500 +0200
@@ -11,8 +11,8 @@
 fastcall_fn_2(16, 3.5)
 fastcall_fn_3(3.5)
 fastcall_fn_4(1, 2, 3.0)
-fastcall_fn_5(S { x: 1, y: 2 }, 16)
+fastcall_fn_5(S { x: 1, y: 2 }, 1074266112)
 fastcall_fn_6(S { x: 10, y: 12 })
-fastcall_fn_7(S2 { x: 15, y: 16 }, 3)
+fastcall_fn_7(S2 { x: 15, y: 16 }, 10)
 fastcall_fn_8(S3 { x: [1, 2, 3, 4, 5] }, S3 { x: [6, 7, 8, 9, 10] })
 fastcall_fn_9(1, 3.0)

So far I had no success getting debuggers on Windows to help me with that but I wouldn't be surprised if it's alignment issue.

@mati865 mati865 changed the title Crude proof of concept fix for raw-dylib for MinGW BFD Fix for raw-dylib when linking with MinGW BFD Sep 11, 2021
@mati865 mati865 marked this pull request as ready for review September 11, 2021 17:00
@mati865
Copy link
Contributor Author

mati865 commented Sep 11, 2021

A bit cleaned up the implementation and added 2 MinGW builders to the CI. That way I can test Linux -> Windows and {i686,x86_64} Windows -> {i686,x86_64} Windows cross builds.

Can I have @bors try?

@nagisa
Copy link
Member

nagisa commented Sep 12, 2021

@bors try

@bors
Copy link
Contributor

bors commented Sep 12, 2021

⌛ Trying commit 83c2fea with merge 5e9549d92c173bb99ee4c50a87b6199ac57d62a6...

@bors
Copy link
Contributor

bors commented Sep 12, 2021

☀️ Try build successful - checks-actions
Build commit: 5e9549d92c173bb99ee4c50a87b6199ac57d62a6 (5e9549d92c173bb99ee4c50a87b6199ac57d62a6)

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 13, 2021
@mati865
Copy link
Contributor Author

mati865 commented Sep 13, 2021

It works in all cases except self-contained mode because it's not added to the PATH (even with -C link-self-contained=true).
Bits that handle it for linking are located in compiler\rustc_codegen_ssa\src\back\link.rs so I'll look if I can extract it somewhere.

@nagisa nagisa 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 Sep 17, 2021

fn find_dlltool(sess: &Session) -> OsString {
// When cross-compiling first try binary prefixed with target triple
if sess.host.llvm_target != sess.target.llvm_target {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

llvm_target is probably not the best way to check for this. There are many llvm target triples that are actually equivalent in the end. (e.g. i686-pc-windows-gnu and i686-unknown-windows-gnu and e.g. json targets could specify either).

} else {
prefixed_dlltool.to_string()
};
for dir in env::split_paths(&env::var_os("PATH").unwrap_or_default()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also either verify that the binary is executable at all in some way or provide a way to override this detection. Right now if the system is borked enough to have a non-working i686-w64-mingw32-dlltool but a working dlltool, then there is fairly little the user user can do to work around the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH the more I think about this approach the more hacky it seems.

@@ -1,12 +1,12 @@
# Test the behavior of #[link(.., kind = "raw-dylib")] with alternative calling conventions.

# only-i686-pc-windows-msvc
# only-i686-pc-windows-gnu
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are supposed to eventually test both architectures, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I just didn't want to make spend time on proper fix yet.

@mati865
Copy link
Contributor Author

mati865 commented Sep 17, 2021

With #89025 it's clear how this approach is lacking and won't help much to stabilise raw-dylib.
I'll reach out mstorsjo from LLVM if he can find his patch for long import lib support in LLVM. I think there is no point to keep it open in meantime.

@mati865 mati865 closed this Sep 17, 2021
@iainnicol
Copy link

With #89025 it's clear how this approach is lacking and won't help much to stabilise raw-dylib.

Can you not use just the syntax

func_name @ordinal?

As a separate point, we should probably double-quote the func_name, in case func_name is a def-file keyword.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 14, 2022
…woerister

Implement raw-dylib support for windows-gnu

Add support for `#[link(kind = "raw-dylib")]` on windows-gnu targets.  Work around binutils's linker's inability to read import libraries produced by LLVM by calling out to the binutils `dlltool` utility to create an import library from a temporary .DEF file; this approach is effectively a slightly refined version of `@mati865's` earlier attempt at this strategy in PR rust-lang#88801.  (In particular, this attempt at this strategy adds support for `#[link_ordinal(...)]` as well.)

In support of rust-lang#58713.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 14, 2022
…woerister

Implement raw-dylib support for windows-gnu

Add support for `#[link(kind = "raw-dylib")]` on windows-gnu targets.  Work around binutils's linker's inability to read import libraries produced by LLVM by calling out to the binutils `dlltool` utility to create an import library from a temporary .DEF file; this approach is effectively a slightly refined version of ``@mati865's`` earlier attempt at this strategy in PR rust-lang#88801.  (In particular, this attempt at this strategy adds support for `#[link_ordinal(...)]` as well.)

In support of rust-lang#58713.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 14, 2022
…woerister

Implement raw-dylib support for windows-gnu

Add support for `#[link(kind = "raw-dylib")]` on windows-gnu targets.  Work around binutils's linker's inability to read import libraries produced by LLVM by calling out to the binutils `dlltool` utility to create an import library from a temporary .DEF file; this approach is effectively a slightly refined version of ```@mati865's``` earlier attempt at this strategy in PR rust-lang#88801.  (In particular, this attempt at this strategy adds support for `#[link_ordinal(...)]` as well.)

In support of rust-lang#58713.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 18, 2022
…woerister

Implement raw-dylib support for windows-gnu

Add support for `#[link(kind = "raw-dylib")]` on windows-gnu targets.  Work around binutils's linker's inability to read import libraries produced by LLVM by calling out to the binutils `dlltool` utility to create an import library from a temporary .DEF file; this approach is effectively a slightly refined version of `@mati865's` earlier attempt at this strategy in PR rust-lang#88801.  (In particular, this attempt at this strategy adds support for `#[link_ordinal(...)]` as well.)

In support of rust-lang#58713.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 28, 2022
…dylib, r=michaelwoerister

Fix MinGW target detection in raw-dylib

LLVM target doesn't have to be the same as Rust target so relying on it is wrong.

It was one of concerns in rust-lang#88801 that was not fixed in rust-lang#90782.
@mati865 mati865 deleted the mingw-raw-dylib branch September 24, 2022 13:09
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants