Skip to content

Commit 5ed4f77

Browse files
committed
fix(avr): archive framework objects with gcc-ar (fixes #304)
avr_linker::link() previously iterated the `archives` parameter and passed each path as a raw positional argument to avr-gcc, regardless of whether it was .a or .o. Callers (pipeline::sequential::run_sequential _build_with_libs) pass raw .o files. As a result, framework Tone.cpp.o was always linked into AVR binaries that never called tone(), pulling in __vector_1 (90 B), __vector_3 (206 B) and noTone() (50 B) via weak-symbol override of __vector_default - inflating attiny85 Blink by 342 bytes (+10.1%) vs the equivalent PlatformIO build. Fix: partition the `archives` parameter into actual .a files (pass through) and raw .o files (archive into libframework.a using avr-gcc-ar). gcc-ar writes the LTO bytecode plugin index, preserving compatibility with the default -flto -fuse-linker-plugin profile from avr.json. Plumb gcc_ar_path through AvrLinker::new() from the toolchain's existing get_gcc_ar_path() - already used by pick_archiver in the orchestrator. Regression tests assert the partition helper splits a mixed .o/.a input correctly. The end-to-end size check landed against FastLED is intentionally deferred to FastLED's CI (re-enable --backend fbuild for binary-size checks once this fix ships, verify attiny85_binary_size stays under 9500). Closes #304.
1 parent 57ce6af commit 5ed4f77

2 files changed

Lines changed: 111 additions & 4 deletions

File tree

crates/fbuild-build/src/avr/avr_linker.rs

Lines changed: 107 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ use crate::linker::{LinkExtraArgs, Linker};
1515
pub struct AvrLinker {
1616
gcc_path: PathBuf,
1717
ar_path: PathBuf,
18+
gcc_ar_path: PathBuf,
1819
objcopy_path: PathBuf,
1920
size_path: PathBuf,
2021
mcu: String,
@@ -30,6 +31,7 @@ impl AvrLinker {
3031
pub fn new(
3132
gcc_path: PathBuf,
3233
ar_path: PathBuf,
34+
gcc_ar_path: PathBuf,
3335
objcopy_path: PathBuf,
3436
size_path: PathBuf,
3537
mcu: &str,
@@ -42,6 +44,7 @@ impl AvrLinker {
4244
Self {
4345
gcc_path,
4446
ar_path,
47+
gcc_ar_path,
4548
objcopy_path,
4649
size_path,
4750
mcu: mcu.to_string(),
@@ -54,6 +57,24 @@ impl AvrLinker {
5457
}
5558
}
5659

60+
/// Partition a mixed list of link inputs into (existing archives, raw objects).
61+
///
62+
/// Inputs ending in `.a` (case-sensitive — matches the toolchain convention) are
63+
/// real static archives and should be passed straight to the linker. Everything
64+
/// else (typically `.o` files coming from the framework compile step) is treated
65+
/// as a raw object that needs to be wrapped in an archive so the linker pulls in
66+
/// only members whose strong symbols are referenced.
67+
///
68+
/// Lives at module scope so unit tests can exercise the partition logic without
69+
/// invoking `avr-gcc-ar`.
70+
pub(crate) fn partition_link_inputs(inputs: &[PathBuf]) -> (Vec<PathBuf>, Vec<PathBuf>) {
71+
let (existing_archives, raw_objects): (Vec<PathBuf>, Vec<PathBuf>) = inputs
72+
.iter()
73+
.cloned()
74+
.partition(|p| p.extension().and_then(|e| e.to_str()) == Some("a"));
75+
(existing_archives, raw_objects)
76+
}
77+
5778
impl Linker for AvrLinker {
5879
fn archive(&self, objects: &[PathBuf], output: &Path) -> Result<()> {
5980
crate::linker::LinkerBase::archive(&self.ar_path, objects, output, "avr-ar")
@@ -90,9 +111,31 @@ impl Linker for AvrLinker {
90111
args.push(obj.to_string_lossy().to_string());
91112
}
92113

93-
// Core objects passed directly (not archived) for LTO compatibility
94-
// Python fbuild does the same: archive breaks LTO symbol visibility
95-
for archive in archives {
114+
// Partition: actual .a archives are pass-through; raw .o files get
115+
// archived into libframework.a so the linker only pulls in members
116+
// whose strong symbols are referenced. Without this, unused-but-not-
117+
// eliminable ISRs (e.g. Tone.cpp __vector_1/__vector_3 in the AVR
118+
// framework) are dragged in via weak-symbol override of
119+
// __vector_default, inflating the binary by ~10% on attiny85.
120+
// See FastLED/fbuild#304.
121+
//
122+
// Use gcc-ar (not plain ar) so the LTO bytecode plugin index is
123+
// written — preserves -fuse-linker-plugin compatibility, which the
124+
// default avr.json profile enables via `-flto -fuse-linker-plugin`.
125+
let (existing_archives, raw_objects) = partition_link_inputs(archives);
126+
127+
if !raw_objects.is_empty() {
128+
let framework_archive = output_dir.join("libframework.a");
129+
crate::linker::LinkerBase::archive(
130+
&self.gcc_ar_path,
131+
&raw_objects,
132+
&framework_archive,
133+
"avr-gcc-ar",
134+
)?;
135+
args.push(framework_archive.to_string_lossy().to_string());
136+
}
137+
138+
for archive in existing_archives {
96139
args.push(archive.to_string_lossy().to_string());
97140
}
98141

@@ -155,6 +198,7 @@ mod tests {
155198
let linker = AvrLinker::new(
156199
PathBuf::from("/bin/avr-gcc"),
157200
PathBuf::from("/bin/avr-ar"),
201+
PathBuf::from("/bin/avr-gcc-ar"),
158202
PathBuf::from("/bin/avr-objcopy"),
159203
PathBuf::from("/bin/avr-size"),
160204
"atmega328p",
@@ -167,5 +211,65 @@ mod tests {
167211
assert_eq!(linker.mcu, "atmega328p");
168212
assert_eq!(linker.max_flash, Some(32256));
169213
assert_eq!(linker.max_ram, Some(2048));
214+
assert_eq!(linker.gcc_ar_path, PathBuf::from("/bin/avr-gcc-ar"));
215+
}
216+
217+
#[test]
218+
fn link_partitions_archives_and_objects() {
219+
// Regression test for FastLED/fbuild#304: link() must split mixed
220+
// .a/.o inputs so .o files are archived (and thus subject to
221+
// unreferenced-member elimination by the linker) while real .a
222+
// files pass through unchanged.
223+
let inputs = vec![
224+
PathBuf::from("/tmp/Tone.cpp.o"),
225+
PathBuf::from("/tmp/libfoo.a"),
226+
PathBuf::from("/tmp/main.cpp.o"),
227+
];
228+
let (archives, objects) = partition_link_inputs(&inputs);
229+
assert_eq!(archives, vec![PathBuf::from("/tmp/libfoo.a")]);
230+
assert_eq!(
231+
objects,
232+
vec![
233+
PathBuf::from("/tmp/Tone.cpp.o"),
234+
PathBuf::from("/tmp/main.cpp.o"),
235+
]
236+
);
237+
}
238+
239+
#[test]
240+
fn partition_handles_all_archives() {
241+
let inputs = vec![PathBuf::from("/tmp/a.a"), PathBuf::from("/tmp/b.a")];
242+
let (archives, objects) = partition_link_inputs(&inputs);
243+
assert_eq!(archives.len(), 2);
244+
assert!(objects.is_empty());
245+
}
246+
247+
#[test]
248+
fn partition_handles_all_objects() {
249+
let inputs = vec![PathBuf::from("/tmp/x.o"), PathBuf::from("/tmp/y.o")];
250+
let (archives, objects) = partition_link_inputs(&inputs);
251+
assert!(archives.is_empty());
252+
assert_eq!(objects.len(), 2);
253+
}
254+
255+
#[test]
256+
fn partition_handles_empty() {
257+
let (archives, objects) = partition_link_inputs(&[]);
258+
assert!(archives.is_empty());
259+
assert!(objects.is_empty());
260+
}
261+
262+
#[test]
263+
fn partition_treats_unknown_extensions_as_objects() {
264+
// Defensive: anything that isn't .a (e.g. .lo, .obj, no extension)
265+
// is treated as an object and archived. This is safer than letting
266+
// an unknown extension fall through to a raw avr-gcc positional arg.
267+
let inputs = vec![
268+
PathBuf::from("/tmp/weird.lo"),
269+
PathBuf::from("/tmp/no_ext"),
270+
];
271+
let (archives, objects) = partition_link_inputs(&inputs);
272+
assert!(archives.is_empty());
273+
assert_eq!(objects.len(), 2);
170274
}
171275
}

crates/fbuild-build/src/avr/orchestrator.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,10 +237,13 @@ impl BuildOrchestrator for AvrOrchestrator {
237237
.with_build_unflags(ctx.build_unflags.clone())
238238
.with_eh_frame_policy(eh_frame_policy);
239239

240-
// 7. Create linker
240+
// 7. Create linker — pass gcc-ar so framework .o inputs can be
241+
// archived into libframework.a with the LTO bytecode plugin index
242+
// intact (preserves `-fuse-linker-plugin`). See FastLED/fbuild#304.
241243
let linker = AvrLinker::new(
242244
toolchain.get_gcc_path(),
243245
toolchain.get_ar_path(),
246+
toolchain.get_gcc_ar_path(),
244247
toolchain.get_objcopy_path(),
245248
toolchain.get_size_path(),
246249
&ctx.board.mcu,

0 commit comments

Comments
 (0)