Enable fmt-write-bloat for Windows by dpaoliello · Pull Request #142841 · rust-lang/rust

dpaoliello · 2025-06-21T19:01:14Z

Seems to be working fine for MSVC once it has the correct binary name.

Addresses item in #128602

try-job: x86_64-mingw-*
try-job: x86_64-msvc-*
try-job: i686-msvc-*

rustbot · 2025-06-21T19:01:18Z

jieyouxu is not on the review rotation at the moment.
They may take a while to respond.

rustbot · 2025-06-21T19:01:19Z

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-06-21T19:01:21Z

This PR modifies run-make tests.

cc @jieyouxu

jieyouxu · 2025-06-22T00:58:55Z

@bors2 delegate=try
@bors2 try

rust-bors · 2025-06-22T00:58:58Z

@dpaoliello can now perform try builds on this pull request

rust-bors · 2025-06-22T00:58:59Z

⌛ Trying commit 33b3ea2 with merge f335da1…

To cancel the try build, run the command @bors2 try cancel.

Enable fmt-write-bloat for Windows Seems to be working fine for MSVC once it has the correct binary name. Addresses item in #128602 --- try-job: x86_64-mingw-* try-job: x86_64-msvc-* try-job: i686-msvc-*

rust-bors · 2025-06-22T03:38:38Z

☀️ Try build successful (CI)
Build commit: f335da1 (f335da1046ddc4c9f75ad83c72feb85b7c046efb, parent: d4e1159b8c97478778b09a4cc1c7adce5653b8bf)

jieyouxu

Kewl

jieyouxu · 2025-06-22T04:00:57Z

Thanks
@bors r+ rollup

bors · 2025-06-22T04:01:00Z

📌 Commit 33b3ea2 has been approved by jieyouxu

It is now in the queue for this repository.

Rollup of 10 pull requests Successful merges: - #140254 (Pass -Cpanic=abort for the panic_abort crate) - #142600 (Port `#[rustc_pub_transparent]` to the new attribute system) - #142617 (improve search graph docs, reset `encountered_overflow` between reruns) - #142747 (rustdoc_json: conversion cleanups) - #142776 (All HIR attributes are outer) - #142800 (integer docs: remove extraneous text) - #142841 (Enable fmt-write-bloat for Windows) - #142845 (Enable textrel-on-minimal-lib for Windows) - #142850 (remove asm_goto feature annotation, for it is now stabilized) - #142860 (Notify me on tidy changes) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #142841 - dpaoliello:fmt-write-bloat, r=jieyouxu Enable fmt-write-bloat for Windows Seems to be working fine for MSVC once it has the correct binary name. Addresses item in #128602 --- try-job: x86_64-mingw-* try-job: x86_64-msvc-* try-job: i686-msvc-*

ChrisDenton · 2025-07-08T22:16:18Z

tests/run-make/fmt-write-bloat/rmake.rs

        panic_syms.extend_from_slice(&["panicking", "panic_fmt", "pad_integral", "Display"]);
    }
-    assert!(!any_symbol_contains("main", &panic_syms));
+    assert!(!any_symbol_contains(bin_name("main"), &panic_syms));


Won't this assert trivially pass if there are no symbols at all?

Good point, this should be more like a codegen test hmm

Make sure `fmt-write-bloat` doesn't vacuously pass on no symbols Previously, the test only checks for the absence of certain panic symbols, but having no symbols was also a possible albeit vacuous way to satisfy this assertion. Fix this by checking we at least observe the `main` symbol which is always expected to be present. Noticed in #142841 (comment). r? `@ChrisDenton` try-job: x86_64-msvc-*

[WIP] Make sure `fmt-write-bloat` doesn't vacuously pass on no symbols Previously, the test only checks for the absence of certain panic symbols, but having no symbols was also a possible albeit vacuous way to satisfy this assertion. Fix this by checking we at least observe the `main` symbol which is always expected to be present. Noticed in #142841 (comment). r? `@ChrisDenton` try-job: aarch64-apple try-job: x86_64-apple-1 try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: x86_64-mingw-1

