core: add unstable no_fp_fmt_parse to disable float formatting code by nbdd0121 · Pull Request #86048 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation23 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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)].
r? @scottmcm
(rust-highfive has picked a reviewer for you, use r? to override)
This comment has been minimized.
This comment has been minimized.
They are used by integer formatting as well and is not exclusive to float.
I think this is bigger than my lib-impl hat's purview, so I've nominated it for libs team discussion.
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.
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?)
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-pointfeature rather than ano-floating-pointone).
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?
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….
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
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.
I don't understand why this is needed: if you don't use
f32/f64in 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.
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,f64andi128. However such style issues are better addressed using lints. Clippy already has a float_arithmetic lint which can be used to disallow the use off32andf64. - 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
corewhich are unlikely to ever be used in the kernel such as float parsing, manyDebugimpls, etc.
I'm a bit conflicted about this PR. Fundamentally, the
compiler-builtinsfunctions 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
corewhich are unlikely to ever be used in the kernel such as float parsing, manyDebugimpls, 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.
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+
📌 Commit 0e8c41dfd748517b55bfd6ad96d2d7e43b6352f2 has been approved by Amanieu
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.
Status: Awaiting decision from the relevant subteam (see the T- label).
labels
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.
As requested in #86048 (comment)
@bors r-
(If I'm misunderstanding the request, please ping me and I'll re-r.)
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
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)].
@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?
core: add unstable no_floating_point to disable float formatting code core: add unstable no_fp_fmt_parse to disable float formatting code
42 tasks
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).
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+
📌 Commit ec7292a has been approved by Amanieu
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
This was referenced
Oct 11, 2021
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request
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
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
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.
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request
core: add unstable no_fp_fmt_parse to disable float formatting code
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)].
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the library team, which will review and decide on the PR/issue.