-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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:"); | ||
|
||
|
@@ -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)); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot to ask - what exactly makes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
TL;DR we now manually add dllexport/dllimport. With version-scripts LD sort of scans the symbols to decide when to automatically add dllexport. 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 { | ||
|
@@ -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); | ||
|
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() {} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my fault, I must have reverted dllexport change when testing stupid ideas and committed it like that: #72049 (comment)
Me too 😞
There was a problem hiding this comment.
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
.