Skip to content

Conversation

@jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Nov 2, 2025

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.

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
Copy link
Member

Thank you for the attempt to get it working!

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Nov 2, 2025

📌 Commit 76066d7 has been approved by ChrisDenton

It is now in the queue for this repository.

@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. labels Nov 2, 2025
@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 2, 2025
fn main() {
rustc().input("main.rs").opt().run();
// panic machinery identifiers, these should not appear in the final binary
let mut panic_syms = vec!["panic_bounds_check", "Debug"];
Copy link
Member

Choose a reason for hiding this comment

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

I try to add a similar test in #148105, what do you think about that? It's robust against debug assertions because it uses build-std, and I included significantly more symbol substrings to check for panics (and I'll likely extend the test to also run a control run first against something known to panic).

Copy link
Member Author

@jieyouxu jieyouxu Nov 2, 2025

Choose a reason for hiding this comment

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

For that PR, if you have a more consistent set of panic symbols (or panic symbol substrings) to match on, I think that seems okay. But wait, have you ran your run-make test against the Windows targets? I don't see any //@ ignore-cross-compile or //@ ignore-windows directives in your run-make tests, so I'd assume that your test might also fail against one of x86-64-msvc-1, i686-msvc-1, test-various, dist-various-1, armhf-gnu.

Basically, you might run into the same problems that I did trying to get this test to actually test what it needs to test in #143669.

Copy link
Member

Choose a reason for hiding this comment

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

I've been trying to figure out if they work on Windows, and am happy to ignore it there if it doesn't 😁

bors added a commit that referenced this pull request Nov 2, 2025
Rollup of 9 pull requests

Successful merges:

 - #147137 (Mention crate being analyzed in query description)
 - #147155 (arm-linux.md: various fixes/improvements)
 - #147642 (Miscellaneous const-generics-related fixes)
 - #147806 (Ignore test-dashboard related files)
 - #147947 (Implement `strip_circumfix` lib feature)
 - #148346 (Change cfg_trace, cfg_attr_trace symbol values)
 - #148348 (dangling ptr lint cleanup)
 - #148393 (Remove `tests/run-make/fmt-write-bloat/`)
 - #148400 (Better warning message for crate type unsupported by codegen backend)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 22b5b3f into rust-lang:master Nov 2, 2025
11 checks passed
@rustbot rustbot added this to the 1.93.0 milestone Nov 2, 2025
rust-timer added a commit that referenced this pull request Nov 2, 2025
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
@jieyouxu jieyouxu deleted the remove-fmt-write-bloat branch November 2, 2025 23:31
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Nov 3, 2025
Rollup of 9 pull requests

Successful merges:

 - rust-lang/rust#147137 (Mention crate being analyzed in query description)
 - rust-lang/rust#147155 (arm-linux.md: various fixes/improvements)
 - rust-lang/rust#147642 (Miscellaneous const-generics-related fixes)
 - rust-lang/rust#147806 (Ignore test-dashboard related files)
 - rust-lang/rust#147947 (Implement `strip_circumfix` lib feature)
 - rust-lang/rust#148346 (Change cfg_trace, cfg_attr_trace symbol values)
 - rust-lang/rust#148348 (dangling ptr lint cleanup)
 - rust-lang/rust#148393 (Remove `tests/run-make/fmt-write-bloat/`)
 - rust-lang/rust#148400 (Better warning message for crate type unsupported by codegen backend)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants