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: enable dllexport/dllimport #72049

Merged
merged 2 commits into from
Jul 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/librustc_codegen_llvm/callee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,12 @@ pub fn get_fn(cx: &CodegenCx<'ll, 'tcx>, instance: Instance<'tcx>) -> &'ll Value
}
}

if cx.use_dll_storage_attrs && tcx.is_dllimport_foreign_item(instance_def_id) {
// MinGW: For backward compatibility we rely on the linker to decide whether it
// should use dllimport for functions.
if cx.use_dll_storage_attrs
&& tcx.is_dllimport_foreign_item(instance_def_id)
&& tcx.sess.target.target.target_env != "gnu"
Copy link
Contributor

Choose a reason for hiding this comment

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

If dllimport is no longer emitted for functions and this PR only affects statics now, then I'm not sure how it can fix #72319 or #50176 which are not about statics.

Copy link
Contributor Author

@mati865 mati865 Jun 10, 2020

Choose a reason for hiding this comment

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

Those 2 issues are not caused by missing dllimport but dllexport since they are about using symbols exported by Rust.
#50176 with nightly vs this PR:

$ cargo b --target i686-pc-windows-gnu
   Compiling cdylib v0.1.0 (D:\msys64\tmp\cdylib)
    Finished dev [unoptimized + debuginfo] target(s) in 1.75s

mateusz@DESKTOP-mati MINGW64 /tmp/cdylib
$ llvm-readobj --coff-exports target/i686-pc-windows-gnu/debug/cdylib.dll

File: target/i686-pc-windows-gnu/debug/cdylib.dll
Format: COFF-i386
Arch: i386
AddressSize: 32bit
Export {
  Ordinal: 1
  Name: rust_eh_personality
  RVA: 0x33070
}
Export {
  Ordinal: 2
  Name: rust_eh_register_frames
  RVA: 0x33050
}
Export {
  Ordinal: 3
  Name: rust_eh_unregister_frames
  RVA: 0x33060
}

mateusz@DESKTOP-mati MINGW64 /tmp/cdylib
$ cargo +stage2 b --target i686-pc-windows-gnu
   Compiling cdylib v0.1.0 (D:\msys64\tmp\cdylib)
    Finished dev [unoptimized + debuginfo] target(s) in 1.13s

mateusz@DESKTOP-mati MINGW64 /tmp/cdylib
$ llvm-readobj --coff-exports target/i686-pc-windows-gnu/debug/cdylib.dll

File: target/i686-pc-windows-gnu/debug/cdylib.dll
Format: COFF-i386
Arch: i386
AddressSize: 32bit
Export {
  Ordinal: 1
  Name: SomeFunction
  RVA: 0x1460
}
Export {
  Ordinal: 2
  Name: rust_eh_personality
  RVA: 0x41AC0
}
Export {
  Ordinal: 3
  Name: rust_eh_register_frames
  RVA: 0x41BD0
}
Export {
  Ordinal: 4
  Name: rust_eh_unregister_frames
  RVA: 0x41BE0
}

Guessing which functions to mark with dllexport is hard because linker doesn't know how you are going to use them. LD does it a bit better than coin toss.
Automatic dllimport is a bit complicated but works well for functions with both linkers.

I'll see if Rust testuite passes with all dllimports disabled for MinGW.

Copy link
Contributor

@petrochenkov petrochenkov Jun 10, 2020

Choose a reason for hiding this comment

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

Ok, I no longer understand what happens.
How does this PR change dllexports? use_dll_storage_attrs doesn't affect dllexports at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh shit, I never noticed the non-assert change in src/librustc_codegen_ssa/back/write.rs.

Copy link
Contributor

@petrochenkov petrochenkov Jun 10, 2020

Choose a reason for hiding this comment

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

Now I need to rethink everything I know about this PR.

(However, if this can be done by changing only the dllexport side but not the dllimport side, it would be great.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh shit, I never noticed the non-assert change in src/librustc_codegen_ssa/back/write.rs.

That was my fault, I must have reverted dllexport change when testing stupid ideas and committed it like that: #72049 (comment)

Now I need to rethink everything I know about this PR.

Me too 😞

Copy link
Contributor Author

@mati865 mati865 Jun 10, 2020

Choose a reason for hiding this comment

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

OK, statics do not work without dllimport.

Note to myself: automatic dllimport very likely doesn't work with uwp-windows-gnu.

{
unsafe {
llvm::LLVMSetDLLStorageClass(llfn, llvm::DLLStorageClass::DllImport);
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_codegen_llvm/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ impl CodegenCx<'ll, 'tcx> {
// argument validation.
debug_assert!(
!(self.tcx.sess.opts.cg.linker_plugin_lto.enabled()
&& self.tcx.sess.target.target.options.is_like_msvc
&& self.tcx.sess.target.target.options.is_like_windows
&& self.tcx.sess.opts.cg.prefer_dynamic)
);

Expand Down
15 changes: 12 additions & 3 deletions src/librustc_codegen_llvm/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,16 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
// attributes in LLVM IR as well as native dependencies (in C these
// correspond to `__declspec(dllimport)`).
//
// Whenever a dynamic library is built by MSVC it must have its public
// LD (BFD) in MinGW mode can often correctly guess `dllexport` but
// relying on that can result in issues like #50176.
// LLD won't support that and expects symbols with proper attributes.
// Because of that we make MinGW target emit dllexport just like MSVC.
// When it comes to dllimport we use it for constants but for functions
// rely on the linker to do the right thing. Opposed to dllexport this
// task is easy for them (both LD and LLD) and allows us to easily use
// symbols from static libraries in shared libraries.
//
// Whenever a dynamic library is built on Windows it must have its public
// interface specified by functions tagged with `dllexport` or otherwise
// they're not available to be linked against. This poses a few problems
// for the compiler, some of which are somewhat fundamental, but we use
Expand Down Expand Up @@ -254,8 +263,8 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
// this effect) by marking very little as `dllimport` and praying the
// linker will take care of everything. Fixing this problem will likely
// require adding a few attributes to Rust itself (feature gated at the
// start) and then strongly recommending static linkage on MSVC!
let use_dll_storage_attrs = tcx.sess.target.target.options.is_like_msvc;
// start) and then strongly recommending static linkage on Windows!
petrochenkov marked this conversation as resolved.
Show resolved Hide resolved
let use_dll_storage_attrs = tcx.sess.target.target.options.is_like_windows;

let check_overflow = tcx.sess.overflow_checks();

Expand Down
23 changes: 21 additions & 2 deletions src/librustc_codegen_ssa/back/linker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -523,8 +523,9 @@ impl<'a> Linker for GccLinker<'a> {
return;
}

let is_windows = self.sess.target.target.options.is_like_windows;
let mut arg = OsString::new();
let path = tmpdir.join("list");
let path = tmpdir.join(if is_windows { "list.def" } else { "list" });
petrochenkov marked this conversation as resolved.
Show resolved Hide resolved

debug!("EXPORTED SYMBOLS:");

Expand All @@ -540,6 +541,21 @@ impl<'a> Linker for GccLinker<'a> {
if let Err(e) = res {
self.sess.fatal(&format!("failed to write lib.def file: {}", e));
}
} else if is_windows {
let res: io::Result<()> = try {
let mut f = BufWriter::new(File::create(&path)?);

// .def file similar to MSVC one but without LIBRARY section
// because LD doesn't like when it's empty
writeln!(f, "EXPORTS")?;
for symbol in self.info.exports[&crate_type].iter() {
debug!(" _{}", symbol);
writeln!(f, " {}", symbol)?;
}
};
if let Err(e) = res {
self.sess.fatal(&format!("failed to write list.def file: {}", e));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot to ask - what exactly makes .def files work with ld in this PR?
The --version-script -> list.def change caused test failures in #70852 despite the export list being the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to ask - what exactly makes .def files work with ld in this PR?

TL;DR we now manually add dllexport/dllimport.

With version-scripts LD sort of scans the symbols to decide when to automatically add dllexport.
With .def it doesn't use automatic dllexport but dllexport can be manually enabled by what this PR does or writing DATA next to the symbol on the list.

Previously most of the tests worked before because of automatic dllimport, broken tests were using statics which don't work with automatic dllimport.

} else {
// Write an LD version script
let res: io::Result<()> = try {
Expand Down Expand Up @@ -573,7 +589,10 @@ impl<'a> Linker for GccLinker<'a> {
if !self.is_ld {
arg.push("-Wl,")
}
arg.push("--version-script=");
// Both LD and LLD accept export list in *.def file form, there are no flags required
if !is_windows {
arg.push("--version-script=")
}
}

arg.push(&path);
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_codegen_ssa/back/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1846,11 +1846,11 @@ fn msvc_imps_needed(tcx: TyCtxt<'_>) -> bool {
// something is wrong with commandline arg validation.
assert!(
!(tcx.sess.opts.cg.linker_plugin_lto.enabled()
&& tcx.sess.target.target.options.is_like_msvc
&& tcx.sess.target.target.options.is_like_windows
&& tcx.sess.opts.cg.prefer_dynamic)
);

tcx.sess.target.target.options.is_like_msvc &&
tcx.sess.target.target.options.is_like_windows &&
tcx.sess.crate_types().iter().any(|ct| *ct == CrateType::Rlib) &&
// ThinLTO can't handle this workaround in all cases, so we don't
// emit the `__imp_` symbols. Instead we make them unnecessary by disallowing
Expand Down
10 changes: 5 additions & 5 deletions src/librustc_session/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1294,19 +1294,19 @@ pub fn build_session(
// commandline argument, you can do so here.
fn validate_commandline_args_with_session_available(sess: &Session) {
// Since we don't know if code in an rlib will be linked to statically or
// dynamically downstream, rustc generates `__imp_` symbols that help the
// MSVC linker deal with this lack of knowledge (#27438). Unfortunately,
// dynamically downstream, rustc generates `__imp_` symbols that help linkers
// on Windows deal with this lack of knowledge (#27438). Unfortunately,
// these manually generated symbols confuse LLD when it tries to merge
// bitcode during ThinLTO. Therefore we disallow dynamic linking on MSVC
// bitcode during ThinLTO. Therefore we disallow dynamic linking on Windows
// when compiling for LLD ThinLTO. This way we can validly just not generate
// the `dllimport` attributes and `__imp_` symbols in that case.
if sess.opts.cg.linker_plugin_lto.enabled()
&& sess.opts.cg.prefer_dynamic
&& sess.target.target.options.is_like_msvc
&& sess.target.target.options.is_like_windows
{
sess.err(
"Linker plugin based LTO is not supported together with \
mati865 marked this conversation as resolved.
Show resolved Hide resolved
`-C prefer-dynamic` when targeting MSVC",
`-C prefer-dynamic` when targeting Windows-like targets",
);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
include ../tools.mk

# only-windows-gnu

all:
$(RUSTC) foo.rs
# FIXME: we should make sure __stdcall calling convention is used here
# but that only works with LLD right now
nm -g "$(call IMPLIB,foo)" | $(CGREP) bar
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#![crate_type = "cdylib"]

#[no_mangle]
pub extern "system" fn bar() {}
1 change: 1 addition & 0 deletions src/test/run-make-fulldeps/tools.mk
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ ifdef IS_MSVC
STATICLIB = $(TMPDIR)/$(1).lib
STATICLIB_GLOB = $(1)*.lib
else
IMPLIB = $(TMPDIR)/lib$(1).dll.a
STATICLIB = $(TMPDIR)/lib$(1).a
STATICLIB_GLOB = lib$(1)*.a
endif
Expand Down