run-make: explaing why fmt-write-bloat is ignore-windows #128807

ChrisDenton · 2024-08-08T02:15:34Z

The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work.

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

rustbot · 2024-08-08T02:15:42Z

r? @jieyouxu

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 · 2024-08-08T02:15:43Z

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

ChrisDenton · 2024-08-08T02:17:05Z

@bors try

bors · 2024-08-08T02:18:15Z

⌛ Trying commit 06d09ad with merge 5811814...

run-make: run fmt-write-bloat on Windows The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work. try-job: x86_64-msvc try-job: i686-msvc

jieyouxu

Thanks, r=me if try job comes back green.

bors · 2024-08-08T05:01:41Z

💔 Test failed - checks-actions

ChrisDenton · 2024-08-08T05:10:13Z

warning LNK4098: defaultlib 'msvcrt' conflicts with use of other libs

Do we link dynamically on i686 but statically on x86_64? That's going to make things slightly more complicated.

ChrisDenton · 2024-08-08T05:13:12Z

@bors try

bors · 2024-08-08T05:14:22Z

⌛ Trying commit 67518d0 with merge 81ac031...

run-make: run fmt-write-bloat on Windows The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work. try-job: x86_64-msvc try-job: i686-msvc

bors · 2024-08-08T07:00:55Z

☀️ Try build successful - checks-actions
Build commit: 81ac031 (81ac03195aa96073b58ec4de9fa295c33a0f08ed)

jieyouxu

Pog!

jieyouxu · 2024-08-08T07:07:49Z

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

+#[cfg_attr(not(target_env = "msvc"), link(name = "c"))]
+#[cfg_attr(all(target_env = "msvc", target_feature = "crt-static"), link(name = "libcmt"))]
+#[cfg_attr(all(target_env = "msvc", not(target_feature = "crt-static")), link(name = "msvcrt"))]


Remark: this is not obvious, at all lol

Aye. I do think this logic should ideally either be in run-make-support or better yet compiletest could have something like a NO_STD_EXTRA_ARGS variable that any test can use. Or at least I think that's better than individual tests needing to figure it out. But I'm not entirely sure how best to approach that.

Good point. I opened #128821 so this doesn't get lost in a review comment once the PR is merged.

jieyouxu · 2024-08-08T09:20:34Z

Looks good as is, so
@bors r+ rollup

bors · 2024-08-08T09:20:37Z

📌 Commit 67518d0 has been approved by jieyouxu

It is now in the queue for this repository.

run-make: run fmt-write-bloat on Windows The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work. try-job: x86_64-msvc try-job: i686-msvc

…iaskrgr Rollup of 3 pull requests Successful merges: - rust-lang#128795 (Update E0517 message to reflect RFC 2195.) - rust-lang#128804 (run-make: enable msvc for redundant-libs) - rust-lang#128807 (run-make: run fmt-write-bloat on Windows) r? `@ghost` `@rustbot` modify labels: rollup

compiler-errors · 2024-08-08T14:35:21Z

#128819 (comment)

@bors r-

run-make: run fmt-write-bloat on Windows The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work. try-job: x86_64-msvc try-job: x86_64-mingw try-job: i686-msvc try-job: i686-mingw

jieyouxu · 2024-08-08T17:55:31Z

The other news is that I've found the reason why we were ignoring __imp_ symbols. Apparently it is because of i686-mingw (but not any other target).

wtf

bors · 2024-08-08T20:00:44Z

💔 Test failed - checks-actions

ChrisDenton · 2024-08-08T20:05:20Z

Ok, this is not going to be as easy as I thought. Maybe I should try compiling it as a staticlib. However, I'd need to make sure it's still testing everything correctly. EDIT: Hmm... that doesn't really work. The original issue seems quite sensitive to the optimization used when in a staticlib.

ChrisDenton · 2024-08-08T21:35:23Z

@bors try

run-make: run fmt-write-bloat on Windows The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work. try-job: x86_64-msvc try-job: x86_64-mingw try-job: i686-msvc try-job: i686-mingw

bors · 2024-08-08T21:36:33Z

⌛ Trying commit d8969b0 with merge 442f40b...

bors · 2024-08-08T23:57:15Z

💔 Test failed - checks-actions

ChrisDenton · 2024-08-09T21:27:25Z

@jieyouxu ok, I'm going to admit defeat here. At least for the time being. I have updated the test to explain why this test in particular is ignore-windows. I've also kept the changes to not link libc on Windows, even though Windows is ignore, in case it helps someone else who looks at this in the future.

Anyway, the tl;dr version of the comment is that to properly run this test on Windows MSVC we need to parse the debug information from a .pdb file. On Windows MINGW we (currently) can't stop a load of random runtime stuff being included so on that platform at least we'd need to be more specific than e.g. "Debug" when looking for symbols.

