migrate fmt-write-bloat to rmake #128147

lolbinarycat · 2024-07-24T18:16:50Z

try-job: aarch64-apple
try-job: x86_64-gnu-llvm-18
try-job: dist-x86_64-linux

rustbot · 2024-07-24T18:16:58Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jieyouxu (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot · 2024-07-24T18:16:59Z

The run-make-support library was changed

cc @jieyouxu

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

jieyouxu

Thanks for the PR. I have some simplification suggestions which should save us from having to roll our own symbol iter based on object.

src/tools/run-make-support/src/lib.rs

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

src/tools/run-make-support/src/symbols.rs

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

jieyouxu · 2024-07-24T20:30:27Z

Please flip to ready-for-review when you addressed the review comments, so I can run try jobs to double-check.

lolbinarycat · 2024-07-24T21:08:19Z

@rustbot review

jieyouxu · 2024-07-24T21:51:21Z

@bors try

…r=<try> migrate fmt-write-bloat to rmake try-job: aarch64-apple try-job: x86_64-gnu-llvm-18 try-job: dist-x86_64-linux

bors · 2024-07-24T21:52:32Z

⌛ Trying commit 55d5f85 with merge b3a96c9...

bors · 2024-07-24T22:55:25Z

💔 Test failed - checks-actions

lolbinarycat · 2024-07-24T22:58:57Z

@jieyouxu can you try the failing target with the old Makefile test? i kinda suspect that might fail too, since that target doesn't seem to be part of CI

jieyouxu · 2024-07-24T23:05:12Z

can you try the failing target with the old Makefile test? i kinda suspect that might fail too, since that target doesn't seem to be part of CI

AFAIK aarch64-apple is part of full build CI (i.e. not part of CI jobs that are run on every PR, but still need to pass when bors actually try to merge ur PR into master). So I'm fairly sure the old Makefile version passed aarch64-apple -- we can run another try-job if you restore the Makefile version temporarily.

lolbinarycat · 2024-07-24T23:09:52Z

@jieyouxu did a quick revert

jieyouxu · 2024-07-24T23:11:17Z

@bors try

bors · 2024-07-24T23:12:28Z

⌛ Trying commit fa1c001 with merge c88787f...

…r=<try> migrate fmt-write-bloat to rmake try-job: aarch64-apple try-job: x86_64-gnu-llvm-18 try-job: dist-x86_64-linux

this commit cannot easily be squashed, since there is already a PR based on the previous commit.

jieyouxu · 2024-07-30T20:19:26Z

@bors r+

bors · 2024-07-30T20:19:28Z

📌 Commit ebd6718 has been approved by jieyouxu

It is now in the queue for this repository.

tgross35 · 2024-07-30T20:36:32Z

this commit cannot easily be squashed, since there is already a
PR based on the previous commit.

(For reference, you would just need to tell tell git to drop the first commit (4feb949) when rebasing that branch on top of master after this merges. It just does this automatically if it is the exact same commit. But yeah, git nuances...)

…, r=jieyouxu migrate fmt-write-bloat to rmake try-job: aarch64-apple try-job: x86_64-gnu-llvm-18 try-job: dist-x86_64-linux

Rollup of 8 pull requests Successful merges: - rust-lang#125048 (PinCoerceUnsized trait into core) - rust-lang#127681 (derive(SmartPointer): rewrite bounds in where and generic bounds) - rust-lang#127830 (When an archive fails to build, print the path) - rust-lang#128147 (migrate fmt-write-bloat to rmake) - rust-lang#128356 (Migrate `cross-lang-lto-clang` and `cross-lang-lto-pgo-smoketest` `run-make` tests to rmake) - rust-lang#128387 (More detailed note to deprecate ONCE_INIT) - rust-lang#128388 (Match LLVM ABI in `extern "C"` functions for `f128` on Windows) - rust-lang#128412 (Remove `crate_level_only` from `ELIDED_LIFETIMES_IN_PATHS`) r? `@ghost` `@rustbot` modify labels: rollup

…, r=jieyouxu migrate fmt-write-bloat to rmake try-job: aarch64-apple try-job: x86_64-gnu-llvm-18 try-job: dist-x86_64-linux

Rollup of 7 pull requests Successful merges: - rust-lang#122049 (Promote riscv64gc-unknown-linux-musl to tier 2) - rust-lang#128147 (migrate fmt-write-bloat to rmake) - rust-lang#128161 (nested aux-build in tests/rustdoc/ tests) - rust-lang#128404 (Revert recent changes to dead code analysis) - rust-lang#128466 (Update the stdarch submodule) - rust-lang#128483 (Still more `cfg` cleanups) - rust-lang#128494 (MIR required_consts, mentioned_items: ensure we do not forget to fill these lists) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2024-08-02T02:29:02Z

⌛ Testing commit ebd6718 with merge 05e692a...

bors · 2024-08-02T04:53:12Z

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 05e692a to master...

rust-timer · 2024-08-02T12:22:14Z

Finished benchmarking commit (05e692a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (primary 2.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.2%	[2.2%, 2.2%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 757.745s -> 757.516s (-0.03%)
Artifact size: 336.82 MiB -> 336.86 MiB (0.01%)

uses helper functions added in rust-lang#128147, must not be merged before that PR.

…ke, r=<try> port tests/run-make/extern-fn-reachable to rmake uses helper functions added in rust-lang#128147, must not be merged before that PR. try-job: aarch64-apple try-job: armhf-gnu try-job: test-various try-job: x86_64-msvc try-job: x86_64-mingw try-job: i686-msvc try-job: i686-mingw try-job: x86_64-gnu-llvm-17

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

rustbot assigned jieyouxu Jul 24, 2024

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. labels Jul 24, 2024

This comment has been minimized.

Sign in to view

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jul 24, 2024

jieyouxu mentioned this pull request Jul 24, 2024

Tracking Issue for porting run-make tests to use Rust #121876

Closed

jieyouxu requested changes Jul 24, 2024

View reviewed changes

src/tools/run-make-support/src/lib.rs Show resolved Hide resolved

tests/run-make/fmt-write-bloat/rmake.rs Outdated Show resolved Hide resolved

src/tools/run-make-support/src/symbols.rs Show resolved Hide resolved

tests/run-make/fmt-write-bloat/rmake.rs Show resolved Hide resolved

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 Jul 24, 2024

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 Jul 24, 2024

This comment has been minimized.

Sign in to view

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 Jul 24, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 24, 2024

Auto merge of rust-lang#128147 - lolbinarycat:fmt-write-bloat-rmake, …

b3a96c9

…r=<try> migrate fmt-write-bloat to rmake try-job: aarch64-apple try-job: x86_64-gnu-llvm-18 try-job: dist-x86_64-linux

This comment has been minimized.

Sign in to view

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 24, 2024

Auto merge of rust-lang#128147 - lolbinarycat:fmt-write-bloat-rmake, …

c88787f

…r=<try> migrate fmt-write-bloat to rmake try-job: aarch64-apple try-job: x86_64-gnu-llvm-18 try-job: dist-x86_64-linux

tidy

ebd6718

this commit cannot easily be squashed, since there is already a PR based on the previous commit.

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 Jul 30, 2024

tgross35 mentioned this pull request Jul 31, 2024

Rollup of 8 pull requests #128418

Closed

jieyouxu mentioned this pull request Jul 31, 2024

tests/run-make/fmt-write-bloat fails with lld #108137

Closed

tgross35 mentioned this pull request Aug 1, 2024

Rollup of 4 pull requests #128513

Closed

tgross35 mentioned this pull request Aug 1, 2024

Rollup of 5 pull requests #128514

Closed

tgross35 mentioned this pull request Aug 1, 2024

Rollup of 7 pull requests #128518

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 2, 2024

bors merged commit 05e692a into rust-lang:master Aug 2, 2024

rustbot added this to the 1.82.0 milestone Aug 2, 2024

lolbinarycat added a commit to lolbinarycat/rust that referenced this pull request Aug 2, 2024

port tests/run-make/extern-fn-reachable to rmake

594e4c0

uses helper functions added in rust-lang#128147, must not be merged before that PR.

lolbinarycat added a commit to lolbinarycat/rust that referenced this pull request Aug 6, 2024

port tests/run-make/extern-fn-reachable to rmake

f66adb0

uses helper functions added in rust-lang#128147, must not be merged before that PR.

jieyouxu mentioned this pull request Nov 2, 2025

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

Merged

migrate fmt-write-bloat to rmake #128147

migrate fmt-write-bloat to rmake #128147

Uh oh!

Conversation

lolbinarycat commented Jul 24, 2024 • edited by jieyouxu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 24, 2024

Uh oh!

rustbot commented Jul 24, 2024

Uh oh!

This comment has been minimized.

jieyouxu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jieyouxu commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lolbinarycat commented Jul 24, 2024

Uh oh!

This comment has been minimized.

jieyouxu commented Jul 24, 2024

Uh oh!

bors commented Jul 24, 2024

Uh oh!

This comment has been minimized.

bors commented Jul 24, 2024

Uh oh!

lolbinarycat commented Jul 24, 2024

Uh oh!

jieyouxu commented Jul 24, 2024

Uh oh!

lolbinarycat commented Jul 24, 2024

Uh oh!

jieyouxu commented Jul 24, 2024

Uh oh!

bors commented Jul 24, 2024

Uh oh!

jieyouxu commented Jul 30, 2024

Uh oh!

bors commented Jul 30, 2024

Uh oh!

tgross35 commented Jul 30, 2024

Uh oh!

bors commented Aug 2, 2024

Uh oh!

bors commented Aug 2, 2024

Uh oh!

rust-timer commented Aug 2, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lolbinarycat commented Jul 24, 2024 •

edited by jieyouxu

Loading

jieyouxu left a comment •

edited

Loading

jieyouxu commented Jul 24, 2024 •

edited

Loading