Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+101 −7
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c479ca
Re-export `pdb` crate for programmatically reading PDB files
jieyouxu Jul 9, 2025
24830de
Make sure `fmt-write-bloat` doesn't vacuously pass
jieyouxu Jul 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions Cargo.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,6 +1196,12 @@ dependencies = [
"once_cell",
]

[[package]]
name = "fallible-iterator"
version = "0.2.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "4443176a9f2c162692bd3d352d745ef9413eec5782a80d8fd6f8a1ac692a07f7"

[[package]]
name = "fallible-iterator"
version = "0.3.0"
Expand Down Expand Up @@ -1478,7 +1484,7 @@ version = "0.31.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "07e28edb80900c19c28f1072f2e8aeca7fa06b23cd4169cefe1af5aa3260783f"
dependencies = [
"fallible-iterator",
"fallible-iterator 0.3.0",
"indexmap",
"stable_deref_trait",
]
Expand All @@ -1489,7 +1495,7 @@ version = "0.32.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "93563d740bc9ef04104f9ed6f86f1e3275c2cdafb95664e26584b9ca807a8ffe"
dependencies = [
"fallible-iterator",
"fallible-iterator 0.3.0",
"indexmap",
"stable_deref_trait",
]
Expand Down Expand Up @@ -2765,6 +2771,17 @@ version = "0.2.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "df94ce210e5bc13cb6651479fa48d14f601d9858cfe0467f43ae157023b938d3"

[[package]]
name = "pdb"
version = "0.8.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "82040a392923abe6279c00ab4aff62d5250d1c8555dc780e4b02783a7aa74863"
dependencies = [
"fallible-iterator 0.2.0",
"scroll",
"uuid",
]

[[package]]
name = "percent-encoding"
version = "2.3.1"
Expand Down Expand Up @@ -3221,6 +3238,7 @@ dependencies = [
"gimli 0.32.0",
"libc",
"object 0.37.1",
"pdb",
"regex",
"serde_json",
"similar",
Expand Down Expand Up @@ -4930,6 +4948,12 @@ version = "1.2.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49"

[[package]]
name = "scroll"
version = "0.11.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "04c565b551bafbef4157586fa379538366e4385d42082f255bfd96e4fe8519da"

[[package]]
name = "self_cell"
version = "1.2.0"
Expand Down
1 change: 1 addition & 0 deletions src/tools/run-make-support/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ gimli = "0.32"
build_helper = { path = "../../build_helper" }
serde_json = "1.0"
libc = "0.2"
pdb = "0.8.0"

[lib]
crate-type = ["lib", "dylib"]
1 change: 1 addition & 0 deletions src/tools/run-make-support/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ pub use bstr;
pub use gimli;
pub use libc;
pub use object;
pub use pdb;
pub use regex;
pub use serde_json;
pub use similar;
Expand Down
78 changes: 73 additions & 5 deletions tests/run-make/fmt-write-bloat/rmake.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,89 @@
//! `NO_DEBUG_ASSERTIONS=1`). If debug assertions are disabled, then we can check for the absence of
//! additional `usize` formatting and padding related symbols.

// ignore-tidy-linelength

//@ ignore-cross-compile

use run_make_support::artifact_names::bin_name;
use run_make_support::env::no_debug_assertions;
use run_make_support::rustc;
use run_make_support::symbols::any_symbol_contains;
use run_make_support::{bin_name, is_darwin, is_windows_msvc, pdb, rustc, target};

// Not applicable for `extern "C"` symbol decoration handling.
fn sym(sym_name: &str) -> String {
if is_darwin() {
// Symbols are decorated with an underscore prefix on darwin platforms.
format!("_{sym_name}")
} else {
sym_name.to_string()
}
}

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() {
Copy link
Member Author

@jieyouxu jieyouxu Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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
endif

I 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

#143782

// if debug assertions are allowed, we need to allow these,
// otherwise, add them to the list of symbols to deny.
panic_syms.extend_from_slice(&["panicking", "panic_fmt", "pad_integral", "Display"]);
panic_syms.extend_from_slice(&[
sym("panicking"),
sym("panic_fmt"),
sym("pad_integral"),
sym("Display"),
]);
}

if is_windows_msvc() {
use pdb::FallibleIterator;

let file = std::fs::File::open("main.pdb").expect("failed to open `main.pdb`");
let mut pdb = pdb::PDB::open(file).expect("failed to parse `main.pdb`");

let symbol_table = pdb.global_symbols().expect("failed to parse PDB global symbols");
let mut symbols = symbol_table.iter();

let mut found_symbols = vec![];

while let Some(symbol) = symbols.next().expect("failed to parse symbol") {
match symbol.parse() {
Ok(pdb::SymbolData::Public(data)) => {
found_symbols.push(data.name.to_string());
}
_ => {}
}
}

// Make sure we at least have the `main` symbol itself, otherwise even no symbols can
// trivially satisfy the "no panic symbol" assertion.
let main_sym = if is_darwin() {
// Symbols are decorated with an underscore prefix on darwin platforms.
"_main"
} else if target().contains("i686") && is_windows_msvc() {
// `extern "C"` i.e. `__cdecl` on `i686` windows-msvc means that the symbol will be
// decorated with an underscore, but not on `x86_64` windows-msvc.
// See <https://learn.microsoft.com/en-us/cpp/build/reference/decorated-names?view=msvc-170#FormatC>.
"_main"
} else {
"main"
};

assert!(found_symbols.iter().any(|sym| sym == main_sym), "expected `main` symbol");

for found_symbol in found_symbols {
for panic_symbol in &panic_syms {
assert_ne!(
found_symbol,
panic_symbol.as_str(),
"found unexpected panic machinery symbol"
);
}
}
} else {
let panic_syms = panic_syms.iter().map(String::as_str).collect::<Vec<_>>();
// Make sure we at least have the `main` symbol itself, otherwise even no symbols can
// trivially satisfy the "no panic symbol" assertion.
assert!(any_symbol_contains(bin_name("main"), &["main"]));
assert!(!any_symbol_contains(bin_name("main"), &panic_syms));
Comment on lines +100 to +101
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's just because the underlying helper uses fs::read, which doesn't try to account for .exe or not.

}
assert!(!any_symbol_contains(bin_name("main"), &panic_syms));
}
Loading