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

Add cfg(no_128_bit) to core: removes u128/i128 formatting #103126

Closed
wants to merge 2 commits into from

Conversation

GKFX
Copy link
Contributor

@GKFX GKFX commented Oct 16, 2022

This is the u128/i128 equivalent of #86048, which removed floating point formatting from the core library on cfg(no_fp_fmt_parse).

The purpose of this option is to permit linking with a custom compiler-builtins which does not support 128-bit integers. (Or no compiler-builtins at all, if some other source of memcpy etc. is available, although this patch alone is not necessarily sufficient for that). This is a situation that the Linux kernel is in; see https://github.com/torvalds/linux/blob/2df76606db9de579bc96725981db4e8daa281993/rust/compiler_builtins.rs#L51-L63 which is a custom compiler-builtins which just panics on everything. (NB that I am not involved in the Linux kernel efforts myself.)

<i128 as Debug>::fmt is defined to output i128, rather than panic as #86048 does on floats. No other formatting is supported.

Test for this is by generating a static library linked to core but not to compiler-builtins and verifying that it does not have any dependencies on 128-bit compiler-rt functions with objdump. Using 128-bit division (etc.) is not forbidden by this patch so user code may still cause calls to these functions; it is up to users to decide what they want to do in this case, as Rust can't know exactly which operations on which architectures will require compiler-builtins. Users will see a linker error if they try operations which do, so I don't believe that to be a soundness issue.

Linked issues: Rust-for-Linux/linux#514.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Oct 16, 2022
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 16, 2022
@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 Oct 16, 2022
@GKFX GKFX changed the title Add cfg(no_128_bit) to core: removes most u128/i128 operations Add cfg(no_128_bit) to core: removes u128/i128 formatting Oct 16, 2022
// fail or panic, but for an application which has specifically
// recompiled core to avoid i128, the former seems unnecessary
// and the latter needlessly harmful.
f.write_str(stringify!($T))
Copy link
Member

Choose a reason for hiding this comment

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

Why is an i128/u128 impl necessary in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question; I haven’t tried removing it. I think it’s conventional to provide Debug for everything public in the standard library; not sure if that could be waived in this case?

@bjorn3
Copy link
Member

bjorn3 commented Oct 16, 2022

I believe u128 is also used in some of the Duration methods before casting to floats. Maybe these methods should be removed too?

@GKFX
Copy link
Contributor Author

GKFX commented Oct 16, 2022

I believe u128 is also used in some of the Duration methods before casting to floats. Maybe these methods should be removed too?

I considered trying to rip it out more thoroughly and produce a core library which cannot use these functions, but that would be quite hard to maintain and test, not least because the operations which compile to these functions are LLVM’s choice and platform-specific. E.g. there doesn’t appear to be one for addition. My understanding is that usage in an inline or generic method is OK because that doesn’t end up in the finished binaries unless used, so the formatting and parsing code (which isn’t inlined) is what matters.

@rust-log-analyzer

This comment has been minimized.

@scottmcm
Copy link
Member

This seems like a policy decision to me, so I'm going to flip it over to
r? @joshtriplett
to have a team discussion.

@est31
Copy link
Member

est31 commented Oct 17, 2022

I've tried measuring the impact of this PR onto the generated libdemo.a file in the added test. For that I've manually applied this PR to my local rustc checkout, and ran the command:

./x.py test --stage 1 src/test/run-make-fulldeps/core-no-128-bit

To get the state before the PR, I did:

git checkout HEAD~2 library

To turn on optimizations, I added -O to the makefile. This gives the following table (units in kb):

What Size Before Size After Savings
du 4316 4236 80
strip + du 3052 2996 56
-O + du 2124 2084 40
-O + strip + du 1696 1664 32

IIRC kernels usually get deployed without debuginfo, and are usually compiled with optimizations, so the last line is probably the most relevant. 32 kilobytes of savings correspond to about 1.9% relative to the 1696 kb.

I've also tried out the already existing floating point number cfg, this time with this patch applied, but not with the flag to turn off 128 bit numbers enabled.

What Size Before Size After Savings
du 4316 3352 964
strip + du 3052 2360 692
-O + du 2120 1708 412
-O + strip + du 1696 1356 340

So float parsing/formatting has about 340 kb of savings, or 20% of the 1696 binary.


It makes total sense that floats take up more space because their formatting code is way more complicated.

As the second difference between floats and 128 bit numbers, floats require the FPU to be around so are an absolute no-no in kernel space. For 128 bit numbers, they shouldn't use any special hardware so should in theory be able to run in the kernel... the reason to avoid them is just the impact on the binary size I think?

Not a kernel person, but it seems that the kernel already uses GCC's __uint128_t compiler extension. Its adoption has also been attributed with speedups. Although due to ABI issues (#54341), doing similar things as that commit in Rust might not be the best idea at the current point in time.

I think this PR makes sense, even if the impact is rather small compared to turning off float formatting, and 128 bit numbers are "just" integers, so don't require any special hardware outside of CPU, registers and memory.

@ojeda
Copy link
Contributor

