Make sure fmt-write-bloat doesn't vacuously pass on no symbols
#143669
Conversation
rustbot
added
A-run-make
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bors2 try jobs=x86_64-msvc-* |
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-*
|
💔 Test failed
|
|
Hm. Let me try this locally on windows. |
jieyouxu
added
S-waiting-on-author
|
The failure is not surprising to me. On Windows, symbols are not contained in the binary itself and are instead in a separate file (the .pdb). So the |
|
Oh of course... D'oh. Let me see if I can cook something up. |
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
jieyouxu
changed the title
fmt-write-bloat doesn't vacuously pass on no symbolsfmt-write-bloat doesn't vacuously pass on no symbols
|
@bors2 try |
[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
|
|
|
I bet this will also fail on apple because |
|
I forgor to run this locally, it's |
|
Try build cancelled. Cancelled workflows: |
|
Oh. Apparently we have both @bors2 try |
[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
|
💔 Test failed
|
|
Huh, does i686 msvc prefix Symbols found on i686-pc-windows-msvc |
|
See https://learn.microsoft.com/en-us/cpp/build/reference/decorated-names?view=msvc-170#FormatC On i686-msvc |
jieyouxu
force-pushed
the
fmt-write-bloat
branch
from
08ba8e3 to
b63f2b1
Compare
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.
jieyouxu
force-pushed
the
fmt-write-bloat
branch
from
b63f2b1 to
24830de
Compare
|
@bors2 try |
[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
Assorted `run-make-support` maintenance
This PR should contain no functional changes.
- Commit 1: Removes the support library's CHANGELOG. In the very beginning, I thought maybe we would try to version this library. But this is a purely internal test support library, and it's just extra busywork trying to maintain changelog/versions. It's also hopelessly outdated.
- Commit 2: Resets version number to `0.0.0`. Ditto on busywork.
- Commit 3: Bump `run-make-support` to Edition 2024. The support library was already "compliant" with Edition 2024.
- Commit 4: Slightly organizes the support library dependencies.
- Commit 5: Previously, I tried hopelessly to maintain some manual formatting, but that was annoying because it required skipping rustfmt (so export ordering etc. could not be extra formatted). Give up, and do some rearrangements / module prefix tricks to get the `lib.rs` looking at least *reasonable*. IMO this is not a strict improvement, but I rather regain the ability to auto-format it with rustfmt.
- Commit {6,7}: Noticed in rust-lang#143669 that we apparently had *both* {`is_msvc`, `is_windows_msvc`}. This PR removes `is_msvc` in favor of `is_windows_msvc` to make it unambiguous (and only retain one way of gating) as there are some UEFI targets which are MSVC but not Windows.
Best reviewed commit-by-commit.
r? `@Kobzol`
jieyouxu
changed the title
fmt-write-bloat doesn't vacuously pass on no symbolsfmt-write-bloat doesn't vacuously pass on no symbols
|
Try jobs are green. |
rustbot
added
S-waiting-on-review
|
Re. the |
|
My only concern with the I'm not going to insist on it on this PR but I think long term it would be good to use official debugging APIs on Windows hosts and only fallback to the |
| assert!(any_symbol_contains(bin_name("main"), &["main"])); | ||
| assert!(!any_symbol_contains(bin_name("main"), &panic_syms)); |
There was a problem hiding this comment.
wait, I know this is pre-existing but why is this bin_name? That should be a no-op on unix targets but main.exe on Windows, which sounds weird.
There was a problem hiding this comment.
I think it's just because the underlying helper uses fs::read, which doesn't try to account for .exe or not.
Ah, did you mean the Debug Interface Access SDK? |
|
Yeah. But I mean, figuring out how to use it is perhaps more work then you signed up for 😉. |
|
Hm yeah. I think in the long-term, we should probably be using DIA wrappers (but that's quite a bit of Windows-specific work). In the short-term, I could also fallback to just using EDIT: #143737 |
|
Let me try the
approach. @rustbot author |
rustbot
added
S-waiting-on-author
Rollup merge of #143683 - jieyouxu:rms-cleanup, r=Kobzol Assorted `run-make-support` maintenance This PR should contain no functional changes. - Commit 1: Removes the support library's CHANGELOG. In the very beginning, I thought maybe we would try to version this library. But this is a purely internal test support library, and it's just extra busywork trying to maintain changelog/versions. It's also hopelessly outdated. - Commit 2: Resets version number to `0.0.0`. Ditto on busywork. - Commit 3: Bump `run-make-support` to Edition 2024. The support library was already "compliant" with Edition 2024. - Commit 4: Slightly organizes the support library dependencies. - Commit 5: Previously, I tried hopelessly to maintain some manual formatting, but that was annoying because it required skipping rustfmt (so export ordering etc. could not be extra formatted). Give up, and do some rearrangements / module prefix tricks to get the `lib.rs` looking at least *reasonable*. IMO this is not a strict improvement, but I rather regain the ability to auto-format it with rustfmt. - Commit {6,7}: Noticed in #143669 that we apparently had *both* {`is_msvc`, `is_windows_msvc`}. This PR removes `is_msvc` in favor of `is_windows_msvc` to make it unambiguous (and only retain one way of gating) as there are some UEFI targets which are MSVC but not Windows. Best reviewed commit-by-commit. r? `@Kobzol`
|
☔ The latest upstream changes (presumably #143731) made this pull request unmergeable. Please resolve the merge conflicts. |
|
|
||
| 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"]; | ||
| let mut panic_syms = vec![sym("panic_bounds_check"), sym("Debug")]; | ||
| if no_debug_assertions() { |
There was a problem hiding this comment.
Remark: while investigating this test, I realized this is actually incorrect (even in the original Makefile version), and the port preserved the incorrect gating
# Allow for debug_assert!() in debug builds of std.
ifdef NO_DEBUG_ASSERTIONS
PANIC_SYMS += panicking panic_fmt pad_integral Display Debug
endifI believe NO_DEBUG_ASSERTIONS corresponds (confusingly) only to whether rustc has debug assertions enabled, not std.
I have a separate PR that I'll send first.
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) actually check aginst the platform-specific decorated panic symbols.run-make-supportlibrary to allow us to programmatically inspect PDB files, instead of trying to perform textual output matching onllvm-pdbutilwhose textual output is not necessarily guaranteed to remain the same between versions.mainsymbol and isn't completely devoid of symbols, and also fixes the negative check against panic machinery symbols to account for platform differences.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.
Fix this by checking we at least observe the
mainsymbol 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