Uh oh!
There was an error while loading. Please reload this page.
feat(build): #243 #244 strip .eh_frame on release builds across all GCC platforms - #245
Merged
Merged
Conversation
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (16)
📝 WalkthroughWalkthroughThis PR implements an automatic ChangesEhFrame policy and cross-platform implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/orchestrator.rs`:
- Around line 93-97: The fast-path fingerprint metadata currently omits
eh_frame_policy so builds can be reused when
FBUILD_KEEP_EH_FRAME/FBUILD_STRIP_EH_FRAME change; update the metadata
fingerprint construction to include the local eh_frame_policy value wherever
metadata_hash or the fast-path fingerprint is computed (references: the
eh_frame_policy variable, metadata_hash/fingerprint construction code used in
this file and the other indicated blocks around lines 130-146 and 232-233),
i.e., add eh_frame_policy into the set of inputs hashed for the
metadata/fingerprint so changes to the policy invalidate cached artifacts.
In `@crates/fbuild-build/src/esp32/orchestrator.rs`:
- Around line 166-175: The fingerprint metadata for ESP32 currently omits the
computed eh_frame_policy, so add the value produced by
crate::eh_frame_policy_compute::compute_eh_frame_policy (the eh_frame_policy
variable computed from
fbuild_config::sdkconfig::SdkConfigSummary::from_project_dir) into the
Esp32FingerprintMetadata inputs used for hashing/ cache key; update the code
that constructs Esp32FingerprintMetadata (the struct/constructor where
platform-specific hash inputs are collected) to include eh_frame_policy so cache
hits respect strip/preserve mode changes.
In `@crates/fbuild-build/src/rp2040/orchestrator.rs`:
- Around line 72-75: The eh_frame_policy computed by
compute_eh_frame_policy(&ctx, params.profile, None) is not being encoded into
the fast-path fingerprint, so builds can be reused across different EH frame
strip/keep settings; update the fingerprint construction logic (the code that
builds the fast-path fingerprint/key in orchestrator.rs) to include the
eh_frame_policy value (use the eh_frame_policy variable) as part of the
fingerprint metadata so changes to FBUILD_STRIP_EH_FRAME/FBUILD_KEEP_EH_FRAME
produce distinct fingerprints and avoid stale artifact reuse.
In `@crates/fbuild-build/src/teensy/orchestrator.rs`:
- Around line 76-80: The no-op fingerprint currently misses the resolved
eh_frame_policy computed by compute_eh_frame_policy(&ctx, params.profile, None),
so include the eh_frame_policy value into the fingerprint metadata used for
cache/no-op decisions; locate where the no-op fingerprint is assembled and add
eh_frame_policy (or a serialization of it) as an input field so changes to
FBUILD_KEEP_EH_FRAME / FBUILD_STRIP_EH_FRAME (and the computed policy)
invalidate the cache.
In `@crates/fbuild-build/tests/eh_frame_strip_esp32.rs`:
- Around line 102-106: The test must require an explicit ELF path instead of
falling back to firmware_path: replace the chained
.or(preserve_result.firmware_path.clone()) usage when building preserve_elf so
it calls preserve_result.elf_path.clone().expect("preserve build should produce
ELF path") (and do the same change for the similar block around the second
occurrence that currently uses firmware_path fallback) so parsing .eh_frame
always operates on an actual ELF file.
🪄 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: 7c2aa215-0835-46ae-af98-e881234454a6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
crates/fbuild-build/Cargo.tomlcrates/fbuild-build/src/apollo3/orchestrator.rscrates/fbuild-build/src/avr/avr_compiler.rscrates/fbuild-build/src/avr/orchestrator.rscrates/fbuild-build/src/eh_frame_policy.rscrates/fbuild-build/src/eh_frame_policy_compute.rscrates/fbuild-build/src/esp32/esp32_compiler.rscrates/fbuild-build/src/esp32/orchestrator.rscrates/fbuild-build/src/esp8266/esp8266_compiler.rscrates/fbuild-build/src/esp8266/orchestrator.rscrates/fbuild-build/src/generic_arm/arm_compiler.rscrates/fbuild-build/src/lib.rscrates/fbuild-build/src/rp2040/orchestrator.rscrates/fbuild-build/src/stm32/orchestrator.rscrates/fbuild-build/src/teensy/orchestrator.rscrates/fbuild-build/src/teensy/teensy_compiler.rscrates/fbuild-build/tests/eh_frame_strip_esp32.rscrates/fbuild-config/src/lib.rscrates/fbuild-config/src/sdkconfig.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…CC platforms GCC emits .eh_frame unwinding tables by default. On a stock FastLED ESP32-S3 Blink build this is ~225 KB / 36% of the firmware -- dead metadata no _Unwind_* API ever consumes when exceptions and panic backtrace are both off. FastLED's library-side pragma workaround (FastLED/FastLED#2423) is a no-op on modern GCC (audited in FastLED/FastLED#2473); the only reliable fix is cc1-level flags, which is the build system's job. This PR adds a shared eh_frame strip policy that injects `-fno-asynchronous-unwind-tables -fno-unwind-tables` to every TU when appropriate. The decision respects user intent at every layer: explicit env override, debug builds, `-fexceptions` in build_flags, ESP-IDF panic-backtrace / gdbstub / ocdaware / optimization-debug sdkconfig keys. New modules - `crates/fbuild-build/src/eh_frame_policy.rs` -- pure `decide()` function over plain inputs, plus `STRIP_FLAGS` constant. 16 unit tests cover the full precedence matrix. - `crates/fbuild-build/src/eh_frame_policy_compute.rs` -- orchestrator- side helper that reads env vars + extracts BuildContext fields and calls `decide()`. - `crates/fbuild-config/src/sdkconfig.rs` -- minimal sdkconfig parser surfacing the 4 keys the policy reads. Defaults to ESP-IDF Arduino defaults (`panic_print_backtrace = true`) so the safe assumption for an unmodified project is Preserve. 14 unit tests. Per-platform wiring - ESP32, generic ARM (STM32/RP2040/NRF52), Teensy, ESP8266, AVR each get a `with_eh_frame_policy()` setter and a 3-line append in `common_flags()`. Mirrors the existing `with_build_unflags` pattern. - All orchestrators (esp32, generic_arm, teensy, esp8266, avr, stm32, rp2040, apollo3) compute the policy once per build and pass it to each compiler construction site. ESP32 also reads sdkconfig from the project dir. Other platforms pass `None`. - 10 new platform unit tests (2 per platform) assert STRIP_FLAGS presence under `Strip` and absence under default `Preserve`. Integration test (`#[ignore]`, toolchain-required) - `crates/fbuild-build/tests/eh_frame_strip_esp32.rs` builds a tempfile ESP32 project twice (FBUILD_KEEP_EH_FRAME=1 vs FBUILD_STRIP_EH_FRAME=1) and asserts firmware.bin delta >= 150 KB plus `.eh_frame` ELF section delta >= 50 KB via the `object` crate. User-facing overrides - `FBUILD_STRIP_EH_FRAME=1` -- force strip regardless of detection - `FBUILD_KEEP_EH_FRAME=1` -- force preserve Behavior under defaults - ESP32 with stock Arduino framework: Preserve (matches user expectation of esp32_exception_decoder showing readable backtraces). - Teensy / STM32 / RP2040 / NRF52 / ESP8266: Strip (their JSON configs already ship `-fno-exceptions`; nothing in their toolchain consumes eh_frame). Immediate flash savings on every release build. - AVR: Strip wired for uniformity; runtime is a no-op because AVR-gcc only emits eh_frame when `-fexceptions` is on, which AVR's JSON disables. - Debug builds (`build_type = debug`, or `-Og`/`-O0` flags) always Preserve, even on platforms that would otherwise strip. Test coverage - 16 eh_frame_policy unit tests (decision matrix) - 14 sdkconfig unit tests (parser + project-dir lookup) - 10 per-platform compiler tests (STRIP_FLAGS presence/absence) - 1 integration test (`#[ignore]`, runs locally with ESP32 toolchain) - Existing 534 fbuild-build + 120 fbuild-config lib tests stay green. Closes#243. Closes#244. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zackeesforce-pushed
the
feat/issue-243-244-eh-frame-strip
branch
from
May 13, 2026 00:54
42e0459 to
6ba0003Comparezackees
commented
May 13, 2026
MemberAuthor
CodeRabbit review folded in (squashed force-push to 6ba0003):
Verification: workspace |
Uh oh!
There was an error while loading. Please reload this page.
4 tasks
zackees added a commit
that referenced
this pull request
May 13, 2026
…atformIO) (#247) Add a new "PlatformIO compatibility: .eh_frame strip" section to README that explains why fbuild's release binaries on Teensy/STM32/RP2040/NRF52/ ESP8266 are smaller than PlatformIO's, summarizes the EhFramePolicy precedence introduced in #245, and shows the two opt-out paths (build_flags = -funwind-tables in platformio.ini, FBUILD_KEEP_EH_FRAME=1 env var). Also adds a one-line forward reference near the existing "platformio.ini compatible" tagline so users notice the deviation. Closes#246. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#243.
Closes#244.
Summary
GCC emits
.eh_frameunwinding tables by default. On a stock FastLED ESP32-S3 Blink build this is ~225 KB / 36 % of the firmware — dead metadata no_Unwind_*API ever consumes when exceptions and panic backtrace are both off (audit: FastLED/FastLED#2473). FastLED's library-side pragma workaround (PR #2423) is a no-op on modern GCC; the only reliable fix iscc1-level flags, which is the build system's job.This PR adds a shared eh_frame strip policy that injects
-fno-asynchronous-unwind-tables -fno-unwind-tablesto every TU when appropriate. The decision respects user intent at every layer.Architecture
crates/fbuild-build/src/eh_frame_policy.rsdecide(&EhFrameInputs) -> EhFramePolicy. Pure function, 16 unit tests covering the precedence matrix.crates/fbuild-config/src/sdkconfig.rscrates/fbuild-build/src/eh_frame_policy_compute.rsdecide().*_compiler.rsfiles (esp32, generic_arm, teensy, esp8266, avr)with_eh_frame_policy()setter + 3-line append incommon_flags(). Mirrors existingwith_build_unflagspattern.Decision precedence
FBUILD_STRIP_EH_FRAME=1→ StripFBUILD_KEEP_EH_FRAME=1→ Preservebuild_type = debug(or-Og/-O0in build_flags) → Preserve-fexceptions/-funwind-tables/-fasynchronous-unwind-tablesin build_flags (not negated) → Preservepanic_print_backtrace,panic_gdbstub,debug_ocdaware, oroptimization_debugset → PreserveBehavior under defaults
CONFIG_ESP_SYSTEM_PANIC_PRINT_BACKTRACE=ysoesp32_exception_decoderkeeps producing readable crash backtraces.-fno-exceptions; nothing in their toolchain consumes eh_frame. Immediate flash savings on every release build.-fexceptionsis on, which AVR's JSON disables. Wired for uniformity.-Og/-O0builds always keep eh_frame for crash debugging.User-facing overrides
How to verify the binary shrank
FBUILD_KEEP_EH_FRAME=1 fbuild build -e esp32s3 examples/Blink cp .fbuild/build/esp32s3/release/firmware.bin /tmp/blink-preserve.bin FBUILD_STRIP_EH_FRAME=1 fbuild build -e esp32s3 examples/Blink cp .fbuild/build/esp32s3/release/firmware.bin /tmp/blink-strip.bin ls -la /tmp/blink-{preserve,strip}.bin # Expected: preserve ~657 KB, strip <= 510 KB (delta >= 150 KB). xtensa-esp32s3-elf-readelf --debug-dump=frames \ .fbuild/build/esp32s3/release/firmware.elf | grep -c "FDE cie"# Expected: preserve ~6500 FDEs, strip near zero from project TUs.The integration test (
crates/fbuild-build/tests/eh_frame_strip_esp32.rs,#[ignore]-marked) automates this: builds twice, assertsfirmware.bindelta ≥ 150 KB and.eh_framesection delta ≥ 50 KB via theobjectcrate.Test plan
uv run soldr cargo fmt --all -- --check(clean)uv run soldr cargo check --workspace --all-targets(clean)uv run soldr cargo clippy --workspace --all-targets -- -D warnings(clean, only pre-existing MSRV-mismatch warning)uv run soldr cargo test -p fbuild-build --lib→ 534 passed (+10 platform tests from this PR)uv run soldr cargo test -p fbuild-config→ 120 passed (+14 sdkconfig tests from this PR)uv run soldr cargo test -p fbuild-build eh_frame_policy→ 16 / 16 passeduv run soldr cargo build --tests→ all test crates compile, including the#[ignore]-marked integration test--ignoredon a host with the ESP32 toolchain to confirm the ≥ 150 KB savings claim end-to-end. Deferred to post-merge / a follow-up CI gate.TDD workflow used
Wave 1 (in parallel via subagents): two pure modules (
eh_frame_policy+sdkconfig) with full unit-test matrices. Wave 1 verified clean before Wave 2 started.Wave 2 (single subagent): platform wiring + integration test. Tests assert
STRIP_FLAGSpresence underEhFramePolicy::Stripand absence under defaultPreservefor every compiler. All 5 GCC compilers + 8 orchestrators wired uniformly.Out of scope
FL_NO_UNWIND_BEGIN/_ENDpragma macros. Harmless to leave in place.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests