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

move to the WIP Rust port of compiler-rt #36992

Closed
wants to merge 4 commits into from

Conversation

japaric
Copy link
Member

@japaric japaric commented Oct 6, 2016

this replaces the in-tree compiler_builtins crate with the out-of-tree WIP Rust port of compiler-rt.

warning very lightly tested: make rustc-stage1 works with the Makefiles and with rustbuild but I haven't run make check yet.

I had to do some interesting changes in the upstream "rustc-builtins" crate but I'll open a PR in that repository to discuss those.

One thing to still needs to be done is updating bootstrap to handle recursive submodules. I got the initialization of recursive submodules working but I haven't done the part that handles dirty/outdated recursive submodules.

cc @alexcrichton can we throw this into the try buildbots?
cc @brson
cc @nagisa

@japaric
Copy link
Member Author

japaric commented Oct 6, 2016

r? @alexcrichton

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@bors
Copy link
Contributor

bors commented Oct 8, 2016

☔ The latest upstream changes (presumably #37039) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -17,3 +14,7 @@
[submodule "src/liblibc"]
path = src/liblibc
url = https://github.com/rust-lang/libc.git
[submodule "src/libcompiler_builtins"]
path = src/libcompiler_builtins
url = git://github.com/japaric/rustc-builtins.git
Copy link
Member

Choose a reason for hiding this comment

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

Before we land this I think we'll want to be sure to move this to rust-lang-nursery under the name "compiler-builtins"

@alexcrichton
Copy link
Member

Sorry for being slow to take a look, want to rebase this and update with some recent changes and I'll throw it at the try bots?

@alexcrichton
Copy link
Member

alexcrichton commented Oct 11, 2016

@japaric is this ready for the try bots? Sorry if I missed it!

Also one thought I just remembered I've been meaning to write down. We compile all C builtins with -fvisibility=hidden which I believe affects symbol visibility, especially when statically linking. I think, however, we're not doing the equivalent for the compiler-builtins crate written in Rust. This in turn might mean that the symbols are publicly exported from staticlibs, dylibs, etc, which ideally is something we'd prefer to avoid.

Could you take a look at this and ensure that we compile compiler-builtins Rust-defined symbols with -fvisibility=hidden?

@brson
Copy link
Contributor

brson commented Oct 11, 2016

+1

@nagisa
Copy link
Member

nagisa commented Oct 11, 2016

NB: I’ll likely have to move this in-tree in order to get i128 intrinsics implemented, as we haven’t a way to stage out-of-tree dependencies AFAICT.

@japaric
Copy link
Member Author

japaric commented Oct 11, 2016

Could you take a look at this and ensure that we compile compiler-builtins Rust-defined symbols with -fvisibility=hidden?

So, I checked and that's not the case. The Rust intrinsics appear as global/external symbols in dylibs (.so) whereas the C intrinsics don't. Seems like we'll have to do some compiler changes to change that behavior.

@bors
Copy link
Contributor

bors commented Oct 28, 2016

☔ The latest upstream changes (presumably #37321) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

Ok, closing due to inactivity for now. I've added an entry to rust-lang/compiler-builtins#66 to track compiling the Rust versions with hidden visibility

@alexcrichton
Copy link
Member

@japaric would you be interested in implementing such a patch to the compiler? If not I can take a look in the next few days

@japaric
Copy link
Member Author

japaric commented Nov 11, 2016

@alexcrichton All yours! (I've been meaning to look into this compiler change and how it relates to #37530 but I keep getting distracted with something else.)

@alexcrichton
Copy link
Member

Ok, PR opened over at #37714

@japaric
Copy link
Member Author

japaric commented Nov 16, 2016

Reopening now that #37714 landed. Rebased as well. Still testing locally but rustbuild is already past rustc-stage1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants