Implement RFC 1717 #37973

Merged
merged 9 commits into from Dec 6, 2016

Projects

None yet

6 participants

@vadimcn
Contributor
vadimcn commented Nov 24, 2016

Implement the first two points from #37403.

r? @alexcrichton

@vadimcn
Contributor
vadimcn commented Nov 24, 2016
@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?
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);
@alexcrichton
alexcrichton Nov 28, 2016 Member

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.

@vadimcn
vadimcn Nov 28, 2016 Contributor

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.

@alexcrichton
alexcrichton Nov 28, 2016 Member

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.

@vadimcn
vadimcn Nov 29, 2016 Contributor

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

@vadimcn
vadimcn Nov 30, 2016 Contributor

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

@alexcrichton
alexcrichton Dec 1, 2016 Member

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?

+ cfg: None,
+ foreign_items: Vec::new(),
+ };
+ register_native_lib(self.sess, self.cstore, None, lib);
@alexcrichton
alexcrichton Nov 28, 2016 Member

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

@vadimcn
vadimcn Nov 28, 2016 Contributor

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

@alexcrichton
alexcrichton Nov 28, 2016 Member

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

@@ -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")]
@alexcrichton
alexcrichton Nov 28, 2016 Member

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

@vadimcn
vadimcn Nov 28, 2016 edited Contributor

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?

@alexcrichton
alexcrichton Nov 28, 2016 Member

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
Member

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
Contributor
vadimcn commented Nov 28, 2016 edited

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
Member

@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
Member

@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
Contributor
vadimcn commented Nov 28, 2016

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
Member

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

@vadimcn
Contributor
vadimcn commented Nov 29, 2016

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
Member
retep998 commented Nov 29, 2016 edited

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
Member

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
Member

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

@vadimcn
Contributor
vadimcn commented Nov 29, 2016

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
Member

@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
Member

@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
Contributor
vadimcn commented Dec 2, 2016

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

@alexcrichton
Member

@bors: r+

Looks great to me, thanks @vadimcn!

@bors
Contributor
bors commented Dec 2, 2016

📌 Commit a23c470 has been approved by alexcrichton

src/librustc_metadata/creader.rs
+ lib.name = Symbol::intern(new_name);
+ }
+ found = true;
+ break;
@arielb1
arielb1 Dec 2, 2016 Contributor

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

@vadimcn
Contributor
vadimcn commented Dec 2, 2016 edited

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

You are right, it doesn't :(

@bors: r-

@vadimcn vadimcn Rename _all_ library instances.
923034f
@vadimcn
Contributor
vadimcn commented Dec 3, 2016

@bors: r=alexcrichton

@bors
Contributor
bors commented Dec 3, 2016

📌 Commit 923034f has been approved by alexcrichton

@bors
Contributor
bors commented Dec 3, 2016

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

@bors bors added a commit that referenced this pull request Dec 3, 2016
@bors bors Auto merge of #37973 - vadimcn:dllimport, r=alexcrichton
Implement RFC 1717

Implement the first two points from #37403.

r? @alexcrichton
32e99bd
@bors
Contributor
bors commented Dec 3, 2016

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

@vadimcn
Contributor
vadimcn commented Dec 3, 2016

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

@vadimcn vadimcn Ignore test on -windows-gnu.
2e03549
@vadimcn
Contributor
vadimcn commented Dec 3, 2016

@bors: r=alexcrichton

@bors
Contributor
bors commented Dec 3, 2016

📌 Commit 2e03549 has been approved by alexcrichton

@bors
Contributor
bors commented Dec 4, 2016

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

@bors bors added a commit that referenced this pull request Dec 4, 2016
@bors bors Auto merge of #37973 - vadimcn:dllimport, r=alexcrichton
Implement RFC 1717

Implement the first two points from #37403.

r? @alexcrichton
925bc9b
@bors
Contributor
bors commented Dec 4, 2016

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

vadimcn added some commits Dec 6, 2016
@vadimcn vadimcn Annotate more tests with kind="static" b700dd3
@vadimcn vadimcn Consider only libs that aren't excluded by #[link(cfg=...)]
7d05d1e
@vadimcn
Contributor
vadimcn commented Dec 6, 2016

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

@alexcrichton
Member

@bors: r+

@bors
Contributor
bors commented Dec 6, 2016

📌 Commit 7d05d1e has been approved by alexcrichton

@bors
Contributor
bors commented Dec 6, 2016

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

@bors bors added a commit that referenced this pull request Dec 6, 2016
@bors bors Auto merge of #37973 - vadimcn:dllimport, r=alexcrichton
Implement RFC 1717

Implement the first two points from #37403.

r? @alexcrichton
1692c0b
@bors bors merged commit 7d05d1e into rust-lang:master Dec 6, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@vadimcn vadimcn deleted the vadimcn:dllimport branch Dec 6, 2016
+ 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;
@Ericson2314
Ericson2314 Jan 7, 2017 Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment