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

core: add unstable no_fp_fmt_parse to disable float formatting code #86048

Merged
merged 2 commits into from
Jul 4, 2021

Conversation

nbdd0121
Copy link
Contributor

@nbdd0121 nbdd0121 commented Jun 6, 2021

In some projects (e.g. kernel), floating point is forbidden. They can disable
hardware floating point support and use +soft-float to avoid fp instructions
from being generated, but as libcore contains the formatting code for f32
and f64, some fp intrinsics are depended. One could define stubs for these
intrinsics that just panic 1, but it means that if any formatting functions
are accidentally used, mistake can only be caught during the runtime rather
than during compile-time or link-time, and they consume a lot of space without
LTO.

This patch provides an unstable cfg no_fp_fmt_parse to disable these.
A panicking stub is still provided for the Debug implementation (unfortunately)
because there are some SIMD types that use #[derive(Debug)].

@rust-highfive
Copy link
Collaborator

r? @scottmcm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 6, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

They are used by integer formatting as well and is not exclusive to float.
@scottmcm scottmcm added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 6, 2021
@scottmcm
Copy link
Member

scottmcm commented Jun 6, 2021

I think this is bigger than my lib-impl hat's purview, so I've nominated it for libs team discussion.

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Jun 6, 2021

I am also preparing a patch that adds a no_i128 cfg to remove some i128 parsing code. Maybe the discussion can include that as well?

The goal is not to remove the type f32/f64/i128/u128 completely; we just want to build a libcore without depending on any intrinsics that needs to be implemented as function calls.

@nagisa
Copy link
Member

nagisa commented Jun 6, 2021

Consider using Cargo's feature infrastructure for the implementation of this. Furthermore, features should ideally be additive (i.e. in this case you'd add an enabled-by-default floating-point feature rather than a no-floating-point one).

(as another nit: since these don't remove the types, but just the formatting code, perhaps this should be reflected in the name?)

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Jun 7, 2021

Consider using Cargo's feature infrastructure for the implementation of this. Furthermore, features should ideally be additive (i.e. in this case you'd add an enabled-by-default floating-point feature rather than a no-floating-point one).

I followed a previous PR that adds a cfg to remove OOM handling code, see #84266 (comment). It seems that libs team is not comfortable with enable-by-default features.

(as another nit: since these don't remove the types, but just the formatting code, perhaps this should be reflected in the name?)

Well, this removes both parsing and formatting code. What would be a better name, no_floating_point_parse_and_fmt? Or maybe into two flags?

@nagisa
Copy link
Member

nagisa commented Jun 7, 2021

Two cfgs do definitely seem to make naming these easier: --cfg no_fp_fmt and --cfg no_fp_parse with whatever in place of fp: flt, float, floating_point….

@m-ou-se m-ou-se added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 23, 2021
@Amanieu
Copy link
Member

Amanieu commented Jun 23, 2021

I don't understand why this is needed: if you don't use f32/f64 in your code then the linker will see that the float formatting code is unreachable and will not link it in.

@Amanieu Amanieu assigned Amanieu and unassigned scottmcm Jun 23, 2021
@nbdd0121
Copy link
Contributor Author

I don't understand why this is needed: if you don't use f32/f64 in your code then the linker will see that the float formatting code is unreachable and will not link it in.

For context this cfg is wanted by Rust-for-Linux project. A few reasons that we need to remove them instead of relying on LTO:

  • Linux does not yet have a good LTO support
  • Linux supports out-of-tree modules, so in general even if a symbol is unused it might still be depended on and cannot be removed.

@Amanieu
Copy link
Member

Amanieu commented Jun 30, 2021

LTO is not needed for this: rustc performs the equivalent of -ffunction-sections which allows the linker to eliminate unused functions. However you make a good point about loadable modules: since a module could use floating-point numbers in theory, the linker has to keep the float formatting code just in case.

The reason excluding the formatting code works at all in your case is because most of core consists of inline functions which are only instantiated when needed. One major exception is the formatting code which is out-of-line and pre-compiled in libcore.rlib.

I'm a bit conflicted about this PR. Fundamentally, the compiler-builtins functions are required for code generated by LLVM and need to be made available. There are technical reasons for disallowing floating-point types in a kernel, but these are mainly due to issues with the use of FP registers which don't apply to code compiled with +soft-float.

This leaves two arguments for removing float formatting support:

  • Code style. The kernel code style strongly discourages the use of f32, f64 and i128. However such style issues are better addressed using lints. Clippy already has a float_arithmetic lint which can be used to disallow the use of f32 and f64.
  • Binary size. Since floating-point types are not used in the kernel, the float formatting logic is just dead code which is never going to be called by the kernel, but must be included by the linker just in case it is called by a kernel module. However this PR doesn't fully address this aspect either: there are many parts of core which are unlikely to ever be used in the kernel such as float parsing, many Debug impls, etc.

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Jul 1, 2021

I'm a bit conflicted about this PR. Fundamentally, the compiler-builtins functions are required for code generated by LLVM and need to be made available. There are technical reasons for disallowing floating-point types in a kernel, but these are mainly due to issues with the use of FP registers which don't apply to code compiled with +soft-float.

Well, given that we disallow usage of f32, f64 entirely and discourage i128 in kernel, it does not make much sense for us to add compiler-builtins as a dependency. The kernel C code can be compiled with clang but it never requires compiler-rt.

  • Binary size. Since floating-point types are not used in the kernel, the float formatting logic is just dead code which is never going to be called by the kernel, but must be included by the linker just in case it is called by a kernel module. However this PR doesn't fully address this aspect either: there are many parts of core which are unlikely to ever be used in the kernel such as float parsing, many Debug impls, etc.

This PR also removes float parsing. Debug impls are less an issue because 1) sometimes it does end up being used 2) most Debug impls are generics, and the ones don't usually share a lot of code with Display and 3) it does not depend on compiler-builtin.

