Skip to content

Commit 11ea206

Browse files
committed
fix(symbols): rustfmt + clippy + dylint allowlist for #424
CI on PR #424 surfaced three style-gate failures that are easy to miss locally because the relevant tools each guard a different sub-policy: - `manual_pattern_char_comparison` / `manual_saturating_arithmetic` clippy lints — convert two `.rsplit(|c| c == '/' || c == '\')` closures to `.rsplit(['/', '\'])` and the `checked_add(...) .unwrap_or(u64::MAX)` overflow guard to `saturating_add(...)`. Same numerical behavior; CI's `-D warnings` posture rejected the longhand forms. - `cargo fmt` diff: minor formatting drift in the new `symbol_analysis.rs` test bodies that local `cargo fmt` had not applied at commit time. - Dylint `ban_raw_subprocess` rule: the c++filt-demangling driver in `crates/fbuild-build/src/symbol_analyzer.rs` spawns `std::process::Command` directly because the captured `run_command` helper is stdin-Null only. The pipe-buffer-deadlock workaround (separate writer thread + `wait_with_output`) requires raw spawn, so add the file to `dylints/ban_raw_subprocess/src/allowlist.txt` with the same documented-rationale shape used for the existing allowlist entries. No behavior change; tests still 11/11 green (9 in fbuild-core + 2 in fbuild-build).
1 parent b167ec4 commit 11ea206

5 files changed

Lines changed: 51 additions & 19 deletions

File tree

crates/fbuild-build/src/symbol_analyzer.rs

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -87,9 +87,10 @@ pub fn demangle_batch(mangled: &[String], cppfilt_path: &Path) -> Result<Vec<Str
8787

