From 84ae2ea28f944eddab158a13b0c42aaf69490cc4 Mon Sep 17 00:00:00 2001 From: Zachary Vorhies Date: Sat, 30 May 2026 13:13:18 -0700 Subject: [PATCH] fix(avr): archive framework objects with gcc-ar (fixes #304) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On AVR, an empty FastLED Blink sketch built with fbuild was ~10% larger than the same sketch built with PlatformIO. Symbol diff on attiny85 attributed the entire 342-byte delta to three symbols from `Tone.cpp` in the ATtinyCore framework (`__vector_3`, `__vector_1`, `noTone`) that PlatformIO drops as unreferenced archive members but that fbuild was pulling in because it linked framework `.o` files directly. The mechanism: `.o` files passed positionally to `avr-gcc` are always kept by the linker. Section-GC doesn't help because the ISRs override AVR libc's weak `__vector_default` and are reachable from the AVR interrupt vector table. `noTone` and its static state come along for the ride. Fix: partition link inputs into existing `.a` archives (pass-through) and raw `.o` objects, archive the raw objects into `libframework.a` with `avr-gcc-ar`, and feed only archives to the linker. Archive- member-selection drops members whose strong symbols are unreferenced before the vector table sees them — matches PlatformIO's behaviour. `gcc-ar` (not plain `ar`) is required because the default avr.json profile enables `-flto -fuse-linker-plugin`; `gcc-ar` writes the LTO bytecode plugin index so the linker can still resolve LTO objects. Patch shape: - `AvrLinker` gains a `gcc_ar_path: PathBuf` field; constructor takes it; orchestrator passes `toolchain.get_gcc_ar_path()` (already exposed alongside `get_ar_path` and already used by `pick_archiver`). - `partition_link_inputs` factored to module scope so the partition logic is unit-tested without invoking real `avr-gcc-ar`. - `link()` does the archive-then-link dance; `build_link_args` stays a pure argv builder. Tests: - `link_partitions_archives_and_objects` — mixed input partitioning - `partition_handles_all_archives`, `_all_objects`, `_empty`, `_treats_unknown_extensions_as_objects` - `test_avr_linker_creation` updated for new `gcc_ar_path` field Out of scope: same shape likely affects Teensy / RP2040 / STM32, but the symbol-diff evidence is AVR-only and broader changes need platform-by-platform fixture validation. Tracked separately if reproductions surface. Co-Authored-By: Claude Opus 4.7 --- crates/fbuild-build/src/avr/avr_linker.rs | 120 +++++++++++++++++++- crates/fbuild-build/src/avr/orchestrator.rs | 5 +- 2 files changed, 120 insertions(+), 5 deletions(-) diff --git a/crates/fbuild-build/src/avr/avr_linker.rs b/crates/fbuild-build/src/avr/avr_linker.rs index ad2a20d0..c227a4b9 100644 --- a/crates/fbuild-build/src/avr/avr_linker.rs +++ b/crates/fbuild-build/src/avr/avr_linker.rs @@ -15,6 +15,7 @@ use crate::linker::{LinkExtraArgs, Linker}; pub struct AvrLinker { gcc_path: PathBuf, ar_path: PathBuf, + gcc_ar_path: PathBuf, objcopy_path: PathBuf, size_path: PathBuf, mcu: String, @@ -30,6 +31,7 @@ impl AvrLinker { pub fn new( gcc_path: PathBuf, ar_path: PathBuf, + gcc_ar_path: PathBuf, objcopy_path: PathBuf, size_path: PathBuf, mcu: &str, @@ -42,6 +44,7 @@ impl AvrLinker { Self { gcc_path, ar_path, + gcc_ar_path, objcopy_path, size_path, mcu: mcu.to_string(), @@ -54,11 +57,32 @@ impl AvrLinker { } } +/// Partition a mixed list of link inputs into (existing archives, raw objects). +/// +/// Inputs ending in `.a` (case-sensitive — matches the toolchain convention) are +/// real static archives and should be passed straight to the linker. Everything +/// else (typically `.o` files coming from the framework compile step) is treated +/// as a raw object that needs to be wrapped in an archive so the linker pulls in +/// only members whose strong symbols are referenced. +/// +/// Lives at module scope so unit tests can exercise the partition logic without +/// invoking `avr-gcc-ar`. +pub(crate) fn partition_link_inputs(inputs: &[PathBuf]) -> (Vec, Vec) { + let (existing_archives, raw_objects): (Vec, Vec) = inputs + .iter() + .cloned() + .partition(|p| p.extension().and_then(|e| e.to_str()) == Some("a")); + (existing_archives, raw_objects) +} + impl AvrLinker { /// Build the argv that will be passed to `avr-gcc` for the link step. /// /// Factored out so it can be unit-tested without invoking the toolchain - /// (see #305 — assert that `-Wl,-Map=` is present). + /// (see #305 — assert that `-Wl,-Map=` is present). `archives` is the + /// already-partitioned archive list (real `.a` files + any framework + /// archive synthesised by `link()` for #304); this function does not + /// touch the filesystem. fn build_link_args( &self, objects: &[PathBuf], @@ -92,8 +116,11 @@ impl AvrLinker { args.push(obj.to_string_lossy().to_string()); } - // Core objects passed directly (not archived) for LTO compatibility - // Python fbuild does the same: archive breaks LTO symbol visibility + // Archives (already partitioned by `link()`): the framework archive + // synthesised from raw `.o` framework objects + any real `.a` files + // passed in. Archive-member selection drops unreferenced members so + // unused-but-not-eliminable ISRs (e.g. Tone.cpp __vector_1/3 on AVR) + // no longer get pulled in. See FastLED/fbuild#304. for archive in archives { args.push(archive.to_string_lossy().to_string()); } @@ -123,7 +150,33 @@ impl Linker for AvrLinker { std::fs::create_dir_all(output_dir)?; let elf_path = output_dir.join("firmware.elf"); - let args = self.build_link_args(objects, archives, output_dir, &elf_path, extra); + // Partition: real `.a` archives are pass-through; raw `.o` files get + // archived into `libframework.a` so the linker only pulls in members + // whose strong symbols are referenced. Without this, unused-but-not- + // eliminable ISRs (e.g. Tone.cpp __vector_1/__vector_3 in the AVR + // framework) are dragged in via weak-symbol override of + // __vector_default, inflating the binary by ~10% on attiny85. + // See FastLED/fbuild#304. + // + // Use gcc-ar (not plain ar) so the LTO bytecode plugin index is + // written — preserves -fuse-linker-plugin compatibility, which the + // default avr.json profile enables via `-flto -fuse-linker-plugin`. + let (existing_archives, raw_objects) = partition_link_inputs(archives); + let mut linker_archives: Vec = + Vec::with_capacity(existing_archives.len() + usize::from(!raw_objects.is_empty())); + if !raw_objects.is_empty() { + let framework_archive = output_dir.join("libframework.a"); + crate::linker::LinkerBase::archive( + &self.gcc_ar_path, + &raw_objects, + &framework_archive, + "avr-gcc-ar", + )?; + linker_archives.push(framework_archive); + } + linker_archives.extend(existing_archives); + + let args = self.build_link_args(objects, &linker_archives, output_dir, &elf_path, extra); if self.verbose { eprintln!("link: {}", args.join(" ")); @@ -191,6 +244,7 @@ mod tests { let linker = AvrLinker::new( PathBuf::from("/bin/avr-gcc"), PathBuf::from("/bin/avr-ar"), + PathBuf::from("/bin/avr-gcc-ar"), PathBuf::from("/bin/avr-objcopy"), PathBuf::from("/bin/avr-size"), "atmega328p", @@ -203,6 +257,63 @@ mod tests { assert_eq!(linker.mcu, "atmega328p"); assert_eq!(linker.max_flash, Some(32256)); assert_eq!(linker.max_ram, Some(2048)); + assert_eq!(linker.gcc_ar_path, PathBuf::from("/bin/avr-gcc-ar")); + } + + #[test] + fn link_partitions_archives_and_objects() { + // Regression test for FastLED/fbuild#304: link() must split mixed + // .a/.o inputs so .o files are archived (and thus subject to + // unreferenced-member elimination by the linker) while real .a + // files pass through unchanged. + let inputs = vec![ + PathBuf::from("/tmp/Tone.cpp.o"), + PathBuf::from("/tmp/libfoo.a"), + PathBuf::from("/tmp/main.cpp.o"), + ]; + let (archives, objects) = partition_link_inputs(&inputs); + assert_eq!(archives, vec![PathBuf::from("/tmp/libfoo.a")]); + assert_eq!( + objects, + vec![ + PathBuf::from("/tmp/Tone.cpp.o"), + PathBuf::from("/tmp/main.cpp.o"), + ] + ); + } + + #[test] + fn partition_handles_all_archives() { + let inputs = vec![PathBuf::from("/tmp/a.a"), PathBuf::from("/tmp/b.a")]; + let (archives, objects) = partition_link_inputs(&inputs); + assert_eq!(archives.len(), 2); + assert!(objects.is_empty()); + } + + #[test] + fn partition_handles_all_objects() { + let inputs = vec![PathBuf::from("/tmp/x.o"), PathBuf::from("/tmp/y.o")]; + let (archives, objects) = partition_link_inputs(&inputs); + assert!(archives.is_empty()); + assert_eq!(objects.len(), 2); + } + + #[test] + fn partition_handles_empty() { + let (archives, objects) = partition_link_inputs(&[]); + assert!(archives.is_empty()); + assert!(objects.is_empty()); + } + + #[test] + fn partition_treats_unknown_extensions_as_objects() { + // Defensive: anything that isn't .a (e.g. .lo, .obj, no extension) + // is treated as an object and archived. This is safer than letting + // an unknown extension fall through to a raw avr-gcc positional arg. + let inputs = vec![PathBuf::from("/tmp/weird.lo"), PathBuf::from("/tmp/no_ext")]; + let (archives, objects) = partition_link_inputs(&inputs); + assert!(archives.is_empty()); + assert_eq!(objects.len(), 2); } /// #305: every per-platform linker must emit a `firmware.map` next to @@ -212,6 +323,7 @@ mod tests { let linker = AvrLinker::new( PathBuf::from("/bin/avr-gcc"), PathBuf::from("/bin/avr-ar"), + PathBuf::from("/bin/avr-gcc-ar"), PathBuf::from("/bin/avr-objcopy"), PathBuf::from("/bin/avr-size"), "atmega328p", diff --git a/crates/fbuild-build/src/avr/orchestrator.rs b/crates/fbuild-build/src/avr/orchestrator.rs index 43b62fb4..f6ad77bf 100644 --- a/crates/fbuild-build/src/avr/orchestrator.rs +++ b/crates/fbuild-build/src/avr/orchestrator.rs @@ -237,10 +237,13 @@ impl BuildOrchestrator for AvrOrchestrator { .with_build_unflags(ctx.build_unflags.clone()) .with_eh_frame_policy(eh_frame_policy); - // 7. Create linker + // 7. Create linker — pass gcc-ar so framework .o inputs can be + // archived into libframework.a with the LTO bytecode plugin index + // intact (preserves `-fuse-linker-plugin`). See FastLED/fbuild#304. let linker = AvrLinker::new( toolchain.get_gcc_path(), toolchain.get_ar_path(), + toolchain.get_gcc_ar_path(), toolchain.get_objcopy_path(), toolchain.get_size_path(), &ctx.board.mcu,