jieyouxu · 2024-08-10T00:37:43Z

Thank you for trying! Knowing more specifically why it doesn't work is very useful!

@bors r+ rollup

bors · 2024-08-10T00:37:46Z

📌 Commit ef90df6 has been approved by jieyouxu

It is now in the queue for this repository.

…llaumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#128273 (Improve `Ord` violation help) - rust-lang#128807 (run-make: explaing why fmt-write-bloat is ignore-windows) - rust-lang#128903 (rustdoc-json-types `Discriminant`: fix typo) - rust-lang#128905 (gitignore: Add Zed and Helix editors) - rust-lang#128908 (diagnostics: do not warn when a lifetime bound infers itself) - rust-lang#128909 (Fix dump-ice-to-disk for RUSTC_ICE=0 users) - rust-lang#128910 (Differentiate between methods and associated functions in diagnostics) - rust-lang#128923 ([rustdoc] Stop showing impl items for negative impls) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#128807 - ChrisDenton:bloat, r=jieyouxu run-make: explaing why fmt-write-bloat is ignore-windows The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work. try-job: x86_64-msvc try-job: x86_64-mingw try-job: i686-msvc try-job: i686-mingw

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 Aug 8, 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 Aug 8, 2024

ChrisDenton mentioned this pull request Aug 8, 2024

run-make: audit the ignore-{windows,msvc,windows-msvc} tests #128602

Open

28 tasks

jieyouxu approved these changes Aug 8, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

bors 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 Aug 8, 2024

jieyouxu approved these changes Aug 8, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 8, 2024

matthiaskrgr mentioned this pull request Aug 8, 2024

Rollup of 3 pull requests #128819

Closed

jieyouxu mentioned this pull request Aug 8, 2024

run-make: investigate if we can make tests with linkage less error-prone #128821

Open

This comment has been minimized.

Sign in to view

ChrisDenton force-pushed the bloat branch from a33003b to d8969b0 Compare August 8, 2024 21:34

This comment has been minimized.

Sign in to view

ChrisDenton added 2 commits August 9, 2024 21:17

Only link libc on *nix platforms

2cc029e

Update reason why fmt-write-bloat ignores windows

ef90df6

ChrisDenton force-pushed the bloat branch from d8969b0 to ef90df6 Compare August 9, 2024 21:22

ChrisDenton changed the title ~~run-make: run fmt-write-bloat on Windows~~ run-make: explaing why fmt-write-bloat is ignore-windows Aug 9, 2024

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 10, 2024

GuillaumeGomez mentioned this pull request Aug 10, 2024

Rollup of 8 pull requests #128927

Merged

bors merged commit a7e188a into rust-lang:master Aug 10, 2024

rustbot added this to the 1.82.0 milestone Aug 10, 2024

ChrisDenton deleted the bloat branch August 10, 2024 21:05

jieyouxu mentioned this pull request Nov 2, 2025

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

Merged

run-make: explaing why fmt-write-bloat is ignore-windows #128807

run-make: explaing why fmt-write-bloat is ignore-windows #128807

Uh oh!

Conversation

ChrisDenton commented Aug 8, 2024 • edited by jieyouxu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 8, 2024

Uh oh!

rustbot commented Aug 8, 2024

Uh oh!

ChrisDenton commented Aug 8, 2024

Uh oh!

bors commented Aug 8, 2024

Uh oh!

jieyouxu left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

bors commented Aug 8, 2024

Uh oh!

ChrisDenton commented Aug 8, 2024

Uh oh!

ChrisDenton commented Aug 8, 2024

Uh oh!

bors commented Aug 8, 2024

Uh oh!

bors commented Aug 8, 2024

Uh oh!

jieyouxu left a comment

Choose a reason for hiding this comment

Uh oh!

jieyouxu Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

ChrisDenton Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

jieyouxu Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

jieyouxu commented Aug 8, 2024

Uh oh!

bors commented Aug 8, 2024

Uh oh!

compiler-errors commented Aug 8, 2024

Uh oh!

jieyouxu commented Aug 8, 2024

Uh oh!

This comment has been minimized.

bors commented Aug 8, 2024

Uh oh!

ChrisDenton commented Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisDenton commented Aug 8, 2024

Uh oh!

bors commented Aug 8, 2024

Uh oh!

This comment has been minimized.

bors commented Aug 8, 2024

Uh oh!

ChrisDenton commented Aug 9, 2024

Uh oh!

jieyouxu commented Aug 10, 2024

Uh oh!

bors commented Aug 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ChrisDenton commented Aug 8, 2024 •

edited by jieyouxu

Loading

ChrisDenton commented Aug 8, 2024 •

edited

Loading