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

Merge compiler-builtins into core #49380

Closed
japaric opened this Issue Mar 26, 2018 · 8 comments

Comments

Projects
None yet
6 participants
@japaric
Copy link
Member

japaric commented Mar 26, 2018

This is a P-high embedded-WG issue that needs to be fixed to make embedded Rust work on stable.

Background:

The compiler-builtins crate contains compiler intrinsics that LLVM may call when lowering operations like i64 multiplication to machine code. This crate currently is its own crate due to these requirements: its object file needs to appear at the end of the linker argument list and it needs be marked with the #![compiler_builtins] attribute (symbol visibility, etc.).

On #![no_std] compiler-builtins needs to appear in the dependency graph to avoid linker errors but that requires the #[feature(compiler_builtins_lib)] feature gate because the crate is unstable -- it's a compiler implementation detail.

The fix is to put the compiler intrinsics into core and eliminate the compiler-builtins crate from the std facade. This has become possible thanks to recent progress in multiple codegen units.

What roughly needs to be done

  • Include compiler-builtins as source code into core using something like #[path = "../compiler-builtins/src/lib.rs"] mod intrinsics.

  • Create some attribute to mark the whole intrinsics module as #![compiler_builtins] and to force the whole module to be into its own codegen unit so it gets its own object file.

cc @michaelwoerister @eddyb @alexcrichton

@jamesmunns

This comment has been minimized.

Copy link
Member

jamesmunns commented Mar 27, 2018

Alternative options discussed during meeting 1:

  • Built in, and automatically include extern crate compiler_builtins; in the #[no_std] prelude, similar to how extern crate core; is inserted when you use #[no_std] (suggested by @oli-obk)
  • We don't have to cover every use case in the stable situation, just "sane defaults", we may want to make sure that the symbols are defined as weak so that they can be overrided by users on nightly.
@whitequark

This comment has been minimized.

Copy link
Member

whitequark commented Mar 29, 2018

@japaric What about the mem feature of compiler-builtins? Do we go back to using rlibc?

@japaric

This comment has been minimized.

Copy link
Member Author

japaric commented Mar 29, 2018

@whitequark we are going to go ahead with the first approach in @jamesmunns' comment. (a) There's not going to be a compiler-builtins / core merge, (b) the expansion of #![no_std] will include extern crate compiler_builtins and won't require a feature gate and (c) we'll build and ship a rust-std component for the thumb* and msp430 targets; that component will include a pre-compiled core, compiler-builtins (+mem), alloc, etc.

@japaric

This comment has been minimized.

Copy link
Member Author

japaric commented Mar 29, 2018

With this approach we can still do the merge sometime in the future.

@whitequark

This comment has been minimized.

Copy link
Member

whitequark commented Mar 29, 2018

Sounds great!

@ppannuto ppannuto referenced this issue Apr 5, 2018

Closed

Tracking: Minimize dependencies to build kernel #866

4 of 5 tasks complete
@japaric

This comment has been minimized.

Copy link
Member Author

japaric commented Apr 7, 2018

#49503 has landed so I'm going to remove the WG-embedded request label.

@alexcrichton you expressed interest in seeing the compiler-builtins / core merge implemented. Do you want to keep this ticket open to track that?

@japaric japaric removed the WG-embedded label Apr 7, 2018

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Apr 7, 2018

We'll still need this to avoid mentioning compiler-builtins on the Cargo file, so yeah I hope work continues.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 7, 2018

@japaric indeed! I don't think the need is pressing though so I'm fine closing this and making a new issue if it ever becomes pressing

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.