@Amanieu
Copy link
Member

Amanieu commented Jul 2, 2021

Hmm I think we can provisionally accept this but with the reservation that we may remove this cfg in the future and replace it with some other mechanism for disabling float support.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 2, 2021

📌 Commit 0e8c41dfd748517b55bfd6ad96d2d7e43b6352f2 has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 2, 2021
@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Jul 2, 2021

I still had to rename the feature gate, please r- for now

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Jul 2, 2021

An option to completely disable floating point would actually be even better for us. However that change seems very big and not trivial to make, so in the short term we want at least to remove the parsing/formatting.

@scottmcm
Copy link
Member

scottmcm commented Jul 2, 2021

As requested in #86048 (comment)
@bors r-

(If I'm misunderstanding the request, please ping me and I'll re-r.)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 2, 2021
In some projects (e.g. kernel), floating point is forbidden. They can disable
hardware floating point support and use `+soft-float` to avoid fp instructions
from being generated, but as libcore contains the formatting code for `f32`
and `f64`, some fp intrinsics are depended. One could define stubs for these
intrinsics that just panic [1], but it means that if any formatting functions
are accidentally used, mistake can only be caught during the runtime rather
than during compile-time or link-time, and they consume a lot of space without
LTO.

This patch provides an unstable cfg `no_fp_fmt_parse` to disable these.
A panicking stub is still provided for the `Debug` implementation (unfortunately)
because there are some SIMD types that use `#[derive(Debug)]`.

[1]: https://lkml.org/lkml/2021/4/14/1028
@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Jul 2, 2021

@nagisa it seems that parsing and formatting code are quite intertwined and it's not very easy to split the two. Currently I rename the flag to no_fp_fmt_parse. What's your opinion?

@nbdd0121 nbdd0121 changed the title core: add unstable no_floating_point to disable float formatting code core: add unstable no_fp_fmt_parse to disable float formatting code Jul 2, 2021
@ojeda
Copy link
Contributor

ojeda commented Jul 4, 2021

Hmm I think we can provisionally accept this but with the reservation that we may remove this cfg in the future and replace it with some other mechanism for disabling float support.

That is fine, thank you. We do not mind having to change the mechanism later on if needed (at least until things mature quite a bit more on our side).

@Amanieu
Copy link
Member

Amanieu commented Jul 4, 2021

Something that might be worth exploring is having each kernel module bundle a separate copy of core to allow the linker to eliminate unused symbols. The vast majority of the code in core consists of inline functions so this should end up as a net size reduction.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 4, 2021

📌 Commit ec7292a has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 4, 2021
@bors
Copy link
Contributor

bors commented Jul 4, 2021

⌛ Testing commit ec7292a with merge 9044245...

@bors
Copy link
Contributor

bors commented Jul 4, 2021

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 9044245 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 4, 2021
@bors bors merged commit 9044245 into rust-lang:master Jul 4, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 4, 2021
@nbdd0121 nbdd0121 deleted the no_floating_point branch September 4, 2021 02:55
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 21, 2022
`alloc`: add unstable cfg features `no_rc` and `no_sync`

In Rust for Linux we are using these to make `alloc` a bit more modular.

See rust-lang#86048 and rust-lang#84266 for similar requests.

Of course, the particular names are not important.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 21, 2022
`alloc`: add unstable cfg features `no_rc` and `no_sync`

In Rust for Linux we are using these to make `alloc` a bit more modular.

See rust-lang#86048 and rust-lang#84266 for similar requests.

Of course, the particular names are not important.
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Feb 10, 2023
`alloc`: add unstable cfg features `no_rc` and `no_sync`

In Rust for Linux we are using these to make `alloc` a bit more modular.

See rust-lang/rust#86048 and rust-lang/rust#84266 for similar requests.

Of course, the particular names are not important.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants