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

RFC: Float-free libcore #1596

Closed
wants to merge 5 commits into from
Closed

Conversation

strake
Copy link
Contributor

@strake strake commented Apr 28, 2016

phil-opp made the PR on rust-lang/rust repo, but this is blocking me so i wrote the RFC. phil-opp is welcome to claim this from me if they wish.

# Detailed design
[design]: #detailed-design

Add an optional `has_floating_point` property, default true, gated as `cfg_target_has_floating_point`. Disable all floating-point use in libcore if this flag is false.

Choose a reason for hiding this comment

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

As noted in the motivation section, this flag needs to affect code generation, not only Rust code. Throwing cfg(target_has_floating_point) is the easy part, what's the detailed design for the code generation changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The notion is, this flag merely disables all floating-point code in libcore, and the "features" flag affects code generation. So one would define a target with has_floating_point: false and features: -mmx, -sse, ...

@oli-obk
Copy link
Contributor

oli-obk commented Apr 28, 2016

cc @phil-opp

@strake
Copy link
Contributor Author

strake commented Apr 29, 2016

cc @emk who was also interested in this

@aturon aturon added T-libs-api Relevant to the library API team, which will review and decide on the RFC. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Apr 29, 2016
@aturon aturon self-assigned this Apr 29, 2016
@ketsuban
Copy link
Contributor

If has_floating_point is false, is it legal to use f32 and f64?

My gut instinct is to say no, with the provision that a future RFC can specify soft-float lang items which if provided would unlock these types in “float-free” code.

@Amanieu
Copy link
Member

Amanieu commented May 2, 2016

There was an interesting idea proposed in the internals thread:

  • Split the float-related parts of libcore into libcorefloat.
  • Make #![no_std] import both libcore and libcorefloat.
  • Add a #![no_float] attribute which works like #![no_std] but only imports libcore.

Like no_std, this will allow the creation of a crate ecosystem that are usable in float-free environments. It feels like a "cleaner" solution, however it will require crates to be marked as no_float to be usable in such projects, which will require some coordination. This is similar to the situation when no_std was introduced and many crates had to be modified to support it.

@eternaleye
Copy link

eternaleye commented May 2, 2016

@Amanieu, I disagree it's a "cleaner" solution:

  • The seeming difficulty in building up a float-free rate ecosystem boils down to that Cargo is not aware of stdlib dependencies, which causes other pain points as well (and if resolved, would address such issues generically, rather than requiring a new ad-hoc attribute each time)
  • It puts something metadata-relevant (supporting float-free core) into source code, requiring more effort to assess - for example, a Cargo subcommand along the lines of cargo-lichking that assesses float-freeness would be trivial with features (and proper dependency handling), whereas an attribute would require such a subcommand to parse Rust source.
  • Adding #![no_float], unlike #![no_std], bloats the compiler with things it shouldn't care about - the latter suppresses an existing compiler behavior (prelude injection and default linking), while the former adds a constraint on how libcore was built (not just its existence). That's a "has a feature" question at its root.
  • Actually using crates in a float-free environment (such as a kernel) requires crates be no-float anyways. Whether that's opting out of libcore's default features (needed for back-compat) or specifying #![no_float], it's unavoidable.

Splitting libcorefloat out into a separate lib is (IMO) an orthogonal question - one again likely better handled by features, as conditional dependencies are a thing.

@aturon aturon removed the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label May 2, 2016
@strake
Copy link
Contributor Author

strake commented May 3, 2016

Responding to @brson's comment in internals thread: I tried to clarify and noted the points you made.

About this in particular:

It should address how this will affect the ecosystem. Does every crate that wants to work in a float-free environment need to take special precautions? Every no_std-capable crate would potentially and unexpectedly fail to compile in a not(has_floating_point) environment. That's a big impact. Why is it ok and how to we mitigate it?

I had an idea, noted briefly as alternative in RFC:
We could add a needs_floating_point attribute which enables floating-point instruction emission for an item, and add it to all items of libcore which need it. So rather than a has_floating_point target property, we could have a property called minimal_floating_point or some such, which disables unneccessary floating-point instruction emission (e.g. for memcpy), and would allow using libcore where floating-point is forbidden, but not make building crates fail.

@Ericson2314
Copy link
Contributor

Ericson2314 commented May 4, 2016

I definitely prefer the feature route, as eloquently articulated in @eternaleye's post. Additionally, with the current route, code can be "silently" non-portable which is never a good thing.

It may still be good to add the field to the target spec insofar as that allows one to indicate the feature is unavailable on platforms without it.

@DemiMarie
Copy link

I think there should also be an unsafe way that a library that does use floating point can be explicitly linked into an otherwise float-free kernel codebase. Specifically, Linux allows for the use of floating point on some architectures by explicitly saving and restoring the FPU/SIMD/etc state, and Rust should allow this.

@Ericson2314
Copy link
Contributor

@drbo Cargo assumes its always safe to build deps with more features than required, and since the the float feature would simply add more definitions, this is actually the case. The whole build can be performed with the float-supporting target configuration and things should just work.

@strake
Copy link
Contributor Author

strake commented May 13, 2016

I'm thinking we ought await the judgement of PR 1133, which, if accepted, might mitigate some of the worries raised here.

@phil-opp
Copy link

I think the fundamental issue is: Rust assumes that all targets support f32 and f64.

Almost all targets have floating point support (either in hardware or in software), but there are a few exceptions (e.g. x86_64 with SSE disabled). On these targets, (almost) every use of f32 and f64 is illegal and leads to a compile time error (e.g. LLVM ERROR: SSE register return with SSE disabled on x86_64 with SSE disabled).

We have the following options:

  1. Implement some kind of software float for these targets. Cleanest solution, but requires a lot of work.
  2. Add a #[no_float]-like attribute for crates without floating point. We would still see a compile time error when (transitively) using f32 and f64, but we could emit nicer error messages.
  3. Leave everything as it is and let the programmer of the niche target figure it out. No additonal work or mental burden for crate authors, but the programmer might see ugly LLVM errors for these targets.

Personally, I'm happy with option 3, because only few special-purpose targets have neither hard- nor soft-float. However, I think there should be some way to build libcore for these targets without manually patching it.

This could be either a cargo feature or an option in the target file. Personally, I prefer the target file option because… well, it's a property of the target. It allows other library authors to conditionally exclude floats if they want to. It also makes it easy to build a cross sysroot using xargo.

This RFC does not make anything worse. It just adds a new target property to describe that a target supports neither hard- nor soft-floats. Then it uses this property to allow compiling libcore for these targets. Like before, “every no_std-capable crate would potentially and unexpectedly fail to compile in a not(has_floating_point) environment”, but now we can at least compile libcore and add special cases for other crates (if we want to).

@Ericson2314
Copy link
Contributor

Ericson2314 commented May 13, 2016

I should also add, this is one of the downsides of having our primitive types always in scope, rather than just exposed as lang items. Had we taken the latter route, we could just cfg them too out in libcore, and not only would the operations on those types be gone, but the type itself would be too.

@phil-opp The slight area where this makes things worse is downstream crates don't express whether they need float support or not from core. That is what I meant by "'silently' unportable" in #1596 (comment) in the Cargo file or even source---of the downstream crate opts-in to non-portability. But if core had a float feature, enabled by default, downstream crates can signify they are float-free, by doing something like:

[dependencies.core]
default-features = false

Given backwards compatibility, I think that opt-out is the best we can do.

@emk
Copy link

emk commented May 15, 2016

Almost all targets have floating point support (either in hardware or in software), but there are a few exceptions (e.g. x86_64 with SSE disabled).

Yeah, essentially you can divide targets into two types:

  1. Normal targets in user space. Virtually all of these support floating point, either in hardware or in software.
  2. Bare metal targets. These include x86 kernel space (assuming you don't want dump and restore floating point state every time you enter the kernel, which is really expensive), and various embedded targets.

This issue comes up because Rust is just an absolutely terrific language for (2), and people want to use it there. (And it's only going to become more tempting with recent improvements to cross-compilation.) But in this case, they're probably also using core instead of std, and so on. So it's certainly tempting to say, "This is a property of certain specialized targets, typically the same ones which don't try to support things like std." I'm definitely OK with the idea that std always requires floating point support.

Given backwards compatibility, I think that opt-out is the best we can do.

Opt-out is appropriate here, I think. Rust and Cargo should assume that f32 and f64 are available in all normal cases, and treat kernel-space libraries as the special cases.

@aturon
Copy link
Member

aturon commented May 22, 2016

Heads-up: I've opened a discuss thread for talking about how we want to handle some of these API questions in general. Please join in!

@jdub
Copy link

jdub commented Jul 31, 2016

Worth checking recent activity in #1364 re: soft-float.

@aturon
Copy link
Member

aturon commented Jan 31, 2017

Note: there's now an RFC for a portability lint, which should pave the way for handling this case.

@aturon
Copy link
Member

aturon commented Jan 31, 2017

@rust-lang/libs: at what point are we comfortable moving forward with RFCs like this, given the state of play with the portability lint?

@Amanieu
Copy link
Member

Amanieu commented Jan 31, 2017

@aturon I think the consensus from #1364 is that no changes to libcore are needed for float-free code. All you need is to enable the "soft-float" LLVM feature so that all floating-point instructions are replaced with library calls. This solves the same problems that this RFC was aiming to solve.

@aturon
Copy link
Member

aturon commented Feb 1, 2017

@Amanieu Ah, interesting, thank you!

Given that there's a pretty nice way to handle this today, I'm going to move to close this RFC for the time being.

@rfcbot fcp close

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 1, 2017

Team member @aturon has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@steveklabnik
Copy link
Member

@Amanieu while that is true generally, someone has been having a lot of trouble with it lately: https://users.rust-lang.org/t/kernel-modules-made-from-rust/9191/11

@emk
Copy link

emk commented Feb 7, 2017

I would be reluctant to close this issue unless @steveklabnik and other people doing embedded work can actually confirm that @Amanieu's proposal works in practice. Does anybody have a good test case with xargo? (My toy kernel won't compile against the lastest nightlies without more tweaking, or I'd use that to investigate.)

@phil-opp
Copy link

phil-opp commented Feb 7, 2017

@emk I use xargo and +soft-float for os.phil-opp.com and it works fine. IntermezzOS and the Redox kernel seem to use +soft-float too.

@emk
Copy link

emk commented Feb 7, 2017

@phil-opp Thank you, that sounds great! As one of the original interested parties, I'm personally content with a fix using xargo and soft-float.

This probably means that if somebody needs to port Rust an embedded platform, they might also need to fix LLVM soft-float support if it isn't already available. But if that works for the more common platforms, that's probably good enough for now.

@steveklabnik
Copy link
Member

I second @phil-opp here; we have no issues with using libcore today.

@aturon
Copy link
Member

aturon commented Feb 28, 2017

OK, we discussed this in libs triage, and it appears that for the time being everyone is happy with soft floats as a solution.

We're happy to revisit this later if it turns out there's a need for it. I'm closing in the meantime.

@aturon aturon closed this Feb 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.