Implement RFC 1717 #37973

vadimcn · 2016-11-24T00:22:08Z

Implement the first two points from #37403.

r? @alexcrichton

vadimcn · 2016-11-24T00:23:11Z

cc @retep998

alexcrichton

Looks good! Some further thoughts:

At the same time, this should supplant #[linked_from], can all usages of that be replaced with this?
Shouldn't dllexport get hooked up into this infrastructure as well?

alexcrichton · 2016-11-28T20:16:03Z

src/librustc_metadata/cstore.rs

+            if item.name == name {
+                item.kind = new_kind;
+                if let Some(new_name) = new_name {
+                    item.name = Symbol::intern(new_name);


Could we take an extra conservative route here and avoid multiple indirections of libraries? Ideally each library would be renamed at most once and wouldn't have to worry about what order we process options in.

Not sure what you mean by "multiple indirections"... Is this about the fact that there may be several NativeLibrary entries for the same library? I don't think we can just merge them,- because library ordering on linker's command line is important.

Oh I mean something like:

rustc -l foo:bar -l bar:baz -l baz:real-name

We shouldn't allow something like that and a lib should be "renamed" at most once.

Ok. Just want to note that this behavior is occasionally useful when you want to override something by appending to a pre-composed command line (usually comes up in makefiles and such).

@alexcrichton: How far did you want to take this checking?

Currently the following are allowed:
-l foo -l foo
-l static=foo -l dylib=foo
which would result in adding foo to the linker command line twice.

Under the new rules, this will merely change the kind of foo two times (assuming it's been defined in crate source, and if not... I am not sure).

It feels a bit weird to allow chaining for kind alterations, but not for names.

Perhaps this part of the RFC merits additional discussion.

Hm that does sound like an unfortunate "regression", but I'd imagine that in practice that was doomed to never link correctly anyway?

It does seem like we can't strictly require that -lfoo isn't specified more than once, but perhaps we can still be strict about renamings where a lib is only renamed once?

alexcrichton · 2016-11-28T20:16:35Z

src/librustc_metadata/creader.rs

+                    cfg: None,
+                    foreign_items: Vec::new(),
+                };
+                register_native_lib(self.sess, self.cstore, None, lib);


In this case the new_name is thrown away, but isn't that the name the library should be linked under?

Good point. Yes, I should use new_name here, if available.
Probably also emit a warning?

Hm actually given the option to do so let's make this a hard error to be conservative.

alexcrichton · 2016-11-28T20:22:17Z

src/test/run-pass/abi-sysv64-arg-passing.rs

@@ -98,7 +98,7 @@ mod tests {
    #[derive(Copy, Clone)]
    pub struct Floats { a: f64, b: u8, c: f64 }

-    #[link(name = "rust_test_helpers")]
+    #[link(name = "rust_test_helpers", kind = "static")]


Were these changes (and those below) required? If so that may be quite worrisome as this is a breaking change...

rust_test_helpers is a static lib, so it started failing on Windows after this change. Granted it was only a couple of tests that used the rust_dbg_static_mut export, but I did a bulk change for consistency sake.

So yeah, there is some potential for breakage, but importing data from a library is a relatively rare thing, and it was already broken on Windows half the time. Not sure how to assess the extent of this breakage though. There is no such thing as Windows crater, right?

Yeah unfortunately no crater coverage just yet, but to be clear the failure mode here was:

The linkage was incorrectly tagged

The library only pulled in statics

We then spit out a linker error

If that's the case then yeah seems like ok breakage. There's a way to fix it to work on all rustc versions, and otherwise we're just fixing a bug.

retep998 · 2016-11-28T21:26:01Z

Shouldn't dllexport get hooked up into this infrastructure as well?

Specifically, if an extern symbol is known to come from a static library, and that extern symbol is reachable in a dylib (including indirectly via inlining and monomorphization) or publicly re-exported in a cdylib, then it should go in the list of symbols to be exported in the .def file that rustc passes to the linker.

vadimcn · 2016-11-28T21:47:00Z

Shouldn't dllexport get hooked up into this infrastructure as well?

I thought we already did this (export all reachable public symbols via a .def file)?
@retep998, which cases aren't working right now?

retep998 · 2016-11-28T21:55:51Z

@vadimcn We do export all reachable stuff right now, except for extern symbols pulled in from static libraries. If libfoo.dll (crate type of dylib) links to the static library bar.lib (kind of static or static-nobundle), and some symbols from bar.lib are reachable externally from the dylib, then they will need to be exported. This edge case is what #[linked_from] was specifically designed to fix, and because this PR will replace #[linked_from], it will need to be able to handle that edge case.

alexcrichton · 2016-11-28T23:13:00Z

@vadimcn I suppose both of my points should get lumped into one. The #[linked_from] is that infrastructure, but the purpose of the RFC was to delete #[linked_from]

vadimcn · 2016-11-28T23:49:58Z

As-implemented, #[link] does the same thing as #[linked_from]. So I'll remove #[linked_from] then...

I've noticed, though, that upstream publics are exported only for dylibs. Cdylibs export just their own public symbols. @alexcrichton: is it supposed to be this way, or was this case overlooked when cdylibs were added?

alexcrichton · 2016-11-28T23:53:09Z

@vadimcn hm I believe that's overlooked, cdylibs shouldn't be exporting statically linked libraries.

vadimcn · 2016-11-29T00:06:23Z

I believe that's overlooked, cdylibs shouldn't be exporting statically linked libraries.

Shouldn't or should? Right now they don't, so this sentence seems self-contradictory :-/

retep998 · 2016-11-29T00:07:42Z

If I have an extern "C" { fn foo(...); } somewhere and then in the root of a cdylib I do pub use whatever::foo;, and foo comes from a static library, then I'd very much expect foo to be exported from the cdylib. If I don't do pub use whatever::foo; however, then it should not be exported.

alexcrichton · 2016-11-29T00:08:50Z

Oh sorry, I misinterpreted. If we have a "bug" where we export fewer symbols let's keep it that way. Easier to later export symbols than to hide them.

retep998 · 2016-11-29T00:09:47Z

We do currently incorrectly export some functions from cdylibs which shouldn't be exported, so fixing that would be really nice. #34493

vadimcn · 2016-11-29T00:16:32Z

I'm talking about this case:

#![crate_type = "cdylib"]

#[link(name = "native", kind="static")]
extern "C" {
    pub fn static_func2(x: i32) -> i32;
    pub static static_global2: i32;
}

#[no_mangle]
pub extern "C" fn local() {}

Right now only the local gets exported.

retep998 · 2016-11-29T00:23:37Z

@vadimcn Because static_func2 and static_global2 are both pub, have unmangled names, and are in the root of a cdylib, I'd definitely prefer that they be exported as well.

alexcrichton · 2016-11-29T21:21:51Z

@vadimcn I'm fine classifying that as a bug, but I'm also fine not explicitly fixing that here unless it just happens to fall out naturally.

vadimcn · 2016-12-02T01:01:25Z

Removed "linked_from" and added more error checking.
r?

alexcrichton · 2016-12-02T21:08:14Z

@bors: r+

Looks great to me, thanks @vadimcn!

bors · 2016-12-02T21:08:15Z

📌 Commit a23c470 has been approved by alexcrichton

arielb1 · 2016-12-02T22:01:18Z

src/librustc_metadata/creader.rs

+                        lib.name = Symbol::intern(new_name);
+                    }
+                    found = true;
+                    break;


How does this break live along with the (there may be more than one) comment?

vadimcn · 2016-12-02T23:15:28Z

How does this break live along with the (there may be more than one) comment?

You are right, it doesn't :(

@bors: r-

vadimcn · 2016-12-03T01:53:14Z

@bors: r=alexcrichton

bors · 2016-12-03T01:53:15Z

📌 Commit 923034f has been approved by alexcrichton

bors · 2016-12-03T02:08:56Z

⌛ Testing commit 923034f with merge 32e99bd...

bors · 2016-12-03T03:52:56Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

vadimcn · 2016-12-03T22:13:19Z

Hrm, is there a way to restrict a test to -windows-msvc only?

vadimcn · 2016-12-03T23:02:45Z

@bors: r=alexcrichton

bors · 2016-12-03T23:02:46Z

📌 Commit 2e03549 has been approved by alexcrichton

bors · 2016-12-04T09:51:12Z

⌛ Testing commit 2e03549 with merge 925bc9b...

bors · 2016-12-04T11:21:43Z

💔 Test failed - auto-win-msvc-64-opt-rustbuild

vadimcn · 2016-12-06T05:22:18Z

Turns out I never ran the full test suite on a Windows host. :-/
Added code to handle #[link(cfg=...)].
r?

alexcrichton · 2016-12-06T07:59:37Z

@bors: r+

bors · 2016-12-06T07:59:39Z

📌 Commit 7d05d1e has been approved by alexcrichton

bors · 2016-12-06T10:49:47Z

⌛ Testing commit 7d05d1e with merge 1692c0b...

@alexcrichton

Implement RFC 1717 Implement the first two points from #37403. r? @alexcrichton

bors · 2016-12-06T14:11:42Z

Ericson2314 · 2017-01-07T00:46:43Z

src/librustc_metadata/creader.rs

+            let mut found = false;
+            for lib in self.cstore.get_used_libraries().borrow_mut().iter_mut() {
+                if lib.name == name as &str {
+                    lib.kind = kind;


Does this mean the a kind specified in the source will be silently overwritten?

rust-highfive assigned alexcrichton Nov 24, 2016

alexcrichton reviewed Nov 28, 2016

View reviewed changes

retep998 mentioned this pull request Dec 1, 2016

Add option to not include .o files from a .a in the .rlib which uses them #38095

Closed

vadimcn added 3 commits December 1, 2016 16:22

Emit 'dllimport' attribute for dylib foreign items on Windows.

bc019df

Fix rust_test_helpers linkage.

4508e8a

Implement native library kind and name overrides from the command line.

13477c7

vadimcn force-pushed the dllimport branch from 459d419 to 0c6b879 Compare December 2, 2016 00:37

vadimcn added 2 commits December 1, 2016 16:56

Remove the "linked_from" feature.

a9a6f8c

Tighten up error checking of library renames.

a23c470

vadimcn force-pushed the dllimport branch from 0c6b879 to a23c470 Compare December 2, 2016 00:57

alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Dec 2, 2016

arielb1 reviewed Dec 2, 2016

View reviewed changes

Rename _all_ library instances.

923034f

Ignore test on -windows-gnu.

2e03549

vadimcn added 2 commits December 5, 2016 16:40

Annotate more tests with kind="static"

b700dd3

Consider only libs that aren't excluded by #[link(cfg=...)]

7d05d1e

bors added a commit that referenced this pull request Dec 6, 2016

Auto merge of #37973 - vadimcn:dllimport, r=alexcrichton

1692c0b

Implement RFC 1717 Implement the first two points from #37403. r? @alexcrichton

bors merged commit 7d05d1e into rust-lang:master Dec 6, 2016

vadimcn deleted the dllimport branch December 6, 2016 23:17

retep998 mentioned this pull request Dec 11, 2016

Allow users to have 2 different build scripts for Windows depending if rust is the msvc or if it is the gnu build. #38285

Closed

Ericson2314 reviewed Jan 7, 2017

View reviewed changes

alexcrichton mentioned this pull request Jan 10, 2017

"Renaming of the library C was specified" error only on nightly #38850

Closed

cramertj mentioned this pull request Jan 17, 2018

Tracking issue for linked_from feature #29629

Closed

ehuss mentioned this pull request Mar 1, 2019

Update attribute documentation. rust-lang/reference#528

Merged

Implement RFC 1717 #37973

Implement RFC 1717 #37973

Conversation

vadimcn commented Nov 24, 2016

vadimcn commented Nov 24, 2016

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimcn Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retep998 commented Nov 28, 2016

vadimcn commented Nov 28, 2016 • edited Loading

retep998 commented Nov 28, 2016

alexcrichton commented Nov 28, 2016

vadimcn commented Nov 28, 2016

alexcrichton commented Nov 28, 2016

vadimcn commented Nov 29, 2016

retep998 commented Nov 29, 2016 • edited Loading

alexcrichton commented Nov 29, 2016

retep998 commented Nov 29, 2016

vadimcn commented Nov 29, 2016

retep998 commented Nov 29, 2016

alexcrichton commented Nov 29, 2016

vadimcn commented Dec 2, 2016

alexcrichton commented Dec 2, 2016

bors commented Dec 2, 2016

Choose a reason for hiding this comment

vadimcn commented Dec 2, 2016 • edited Loading

vadimcn commented Dec 3, 2016

bors commented Dec 3, 2016

bors commented Dec 3, 2016

bors commented Dec 3, 2016

vadimcn commented Dec 3, 2016

vadimcn commented Dec 3, 2016

bors commented Dec 3, 2016

bors commented Dec 4, 2016

bors commented Dec 4, 2016

vadimcn commented Dec 6, 2016

alexcrichton commented Dec 6, 2016

bors commented Dec 6, 2016

bors commented Dec 6, 2016

bors commented Dec 6, 2016

Choose a reason for hiding this comment

vadimcn Nov 28, 2016 •

edited

Loading

vadimcn commented Nov 28, 2016 •

edited

Loading

retep998 commented Nov 29, 2016 •

edited

Loading

vadimcn commented Dec 2, 2016 •

edited

Loading