8888
// Move stdin into a writer thread so stdout draining can happen
8989
// concurrently in the main thread (avoids the pipe-buffer deadlock).
90-
let stdin_handle = child.stdin.take().ok_or_else(|| {
91-
FbuildError::BuildFailed("c++filt stdin pipe unavailable".to_string())
92-
})?;
90+
let stdin_handle = child
91+
.stdin
92+
.take()
93+
.ok_or_else(|| FbuildError::BuildFailed("c++filt stdin pipe unavailable".to_string()))?;
9394
let writer = std::thread::spawn(move || -> std::io::Result<()> {
9495
let mut stdin = stdin_handle;
9596
stdin.write_all(stdin_data.as_bytes())?;
@@ -194,7 +195,10 @@ pub fn analyze_elf(cfg: AnalyzeConfig<'_>) -> Result<FineGrainedSymbolMap> {
194195
/// archive + object + section attribution and demangled names.
195196
pub fn format_text_report(map: &FineGrainedSymbolMap, top_n: usize) -> String {
196197
let mut lines = Vec::new();
197-
lines.push(format!("=== Fine-grained symbol analysis: {} ===", map.elf_path));
198+
lines.push(format!(
199+
"=== Fine-grained symbol analysis: {} ===",
200+
map.elf_path
201+
));
198202
if let Some(ref m) = map.map_path {
199203
lines.push(format!("Map file: {m}"));
200204
}
@@ -225,7 +229,10 @@ pub fn format_text_report(map: &FineGrainedSymbolMap, top_n: usize) -> String {
225229
) {
226230
let mut syms: Vec<_> = map.symbols.iter().filter(|s| s.region == region).collect();
227231
syms.sort_by(|a, b| b.size.cmp(&a.size));
228-
lines.push(format!("--- Top {} {title} symbols ---", top_n.min(syms.len())));
232+
lines.push(format!(
233+
"--- Top {} {title} symbols ---",
234+
top_n.min(syms.len())
235+
));
229236
lines.push(format!(
230237
"{:>8} {:<24} {:<28} {:<14} symbol",
231238
"BYTES", "ARCHIVE", "OBJECT", "SECTION"
@@ -259,7 +266,13 @@ pub fn format_text_report(map: &FineGrainedSymbolMap, top_n: usize) -> String {
259266
top_n,
260267
"FLASH",
261268
);
262-
emit_region_block(&mut lines, map, fbuild_core::MemoryRegion::Ram, top_n, "RAM");
269+
emit_region_block(
270+
&mut lines,
271+
map,
272+
fbuild_core::MemoryRegion::Ram,
273+
top_n,
274+
"RAM",
275+
);
263276

264277
// Per-archive roll-ups (flash only — biggest leverage for bloat).
265278
let mut by_archive: std::collections::BTreeMap<String, u64> = std::collections::BTreeMap::new();
@@ -302,7 +315,9 @@ mod tests {
302315
let nm = PathBuf::from("/tools/xtensa-esp32s3-elf-nm.exe");
303316
let cppfilt = derive_cppfilt_path(&nm);
304317
assert!(
305-
cppfilt.to_string_lossy().ends_with("xtensa-esp32s3-elf-c++filt.exe"),
318+
cppfilt
319+
.to_string_lossy()
320+
.ends_with("xtensa-esp32s3-elf-c++filt.exe"),
306321
"got {}",
307322
cppfilt.display()
308323
);

crates/fbuild-cli/src/cli/symbols_cmd.rs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,9 @@ pub fn run_symbols(
4747
}
4848
});
4949

50-
let map_path_owned = map.map(PathBuf::from).or_else(|| default_map_path(&elf_path));
50+
let map_path_owned = map
51+
.map(PathBuf::from)
52+
.or_else(|| default_map_path(&elf_path));
5153
let map_path_ref: Option<&Path> = map_path_owned.as_deref();
5254

5355
let cfg = AnalyzeConfig {
@@ -85,8 +87,7 @@ pub fn run_symbols(
8587
/// Locate `nm` on PATH. The user can always override with `--nm`.
8688
fn find_nm_on_path() -> Result<PathBuf> {
8789
let exe_name = if cfg!(windows) { "nm.exe" } else { "nm" };
88-
let path = std::env::var_os("PATH")
89-
.ok_or_else(|| FbuildError::Other("PATH not set".into()))?;
90+
let path = std::env::var_os("PATH").ok_or_else(|| FbuildError::Other("PATH not set".into()))?;
9091
for dir in std::env::split_paths(&path) {
9192
let candidate = dir.join(exe_name);
9293
if candidate.exists() {

crates/fbuild-core/src/symbol_analysis.rs

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ pub fn extract_archive_and_object(src: &str) -> (Option<String>, String) {
135135
let archive_end = open + 2; // include ".a"
136136
let archive_path = &trimmed[..archive_end];
137137
let archive = archive_path
138-
.rsplit(|c| c == '/' || c == '\\')
138+
.rsplit(['/', '\\'])
139139
.next()
140140
.unwrap_or(archive_path)
141141
.to_string();
@@ -149,7 +149,7 @@ pub fn extract_archive_and_object(src: &str) -> (Option<String>, String) {
149149
}
150150
// Form B: ".../<file>.o" — no archive, just an object on disk.
151151
let basename = trimmed
152-
.rsplit(|c| c == '/' || c == '\\')
152+
.rsplit(['/', '\\'])
153153
.next()
154154
.unwrap_or(trimmed)
155155
.to_string();
@@ -310,7 +310,7 @@ impl InputSectionIndex {
310310
pub fn lookup(&self, addr: u64) -> Option<&InputSectionRange> {
311311
let (_, &idx) = self.by_start.range(..=addr).next_back()?;
312312
let r = &self.ranges[idx];
313-
if addr < r.addr.checked_add(r.size).unwrap_or(u64::MAX) {
313+
if addr < r.addr.saturating_add(r.size) {
314314
Some(r)
315315
} else {
316316
None
@@ -334,7 +334,11 @@ pub fn rollup_sections(ranges: &[InputSectionRange]) -> Vec<SectionBytes> {
334334
if !is_firmware_section(&r.output_section) {
335335
continue;
336336
}
337-
let key = (r.archive.clone(), r.object.clone(), r.output_section.clone());
337+
let key = (
338+
r.archive.clone(),
339+
r.object.clone(),
340+
r.output_section.clone(),
341+
);
338342
*acc.entry(key).or_insert(0) += r.size;
339343
}
340344
let mut out: Vec<SectionBytes> = acc
@@ -455,9 +459,7 @@ mod tests {
455459

456460
#[test]
457461
fn extract_bare_object_no_archive() {
458-
let (arc, obj) = extract_archive_and_object(
459-
".pio/build/esp32s3/src/main.cpp.o",
460-
);
462+
let (arc, obj) = extract_archive_and_object(".pio/build/esp32s3/src/main.cpp.o");
461463
assert!(arc.is_none());
462464
assert_eq!(obj, "main.cpp.o");
463465
}
@@ -557,7 +559,12 @@ Linker script and memory map
557559
archive: Some("libFastLED.a".into()),
558560
object: "fl.channels.cpp.o".into(),
559561
}];
560-
let nm = vec![(0x42000020u64, 0x40u64, 'T', "_ZN2fl7Channel10showPixelsERS_".to_string())];
562+
let nm = vec![(
563+
0x42000020u64,
564+
0x40u64,
565+
'T',
566+
"_ZN2fl7Channel10showPixelsERS_".to_string(),
567+
)];
561568
let demangled = vec!["fl::Channel::showPixels(fl::Channel&)".to_string()];
562569
let map = build_fine_grained_map(
563570
"fw.elf".into(),

dylints/ban_raw_subprocess/src/allowlist.txt

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,3 +45,12 @@ crates/fbuild-build/src/zccache.rs
4545
# `containment::tokio_spawn::spawn_contained` would be a no-op anyway;
4646
# documented inline at each call site.
4747
crates/fbuild-cli/src/cli/clang_tools.rs
48+
49+
# `fbuild symbols` demangles via c++filt. We pipe the mangled symbol
50+
# list through c++filt's stdin and drain stdout via `wait_with_output`
51+
# from a separate writer thread to avoid the Windows pipe-buffer
52+
# deadlock that hits when the symbol pool is large. The captured
53+
# `subprocess::run_command` helper is stdin-Null only, so raw spawn is
54+
# the only way to feed the pipe. Read-only analysis path (no
55+
# containment side effects).
56+
crates/fbuild-build/src/symbol_analyzer.rs

uv.lock

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)