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

mati865 · 2021-09-09T23:37:58Z

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

rust-highfive · 2021-09-09T23:38:01Z

(rust-highfive has picked a reviewer for you, use r? to override)

ricobbe · 2021-09-10T17:07:53Z

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 · 2021-09-10T19:30:46Z

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 · 2021-09-11T00:40:39Z

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 · 2021-09-11T17:03:21Z

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 · 2021-09-12T19:49:15Z

@bors try

bors · 2021-09-12T19:49:23Z

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

bors · 2021-09-12T21:15:09Z

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

mati865 · 2021-09-13T22:55:29Z

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 · 2021-09-17T12:56:44Z

compiler/rustc_codegen_llvm/src/back/archive.rs

+
+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 {


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

nagisa · 2021-09-17T12:59:47Z

compiler/rustc_codegen_llvm/src/back/archive.rs

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


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.

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

nagisa · 2021-09-17T13:01:10Z

src/test/run-make/raw-dylib-alt-calling-convention/Makefile

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

-# only-i686-pc-windows-msvc
+# only-i686-pc-windows-gnu


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

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

mati865 · 2021-09-17T18:49:16Z

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.

iainnicol · 2021-09-20T20:18:00Z

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.

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

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

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

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

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

Crude proof of concept

eeef857

rust-highfive assigned nagisa Sep 9, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2021

mati865 mentioned this pull request Sep 9, 2021

Tracking issue for RFC 2627: #[link(kind="raw-dylib")] #58713

Open

17 tasks

Almost fix i686

e5f5895

mati865 added 2 commits September 11, 2021 18:55

Make dlltool version coexist with LLVM one

d12b0e7

Build MinGW on try

83c2fea

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 marked this pull request as ready for review September 11, 2021 17:00

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

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

nagisa reviewed Sep 17, 2021

View reviewed changes

mati865 closed this Sep 17, 2021

ricobbe mentioned this pull request Nov 10, 2021

Implement raw-dylib support for windows-gnu #90782

Merged

mati865 mentioned this pull request Feb 25, 2022

Fix MinGW target detection in raw-dylib #94365

Merged

mati865 deleted the mingw-raw-dylib branch September 24, 2022 13:09

Uh oh!

Conversation

mati865 commented Sep 9, 2021

Uh oh!

rust-highfive commented Sep 9, 2021

Uh oh!

ricobbe commented Sep 10, 2021

Uh oh!

mati865 commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mati865 commented Sep 11, 2021

Uh oh!

mati865 commented Sep 11, 2021

Uh oh!

nagisa commented Sep 12, 2021

Uh oh!

bors commented Sep 12, 2021

Uh oh!

bors commented Sep 12, 2021

Uh oh!

mati865 commented Sep 13, 2021

Uh oh!

nagisa Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

nagisa Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

mati865 Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

nagisa Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

mati865 Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

mati865 commented Sep 17, 2021

Uh oh!

iainnicol commented Sep 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mati865 commented Sep 10, 2021 •

edited

Loading