Uh oh!
There was an error while loading. Please reload this page.
Avoid panic_bounds_check in fmt::write. - #78122
Conversation
Writing any fmt::Arguments would trigger the inclusion of usize formatting and padding code in the resulting binary, because indexing used in fmt::write would generate code using panic_bounds_check, which prints the index and length. These bounds checks are not necessary, as fmt::Arguments never contains any out-of-bounds indexes. This change replaces them with unsafe get_unchecked, to reduce the amount of generated code, which is especially important for embedded targets.
rust-highfive
commented
Oct 19, 2020
r? @dtolnay (rust_highfive has picked a reviewer for you, use r? to override) |
Related optimization: rust/library/core/src/panicking.rs Lines 44 to 50 in a85e949 That optimzation would not be very effective if even writing the most trivial Not sure if this can (or should) be tested for. Checking if some code results in a binary smaller than some threshold would not be a good test. Coming up with a somewhat complete list of symbols that 'should not be there' is also not easy. |
m-ou-se
commented
Oct 19, 2020
@rustbot modify labels: +A-fmt +I-heavy +libs-impl |
rustbot
commented
Oct 19, 2020
Error: Label libs-impl can only be set by Rust team members Please let |
m-ou-se
commented
Oct 19, 2020
@rustbot modify labels: +A-fmt +I-heavy +T-libs |
Mark-Simulacrum
commented
Oct 19, 2020
I think it would be good to see if we can add a simple codegen test to assert that there's no panic/bounds check here, since it seems easy for it to sneak back in. I would also like debug asserts here - the compiler should be correct, but I'm not sure we're testing that anywhere else (other than kinda via these bounds checks and such). |
m-ou-se
commented
Oct 19, 2020
@Mark-Simulacrum sounds good. Will add that when I have time. @rustbot modify labels: +S-waiting-on-author -S-waiting-on-review |
m-ou-se
commented
Oct 20, 2020
Added the debug_asserts and a test. The test is a @rustbot modify labels: -S-waiting-on-author +S-waiting-on-review |
dtolnay
commented
Oct 20, 2020
src/test/run-make/fmt-write-bloat/main doesn't need to be checked in, right? |
Mark-Simulacrum
left a comment
There was a problem hiding this comment.
This seems good to me -- r=me with the binary file removed.
Uh oh!
There was an error while loading. Please reload this page.
1158ec5 to
356d5b5Comparem-ou-se
commented
Oct 20, 2020
Oh oops. Accidentally committed some things I shouldn't have. Thanks. Updated. |
@bors r=Mark-Simulacrum I confirmed that the new test fails without the changes in the PR. |
bors
commented
Oct 20, 2020
📌 Commit 356d5b5 has been approved by |
m-ou-se
commented
Nov 28, 2020
Looks like there's a lot more runners on which debug assertions are disabled than I originally thought. (i686-gnu, x86_64-gnu-llvm-8, x86_64-gnu-distcheck, i686-gnu-nopt, ..) I've changed the test to only check for the
I only found a I saw one other test that greps through |
Mark-Simulacrum
commented
Nov 28, 2020
This looks good to me. Could you squash the test adding commits? r=me with that done. (FWIW I'm happy on smaller PRs to just always squash commits, and I think generally our policy of "add new commits" does more harm than good on average). |
5df04f3 to
85b217dComparem-ou-se
commented
Nov 28, 2020
For most PRs I personally find it hard to review them with force-pushes between the reviews, which is why I usually use new commits after a review. Squashed the test commits into one. @bors r=Mark-Simulacrum |
bors
commented
Nov 28, 2020
📌 Commit 85b217d1d66d2cb61cf4b6d9cf5c0fc7a28c7831 has been approved by |
bors
commented
Nov 29, 2020
⌛ Testing commit 85b217d1d66d2cb61cf4b6d9cf5c0fc7a28c7831 with merge 614ccbb44cca6b1bee828395ef4cebe629b27c92... |
bors
commented
Nov 29, 2020
💔 Test failed - checks-actions |
It checks that fmt::write by itself doesn't pull in any panicking or or display code.
85b217d to
1da5780Comparem-ou-se
commented
Nov 29, 2020
This failed on Windows because the no_std test (using |
Mark-Simulacrum
commented
Nov 29, 2020
@bors r+ |
bors
commented
Nov 29, 2020
📌 Commit 1da5780 has been approved by |
| } | ||
| fn run(fmt: &mut Formatter<'_>, arg: &rt::v1::Argument, args: &[ArgumentV1<'_>]) -> Result { | ||
| unsafe fn run(fmt: &mut Formatter<'_>, arg: &rt::v1::Argument, args: &[ArgumentV1<'_>]) -> Result { |
There was a problem hiding this comment.
Nit: I'd add a // SAFETY: arg and args must come from the same Arguments comment to the newly unsafe functions as well to indicate the necessary preconditions when calling them.
bors
commented
Nov 29, 2020
bors
commented
Nov 30, 2020
☀️ Test successful - checks-actions |
This test suffers from multiple issues that make it very, very difficult to fix, and even if fixed, it would still be too fragile. For some background context, this test tries to check that the optimization introduced in [PR-78122] is not regressed. The optimization is for eliding `usize` formatting machinery and padding code from the final binary. Previously, writing any `fmt::Arguments` would cause the `usize` formatting and padding machinery to be included in the final binary since indexing used in `fmt::write` generates code using `panic_bounds_check` (that prints the index and length). Those bounds check are never hit, since `fmt::Arguments` never contain any out-of-bounds indicies. The `Makefile` version of `fmt-write-bloat` was ported to the present `rmake.rs` test infra in [PR-128147]. However, this PR just tries to maintain the original test logic. The original test, it turns out, already have multiple limitations: - It only runs on non-Windows, since the `no_std` test of the original version tries to link against a `libc`. [PR-128807] worked around this by using a substitute name. We re-enabled this test in [PR-142841], but it turns out the assertions are too weak, it will even vacuously pass for no symbols at all. - In [PR-143669], we tried to make this test more robust by comparing the set of expected versus unexpected panic-related symbols, subject to if std was built with debug assertions. However, in working on [PR-143669], we've come to realize that this test is fundamentally very fragile: - The set of panic symbols depend on whether the standard library was built with or without debug assertions. - Different platforms often have different sets of panic machinery modules, functions and paths, and thus different sets of panic symbols. For instance, x86_64 msvc and i686 msvc have different panic codepaths. - This comes back to the way the test is trying to gauge the absence of panic symbols -- it tries to look for symbol substring matches for "known" panic symbols. This is fundamentally fragile, because the test is trying to peek into the symbols of the resultant binary post-linking, based on fuzzy matches (the symbols are mangled as well). Based on this assessment, we determined that we should remove this test. This is not intended to exclude the possibility of reintroducing a more robust version of this test. For instance, we could consider some kind of more controllable post-link "end product" integration codegen test suite. [PR-78122]: rust-lang#78122 [PR-128147]: rust-lang#128147 [PR-128807]: rust-lang#128807 [PR-142841]: rust-lang#142841 [PR-143669]: rust-lang#143669
…=ChrisDenton Remove `tests/run-make/fmt-write-bloat/` This test suffers from multiple issues that make it very, very difficult to fix, and even if fixed, it would still be too fragile. So this PR removes `tests/run-make/fmt-write-bloat/`. This PR supersedes rust-lang#143669. r? `@ChrisDenton` (as you reviewed rust-lang#143669 and have context) ### Background context For some background context, this test tries to check that the optimization introduced in [PR-78122] is not regressed. The optimization is for eliding `usize` formatting machinery and padding code from the final binary. Previously, writing any `fmt::Arguments` would cause the `usize` formatting and padding machinery to be included in the final binary since indexing used in `fmt::write` generates code using `panic_bounds_check` (that prints the index and length). Those bounds check are never hit, since `fmt::Arguments` never contain any out-of-bounds indicies. The `Makefile` version of `fmt-write-bloat` was ported to the present `rmake.rs` test infra in [PR-128147]. However, that PR just tries to maintain the original test logic. ### Limitations and problems The original test, it turns out, already have multiple limitations: - It only runs on non-Windows, since the `no_std` test of the original version tries to link against a `libc`. [PR-128807] worked around this by using a substitute name. We re-enabled this test in [PR-142841], but it turns out the assertions are too weak, it will even vacuously pass for no symbols at all. - In [PR-143669], we tried to make this test more robust by comparing the set of expected versus unexpected panic-related symbols, subject to if std was built with debug assertions. However, in working on [PR-143669], we've come to realize that this test is fundamentally very fragile: - The set of panic symbols depend on whether the standard library was built with or without debug assertions. - Different platforms often have different sets of panic machinery modules, functions and paths, and thus different sets of panic symbols. For instance, x86_64 msvc and i686 msvc have different panic code paths. - This comes back to the way the test is trying to gauge the absence of panic symbols -- it tries to look for symbol substring matches for "known" panic symbols. This is fundamentally fragile, because the test is trying to peek into the symbols of the resultant binary post-linking, based on fuzzy matches (the symbols are mangled as well). Based on this assessment, we determined that we should remove this test. This is not intended to exclude the possibility of reintroducing a more robust version of this test. For instance, we could consider some kind of more controllable post-link "end product" integration codegen test suite. [PR-78122]: rust-lang#78122 [PR-128147]: rust-lang#128147 [PR-128807]: rust-lang#128807 [PR-142841]: rust-lang#142841 [PR-143669]: rust-lang#143669
Rollup merge of #148393 - jieyouxu:remove-fmt-write-bloat, r=ChrisDenton Remove `tests/run-make/fmt-write-bloat/` This test suffers from multiple issues that make it very, very difficult to fix, and even if fixed, it would still be too fragile. So this PR removes `tests/run-make/fmt-write-bloat/`. This PR supersedes #143669. r? `@ChrisDenton` (as you reviewed #143669 and have context) ### Background context For some background context, this test tries to check that the optimization introduced in [PR-78122] is not regressed. The optimization is for eliding `usize` formatting machinery and padding code from the final binary. Previously, writing any `fmt::Arguments` would cause the `usize` formatting and padding machinery to be included in the final binary since indexing used in `fmt::write` generates code using `panic_bounds_check` (that prints the index and length). Those bounds check are never hit, since `fmt::Arguments` never contain any out-of-bounds indicies. The `Makefile` version of `fmt-write-bloat` was ported to the present `rmake.rs` test infra in [PR-128147]. However, that PR just tries to maintain the original test logic. ### Limitations and problems The original test, it turns out, already have multiple limitations: - It only runs on non-Windows, since the `no_std` test of the original version tries to link against a `libc`. [PR-128807] worked around this by using a substitute name. We re-enabled this test in [PR-142841], but it turns out the assertions are too weak, it will even vacuously pass for no symbols at all. - In [PR-143669], we tried to make this test more robust by comparing the set of expected versus unexpected panic-related symbols, subject to if std was built with debug assertions. However, in working on [PR-143669], we've come to realize that this test is fundamentally very fragile: - The set of panic symbols depend on whether the standard library was built with or without debug assertions. - Different platforms often have different sets of panic machinery modules, functions and paths, and thus different sets of panic symbols. For instance, x86_64 msvc and i686 msvc have different panic code paths. - This comes back to the way the test is trying to gauge the absence of panic symbols -- it tries to look for symbol substring matches for "known" panic symbols. This is fundamentally fragile, because the test is trying to peek into the symbols of the resultant binary post-linking, based on fuzzy matches (the symbols are mangled as well). Based on this assessment, we determined that we should remove this test. This is not intended to exclude the possibility of reintroducing a more robust version of this test. For instance, we could consider some kind of more controllable post-link "end product" integration codegen test suite. [PR-78122]: #78122 [PR-128147]: #128147 [PR-128807]: #128807 [PR-142841]: #142841 [PR-143669]: #143669
Writing any fmt::Arguments would trigger the inclusion of usize formatting and padding code in the resulting binary, because indexing used in fmt::write would generate code using panic_bounds_check, which prints the index and length.
These bounds checks are not necessary, as fmt::Arguments never contains any out-of-bounds indexes.
This change replaces them with unsafe get_unchecked, to reduce the amount of generated code, which is especially important for embedded targets.
Demonstration of the size of and the symbols in a 'hello world' no_std binary:
Source code
Before:
After: