Consolidate AVR measurement campaigns - #96
Conversation
📝 WalkthroughWalkthroughAVR demos now use krabi-caliper for benchmark execution, stack analysis, and panic auditing. Mega2560 configuration, benchmark reporters, negative controls, campaign definitions, CI workflows, and the legacy Python suite are updated accordingly. ChangesAVR caliper migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CI
participant krabi-caliper
participant AVRExample
participant simavr
CI->>krabi-caliper: run stack analysis campaign
krabi-caliper->>AVRExample: build and execute benchmark case
AVRExample->>simavr: run Mega2560 benchmark
simavr-->>AVRExample: return benchmark and stack data
AVRExample-->>krabi-caliper: report metrics
krabi-caliper-->>CI: publish campaign result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates the embedded-measure crate into the AVR demo to standardize stack usage and execution time benchmarking, replacing custom stack measurement logic and the simavr_wrapper.py tool. The examples (test_picojson.rs, test_serde.rs, and test_streamparser.rs) have been updated to use the new benchmark API. However, there is a critical compilation issue across all three examples: the serial variable is defined conditionally under the ufmt feature flag but is used unconditionally to initialize UfmtReporter, which will cause build failures when the feature is disabled.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Validated against private embedded-measure main merge 953f1eb9e29dfbeb073eef18638d425273f589b4. Check: cargo +nightly check --release (avr_demo)
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
Fixed security issues:
-
Command injection from untrusted input passed to OS command execution (link)
-
In the AVR examples,
let mut dp = arduino_hal::Peripherals::take().unwrap();is created unconditionally but only used under#[cfg(feature = "ufmt")], so non-ufmtbuilds will hit unused-variable warnings; consider guardingdpwith the same cfg or factoring serial/benchmark setup into a helper that’s only compiled when needed. -
The three AVR fixtures (
test_serde,test_picojson,test_streamparser) duplicate nearly identicalBenchmark/UfmtReporter/stack setup andrun_atmega2560_benchmarkwiring; pulling this into a shared helper or wrapper would reduce boilerplate and make it easier to keep the configurations in sync. -
The magic constants in the measurement wiring (e.g.,
LinkerStack::<Avr>::avr_runtime(0x2200)andSome(15_625)for Timer1) would be easier to maintain if factored into named constants or derived from the board/memory configuration, so that future changes to the ATmega2560 setup don’t silently desynchronize the benchmark behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the AVR examples, `let mut dp = arduino_hal::Peripherals::take().unwrap();` is created unconditionally but only used under `#[cfg(feature = "ufmt")]`, so non-`ufmt` builds will hit unused-variable warnings; consider guarding `dp` with the same cfg or factoring serial/benchmark setup into a helper that’s only compiled when needed.
- The three AVR fixtures (`test_serde`, `test_picojson`, `test_streamparser`) duplicate nearly identical `Benchmark`/`UfmtReporter`/stack setup and `run_atmega2560_benchmark` wiring; pulling this into a shared helper or wrapper would reduce boilerplate and make it easier to keep the configurations in sync.
- The magic constants in the measurement wiring (e.g., `LinkerStack::<Avr>::avr_runtime(0x2200)` and `Some(15_625)` for Timer1) would be easier to maintain if factored into named constants or derived from the board/memory configuration, so that future changes to the ATmega2560 setup don’t silently desynchronize the benchmark behavior.
## Individual Comments
### Comment 1
<location path="avr_demo/src/lib.rs" line_range="8-9" />
<code_context>
+
+#[cfg(feature = "neg-controls")]
+#[inline(never)]
+#[unsafe(no_mangle)]
+pub extern "C" fn panic_audit__neg__bounds_check(index: usize, out: *mut u8) {
+ let values = core::hint::black_box([0u8; 4]);
+ let value = values[core::hint::black_box(index)];
</code_context>
<issue_to_address>
**issue (bug_risk):** The `#[unsafe(no_mangle)]` attribute is likely invalid; consider using `#[no_mangle]` and expressing unsafety via the function signature instead.
Rust does not support an `unsafe` attribute here, and `#[no_mangle]` is the standard way to fix symbol names. To express unsafety, declare the function as `pub unsafe extern "C" fn ...` and use `#[no_mangle]` separately. As written, this attribute will either fail to compile or be ignored, so the symbol name for the audit hook may not be stable.
</issue_to_address>
### Comment 2
<location path="avr_demo/src/lib.rs" line_range="9-12" />
<code_context>
+#[cfg(feature = "neg-controls")]
+#[inline(never)]
+#[unsafe(no_mangle)]
+pub extern "C" fn panic_audit__neg__bounds_check(index: usize, out: *mut u8) {
+ let values = core::hint::black_box([0u8; 4]);
+ let value = values[core::hint::black_box(index)];
+ unsafe { *out = core::hint::black_box(value) };
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** These audit functions dereference raw pointers but are exposed as safe `extern` fns; consider either making them `unsafe` or taking `&mut u8` instead.
All three `panic_audit__neg__*` functions currently accept `*mut u8` and dereference it inside an `unsafe` block while remaining safe to call. This lets safe Rust and FFI callers pass null or invalid pointers without any type-system signal, leading to UB. Please either:
- Mark them `pub unsafe extern "C" fn` and document the required pointer invariants, or
- Change the parameter to `&mut u8` so Rust’s borrowing rules enforce validity on the Rust side, keeping `extern`/`#[no_mangle]` only for the symbol.
This ensures the public API correctly reflects the underlying unsafety.
Suggested implementation:
```rust
#[cfg(feature = "neg-controls")]
#[inline(never)]
#[unsafe(no_mangle)]
pub extern "C" fn panic_audit__neg__bounds_check(index: usize, out: &mut u8) {
let values = core::hint::black_box([0u8; 4]);
let value = values[core::hint::black_box(index)];
*out = core::hint::black_box(value);
}
```
Apply the same pattern to the other `panic_audit__neg__*` functions in this file:
- Change their `out: *mut u8` parameter to `out: &mut u8`.
- Remove the `unsafe` block around `*out = ...` and perform a safe write via the mutable reference.
This keeps the functions safe to call while ensuring Rust callers must pass a valid, mutable reference, preventing UB from null/invalid pointers on the Rust side. For any purely FFI-facing usage where a raw pointer is required, consider adding separate `unsafe extern "C"` wrappers with documented pointer invariants instead of relaxing the safety of these audit functions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d9e505314
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/avr_tests.yaml (2)
128-146: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueHarden interpolation in the
run:block to avoid template-injection warnings.zizmor flags
${{ }}expansion inside the shell script. The values here are workflow/matrix-controlled (not attacker input), so risk is low, but the idiomatic fix is to pass them viaenv:and reference shell variables instead of expanding directly into the script body.♻️ Suggested hardening
- name: Audit parser panic paths with negative controls working-directory: avr_demo env: EXAMPLE: ${{ matrix.example }} PROFILE: ${{ matrix.profile }} FEATURES: ${{ steps.feature_set.outputs.features }} INT_TYPE: ${{ matrix.int_type }} PICO_SIZE: ${{ matrix.pico_size }} DEFMT: ${{ matrix.defmt }} run: | cargo krabi-caliper panic-audit \ --package avr_demo \ --artifact-kind example \ --artifact-name "$EXAMPLE" \ --negative-artifact-name minimal \ --target avr-none \ --profile "$PROFILE" \ --no-default-features \ --features "$FEATURES" \ --negative-features neg-controls \ --owned-symbol '^picojson::' \ --owned-symbol '^panic_audit__neg__' \ --expect-negative 'panic_audit__neg__bounds' \ --expect-negative 'panic_audit__neg__unwrap' \ --expect-negative 'panic_audit__neg__expect' \ --json "target/panic-audit/${EXAMPLE}-${INT_TYPE}-${PROFILE}-${PICO_SIZE}-${DEFMT}.json"🤖 Prompt for 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. In @.github/workflows/avr_tests.yaml around lines 128 - 146, Harden the “Audit parser panic paths with negative controls” step by moving all workflow and matrix interpolations used by its run script into step-level environment variables. Update the cargo command and JSON output path to reference the corresponding quoted shell variables, while preserving the existing argument values and behavior.Source: Linters/SAST tools
62-63: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftCache the
krabi-caliperinstall and pin it with--locked. Both jobscargo install krabi-caliperfrom source on every run without--lockedor caching; theavr_panic_checkmatrix (2×2×2×2×2 = 32 legs) rebuilds it from source each leg, which is a large, avoidable CI cost, and the un---lockedinstall risks non-reproducible dependency resolution. The existing cargo-bin cache (Lines 39-49) and thecommand -vinstall gates used forcargo-bloat/cargo-binutilsare the pattern to follow.
.github/workflows/avr_tests.yaml#L62-L63: add--locked, gate withcommand -v, and include~/.cargo/bin/cargo-krabi-caliperin the cargo-bin cache path/key..github/workflows/avr_tests.yaml#L112-L113: apply the same--locked+ cached/gated install so the 32 matrix legs reuse a single cached binary instead of rebuilding.🤖 Prompt for 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. In @.github/workflows/avr_tests.yaml around lines 62 - 63, Update both krabi-caliper install steps at .github/workflows/avr_tests.yaml#L62-L63 and .github/workflows/avr_tests.yaml#L112-L113 to install with --locked and gate installation using command -v, following the existing cargo-bloat/cargo-binutils pattern. Extend the existing cargo-bin cache configuration at .github/workflows/avr_tests.yaml#L39-L49 to include ~/.cargo/bin/cargo-krabi-caliper in the cached paths and key so both jobs reuse the binary.
🤖 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.
Nitpick comments:
In @.github/workflows/avr_tests.yaml:
- Around line 128-146: Harden the “Audit parser panic paths with negative
controls” step by moving all workflow and matrix interpolations used by its run
script into step-level environment variables. Update the cargo command and JSON
output path to reference the corresponding quoted shell variables, while
preserving the existing argument values and behavior.
- Around line 62-63: Update both krabi-caliper install steps at
.github/workflows/avr_tests.yaml#L62-L63 and
.github/workflows/avr_tests.yaml#L112-L113 to install with --locked and gate
installation using command -v, following the existing cargo-bloat/cargo-binutils
pattern. Extend the existing cargo-bin cache configuration at
.github/workflows/avr_tests.yaml#L39-L49 to include
~/.cargo/bin/cargo-krabi-caliper in the cached paths and key so both jobs reuse
the binary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3afa797-9256-4072-9456-1202210c4ec1
📒 Files selected for processing (12)
.github/workflows/avr_tests.yamlavr_demo/.cargo/config.tomlavr_demo/Cargo.tomlavr_demo/examples/minimal.rsavr_demo/examples/test_picojson.rsavr_demo/examples/test_serde.rsavr_demo/examples/test_streamparser.rsavr_demo/krabi-caliper.tomlavr_demo/run_suite.pyavr_demo/simavr_wrapper.pyavr_demo/src/lib.rsavr_demo/src/stack_measurement.rs
💤 Files with no reviewable changes (3)
- avr_demo/simavr_wrapper.py
- avr_demo/src/stack_measurement.rs
- avr_demo/.cargo/config.toml
1 participant
What changed
embedded-measurebenchmark lifecycle and structured report schemaImpact
The parser now uses the same target-side measurement and host-side campaign interface as the crypto clients, demonstrating that the toolkit is not crypto-specific while removing most of the simulator and measurement boilerplate.
Dependency status
This PR intentionally remains a draft.
embedded-measureis not published yet. The manifest declares its future crates.io version and uses a workspace-local[patch.crates-io]entry pointing at the sibling checkout. A standalone GitHub checkout is therefore expected not to build until the support crate is published or patched locally.Verification
Summary by Sourcery
Unify AVR parser measurement fixtures with the shared krabi-caliper benchmarking and reporting lifecycle, and align CI and tooling around the new stack and panic audit campaigns.
New Features:
Enhancements:
CI:
Documentation:
Summary by CodeRabbit
New Features
Refactor