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

Weak external symbol support #11978

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
8 participants
@bnoordhuis
Copy link
Contributor

bnoordhuis commented Feb 1, 2014

The first commit adds a #[weak] attribute, the second commit makes src/libstd/rt/thread.rs link weakly against __pthread_get_minstack(). Avoids the dlopen/dlsym/dlclose cycle for new threads that was introduced in commit 431edac.

I imagine it's possible that you first need to build rustc using the first commit before you can build the second commit. I can split this into two pull requests if that is the case.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Feb 1, 2014

I believe this breaks a current type system assumption that all extern fns are non-null. @nikomatsakis what you say? @alexcrichton do we already have mechanisms for specifying linkage on symbols? Are we going to end up gradually expanding this to every other type of linkage?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 1, 2014

We do not currently have a way to specify the linkage of symbols, and I remember that we removed the ability to do this awhile back. This is an interesting twist though in that it's only allowed on extern symbols rather than on all rust symbols.

This seems reasonable to me and I'd be in favor of it (only on extern symbols), although I agree that going all the way and being able to specify any linkage may be the best way to go. Perhaps this should be behind a feature gate as well? I would slightly lean towards this not being behind a feature gate.

@pcwalton

This comment has been minimized.

Copy link
Contributor

pcwalton commented Feb 2, 2014

This would also potentially be a good solution for out-of-crate lang items as suggested in the libprim change.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 3, 2014

On Sat, Feb 01, 2014 at 12:05:24PM -0800, Brian Anderson wrote:

I believe this breaks a current type system assumption that all
extern fns are non-null. @nikomatsakis what you say?

We do presently assume that. Option<extern "C" fn(...)> is the
official type for a nullable fn pointer.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 4, 2014

@brson, @bnoordhuis, what would you think about a generic #[linkage(foo)] attribute behind a feature gate for now. I don't want to commit to this interface quite yet, but I feel like it's inevitable that we'll need this.

I think that the typesystem concern of expressing a "weak function" is very real, but we can work around it for now.

Support weak/common/private/etc. symbol linkage.
This commit adds a feature-gated #[linkage = "foo"] attribute that can
be applied to extern functions and instructs the linker on how to
resolve the symbol.

"extern_weak" linkage is the most immediately useful one because it
lets us link to library symbols that may or may not exist at run-time.

Note that non-default linkage types require toolchain and (sometimes)
dynamic linker support.  For example, "extern_weak" is accepted but
not actually supported by OS X's dyld.

I decided not to warn for known bad platform/linkage combinations
because clang and gcc don't do so either and because you don't rightly
know whether the user is actually using the problematic toolchain or
dynamic linker.  For all we know, they're implementing their own ELF
loader.
@bnoordhuis

This comment has been minimized.

Copy link
Contributor Author

bnoordhuis commented Feb 6, 2014

@alexcrichton Done as you suggested. PTAL.

Link weakly to __pthread_get_minstack().
Use the weak symbol support that was added in the previous commit to
link weakly against __pthread_get_minstack().  Significantly reduces
the thread creation overhead because Thread:👿:create() no longer
goes through a dlopen/dlsym/dlcose cycle for each new thread.
macro_rules,
managed_boxes,
simd,
thread_local)];

This comment has been minimized.

@alexcrichton

alexcrichton Feb 7, 2014

Member

I'm getting the feeling that libstd essentially has #[feature(*)]...

This comment has been minimized.

@huonw

huonw Feb 7, 2014

Member

It doesn't have quote or non_ascii_idents ;P

@alexcrichton

This comment has been minimized.

Copy link

alexcrichton commented on ffb7d35 Feb 7, 2014

r+, nice work, and thank you!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented on ffb7d35 Feb 7, 2014

saw approval from alexcrichton
at bnoordhuis@ffb7d35

This comment has been minimized.

Copy link
Contributor

bors replied Feb 7, 2014

merging bnoordhuis/rust/weak-symbol-support = ffb7d35 into auto

This comment has been minimized.

Copy link
Contributor

bors replied Feb 7, 2014

bnoordhuis/rust/weak-symbol-support = ffb7d35 merged ok, testing candidate = 0ca7002

This comment has been minimized.

Copy link
Contributor

bors replied Feb 7, 2014

bors added a commit that referenced this pull request Feb 7, 2014

auto merge of #11978 : bnoordhuis/rust/weak-symbol-support, r=alexcri…
…chton

The first commit adds a #[weak] attribute, the second commit makes src/libstd/rt/thread.rs link weakly against __pthread_get_minstack().  Avoids the dlopen/dlsym/dlclose cycle for new threads that was introduced in commit 431edac.

I imagine it's possible that you first need to build rustc using the first commit before you can build the second commit.  I can split this into two pull requests if that is the case.
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 7, 2014

I have canceled the build, @brson reminded me that we should resolve the typesystem issue before merging.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 25, 2014

We discussed this at the meeting today, and the conclusion we reached is that we should only allow weak variables of the form:

extern {
    #[weak] static FOO: *T;
    #[weak] static mut FOO_MUT: *T;
}

This way we can get weak variables without violating the typesystem. This will involve a little bit of extra codegen on LLVM's side I believe. I think we'll have to declare something of type T with the symbol name and weak linkage and then something of type *T which is the address of that.

@bnoordhuis, sorry this took awhile. If you've grown busy in the meantime, I don't mind taking this over.

@bnoordhuis

This comment has been minimized.

Copy link
Contributor Author

bnoordhuis commented Feb 25, 2014

@alexcrichton I won't have time to work on it this week. If you want to take over, that's fine.

@bharrisau

This comment has been minimized.

Copy link
Contributor

bharrisau commented Feb 25, 2014

Can one then have a static #no_mangle in another rust crate that will
generate a compatible strong symbol?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 25, 2014

@bnoordhuis no worries, and thanks for this initial work!

I'll post a PR when I get this implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.