From 4feb949617c9a24f8750d8a69197f086b0005cee Mon Sep 17 00:00:00 2001 From: binarycat Date: Wed, 24 Jul 2024 14:05:58 -0400 Subject: [PATCH 1/2] migrate fmt-write-bloat to rmake --- src/tools/run-make-support/src/env.rs | 7 +++ src/tools/run-make-support/src/lib.rs | 1 + src/tools/run-make-support/src/symbols.rs | 43 +++++++++++++++++++ .../tidy/src/allowed_run_make_makefiles.txt | 1 - tests/run-make/fmt-write-bloat/Makefile | 25 ----------- tests/run-make/fmt-write-bloat/rmake.rs | 35 +++++++++++++++ 6 files changed, 86 insertions(+), 26 deletions(-) create mode 100644 src/tools/run-make-support/src/symbols.rs delete mode 100644 tests/run-make/fmt-write-bloat/Makefile create mode 100644 tests/run-make/fmt-write-bloat/rmake.rs diff --git a/src/tools/run-make-support/src/env.rs b/src/tools/run-make-support/src/env.rs index f52524e7d54cd..e6460fb93d3e9 100644 --- a/src/tools/run-make-support/src/env.rs +++ b/src/tools/run-make-support/src/env.rs @@ -17,3 +17,10 @@ pub fn env_var_os(name: &str) -> OsString { None => panic!("failed to retrieve environment variable {name:?}"), } } + +/// Check if `NO_DEBUG_ASSERTIONS` is set (usually this may be set in CI jobs). +#[track_caller] +#[must_use] +pub fn no_debug_assertions() -> bool { + std::env::var_os("NO_DEBUG_ASSERTIONS").is_some() +} diff --git a/src/tools/run-make-support/src/lib.rs b/src/tools/run-make-support/src/lib.rs index 085120764b463..36ea6125f2780 100644 --- a/src/tools/run-make-support/src/lib.rs +++ b/src/tools/run-make-support/src/lib.rs @@ -21,6 +21,7 @@ pub mod run; pub mod scoped_run; pub mod string; pub mod targets; +pub mod symbols; // Internally we call our fs-related support module as `fs`, but re-export its content as `rfs` // to tests to avoid colliding with commonly used `use std::fs;`. diff --git a/src/tools/run-make-support/src/symbols.rs b/src/tools/run-make-support/src/symbols.rs new file mode 100644 index 0000000000000..52119ba4991a8 --- /dev/null +++ b/src/tools/run-make-support/src/symbols.rs @@ -0,0 +1,43 @@ +use object::{self, Object, ObjectSymbol, SymbolIterator}; +use std::path::Path; + +/// Iterate through the symbols in an object file. +/// +/// Uses a callback because `SymbolIterator` does not own its data. +/// +/// Panics if `path` is not a valid object file readable by the current user. +pub fn with_symbol_iter(path: P, func: F) -> R +where + P: AsRef, + F: FnOnce(&mut SymbolIterator<'_, '_>) -> R, +{ + let raw_bytes = crate::fs::read(path); + let f = object::File::parse(raw_bytes.as_slice()).expect("unable to parse file"); + let mut iter = f.symbols(); + func(&mut iter) +} + +/// Check an object file's symbols for substrings. +/// +/// Returns `true` if any of the symbols found in the object file at +/// `path` contain a substring listed in `substrings`. +/// +/// Panics if `path` is not a valid object file readable by the current user. +pub fn any_symbol_contains(path: impl AsRef, substrings: &[&str]) -> bool { + with_symbol_iter(path, |syms| { + for sym in syms { + for substring in substrings { + if sym + .name_bytes() + .unwrap() + .windows(substring.len()) + .any(|x| x == substring.as_bytes()) + { + eprintln!("{:?} contains {}", sym, substring); + return true; + } + } + } + false + }) +} diff --git a/src/tools/tidy/src/allowed_run_make_makefiles.txt b/src/tools/tidy/src/allowed_run_make_makefiles.txt index a84b89ff4a113..db06040de2b3a 100644 --- a/src/tools/tidy/src/allowed_run_make_makefiles.txt +++ b/src/tools/tidy/src/allowed_run_make_makefiles.txt @@ -10,7 +10,6 @@ run-make/dep-info-spaces/Makefile run-make/dep-info/Makefile run-make/emit-to-stdout/Makefile run-make/extern-fn-reachable/Makefile -run-make/fmt-write-bloat/Makefile run-make/foreign-double-unwind/Makefile run-make/foreign-exceptions/Makefile run-make/incr-add-rust-src-component/Makefile diff --git a/tests/run-make/fmt-write-bloat/Makefile b/tests/run-make/fmt-write-bloat/Makefile deleted file mode 100644 index 70e04b9af51fe..0000000000000 --- a/tests/run-make/fmt-write-bloat/Makefile +++ /dev/null @@ -1,25 +0,0 @@ -include ../tools.mk - -# ignore-windows - -ifeq ($(shell $(RUSTC) -vV | grep 'host: $(TARGET)'),) - -# Don't run this test when cross compiling. -all: - -else - -NM = nm - -PANIC_SYMS = panic_bounds_check Debug - -# Allow for debug_assert!() in debug builds of std. -ifdef NO_DEBUG_ASSERTIONS -PANIC_SYMS += panicking panic_fmt pad_integral Display Debug -endif - -all: main.rs - $(RUSTC) $< -O - $(NM) $(call RUN_BINFILE,main) | $(CGREP) -v $(PANIC_SYMS) - -endif diff --git a/tests/run-make/fmt-write-bloat/rmake.rs b/tests/run-make/fmt-write-bloat/rmake.rs new file mode 100644 index 0000000000000..120c1cbccdd3c --- /dev/null +++ b/tests/run-make/fmt-write-bloat/rmake.rs @@ -0,0 +1,35 @@ +//! Before #78122, writing any `fmt::Arguments` would trigger the inclusion of `usize` formatting +//! and padding code in the resulting binary, because indexing used in `fmt::write` would generate +//! code using `panic_bounds_check`, which prints the index and length. +//! +//! These bounds checks are not necessary, as `fmt::Arguments` never contains any out-of-bounds +//! indexes. The test is a `run-make` test, because it needs to check the result after linking. A +//! codegen or assembly test doesn't check the parts that will be pulled in from `core` by the +//! linker. +//! +//! In this test, we try to check that the `usize` formatting and padding code are not present in +//! the final binary by checking that panic symbols such as `panic_bounds_check` are **not** +//! present. +//! +//! Some CI jobs try to run faster by disabling debug assertions (through setting +//! `NO_DEBUG_ASSERTIONS=1`). If debug assertions are disabled, then we can check for the absence of +//! additional `usize` formatting and padding related symbols. + +// Reason: This test is `ignore-windows` because the `no_std` test (using `#[link(name = "c")])` +// doesn't link on windows. +//@ ignore-windows +//@ ignore-cross-compile + +use run_make_support::{env::no_debug_assertions, rustc, symbols::any_symbol_contains}; + +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"]; + if no_debug_assertions() { + // 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"]); + } + assert!(!any_symbol_contains("main", &panic_syms)); +} From ebd6718218642b8b63584214e35f7d69d24ae845 Mon Sep 17 00:00:00 2001 From: binarycat Date: Tue, 30 Jul 2024 13:33:25 -0400 Subject: [PATCH 2/2] tidy this commit cannot easily be squashed, since there is already a PR based on the previous commit. --- src/tools/run-make-support/src/symbols.rs | 3 ++- tests/run-make/fmt-write-bloat/rmake.rs | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/tools/run-make-support/src/symbols.rs b/src/tools/run-make-support/src/symbols.rs index 52119ba4991a8..fd0c866bcc927 100644 --- a/src/tools/run-make-support/src/symbols.rs +++ b/src/tools/run-make-support/src/symbols.rs @@ -1,6 +1,7 @@ -use object::{self, Object, ObjectSymbol, SymbolIterator}; use std::path::Path; +use object::{self, Object, ObjectSymbol, SymbolIterator}; + /// Iterate through the symbols in an object file. /// /// Uses a callback because `SymbolIterator` does not own its data. diff --git a/tests/run-make/fmt-write-bloat/rmake.rs b/tests/run-make/fmt-write-bloat/rmake.rs index 120c1cbccdd3c..4ae226ec0e234 100644 --- a/tests/run-make/fmt-write-bloat/rmake.rs +++ b/tests/run-make/fmt-write-bloat/rmake.rs @@ -20,7 +20,9 @@ //@ ignore-windows //@ ignore-cross-compile -use run_make_support::{env::no_debug_assertions, rustc, symbols::any_symbol_contains}; +use run_make_support::env::no_debug_assertions; +use run_make_support::rustc; +use run_make_support::symbols::any_symbol_contains; fn main() { rustc().input("main.rs").opt().run();