ojeda commented Oct 18, 2022

floats require the FPU to be around so are an absolute no-no in kernel space

Nit: floats can be used where available, but it is still good to be able to remove them when not used (see below).

For 128 bit numbers, they shouldn't use any special hardware so should in theory be able to run in the kernel... the reason to avoid them is just the impact on the binary size I think?

In general, everything that can reasonably be removed should be so (conditionally depending on the kernel configuration, in many cases), but this does not mean the features are never used by any configuration.

In addition, even if the kernel does not use something in any config today, that itself may change over time, e.g. see The road to Zettalinux by willy.

When I made the original list long ago, I picked a few "big" things that I could think of, but really, the more that we can configure in/out, the better (just like the kernel itself).

I imagined other use cases (i.e. outside the Linux kernel) would also appreciate these options too. For instance, in embedded, constrained devices, every saved text page is welcome, and could open Rust for more use cases. Security-wise, it is also a good idea to avoid dead code. And perhaps in some safety-critical systems it may facilitate the process.

For some of these use cases it may be possible to strip things out after compilation, but probably not for all of them, at least as-is/easily (e.g. in the kernel we support external modules, so we export core, but we would need a way to mark which parts may be stripped or not, both from objects and rmetas). And even if it is possible, you lose second order benefits like compilation speed etc.

So all effort towards what I called "modularization of core" elsewhere would be welcome by several projects, I think.

@albertlarsan68
Copy link
Member

Tests have moved from src/test to tests, please rebase on top of new master, if this PR is going any further. Post @rustbot ready once the rebase is complete.
Thanks!
@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 18, 2023
@rust-log-analyzer

This comment has been minimized.

@GKFX
Copy link
Contributor Author

GKFX commented Jan 23, 2023

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 23, 2023
@GKFX GKFX marked this pull request as ready for review January 23, 2023 00:05
@rustbot
Copy link
Collaborator

rustbot commented Jan 23, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@bors
Copy link
Contributor

bors commented Feb 21, 2023

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

@ChrisDenton
Copy link
Member

This seems like a policy decision to me so I'm going to flip it over to... have a team discussion.

Ah, so this is waiting on a team decision? I'll add the label.

@ChrisDenton ChrisDenton added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Mar 26, 2023
@@ -62,6 +62,7 @@ macro_rules! int_impl {
#[doc = concat!("assert_eq!(", stringify!($SelfT), "::from_str_radix(\"A\", 16), Ok(10));")]
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
$( #[$intrinsics_cfg] )?
Copy link
Member

Choose a reason for hiding this comment

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

This is doing more than just controlling the removal of formatting: this function is about parsing. If not called, the function should not be included by the linker, different to what's happening to stuff called by the fmt machinery that is always included the moment anything that uses the fmt machinery is included.

@@ -166,13 +169,15 @@ nonzero_integers! {
#[stable(feature = "nonzero", since = "1.28.0")] #[rustc_const_stable(feature = "nonzero", since = "1.28.0")] NonZeroU16(u16);
#[stable(feature = "nonzero", since = "1.28.0")] #[rustc_const_stable(feature = "nonzero", since = "1.28.0")] NonZeroU32(u32);
#[stable(feature = "nonzero", since = "1.28.0")] #[rustc_const_stable(feature = "nonzero", since = "1.28.0")] NonZeroU64(u64);
#[stable(feature = "nonzero", since = "1.28.0")] #[rustc_const_stable(feature = "nonzero", since = "1.28.0")] NonZeroU128(u128);
#[stable(feature = "nonzero", since = "1.28.0")] #[rustc_const_stable(feature = "nonzero", since = "1.28.0")]
#[cfg(not(no_128_bit))] NonZeroU128(u128);
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I don't think it should be cfg'd out in this PR.

@Dylan-DPC Dylan-DPC removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 17, 2023
@Amanieu Amanieu removed the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label May 31, 2023
@Amanieu
Copy link
Member

Amanieu commented May 31, 2023

We discussed this in the libs meeting today and we are not inclined the add more opt-out configuration flags. The current set is already questionable and doesn't have a path to specialization.

While I can appreciate the concerns about binary size for the Linux kernel, this is fundamentally a problem caused by the way Linux does dynamic linking of modules, which prevents the linker from eliminating unused symbols. As such, this flag is not useful for the majority of embedded projects which use static linking.

The existing no_fp_fmt_parse flag also has no path to stabilization, but we're willing to keep it for now considering the large impact this has on Linux (~300KB of code). This no_128_bit only saves ~30KB, and if this is still significant for a particular kernel build then it might be worth considering disabling module support so that the linker can eliminate unused symbols.

@rfcbot fcp close

@rfcbot
Copy link

rfcbot commented May 31, 2023

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 31, 2023
@rfcbot
Copy link

rfcbot commented Jun 1, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jun 1, 2023
@m-ou-se m-ou-se removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jun 7, 2023
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 11, 2023
@rfcbot
Copy link

rfcbot commented Jun 11, 2023

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@Amanieu Amanieu closed this Jun 12, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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.