[WIP] Make sure `fmt-write-bloat` doesn't vacuously pass on no symbols Previously, the test only checks for the absence of certain panic symbols, but having no symbols was also a possible albeit vacuous way to satisfy this assertion. Fix this by checking we at least observe the `main` symbol which is always expected to be present. Noticed in #142841 (comment). r? `@ChrisDenton` //try-job: aarch64-apple //try-job: x86_64-apple-1 //try-job: x86_64-msvc-1 try-job: i686-msvc-1 //try-job: x86_64-mingw-1

[WIP] Make sure `fmt-write-bloat` doesn't vacuously pass on no symbols Previously, the test only checks for the absence of certain panic symbols, but having no symbols was also a possible albeit vacuous way to satisfy this assertion. Fix this by checking we at least observe the `main` symbol which is always expected to be present. Noticed in #142841 (comment). r? `@ChrisDenton` try-job: aarch64-apple try-job: x86_64-apple-1 try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: x86_64-mingw-1

…enton Make sure `fmt-write-bloat` doesn't vacuously pass on no symbols This PR fixes the `tests/run-make/fmt-write-float/` run-make test to both (1) not vacuously pass on no symbols at all, and (2) compare panics symbols under optimizations versus no optimizations. ### Context Previously, the test only checks for the absence of certain panic symbols, but having no symbols was also a possible albeit vacuous way to satisfy this assertion. Noticed in rust-lang#142841 (comment). r? `@ChrisDenton` try-job: aarch64-apple try-job: aarch64-msvc-1 try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: x86_64-mingw-1

Make sure `fmt-write-bloat` doesn't vacuously pass on no symbols This PR fixes the `tests/run-make/fmt-write-float/` run-make test to both (1) not vacuously pass on no symbols at all, and (2) compare panics symbols under optimizations versus no optimizations. ### Context Previously, the test only checks for the absence of certain panic symbols, but having no symbols was also a possible albeit vacuous way to satisfy this assertion. Noticed in #142841 (comment). r? `@ChrisDenton` try-job: aarch64-apple try-job: aarch64-msvc-1 try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: x86_64-mingw-1

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

Enable fmt-write-bloat for Windows

33b3ea2

rustbot assigned jieyouxu Jun 21, 2025

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 Jun 21, 2025

jieyouxu approved these changes Jun 22, 2025

View reviewed changes

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 Jun 22, 2025

jhpratt mentioned this pull request Jun 22, 2025

Rollup of 10 pull requests #142864

Merged

bors merged commit cbfb654 into rust-lang:master Jun 22, 2025
10 checks passed

rustbot added this to the 1.89.0 milestone Jun 22, 2025

dpaoliello deleted the fmt-write-bloat branch June 23, 2025 16:58

ChrisDenton reviewed Jul 8, 2025

View reviewed changes

jieyouxu mentioned this pull request Jul 9, 2025

Make sure fmt-write-bloat doesn't vacuously pass on no symbols #143669

Closed

jieyouxu mentioned this pull request Nov 2, 2025

Remove tests/run-make/fmt-write-bloat/ #148393

Merged

Uh oh!

Conversation

dpaoliello commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 21, 2025

Uh oh!

rustbot commented Jun 21, 2025

Uh oh!

rustbot commented Jun 21, 2025

Uh oh!

jieyouxu commented Jun 22, 2025

Uh oh!

rust-bors bot commented Jun 22, 2025

Uh oh!

rust-bors bot commented Jun 22, 2025

Uh oh!

rust-bors bot commented Jun 22, 2025

Uh oh!

jieyouxu left a comment

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Jun 22, 2025

Uh oh!

bors commented Jun 22, 2025

Uh oh!

Uh oh!

ChrisDenton Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dpaoliello commented Jun 21, 2025 •

edited

Loading