fix(avr): archive framework objects with gcc-ar (fixes #304) by zackees · Pull Request #306 · FastLED/fbuild

zackees · 2026-05-30T17:56:04Z

Summary

Fixes #304 - fbuild was producing 10.1% larger AVR binaries than PlatformIO because avr_linker::link() linked framework .o files directly instead of archiving them. Unused-but-link-reachable ISRs (Tone.cpp::__vector_1/__vector_3) and helpers (noTone) were being dragged into binaries that never called tone().

Patch shape

Added gcc_ar_path field to AvrLinker, plumbed through new() (test fixture updated).
Orchestrator passes toolchain.get_gcc_ar_path() (already available alongside get_ar_path and already used by pick_archiver).
In link(), the archives parameter is now partitioned into real .a files (pass-through) and .o files (archived into libframework.a via gcc-ar). gcc-ar is the right tool because the default avr.json profile enables -flto -fuse-linker-plugin, and gcc-ar writes the LTO bytecode plugin index that plain ar does not.
Partition logic factored into a pub(crate) fn partition_link_inputs so it can be unit-tested without invoking real avr-gcc-ar.

Verification

soldr cargo check -p fbuild-build - clean.
soldr cargo test -p fbuild-build --lib avr_linker - 6 passed / 0 failed (5 new regression tests + updated test_avr_linker_creation fixture covering the new gcc_ar_path field).
soldr cargo test -p fbuild-build --test avr_build - 1 passed / 4 ignored / 0 failed (the 4 ignored tests require avr-gcc toolchain and were already ignored on main).
soldr cargo clippy -p fbuild-build --all-targets -- -D warnings - clean (no new warnings introduced).

End-to-end size verification (attiny85 Blink dropping from 3728 -> 3386 bytes) is intentionally deferred to FastLED CI once they re-enable --backend fbuild for binary-size checks.

Out of scope

Same issue likely affects Teensy / RP2040 / STM32 paths through pipeline/sequential.rs. This PR is AVR-only because the symbol-diff evidence is AVR-only and a broader change needs platform-by-platform fixture validation. Track separately if reproductions surface.

Summary by CodeRabbit

Improvements
- Linker now separates pre-built static archives from object inputs and creates a generated archive (using the appropriate archiver) so unreferenced members can be eliminated; build orchestration now supplies the archiver path to the linker.
Tests
- Expanded tests covering input partitioning: mixed archives/objects, all-archives, all-objects, empty inputs, and unknown extensions.

coderabbitai · 2026-05-30T17:56:14Z

Warning

Review limit reached

@zackees, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 44 minutes and 32 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0aeb04a0-1dd7-413f-9d32-3d9b9d9ad791

📥 Commits

Reviewing files that changed from the base of the PR and between 75fcd8d and 84ae2ea.

📒 Files selected for processing (2)

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

📝 Walkthrough

Walkthrough

AvrLinker now accepts an avr-gcc-ar path and partitions link inputs: static archives (.a) pass through unchanged, while non-archive objects are archived into libframework.a using avr-gcc-ar before forwarding all archives to the linker. The orchestrator wires the toolchain's gcc-ar path into the linker constructor, and tests validate partitioning across mixed archives and objects.

Changes

AVR linker framework archiving

Layer / File(s)	Summary
AvrLinker struct, constructor, and partition helper `crates/fbuild-build/src/avr/avr_linker.rs`	`AvrLinker` gains `gcc_ar_path: PathBuf` field; `AvrLinker::new()` constructor signature updated to accept and initialize the new path; `partition_link_inputs` helper function splits input paths into existing `.a` archives vs. non-`.a` objects.
Linker archive and partition logic with tests `crates/fbuild-build/src/avr/avr_linker.rs`	`link()` method uses `partition_link_inputs` to separate inputs, archives non-archive objects via `avr-gcc-ar` into `libframework.a`, and forwards the resulting archive alongside existing `.a` archives to the linker. Constructor instantiation test updated to assert `gcc_ar_path`; new test suite covers mixed `.a`/object inputs, all-archive, all-object, empty input, and unknown-extension scenarios.
Orchestrator gcc-ar path wiring `crates/fbuild-build/src/avr/orchestrator.rs`	AVR build orchestrator passes `toolchain.get_gcc_ar_path()` to `AvrLinker::new()` constructor alongside existing toolchain paths; comment clarifies that `gcc-ar` preserves linker-plugin behavior for framework object archiving.

Sequence Diagram

sequenceDiagram
  participant Orchestrator
  participant AvrLinker
  participant GccAr as avr-gcc-ar
  participant Linker as avr-gcc (linker)
  Orchestrator->>AvrLinker: new(gcc_path, ar_path, gcc_ar_path, objcopy_path)
  Orchestrator->>AvrLinker: link(inputs)
  AvrLinker->>AvrLinker: partition_link_inputs(inputs)
  AvrLinker->>GccAr: rcs libframework.a [non-.a objects]
  GccAr->>AvrLinker: libframework.a
  AvrLinker->>Linker: libframework.a + existing .a archives
  Linker->>Linker: archive member selection + gc-sections
  Linker->>AvrLinker: firmware.elf (unreferenced symbols dropped)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 The framework objects pack tight,
In archives small and light,
gcc-ar selects with care,
Dead code vanishes in air,
Lean binaries take flight! 🎯

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(avr): archive framework objects with gcc-ar (fixes `#304`)' clearly summarizes the main change: archiving AVR framework objects using gcc-ar to resolve issue `#304`.
Linked Issues check	✅ Passed	The PR fully implements coding requirements from issue `#304`: archives framework objects using gcc-ar, adds gcc_ar_path to AvrLinker, partitions inputs to separate archives from objects, and includes comprehensive unit tests for the new behavior.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to issue `#304`'s AVR-only requirements; no modifications to Teensy, RP2040, or STM32 platforms were made, consistent with the stated deferral in the PR objectives.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/304-archive-framework-objects-avr

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/fbuild-build/src/avr/avr_linker.rs`:
- Around line 262-274: The test partition_treats_unknown_extensions_as_objects
is formatted across multiple lines causing cargo fmt --check to fail; edit the
test so the inputs Vec is on a single line (e.g. replace the multi-line let
inputs = vec![ PathBuf::from("/tmp/weird.lo"), PathBuf::from("/tmp/no_ext"), ];)
while keeping the same values and trailing comma style to satisfy rustfmt and
preserve partition_link_inputs usage.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: df126307-5f37-48a4-ab4e-f1edf61ad91d

📥 Commits

Reviewing files that changed from the base of the PR and between 57ce6af and 5ed4f77.

📒 Files selected for processing (2)

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

rustfmt wants the two-element vec! on one line. Matches the style of the other partition tests in the same module (single-line vec! when the elements fit). Addresses CodeRabbit review feedback on #306. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

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 <noreply@anthropic.com>

…pawn lint (#317) After f86817a (refactor lib.rs → submodules) the four std::process::Command::new sites in fbuild-python/daemon.rs ended up inside `match` arms two lines below their `allow-direct-spawn:` comment. The detector in ci/find_direct_subprocess.py only looks at "same line or the line immediately above", so the annotation no longer covered the actual call sites and CI started reporting: NEW direct spawns without an `allow-direct-spawn: <reason>` marker: crates/fbuild-python/src/daemon.rs:96 crates/fbuild-python/src/daemon.rs:97 crates/fbuild-python/src/daemon.rs:247 crates/fbuild-python/src/daemon.rs:248 This blocks every PR's "Lint subprocess spawns" check (e.g. #306 for fbuild#304, which is the actual FastLED-vs-PIO size regression we need to land). Fix: add an inline `// allow-direct-spawn:` annotation immediately above each of the four arms. Behavior unchanged.

…lib (clears 27/39 drift) (#318) * fix(lint): annotate Python daemon Command::new sites for subprocess-spawn lint After f86817a (refactor lib.rs → submodules) the four std::process::Command::new sites in fbuild-python/daemon.rs ended up inside `match` arms two lines below their `allow-direct-spawn:` comment. The detector in ci/find_direct_subprocess.py only looks at "same line or the line immediately above", so the annotation no longer covered the actual call sites and CI started reporting: NEW direct spawns without an `allow-direct-spawn: <reason>` marker: crates/fbuild-python/src/daemon.rs:96 crates/fbuild-python/src/daemon.rs:97 crates/fbuild-python/src/daemon.rs:247 crates/fbuild-python/src/daemon.rs:248 This blocks every PR's "Lint subprocess spawns" check (e.g. #306 for fbuild#304, which is the actual FastLED-vs-PIO size regression we need to land). Fix: add an inline `// allow-direct-spawn:` annotation immediately above each of the four arms. Behavior unchanged. * fix(boards): catch up tinyuf2 partition rename + esp_sr drift; whitelist cmsis_dsp_lib `Validate Board Definitions` workflow has been failing 39 boards drifted from PlatformIO. The two largest categories are pure schema drift that this PR resolves; the remaining ~14 are configuration- choice differences that need maintainer review. ## Fixed: tinyuf2 partition CSV rename (18 boards) Upstream Adafruit (and the espressif32 platform's Adafruit board defs) renamed `tinyuf2-partitions-<size>.csv` -> `partitions-<size>-tinyuf2.csv`. Our bundled board JSONs still pointed to the old name. Renamed in: adafruit_feather_esp32s2{,_reversetft,_tft} adafruit_feather_esp32s3{,_nopsram,_reversetft,_tft} adafruit_funhouse_esp32s2 adafruit_magtag29_esp32s2 adafruit_matrixportal_esp32s3 adafruit_metro_esp32s2 adafruit_metro_esp32s3 adafruit_qtpy_esp32s2 adafruit_qtpy_esp32s3_n4r2 adafruit_qtpy_esp32s3_nopsram adafruit_qualia_s3_rgb666 featheresp32-s2 wt32-sc01-plus All swaps are mechanical s/tinyuf2-partitions-<N>MB.csv/partitions-<N>MB-tinyuf2.csv/. ## Fixed: 4d_systems_esp32s3_gen4_r8n16 partitions `esp_sr_16.csv` -> `default_16MB.csv` per upstream. ## Fixed: validator whitelist for cmsis_dsp_lib (8 teensy boards) `build.cmsis_dsp_lib` was added by fbuild#300 (PR#313) as an intentional fbuild-only schema extension — Teensyduino's makefile picks the matching `arm_cortexM*_math` archive at link time, but fbuild needs it declared statically. The validator was flagging all 8 teensy variants because the diff treats every extra key in the actual asset as drift. Added `FBUILD_EXTENSION_BUILD_FIELDS` whitelist (currently just `cmsis_dsp_lib`) and strip those from the actual side before diffing. Documented in-place so future fbuild-only extensions have a place to land with a one-line note. ## Not in scope Remaining ~14 boards have configuration-choice drift (UM boards `-DARDUINO_USB_MODE=1` difference, m5stack variant rename, seeed_ xiao_esp32c6 USB defines, arduino_nano_esp32 BOARD_USES_HW_GPIO_NUMBERS vs BOARD_HAS_PIN_REMAP, um_tinys2 extras). Each requires a real decision about which value is correct for fbuild's compile path — not auto-resolvable by a snapshot rebase. Leaving those for a separate maintainer-driven sweep.

…320) * fix(lint): annotate Python daemon Command::new sites for subprocess-spawn lint After f86817a (refactor lib.rs → submodules) the four std::process::Command::new sites in fbuild-python/daemon.rs ended up inside `match` arms two lines below their `allow-direct-spawn:` comment. The detector in ci/find_direct_subprocess.py only looks at "same line or the line immediately above", so the annotation no longer covered the actual call sites and CI started reporting: NEW direct spawns without an `allow-direct-spawn: <reason>` marker: crates/fbuild-python/src/daemon.rs:96 crates/fbuild-python/src/daemon.rs:97 crates/fbuild-python/src/daemon.rs:247 crates/fbuild-python/src/daemon.rs:248 This blocks every PR's "Lint subprocess spawns" check (e.g. #306 for fbuild#304, which is the actual FastLED-vs-PIO size regression we need to land). Fix: add an inline `// allow-direct-spawn:` annotation immediately above each of the four arms. Behavior unchanged. * fix(boards): catch up tinyuf2 partition rename + esp_sr drift; whitelist cmsis_dsp_lib `Validate Board Definitions` workflow has been failing 39 boards drifted from PlatformIO. The two largest categories are pure schema drift that this PR resolves; the remaining ~14 are configuration- choice differences that need maintainer review. ## Fixed: tinyuf2 partition CSV rename (18 boards) Upstream Adafruit (and the espressif32 platform's Adafruit board defs) renamed `tinyuf2-partitions-<size>.csv` -> `partitions-<size>-tinyuf2.csv`. Our bundled board JSONs still pointed to the old name. Renamed in: adafruit_feather_esp32s2{,_reversetft,_tft} adafruit_feather_esp32s3{,_nopsram,_reversetft,_tft} adafruit_funhouse_esp32s2 adafruit_magtag29_esp32s2 adafruit_matrixportal_esp32s3 adafruit_metro_esp32s2 adafruit_metro_esp32s3 adafruit_qtpy_esp32s2 adafruit_qtpy_esp32s3_n4r2 adafruit_qtpy_esp32s3_nopsram adafruit_qualia_s3_rgb666 featheresp32-s2 wt32-sc01-plus All swaps are mechanical s/tinyuf2-partitions-<N>MB.csv/partitions-<N>MB-tinyuf2.csv/. ## Fixed: 4d_systems_esp32s3_gen4_r8n16 partitions `esp_sr_16.csv` -> `default_16MB.csv` per upstream. ## Fixed: validator whitelist for cmsis_dsp_lib (8 teensy boards) `build.cmsis_dsp_lib` was added by fbuild#300 (PR#313) as an intentional fbuild-only schema extension — Teensyduino's makefile picks the matching `arm_cortexM*_math` archive at link time, but fbuild needs it declared statically. The validator was flagging all 8 teensy variants because the diff treats every extra key in the actual asset as drift. Added `FBUILD_EXTENSION_BUILD_FIELDS` whitelist (currently just `cmsis_dsp_lib`) and strip those from the actual side before diffing. Documented in-place so future fbuild-only extensions have a place to land with a one-line note. ## Not in scope Remaining ~14 boards have configuration-choice drift (UM boards `-DARDUINO_USB_MODE=1` difference, m5stack variant rename, seeed_ xiao_esp32c6 USB defines, arduino_nano_esp32 BOARD_USES_HW_GPIO_NUMBERS vs BOARD_HAS_PIN_REMAP, um_tinys2 extras). Each requires a real decision about which value is correct for fbuild's compile path — not auto-resolvable by a snapshot rebase. Leaving those for a separate maintainer-driven sweep. * fix(samd): fall back to cores/arduino when build.core dir is missing (#319) Every `Build SAMD51J` workflow run on main fails at: fatal error: WVariant.h: No such file or directory variants/feather_m4/variant.h:43:10 43 | #include "WVariant.h" Root cause: Adafruit's ArduinoCore-samd v1.7.16 (the SAMD framework fbuild installs) ships only `cores/arduino/` — verified via GitHub API. Every Adafruit SAMD board declares `build.core = "adafruit"` in its board JSON (matching PlatformIO upstream, so the board-validator is happy). fbuild's `SamdCores::get_core_dir(core_name)` literally joined `cores/<core_name>`, so it asked for `cores/adafruit/`, which doesn't exist. The variant compile's include-path then pointed at a phantom directory and `WVariant.h` (which lives in `cores/arduino/`) was unreachable. PlatformIO's atmelsam builder handles this by treating `build.core` as a vendor brand label rather than a literal subdirectory name. Mirror that behavior: if `cores/<core_name>/` doesn't exist, fall back to `cores/arduino/` (the canonical Arduino-compatible cores directory name). Refactored the fallback into a small `resolve_core_dir_with_arduino_ fallback` helper that's straightforwardly unit-testable. Three new tests cover the three branches (literal name wins, fallback when named dir absent, return literal when both absent so the eventual file-open error has a meaningful path). Local test results: 9/9 samd_core tests pass, 412/412 fbuild-packages test suite passes. Fixes #319. * fix(samd): inject resolved core_dir into compile include path (#319 follow-up) The get_core_dir fallback in the previous commit is necessary but not sufficient. `BoardConfig::get_include_paths(framework_root)` builds the compile-time include list using the LITERAL `build.core` value from the board JSON — for Adafruit SAMD boards that's "adafruit", which produces a `-I.../cores/adafruit` path that doesn't exist. The variant compile's `#include "WVariant.h"` then misses because: - same-dir lookup (variants/feather_m4/) — not there - `-I.../cores/adafruit` (phantom) — directory doesn't exist - other -I entries (system includes, variant dir) — no WVariant.h `install_samd_core` already has a resolved `core_dir` (from `SamdCores::get_core_dir`, which falls back to cores/arduino/ per the previous commit). Inject it into the system_includes vector returned by `install_samd_core` so the orchestrator's eventual compile sees `-I.../cores/arduino` and the headers resolve. Inserted at position 0 to make the active core dir the first search hit, matching what PlatformIO's atmelsam builder does. Refs #319.

coderabbitai Bot requested changes May 30, 2026

View reviewed changes

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

zackees mentioned this pull request May 30, 2026

fix(teensy): derive map_path from elf_path (unbreak main CI) #316

Merged

3 tasks

coderabbitai Bot approved these changes May 30, 2026

View reviewed changes

zackees force-pushed the fix/304-archive-framework-objects-avr branch from 75fcd8d to 088b17a Compare May 30, 2026 20:20

zackees force-pushed the fix/304-archive-framework-objects-avr branch from 088b17a to 84ae2ea Compare May 30, 2026 20:22

zackees mentioned this pull request May 30, 2026

fix(lint): annotate Python daemon Command::new sites #317

Merged

zackees merged commit b7560aa into main May 30, 2026
75 of 82 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(avr): archive framework objects with gcc-ar (fixes #304)#306

fix(avr): archive framework objects with gcc-ar (fixes #304)#306
zackees merged 1 commit into
mainfrom
fix/304-archive-framework-objects-avr

zackees commented May 30, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 30, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Sequence Diagram

Estimated code review effort

Poem

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

zackees commented May 30, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Patch shape

Verification

Out of scope

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Sequence Diagram

Estimated code review effort

Poem

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

zackees commented May 30, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 30, 2026 •

edited

Loading