feat: Add no-heap static allocation PAL backend for lockstep mode - #268
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds selectable ETL-based static allocation with bounded message and byte-buffer pools, platform container aliases, intrusive message ownership, static PIMPL storage, migrated protocol APIs, no-heap verification, memory-budget tooling, and host/RTOS CI coverage. ChangesStatic allocation backend
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant CMake
participant Build
participant StaticBackend
participant Tests
CMake->>Build: configure SOMEIP_USE_STATIC_ALLOC
Build->>StaticBackend: select ETL, pools, and platform headers
StaticBackend->>Tests: expose static targets and malloc trap
Tests->>StaticBackend: initialize pools and exercise bounded APIs
sequenceDiagram
participant MessagePtr
participant Message
participant StaticMemory
participant BufferPool
MessagePtr->>Message: add or release intrusive reference
Message->>StaticMemory: return object when refcount reaches zero
StaticMemory->>BufferPool: initialize and manage byte-buffer pools
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 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 |
![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: 892f880cfb
ℹ️ 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".
2b9db83 to
d731466
Compare
…p mode Add a complete static allocation backend for opensomeip enabling deployment on safety-critical lockstep ECUs where dynamic memory violates ISO 26262 freedom-from-interference requirements. Core components: - SOMEIP_USE_STATIC_ALLOC CMake option with ETL FetchContent - Three-tier slab byte-buffer pool (small/medium/large) with free-stacks - ByteBuffer class with std::vector<uint8_t>-compatible API - Static Message pool with placement new and PAL mutex - IntrusivePtr<T> with embedded atomic ref_count_ in Message - ETL container aliases (Vector, String, UnorderedMap, Queue, Function) - Malloc trap for link-time no-heap verification Documentation and safety: - 20 new Sphinx-needs requirements (REQ_ARCH_008, REQ_PAL_*, REQ_PLATFORM_STATIC_*) - FMEA for static allocation failure modes - static_memory_budget.py footprint calculator Testing: - 30+ unit tests covering pools, ByteBuffer, IntrusivePtr, concurrency - All 16 tests pass (static), all 15 tests pass (dynamic) Co-authored-by: Cursor <cursoragent@cursor.com>
d731466 to
2a04600
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/requirements/implementation/platform.rst (1)
483-485:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the
std::shared_ptrhard-coding.
REQ_PAL_MEM_ALLOCshould stay backend-agnostic. It currently conflicts withREQ_PAL_INTRUSIVE_PTRand the static message-pool requirement, which use intrusive ref-counting instead of a shared_ptr control block.🔧 Proposed fix
- ``MessagePtr`` (``std::shared_ptr<Message>``) owning a fully constructed, usable - ``Message`` object. + ``MessagePtr`` owning a fully constructed, usable ``Message`` object.Also applies to: 966-977
🤖 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 `@docs/requirements/implementation/platform.rst` around lines 483 - 485, REQ_PAL_MEM_ALLOC currently hard-codes std::shared_ptr in the spec: change the wording around someip::platform::allocate_message() and MessagePtr so they do not mandate std::shared_ptr; instead describe MessagePtr as an abstract/opaque smart-pointer type (or "pointer-like handle") owning a fully constructed Message and allow backend choice (e.g., intrusive ref-counting or static pool-backed handles) so it remains compatible with REQ_PAL_INTRUSIVE_PTR and the static message-pool requirement; update all occurrences (including the section referencing lines ~966-977) to remove "std::shared_ptr<Message>" and replace with this backend-agnostic description, while keeping the contract that callers receive an owning MessagePtr with a usable Message instance.
🤖 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 `@docs/requirements/implementation/architecture.rst`:
- Around line 202-203: Update the acceptance text that currently instructs
verifying no "malloc"/"new" calls by expanding it to cover deallocations too:
change the verification sentence that references SOMEIP_USE_STATIC_ALLOC and
REQ_PAL_NOOP_HEAP_VERIFY so it requires verifying no "malloc"/"new" or
"free"/"delete" calls during protocol operation and that the heap-interception
check covers both allocation and deallocation failures; keep the existing
guidance to inspect container and pool types for compile-time capacity bounds
and mention the combined allocation/deallocation surface to match the no-heap
trap behavior.
In `@docs/requirements/implementation/platform.rst`:
- Around line 734-817: The requirement names
(StaticVector/StaticString/StaticUnorderedMap/StaticQueue/StaticFunction) do not
match the actual exported API (Vector/String/... in
include/platform/static/containers_impl.h); update the implementation to expose
the public symbols named in the docs (either rename the types in
containers_impl.h to StaticVector, StaticString, StaticUnorderedMap,
StaticQueue, StaticFunction or add clear type aliases with those Static* names
that forward to the existing Vector/String/... types), and update the
corresponding backend requirement text to reference the same Static* symbols so
doc and code agree.
In `@docs/safety/FMEA_STATIC_ALLOCATION.md`:
- Around line 126-170: Residual-risk claims rely on ETL being built with
error-logging enabled and exceptions disabled but the build lacks those defines;
update the build config (CMakeLists.txt) to add the compile definitions
ETL_LOG_ERRORS=1 and ETL_THROW_EXCEPTIONS=0 (use target_compile_definitions or
add_compile_definitions for the relevant targets) and ensure the custom ETL
error handler object/library is linked into safety builds so the handler
registration code is included; after changing, run a safety-build to verify the
ETL handler symbol is present and that overflow paths exercise the custom
handler.
- Line 124: The FMEA claims a saturation check but intrusive_ptr_add_ref in
src/someip/message.cpp (under SOMEIP_STATIC_ALLOC) unconditionally does
p->ref_count_.fetch_add(1...), so fix intrusive_ptr_add_ref to perform an atomic
read/compare-and-swap loop that ensures ref_count_ < UINT16_MAX before
incrementing (or else log an error and refuse to increment/return a
failure/no-op copy), keep a debug-build saturation assertion, and update related
unit test TC_INTRUSIVE_PTR_LIFETIME and any copy sites (e.g., MessagePtr copy
logic) to handle the refused-increment path; alternatively, if you decide not to
implement the guard, update the FMEA entry to match current behavior.
- Around line 78-92: The build is missing the ETL safety compile definitions
described in the FMEA; add target_compile_definitions for ETL_LOG_ERRORS=1 and
ETL_THROW_EXCEPTIONS=0 to the actual CMake target (opensomeip) in
src/CMakeLists.txt so ETL uses non-terminating error propagation, and update the
documentation snippet in docs/safety/FMEA_STATIC_ALLOCATION.md to reference the
real target name opensomeip instead of someip; ensure the compile definitions
are applied PRIVATE to the opensomeip target and include a short comment linking
to the FMEA section for traceability.
In `@include/platform/static/buffer_pool_impl.h`:
- Around line 130-184: The code currently calls ensure_capacity(...) which may
fail to grow the buffer but leaves slot_ non-null, so resize(size_t),
resize(size_t,uint8_t), push_back(uint8_t) and insert(...) can still write past
the slot_'s capacity; after each ensure_capacity(new_size) (or
ensure_capacity(cur + 1) / ensure_capacity(min_capacity) where used) add a
post-check that the reserved capacity is sufficient (e.g., if (!slot_ ||
capacity() < new_size) return; or compare slot_->capacity) and return early to
avoid writing beyond slot_->capacity; apply the same guard to the other affected
sites (the overload resize(size_t,uint8_t), push_back, and insert) and ensure
slot_->size is only updated when the capacity check passes.
- Around line 190-193: end() and the const end() perform pointer arithmetic on
data() which may be nullptr for empty/default-constructed ByteBuffer; change
them to guard against nullptr by returning data() when data() == nullptr instead
of data() + size(). Update iterator end() and const_iterator end() (and keep
begin()/begin() const as-is) to compute: return data() ? data() + size() :
data(); so no nullptr + 0 occurs.
In `@include/platform/static/static_config.h`:
- Around line 35-80: Add compile-time range checks for all macro overrides
(SOMEIP_MESSAGE_POOL_SIZE, SOMEIP_MAX_SESSIONS, SOMEIP_MAX_SD_ENTRIES,
SOMEIP_MAX_MULTICAST_GROUPS, SOMEIP_MAX_CONCURRENT_TP, SOMEIP_MAX_RECEIVE_QUEUE,
SOMEIP_BYTE_POOL_SMALL_COUNT, SOMEIP_BYTE_POOL_MEDIUM_COUNT,
SOMEIP_BYTE_POOL_LARGE_COUNT, SOMEIP_BYTE_POOL_SMALL_SIZE,
SOMEIP_BYTE_POOL_MEDIUM_SIZE, SOMEIP_BYTE_POOL_LARGE_SIZE) so invalid zero or
>UINT16_MAX values cannot silently wrap; implement either preprocessor guards
(`#if` ... `#error`) or constexpr/static_assert immediately after each `#define` to
enforce 1 <= count <= 65535 for pool counts and 1 <= size <= 65535 (or
appropriate max) for byte sizes, matching the uint16_t indices used in
src/platform/static/buffer_pool.cpp and src/platform/static/memory.cpp. Ensure
messages identify the offending macro name in the `#error/static_assert` text so
integrators can fix their -D overrides.
In `@scripts/run_clang_tidy.sh`:
- Around line 67-77: Replace the fragile inline Python invocation with a small
robust script that reads BUILD_DIR and SOURCE_DIR from os.environ (instead of
shell-interpolating paths), opens compile_commands.json using a with open(...,
encoding='utf-8') context manager, wraps json.load in a try/except catching
json.JSONDecodeError and exits non‑zero on failure, and then performs the same
realpath/filter logic for .cpp files; reference the embedded Python block in
scripts/run_clang_tidy.sh and the BUILD_DIR/SOURCE_DIR variables when locating
compile_commands.json and the src directory.
- Around line 64-80: The current selection reads every entry's e['file'] from
compile_commands.json and can pick commands from other CMake configs; change the
filter to only include entries whose e['directory'] (or directory field) belongs
to the active build directory ($BUILD_DIR) and whose realpath file starts with
the project src ($SOURCE_DIR/src) and ends with .cpp, and deduplicate the list
(e.g., via set or sort -u) before assigning SOURCE_FILES; update the python
snippet that populates SOURCE_FILES (references: BUILD_DIR, SOURCE_DIR,
SOURCE_FILES, e['file'], e['directory'], compile_commands.json) to perform this
directory check and de-duplication so clang-tidy only analyzes files from the
active CMake configuration.
In `@scripts/static_memory_budget.py`:
- Around line 25-31: Add an explicit Python minimum-version note so the script
won’t be misused on older runtimes: update the top of the module to include a
module docstring stating "Requires Python 3.10+ (repo uses Python 3.12+)" and
also include the same note in the argparse description/help so command-line
users see it; locate the module header and the argparse setup (the function or
main entry that accepts argv: list[str] | None) and add the short clear version
requirement there.
In `@src/CMakeLists.txt`:
- Around line 51-56: The comment about allocation being "listed before
threading" is misplaced next to the SOMEIP_ALLOC_IMPL_DIR variable and should be
moved to where include ordering is actually set; relocate or duplicate the
comment near the target_include_directories call (the block that adds
${SOMEIP_ALLOC_IMPL_DIR} and threading include dirs) so it clearly documents why
${SOMEIP_ALLOC_IMPL_DIR} must appear before the threading include dirs; keep
references to SOMEIP_USE_STATIC_ALLOC and SOMEIP_ALLOC_IMPL_DIR so future
readers can find the related conditional and include-path logic.
In `@src/platform/static/malloc_trap.cpp`:
- Around line 37-63: The current trap implements malloc/free/calloc/realloc but
misses C++ heap operators so non-placement new bypasses it; add matching global
operator new, operator new[](std::size_t) and their corresponding operator
delete/operator delete[](void*) (and the nothrow/new(size, std::nothrow_t)
overloads if used) to abort with an explanatory fprintf like the existing
functions, referencing the same behavior as malloc/free handlers (e.g.,
implement operator new(std::size_t) and operator new[](std::size_t) to fprintf
"MALLOC TRAP: operator new(%zu) detected" and call std::abort(), and implement
operator delete/operator delete[] to detect frees and abort similarly) so all
non-placement C++ allocations are trapped.
In `@tests/CMakeLists.txt`:
- Around line 110-117: The test target test_static_alloc is not linked against
the someip_malloc_trap OBJECT library so heap-interception won't be enforced;
update the CMake target for test_static_alloc (target name "test_static_alloc")
to add the someip_malloc_trap object library via target_link_libraries (e.g.,
link someip_malloc_trap as a PRIVATE dependency) so the malloc trap defined as
someip_malloc_trap in src/CMakeLists.txt is included at link time and enables
no-heap verification.
- Line 114: Remove the redundant PRIVATE definition of SOMEIP_STATIC_ALLOC from
the test target: delete the line calling
target_compile_definitions(test_static_alloc PRIVATE SOMEIP_STATIC_ALLOC) since
SOMEIP_STATIC_ALLOC is already defined PUBLIC on the opensomeip target and will
propagate to test_static_alloc through linking; ensure opensomeip remains a
PUBLIC-linked target for test_static_alloc so the definition still applies.
In `@tests/test_static_alloc.cpp`:
- Around line 385-411: The test BufferPoolThreadSafety currently uses a weak
smoke assertion (EXPECT_GT(success_count.load(), 0)); replace it with a stronger
correctness check that validates all attempted acquires succeeded by asserting
EXPECT_EQ(success_count.load(), kThreads * kOpsPerThread) (referencing kThreads,
kOpsPerThread, success_count, and the test name) so the test fails if any
acquisition/release under contention is lost; after changing the assertion,
consider enabling ThreadSanitizer in CI for additional race detection.
---
Outside diff comments:
In `@docs/requirements/implementation/platform.rst`:
- Around line 483-485: REQ_PAL_MEM_ALLOC currently hard-codes std::shared_ptr in
the spec: change the wording around someip::platform::allocate_message() and
MessagePtr so they do not mandate std::shared_ptr; instead describe MessagePtr
as an abstract/opaque smart-pointer type (or "pointer-like handle") owning a
fully constructed Message and allow backend choice (e.g., intrusive ref-counting
or static pool-backed handles) so it remains compatible with
REQ_PAL_INTRUSIVE_PTR and the static message-pool requirement; update all
occurrences (including the section referencing lines ~966-977) to remove
"std::shared_ptr<Message>" and replace with this backend-agnostic description,
while keeping the contract that callers receive an owning MessagePtr with a
usable Message instance.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7022fc2e-e97d-4c07-b656-c9fe3e100de8
📒 Files selected for processing (24)
CMakeLists.txtdocs/requirements/implementation/architecture.rstdocs/requirements/implementation/platform.rstdocs/safety/FMEA_STATIC_ALLOCATION.mdinclude/platform/buffer_pool.hinclude/platform/containers.hinclude/platform/dynamic/buffer_pool_impl.hinclude/platform/dynamic/containers_impl.hinclude/platform/intrusive_ptr.hinclude/platform/static/buffer_pool_impl.hinclude/platform/static/containers_impl.hinclude/platform/static/memory_impl.hinclude/platform/static/static_config.hscripts/run_clang_tidy.shscripts/static_memory_budget.pysrc/CMakeLists.txtsrc/platform/static/buffer_pool.cppsrc/platform/static/malloc_trap.cppsrc/platform/static/memory.cppsrc/someip/message.cpptests/CMakeLists.txttests/fakes/buffer_pool_impl.htests/fakes/containers_impl.htests/test_static_alloc.cpp
Move MessagePtr typedef to PAL dispatch (message_ptr.h -> message_ptr_impl.h) resolved by include-path shadowing, matching the existing PAL pattern. - ref_count_ and intrusive_ptr hooks are now unconditional (2 bytes per Message; negligible overhead, identical class layout across backends) - Dynamic backends gain a trivial release_message() so the unconditional intrusive_ptr_release() compiles and links (dead code in dynamic path) - SOMEIP_STATIC_ALLOC compile definition removed entirely -- no source code references it anymore; backend selection is purely via -I paths Co-authored-by: Cursor <cursoragent@cursor.com>
Blocker: - ByteBuffer: check slot_->capacity >= needed after ensure_capacity() to prevent out-of-bounds writes when pool upgrade fails High: - ByteBuffer: return nullptr from end() when slot_ is null to avoid undefined pointer arithmetic (nullptr + 0) - malloc_trap: add operator new/new[]/delete/delete[] traps alongside C malloc/free/calloc/realloc for complete no-heap verification - Link someip_malloc_trap OBJECT library into test_static_alloc so REQ_PAL_NOOP_HEAP_VERIFY is enforced at test runtime Medium: - static_config.h: add static_assert guards for pool sizes (uint16_t range) and tier size ordering - platform.rst: rename StaticVector/StaticString/StaticUnorderedMap/ StaticQueue/StaticFunction to Vector/String/UnorderedMap/Queue/ Function matching actual containers_impl.h; fix Code Location references to point to containers_impl.h instead of non-existent container_*.h files; update test file references Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/someip/message.cpp (1)
553-557: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMissing overflow protection for reference counter.
The intrusive reference counter uses
uint16_t(max 65,535) with no overflow check. Ifref_count_reaches its maximum andintrusive_ptr_add_refis called again, it will wrap to 0, causing premature deletion when the firstIntrusivePtris destroyed.For ISO 26262 safety-critical deployments, bounded reference counts should either use a wider type or detect overflow and fail safely.
🛡️ Proposed overflow guard
void intrusive_ptr_add_ref(const Message* p) { if (p) { - p->ref_count_.fetch_add(1, std::memory_order_relaxed); + auto old_count = p->ref_count_.fetch_add(1, std::memory_order_relaxed); + // Safety check: detect overflow before it causes wrap-around + if (old_count >= std::numeric_limits<uint16_t>::max()) { + // Overflow: either abort or use a wider type (uint32_t) + std::abort(); // Or log critical error and halt + } } }Alternatively, consider changing
ref_count_tostd::atomic<uint32_t>ininclude/someip/message.hif the 2-byte footprint saving is not critical.🤖 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 `@src/someip/message.cpp` around lines 553 - 557, intrusive_ptr_add_ref currently increments Message::ref_count_ (a uint16_t atomic) without overflow protection; either widen the atomic to std::atomic<uint32_t> in the Message class (preferred) or add an overflow check in intrusive_ptr_add_ref: load the current value, if it equals std::numeric_limits<uint16_t>::max() fail safely (log/abort/throw) and otherwise increment using compare-exchange or fetch_add with a check to avoid wraparound; update any code using ref_count_ accordingly and preserve the intended memory ordering semantics.
🤖 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 `@include/platform/dynamic/message_ptr_impl.h`:
- Around line 10-15: This header declares MessagePtr and MessageConstPtr using
std::shared_ptr (symbols: MessagePtr, MessageConstPtr, namespace someip) but
lacks the required <memory> include; add a direct include of <memory> at the top
of include/platform/dynamic/message_ptr_impl.h so the header no longer relies on
transitive includes for std::shared_ptr and remains robust to include-order
changes.
In `@include/platform/static/message_ptr_impl.h`:
- Around line 10-15: Add a direct include for the intrusive pointer header so
the typedefs compile without relying on transitive includes: include
"platform/intrusive_ptr.h" at the top of message_ptr_impl.h before using
platform::IntrusivePtr; update the file that defines MessagePtr and
MessageConstPtr (the typedefs using platform::IntrusivePtr<Message> and
platform::IntrusivePtr<const Message>) to reference the header explicitly.
In `@tests/fakes/memory_impl.h`:
- Around line 33-35: The current release_message(Message* msg) deletes the raw
pointer while allocate_message() returns std::shared_ptr<Message>, risking
double-free if callers pass ptr.get(); fix by making the ownership contract
explicit: add a clear comment above release_message stating "must never be
called with pointers owned by allocate_message()/std::shared_ptr" and provide a
safe overload release_message(std::shared_ptr<Message> msg) that is a no-op (or
simply consumes the shared_ptr) so accidental calls with shared_ptr objects are
safe; optionally mark the raw-pointer release_message as deprecated in the
comment to discourage use and keep delete behavior only for truly raw-allocated
Message instances.
---
Outside diff comments:
In `@src/someip/message.cpp`:
- Around line 553-557: intrusive_ptr_add_ref currently increments
Message::ref_count_ (a uint16_t atomic) without overflow protection; either
widen the atomic to std::atomic<uint32_t> in the Message class (preferred) or
add an overflow check in intrusive_ptr_add_ref: load the current value, if it
equals std::numeric_limits<uint16_t>::max() fail safely (log/abort/throw) and
otherwise increment using compare-exchange or fetch_add with a check to avoid
wraparound; update any code using ref_count_ accordingly and preserve the
intended memory ordering semantics.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aaeb92b7-dfc8-4aee-a02f-4686b83d3863
📒 Files selected for processing (14)
include/platform/dynamic/message_ptr_impl.hinclude/platform/message_ptr.hinclude/platform/posix/memory_impl.hinclude/platform/static/message_ptr_impl.hinclude/platform/win32/memory_impl.hinclude/someip/message.hsrc/CMakeLists.txtsrc/someip/message.cpptests/CMakeLists.txttests/fakes/memory_impl.htests/fakes/message_ptr_impl.htests/test_pal_freertos_mock.cpptests/test_pal_threadx_mock.cpptests/test_pal_zephyr_mock.cpp
💤 Files with no reviewable changes (1)
- src/CMakeLists.txt
- intrusive_ptr.h: suppress hicpp-explicit-conversions for nullptr_t ctor - posix/memory_impl.h: suppress cppcoreguidelines-owning-memory on delete - message.cpp: add <atomic> include, use explicit nullptr comparisons, suppress const_cast and include-cleaner in intrusive_ptr hooks Co-authored-by: Cursor <cursoragent@cursor.com>
The dispatch header message_ptr.h includes message_ptr_impl.h which must be findable from the backend's include directory. For the host CMake build, include/platform/dynamic/ provides it. But standalone build systems (Zephyr west) only add the backend directory to -I, so each backend needs its own message_ptr_impl.h. Fixes Zephyr S32K388 Renode build failure: fatal error: message_ptr_impl.h: No such file or directory Co-authored-by: Cursor <cursoragent@cursor.com>
- CMake: set ETL_LOG_ERRORS=1, ETL_THROW_EXCEPTIONS=0, ETL_NO_STL=1 when SOMEIP_USE_STATIC_ALLOC=ON — enforces FMEA-prescribed ETL error handling (no abort/exception on container overflow) - Add test_pal_static_alloc_mock.cpp: PAL conformance tests exercising the real static message pool + buffer pool with POSIX threading, verifying the static backend satisfies the same contracts as dynamic - Add debug-mode assert in intrusive_ptr_add_ref to detect ref_count_ saturation (uint16_t overflow) — fires only in debug builds, prevents silent wrap-around in development Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/requirements/implementation/platform.rst (2)
840-846: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify the expected error-handling behavior for capacity exhaustion (line 845).
The parenthetical comment "(ETL container overflow behavior governed by the ETL error handler)" appears incomplete. The requirement text (lines 833–842) states that insertion operations "shall fail gracefully…without crashing, corrupting…or allocating from the heap," but line 845 defers error handling to ETL's handler without explaining what that entails.
For safety-critical contexts, readers need clarity: Does "gracefully" mean ETL logs an error, throws an exception (if
ETL_THROW_EXCEPTIONS=1), or returns a status code? If ETL behavior is externally defined, the requirement should explicitly state the expected outcome (e.g., "insertion returns false on overflow" or "ETL error handler is configured via ETL_LOG_ERRORS=1 and ETL_THROW_EXCEPTIONS=0").📝 Suggested revision
Replace lines 841–845 with explicit expected behavior:
**Error Handling**: Return ``false`` or an error indicator; container - internals remain consistent. + internals remain consistent. When ``SOMEIP_USE_STATIC_ALLOC`` is enabled, + ETL containers are configured with ``ETL_THROW_EXCEPTIONS=0``, + ``ETL_LOG_ERRORS=1``, and ``ETL_NO_STL=1``; insertion failures are + signaled via return codes rather than exceptions. - **Code Location**: ``include/platform/static/containers_impl.h`` + **Code Location**: ``include/platform/static/containers_impl.h``, + ``CMakeLists.txt`` (ETL build flags)🤖 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 `@docs/requirements/implementation/platform.rst` around lines 840 - 846, Clarify that on capacity exhaustion insertion operations (functions in include/platform/static/containers_impl.h) must return false or an error status without allocating from the heap, crashing, or corrupting state; explicitly state that ETL is used only for diagnostics and must be configured so ETL_THROW_EXCEPTIONS=0 and ETL_LOG_ERRORS=1 (so the container returns false on overflow and ETL logs the event) or otherwise document the alternative configuration and outcome; update the parenthetical to read that the ETL error handler is used only to log diagnostics and must be configured not to throw so container methods (e.g., push/insert/emplace in the static containers) deterministically return false on overflow.
981-982: 📐 Maintainability & Code Quality | 🟡 MinorFix docs “Code Location” for Message intrusive ref-count
docs/requirements/implementation/platform.rstlines 981-982 citeinclude/common/message.hfor the embedded intrusive ref-count, butinclude/common/message.hdoes not exist in the repo;Message’s intrusiveref_count_{0}is ininclude/someip/message.h(withintrusive_ptr_add_ref/intrusive_ptr_release). Update the cited path toinclude/someip/message.h.🤖 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 `@docs/requirements/implementation/platform.rst` around lines 981 - 982, The docs "Code Location" entry currently points to a non-existent header; update the Code Location to cite the header that actually defines Message and its intrusive ref-count (the header that contains class Message, ref_count_{0}, and the intrusive_ptr_add_ref / intrusive_ptr_release implementations) — replace the incorrect include/common/message.h with include/someip/message.h and ensure the text mentions Message, ref_count_{0}, intrusive_ptr_add_ref, and intrusive_ptr_release so readers can find the implementation.
🤖 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 `@docs/requirements/implementation/platform.rst`:
- Line 740: platform.rst currently references non-existent test files and
TC_CONTAINER_* IDs (e.g., TC_CONTAINER_VECTOR_PUSH_BACK) which are not present;
update the requirement-to-test traceability by editing
docs/requirements/implementation/platform.rst to either (A) replace references
to tests/test_buffer_pool.cpp, tests/test_no_heap.cpp,
tests/test_pimpl_static.cpp, and tests/test_static_memory.cpp with the
consolidated tests/test_static_alloc.cpp and the actual TC IDs found there
(e.g., TC_BUFPOOL_* , TC_INTRUSIVE_PTR_LIFETIME, TC_BYTEBUFFER_API,
TC_PIMPL_NO_HEAP), and adjust the TC mapping entries so
TC_CONTAINER_VECTOR_PUSH_BACK and other TC_CONTAINER_* entries point to the
correct test or are removed, or (B) restore the missing test sources and their
TC_CONTAINER_* mappings if you intend to keep those IDs; ensure the platform.rst
mapping lines reference the exact test file name tests/test_static_alloc.cpp and
the exact TC identifiers present in that file to restore correct traceability.
---
Outside diff comments:
In `@docs/requirements/implementation/platform.rst`:
- Around line 840-846: Clarify that on capacity exhaustion insertion operations
(functions in include/platform/static/containers_impl.h) must return false or an
error status without allocating from the heap, crashing, or corrupting state;
explicitly state that ETL is used only for diagnostics and must be configured so
ETL_THROW_EXCEPTIONS=0 and ETL_LOG_ERRORS=1 (so the container returns false on
overflow and ETL logs the event) or otherwise document the alternative
configuration and outcome; update the parenthetical to read that the ETL error
handler is used only to log diagnostics and must be configured not to throw so
container methods (e.g., push/insert/emplace in the static containers)
deterministically return false on overflow.
- Around line 981-982: The docs "Code Location" entry currently points to a
non-existent header; update the Code Location to cite the header that actually
defines Message and its intrusive ref-count (the header that contains class
Message, ref_count_{0}, and the intrusive_ptr_add_ref / intrusive_ptr_release
implementations) — replace the incorrect include/common/message.h with
include/someip/message.h and ensure the text mentions Message, ref_count_{0},
intrusive_ptr_add_ref, and intrusive_ptr_release so readers can find the
implementation.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5595ffbc-2a5e-4197-b90c-920c20f33308
📒 Files selected for processing (15)
docs/requirements/implementation/platform.rstinclude/platform/freertos/message_ptr_impl.hinclude/platform/intrusive_ptr.hinclude/platform/posix/memory_impl.hinclude/platform/posix/message_ptr_impl.hinclude/platform/static/buffer_pool_impl.hinclude/platform/static/static_config.hinclude/platform/threadx/message_ptr_impl.hinclude/platform/win32/message_ptr_impl.hinclude/platform/zephyr/message_ptr_impl.hsrc/CMakeLists.txtsrc/platform/static/malloc_trap.cppsrc/someip/message.cpptests/CMakeLists.txttests/test_pal_static_alloc_mock.cpp
- Add TC_CONTAINER_VECTOR_PUSH_BACK, TC_CONTAINER_STRING_APPEND, TC_CONTAINER_MAP_INSERT_LOOKUP, TC_CONTAINER_QUEUE_FIFO, TC_CONTAINER_FUNCTION_INVOKE, TC_CONTAINER_CAPACITY_EXHAUST tests to test_static_alloc.cpp — completes requirement traceability for REQ_PAL_CONTAINER_* in platform.rst - Fix doc references: update Code Location for intrusive ptr to include/someip/message.h, update no-heap test file ref to test_static_alloc.cpp, clarify ETL error handling defines - Remove ETL_NO_STL=1 (conflicts with host STL on non-bare-metal targets; only ETL_LOG_ERRORS and ETL_THROW_EXCEPTIONS needed) Co-authored-by: Cursor <cursoragent@cursor.com>
- architecture.rst: broaden heap-verification scope to include free/delete - FMEA: fix CMake target name (opensomeip, PUBLIC), update refcount saturation entry to match debug-assert implementation, update residual risk assessment Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
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 `@tests/test_static_alloc.cpp`:
- Around line 533-547: Extend the ContainerTest::CapacityExhaust test to verify
overflow behavior when capacity is exhausted: after filling Vector<int,4> v and
Queue<int,2> q to capacity (using Vector::push_back and Queue::push), attempt
one more insertion for each and assert the operation fails gracefully (e.g.,
returns a false/error indicator or does not change state) by checking the return
value if push/push_back return status or by asserting size() remains at capacity
and container contents are unchanged; update the test to use Vector, Queue,
push_back, push, full, and size to locate the relevant checks.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39955ab6-d74f-482d-ac92-8fdd0c7a1d83
📒 Files selected for processing (5)
docs/requirements/implementation/architecture.rstdocs/requirements/implementation/platform.rstdocs/safety/FMEA_STATIC_ALLOCATION.mdsrc/CMakeLists.txttests/test_static_alloc.cpp
💤 Files with no reviewable changes (1)
- src/CMakeLists.txt
Three issues addressed from follow-up review:
1. CI/preset gap: static-alloc backend was never built or tested in CI.
- Add `static-alloc-linux-tests` preset to CMakePresets.json
- Add `build-static` job to host.yml (build + ctest with
SOMEIP_USE_STATIC_ALLOC=ON)
- Register preset in preset-validation.yml
2. Traceability: platform.rst referenced non-existent TC_NO_HEAP_PROTOCOL_RUN.
- Rename to TC_NO_HEAP_VERIFY and update description to reflect
that test_static_alloc links malloc_trap which aborts on any heap
call, so all passing tests implicitly verify no-heap compliance
- Add TC_NO_HEAP_VERIFY and REQ_PAL_NOOP_HEAP_VERIFY to test header
3. Double-release: release_buffer had no guard against returning the
same slot index to the free stack twice.
- Add per-slot `in_use` tracking arrays in buffer_pool.cpp
- Set true on acquire, false on release; reject release if !in_use
- Add DoubleReleaseIsSafe test proving two consumers never get the
same slot after a double-release attempt
Co-authored-by: Cursor <cursoragent@cursor.com>
GCC with -Werror rejects operator new returning nullptr unless declared noexcept/throw(). Since these trap functions always call abort(), the noexcept specification is correct and silences the diagnostic. Co-authored-by: Cursor <cursoragent@cursor.com>
21f0fc2 to
1dc6696
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/platform/static/buffer_pool.cpp (1)
135-144:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
in_usestill allows stale double-release after the slot is reissued.This check only answers whether the slot is leased now; it does not prove the caller owns the current lease. If a slot is released, reacquired, and then an old
BufferSlot*is released again, Line 139 still passes, the live slot is pushed back onto the free stack, and a lateracquire_buffer()can hand the same backing storage to two owners at once. This needs an ownership-carrying handle/token (for example, a generation-bound opaque handle) instead of a per-slot boolean. Please also extendBufferPoolTest.DoubleReleaseIsSafeto reacquire the slot before the second release so this path regresses.🤖 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 `@src/platform/static/buffer_pool.cpp` around lines 135 - 144, The release path allows stale BufferSlot* double-release because in_use_ptrs[t][i] is a boolean that doesn't prove the caller still owns the lease; replace it with an ownership token (e.g., add a per-slot generation counter and return an opaque handle / generation-bound token from acquire_buffer() or embed a generation field in BufferSlot) and require the release function to verify the token/generation matches the current slot generation before accepting it; on successful release increment the slot generation and mark it free (update in_use_ptrs => ownership state), update free_stack_ptrs/stack_top logic to only run after token verification, and extend BufferPoolTest.DoubleReleaseIsSafe to reacquire the slot before attempting the second release to assert the regression is covered.docs/requirements/implementation/platform.rst (1)
956-956:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the actual static-config knob names.
include/platform/static/static_config.hexposesSOMEIP_MESSAGE_POOL_SIZEandSOMEIP_BYTE_POOL_*; theSOMEIP_STATIC_MESSAGE_POOL_SIZE/SOMEIP_STATIC_BUFPOOL_TIER_*names here do not exist, so this points users at the wrong CMake inputs.Suggested fix
- Configure ``SOMEIP_STATIC_MESSAGE_POOL_SIZE``, ``SOMEIP_STATIC_BUFPOOL_TIER_*`` via CMake + Configure ``SOMEIP_MESSAGE_POOL_SIZE`` and ``SOMEIP_BYTE_POOL_*`` via CMake🤖 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 `@docs/requirements/implementation/platform.rst` at line 956, The documentation references non-existent CMake knobs `SOMEIP_STATIC_MESSAGE_POOL_SIZE` and `SOMEIP_STATIC_BUFPOOL_TIER_*`; update the text to use the actual static-config symbol names exported in include/platform/static/static_config.h — namely `SOMEIP_MESSAGE_POOL_SIZE` and `SOMEIP_BYTE_POOL_*` (or the exact tiered `SOMEIP_BYTE_POOL_TIER_*` names used in the header) and state that these are configured via CMake so the generated static_config.h reflects those values and pool sizes at runtime without heap allocation.
♻️ Duplicate comments (1)
docs/requirements/implementation/platform.rst (1)
859-859:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize the remaining traceability refs to the consolidated static suite.
These requirements still point at the pre-consolidation split-test layout. Please retarget the buffer-pool, no-heap, static-memory, and pimpl verifications to
tests/test_static_alloc.cppand the TC IDs that actually live there; the no-heap block should also update its Code Location tosrc/platform/static/malloc_trap.cpp. Same cleanup as the earlier traceability note.Also applies to: 878-878, 896-896, 914-914, 933-933, 1004-1004, 1033-1033, 1056-1056, 1078-1078, 1096-1096
🤖 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 `@docs/requirements/implementation/platform.rst` at line 859, Update the traceability references that still point to the old split-test layout by retargeting the buffer-pool, no-heap, static-memory, and pimpl verification entries to the consolidated static test suite: change their test verification lines to reference tests/test_static_alloc.cpp and the corresponding TC IDs that now live in that file (instead of the old per-component IDs), and update the no-heap Code Location reference to src/platform/static/malloc_trap.cpp; apply the same replacement pattern for the other occurrences listed (lines noted in the comment) so all refs consistently point to tests/test_static_alloc.cpp and the correct TC identifiers and code location.
🤖 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 @.github/workflows/host.yml:
- Line 179: The checkout step currently uses actions/checkout@v4 without
disabling credential persistence; update the checkout invocation for the host
workflow (the actions/checkout@v4 usage) to include persist-credentials: false
so it matches the coverage job and minimizes stored credentials—locate the
actions/checkout@v4 step and add the persist-credentials: false key to its
configuration.
In `@src/platform/static/malloc_trap.cpp`:
- Around line 71-75: Add the same unreachable return pattern used in the C
overrides to the C++ allocation overrides: after the std::abort() call in both
operator new(std::size_t) noexcept and operator new[](std::size_t) noexcept, add
a "return nullptr; // unreachable" to suppress potential compiler warnings and
keep consistency with malloc/calloc/realloc overrides.
- Around line 71-81: Add the nothrow allocation overloads and the <new> include
so nothrow allocations cannot bypass the trap: include <new> at the top, and
implement operator new(std::size_t, const std::nothrow_t&) noexcept and operator
new[](std::size_t, const std::nothrow_t&) noexcept matching the existing trap
behavior (print the MALLOC TRAP message including the size with fprintf and
abort), using the same message style as operator new and operator new[] so all
forms of new are covered.
---
Outside diff comments:
In `@docs/requirements/implementation/platform.rst`:
- Line 956: The documentation references non-existent CMake knobs
`SOMEIP_STATIC_MESSAGE_POOL_SIZE` and `SOMEIP_STATIC_BUFPOOL_TIER_*`; update the
text to use the actual static-config symbol names exported in
include/platform/static/static_config.h — namely `SOMEIP_MESSAGE_POOL_SIZE` and
`SOMEIP_BYTE_POOL_*` (or the exact tiered `SOMEIP_BYTE_POOL_TIER_*` names used
in the header) and state that these are configured via CMake so the generated
static_config.h reflects those values and pool sizes at runtime without heap
allocation.
In `@src/platform/static/buffer_pool.cpp`:
- Around line 135-144: The release path allows stale BufferSlot* double-release
because in_use_ptrs[t][i] is a boolean that doesn't prove the caller still owns
the lease; replace it with an ownership token (e.g., add a per-slot generation
counter and return an opaque handle / generation-bound token from
acquire_buffer() or embed a generation field in BufferSlot) and require the
release function to verify the token/generation matches the current slot
generation before accepting it; on successful release increment the slot
generation and mark it free (update in_use_ptrs => ownership state), update
free_stack_ptrs/stack_top logic to only run after token verification, and extend
BufferPoolTest.DoubleReleaseIsSafe to reacquire the slot before attempting the
second release to assert the regression is covered.
---
Duplicate comments:
In `@docs/requirements/implementation/platform.rst`:
- Line 859: Update the traceability references that still point to the old
split-test layout by retargeting the buffer-pool, no-heap, static-memory, and
pimpl verification entries to the consolidated static test suite: change their
test verification lines to reference tests/test_static_alloc.cpp and the
corresponding TC IDs that now live in that file (instead of the old
per-component IDs), and update the no-heap Code Location reference to
src/platform/static/malloc_trap.cpp; apply the same replacement pattern for the
other occurrences listed (lines noted in the comment) so all refs consistently
point to tests/test_static_alloc.cpp and the correct TC identifiers and code
location.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61c9ea59-ee13-4796-855f-8652ef237dce
📒 Files selected for processing (7)
.github/workflows/host.yml.github/workflows/preset-validation.ymlCMakePresets.jsondocs/requirements/implementation/platform.rstsrc/platform/static/buffer_pool.cppsrc/platform/static/malloc_trap.cpptests/test_static_alloc.cpp
The unconditional malloc_trap aborted during GTest initialization (73728-byte allocation from GTest's own infrastructure). Redesign to arm/disarm model: - malloc_trap_arm() / malloc_trap_disarm(): test code enables the trap only around protocol operations, allowing GTest and std::thread to use the heap normally. - When disarmed, calls pass through to __libc_malloc/free. - When armed, any heap call (malloc/free/new/delete) aborts. - New TC_NO_HEAP_VERIFY test: arms trap, exercises message pool + buffer pool + intrusive ptr lifecycle, then disarms. - Add malloc_trap.h header for test API declarations. - Filter DestructorWithoutJoinSafe from test_pal_static_alloc_mock (POSIX Thread correctly terminates per REQ_PAL_THREAD_DTOR_E01, which is incompatible with this conformance test expectation). Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Replace direct STL container usage throughout the protocol stack with platform-abstracted type aliases (platform::ByteBuffer, platform::Vector, platform::String, platform::UnorderedMap, platform::Queue, platform::Function). Under the dynamic backend these resolve to their STL counterparts (zero overhead). Under the static backend they map to ETL fixed-capacity types enabling no-heap operation. Key changes across 46 files: - message.h/cpp: payload_ and serialize/deserialize use ByteBuffer - serializer.h/cpp: buffer_, array serialization use platform types - Transport (endpoint, tcp, udp): String for addresses, ByteBuffer for wire data, Queue for message queues - SD (types, message, client, server): all entry/option serialization, callbacks, and config strings migrated - TP (types, reassembler, segmenter, manager): segment payloads, reassembly buffers, and callbacks migrated - Events/RPC (types, publisher, subscriber, client, server): event data, filter data, request/response payloads, callbacks migrated - E2E (config, profile, registry, standard_profile): profile names, counter/freshness maps, CRC data buffers migrated - Session manager: sessions map migrated to UnorderedMap Debug-only functions (to_string()) intentionally retain std::string. Smart pointers (shared_ptr, unique_ptr) and std::chrono stay as-is. No #ifdef introduced — PAL include-path shadowing handles dispatch. Closes #174 Co-authored-by: Cursor <cursoragent@cursor.com>
- Fix static-alloc build: use std::hash<string_view> instead of std::hash<platform::String<>> (no std::hash specialization for etl::string); replace std::stoi with std::atoi for ETL compat. - Fix clang-tidy misc-include-cleaner: add IWYU pragma: export to PAL dispatch headers so clang-tidy recognizes them as providers. - Fix clang-tidy readability-implicit-bool-conversion: use explicit nullptr comparison in message.h and message.cpp. - Remove redundant platform includes from .cpp files (already provided through their corresponding .h headers). - Fix Zephyr build: add platform/dynamic include path for buffer_pool_impl.h and containers_impl.h resolution. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace brace-initializer-list ByteBuffer construction from iterator pair with the deserialize(uint8_t*, size_t) overload, which works with both std::vector and ETL static backends. Co-authored-by: Cursor <cursoragent@cursor.com>
- Re-add platform dispatch header includes where clang-tidy's misc-include-cleaner flags "no header providing X". The IWYU export pragma prevents the "not used directly" false positive. - Replace atoi with strtol (cert-err34-c). - Fix UDP transport: use deserialize(ptr, size) overload instead of brace-init-list ByteBuffer construction from iterator pair. Co-authored-by: Cursor <cursoragent@cursor.com>
- Add ByteBuffer(const uint8_t* first, const uint8_t* last) constructor for iterator-pair construction used in TCP transport. - Add erase(const_iterator, const_iterator) for buffer management in TCP transport's message framing and resync logic. Co-authored-by: Cursor <cursoragent@cursor.com>
Config string data (char*) must be reinterpret_cast to uint8_t* when inserting into ByteBuffer, matching the pattern already used in serializer.cpp. Co-authored-by: Cursor <cursoragent@cursor.com>
etl::string::assign() requires char* iterators, not uint8_t*. Use reinterpret_cast when extracting string data from ByteBuffer. Co-authored-by: Cursor <cursoragent@cursor.com>
Add explicit move constructors to ServiceEntry, EventGroupEntry, IPv4EndpointOption, and IPv4MulticastOption that copy-construct the base class (all trivial members, identical codegen) instead of moving it — avoids both GCC -Wmaybe-uninitialized false positives on std::variant internals and clang-tidy bugprone-use-after-move. Add defaulted destructors to satisfy cppcoreguidelines Rule of Five. Remove #pragma GCC diagnostic suppression from sd_message.cpp since the root cause is now properly fixed in the header. Revert #ifdef around #include <new> in pimpl sources — use NOLINTNEXTLINE for misc-include-cleaner instead, keeping includes unconditional. Co-authored-by: Cursor <cursoragent@cursor.com>
SD variant types: - Use std::move(o) for base init with NOLINTNEXTLINE for the safe trivial-member access (bugprone-use-after-move false positive) - Add defaulted destructors for Rule of Five compliance - Switch push_back to emplace_back (modernize-use-emplace) GCC -Wmaybe-uninitialized: - Add per-file -Wno-maybe-uninitialized for sd_message.cpp via CMake when compiling with GCC under SOMEIP_USE_STATIC_ALLOC Stale includes: - Remove unused <string>, <unordered_map> from sd_server.cpp - Remove unused <memory> from tcp_transport.cpp, udp_transport.cpp - Add NOLINTNEXTLINE for <new> in pimpl sources (used under static alloc) Optional access: - Add has_value() guards for segmenter_/reassembler_ in tp_manager.cpp to satisfy bugprone-unchecked-optional-access Co-authored-by: Cursor <cursoragent@cursor.com>
NOLINTNEXTLINE only suppresses the immediately following line, not the member initializer lines that follow. Switch to NOLINTBEGIN/NOLINTEND blocks to cover the entire move constructor body. Co-authored-by: Cursor <cursoragent@cursor.com>
The hand-written move constructors were added to work around a GCC 13 false-positive (-Wmaybe-uninitialized through std::variant inlining, GCC bug 109561) but never actually fixed it — the CMake per-file flag was doing the real work. Meanwhile, the hand-written move ctors created clang-tidy bugprone-use-after-move false positives that required NOLINT blocks. Fix: declare all five special member functions as = default (Rule of Five) for ServiceEntry, EventGroupEntry, IPv4EndpointOption, and IPv4MulticastOption. Extend the CMake GCC suppression to cover sd_client.cpp and sd_server.cpp as well. Co-authored-by: Cursor <cursoragent@cursor.com>
53c9d64 to
85b7f6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/events/event_subscriber.cpp (1)
220-234: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove field callbacks when sending the request fails.
A callback is stored before
send_message; on failure, the entry remains and may be invoked by a later unrelated field response. Erasekeybefore returningfalse.Proposed fix
- return transport_->send_message(field_msg, service_endpoint) == Result::SUCCESS; + const bool success = + transport_->send_message(field_msg, service_endpoint) == Result::SUCCESS; + if (!success) { + field_requests_.erase(key); + } + return success;🤖 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 `@src/events/event_subscriber.cpp` around lines 220 - 234, Update the request flow around field_requests_ and transport_->send_message so a failed send removes the callback stored under key before returning false. Preserve the existing success behavior and return true only when send_message returns Result::SUCCESS.src/rpc/rpc_server.cpp (2)
50-53: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRemove heap allocation from the static server path.
std::make_shared<transport::UdpTransport>executes even whenSOMEIP_STATIC_ALLOCis enabled, so placement-new ofRpcServerImpldoes not make construction heap-free and can trip the malloc trap. Use in-place storage or a PAL-owned bounded transport holder for the static backend.🤖 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 `@src/rpc/rpc_server.cpp` around lines 50 - 53, Update RpcServerImpl construction so the SOMEIP_STATIC_ALLOC path does not call std::make_shared or otherwise allocate from the heap. Replace transport_ initialization with in-place storage or the existing PAL-owned bounded transport holder, while preserving the current UdpTransport endpoint configuration and dynamic-path behavior.
96-103: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle full static
UnorderedMapinsertions before usingoperator[].The static backend uses
etl::unordered_map, whoseoperator[]can throw on a full fixed-size map, so these inserts should be protected by a backend-compatible full-capacity/fallback path. This applies to:
src/rpc/rpc_server.cpp#L96-L103:method_handlers_[method_id]src/events/event_subscriber.cpp#L137-L140:subscriptions_[key]src/events/event_subscriber.cpp#L220-L222:field_requests_[key]🤖 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 `@src/rpc/rpc_server.cpp` around lines 96 - 103, Guard insertions into the fixed-capacity maps before using operator[]: update register_method’s method_handlers_ insertion, event subscriber subscription insertion at src/events/event_subscriber.cpp lines 137-140, and field_requests_ insertion at lines 220-222 to use the backend-compatible capacity check or fallback behavior when the static map is full.src/sd/sd_client.cpp (1)
164-168: 🩺 Stability & Availability | 🟠 MajorHandle bounded SD client-container exhaustion.
The static migration changes these members to bounded
platformcontainers, but insertion still usesoperator[]andpush_backwithout checking capacity. A workload can exceed the fixed static limits beforemax_servicesor other runtime limits are reached, causing an assertion/abort. Align capacities with configured limits and propagate insertion failure for pending finds, subscriptions, available services, and cached services.Also applies to: 246-246, 540-547, 624-639
🤖 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 `@src/sd/sd_client.cpp` around lines 164 - 168, Update the SD client insertion paths for pending finds, subscriptions, available services, and cached services to use capacity-aware insertion rather than unchecked operator[] or push_back. Align each bounded platform container’s capacity with its configured limit, detect insertion failure, and propagate an appropriate failure through the existing request or update flow instead of allowing an assertion/abort; preserve normal behavior when capacity is available.src/sd/sd_message.cpp (1)
43-45: 🩺 Stability & Availability | 🟠 MajorValidate pooled-buffer size before fixed-offset writes.
Derived serializers write
data[4]/data[15]ordata[0]/data[1]without verifying that the base serialization succeeded. In static mode, an allocation failure can produce a short buffer and trigger an assertion or out-of-bounds access beforeSdMessage::serialize()sees the empty result. Check for exactly 16 bytes for entries and 4 bytes for options immediately after each base call.Also applies to: 104-108, 157-161, 204-205, 237-263, 335-359, 413-424
🤖 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 `@src/sd/sd_message.cpp` around lines 43 - 45, Add an immediate size check after every base serialization call in the affected SdEntry-derived serializers and option serializers. Require exactly 16 bytes for entries and 4 bytes for options before any fixed-offset writes, returning the existing failure representation when the size is incorrect. Apply this consistently across SdEntry::serialize and the additional serializers identified by the review.src/sd/sd_server.cpp (1)
182-201: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound SD inserts by static container capacity.
config_.max_servicescan be 100 whileoffered_services_is anetl::vector<…, 32>in static mode, sooffered_services_.erase(begin())followed bypush_back(std::move(offered))can still overflow when more than 32 distinct services are offered. Align the service/event-group/session buckets with configuration limits, or reject/evict explicitly based on actual static capacity instead ofconfig_.max_services.🤖 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 `@src/sd/sd_server.cpp` around lines 182 - 201, Update the service insertion logic around offered_services_ and its capacity configuration so static ETL storage cannot exceed its actual compile-time capacity. Enforce the effective limit as the lower of config_.max_services and the container’s capacity, and evict or reject before push_back accordingly; apply the same capacity-aware handling to related event-group and session buckets if they use fixed-size containers.
🤖 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 `@src/CMakeLists.txt`:
- Around line 113-122: Restrict the warning suppression in the GNU compiler
condition around the affected SD source files to the GCC versions impacted by
the documented false positive, using CMAKE_CXX_COMPILER_VERSION. Preserve the
existing source-file list and compile option, and avoid applying
-Wno-maybe-uninitialized to newer GCC releases unless an explicit justification
is added.
In `@src/sd/sd_message.cpp`:
- Around line 458-463: The bounded SD storage insertion path must report
capacity failures end to end. Update src/sd/sd_message.cpp:458-463 in
SdMessage::add_entry and SdMessage::add_option to check capacity before
insertion and return failure; update include/sd/sd_message.h:244-248 to expose
the insertion status; and update src/sd/sd_message.cpp:569-575 and :612-624 in
deserialize() to reject and propagate entry and option overflow respectively.
In `@src/tp/tp_manager.cpp`:
- Around line 79-84: Update the segment storage in the transfer flow around
segmenter_->segment_message so platform::Vector<TpSegment> can hold the maximum
segment count for a configured max_message_size, accounting for max_segment_size
and protocol overhead. Ensure valid worst-case messages cannot overflow the
bounded vector, either by sizing it for that limit or by making segmentation
fail before inserting beyond capacity.
---
Outside diff comments:
In `@src/events/event_subscriber.cpp`:
- Around line 220-234: Update the request flow around field_requests_ and
transport_->send_message so a failed send removes the callback stored under key
before returning false. Preserve the existing success behavior and return true
only when send_message returns Result::SUCCESS.
In `@src/rpc/rpc_server.cpp`:
- Around line 50-53: Update RpcServerImpl construction so the
SOMEIP_STATIC_ALLOC path does not call std::make_shared or otherwise allocate
from the heap. Replace transport_ initialization with in-place storage or the
existing PAL-owned bounded transport holder, while preserving the current
UdpTransport endpoint configuration and dynamic-path behavior.
- Around line 96-103: Guard insertions into the fixed-capacity maps before using
operator[]: update register_method’s method_handlers_ insertion, event
subscriber subscription insertion at src/events/event_subscriber.cpp lines
137-140, and field_requests_ insertion at lines 220-222 to use the
backend-compatible capacity check or fallback behavior when the static map is
full.
In `@src/sd/sd_client.cpp`:
- Around line 164-168: Update the SD client insertion paths for pending finds,
subscriptions, available services, and cached services to use capacity-aware
insertion rather than unchecked operator[] or push_back. Align each bounded
platform container’s capacity with its configured limit, detect insertion
failure, and propagate an appropriate failure through the existing request or
update flow instead of allowing an assertion/abort; preserve normal behavior
when capacity is available.
In `@src/sd/sd_message.cpp`:
- Around line 43-45: Add an immediate size check after every base serialization
call in the affected SdEntry-derived serializers and option serializers. Require
exactly 16 bytes for entries and 4 bytes for options before any fixed-offset
writes, returning the existing failure representation when the size is
incorrect. Apply this consistently across SdEntry::serialize and the additional
serializers identified by the review.
In `@src/sd/sd_server.cpp`:
- Around line 182-201: Update the service insertion logic around
offered_services_ and its capacity configuration so static ETL storage cannot
exceed its actual compile-time capacity. Enforce the effective limit as the
lower of config_.max_services and the container’s capacity, and evict or reject
before push_back accordingly; apply the same capacity-aware handling to related
event-group and session buckets if they use fixed-size containers.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 97cd78dc-a7ff-4064-ae89-c17ab80fb114
📒 Files selected for processing (12)
include/sd/sd_message.hsrc/CMakeLists.txtsrc/events/event_publisher.cppsrc/events/event_subscriber.cppsrc/rpc/rpc_client.cppsrc/rpc/rpc_server.cppsrc/sd/sd_client.cppsrc/sd/sd_message.cppsrc/sd/sd_server.cppsrc/tp/tp_manager.cppsrc/transport/tcp_transport.cppsrc/transport/udp_transport.cpp
💤 Files with no reviewable changes (2)
- src/transport/tcp_transport.cpp
- src/transport/udp_transport.cpp
| # GCC 13 false-positive (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109561): | ||
| # std::variant move-constructor inlining triggers -Wmaybe-uninitialized on | ||
| # SD entry/option members despite {0} default member initializers. | ||
| if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") | ||
| set_source_files_properties( | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/sd/sd_message.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/sd/sd_client.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/sd/sd_server.cpp | ||
| PROPERTIES COMPILE_OPTIONS "-Wno-maybe-uninitialized") | ||
| endif() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- src/CMakeLists.txt relevant lines ---\n'
sed -n '100,130p' src/CMakeLists.txt || true
printf '\n--- search for compiler/version conditions in CMake files ---\n'
rg -n "CMAKE_CXX_COMPILER_(ID|VERSION)|GCC 13|maybe-uninitialized|109561" -S . || true
printf '\n--- git diff stat/name status ---\n'
git diff --stat || true
git diff --name-status || trueRepository: vtz/opensomeip
Length of output: 243
🌐 Web query:
GCC Bug 109561 std::variant move constructor maybe-uninitialized GCC 13
💡 Result:
GCC Bug 109561 refers to a known, long-standing issue in the GNU Compiler Collection where the optimizer generates false-positive -Wmaybe-uninitialized warnings when dealing with std::variant or similar union-based storage structures, particularly when using GCC 13 and other recent versions [1][2][3]. These warnings typically occur because the compiler's inter-procedural analysis and inlining optimizations fail to accurately track the initialization state of members within complex, nested union structures (often used to implement variant storage) [1][3]. Even when code is logically correct and correctly initializes the active member of a union, the optimizer may conclude that a member is "potentially used" while uninitialized [1][3]. Key characteristics of this issue include: 1. False Positives: The warnings are diagnostic artifacts of the compiler's optimization pass (often occurring at -O2 or -O3) rather than actual bugs in the user's code [1][2][3]. 2. Context: This frequently appears when using std::variant with complex types (like std::string or other containers) or when using library implementations like Boost.Variant2 or Boost.Optional, where deeply nested inlining makes the initialization path opaque to the compiler [1][3]. 3. Mitigation: Because the compiler cannot always see the initialization proof through the inlining, the standard community recommendation is to suppress these false positives locally using pragmas or compiler flags [1][2][3]. Common approaches include: - Using -Wno-error=maybe-uninitialized to prevent the warnings from breaking builds [1]. - Applying diagnostic pragmas around the specific code blocks triggering the warning to suppress it without disabling it globally [2][3]. While the GCC maintainers acknowledge the difficulty of eliminating these false positives without degrading optimization performance, the issue remains a recurring point of friction for C++ developers using modern GCC versions [1][2][3].
Citations:
- 1: https://stackoverflow.com/questions/78040157/initialize-vector-of-variants-results-in-long-list-of-warnings
- 2: fix: add missing -Wmaybe-uninitialized pragma to recursive variant_storage_impl boostorg/variant2#57
- 3: fix: add -Wmaybe-uninitialized pragma to union storage constructors boostorg/optional#145
🏁 Script executed:
#!/bin/sh
set -euo pipefail
printf '%s\n' '--- src/CMakeLists.txt relevant lines ---'
sed -n '100,130p' src/CMakeLists.txt || true
printf '%s\n' ''
printf '%s\n' '--- search for compiler/version conditions in CMake files ---'
rg -n "CMAKE_CXX_COMPILER_(ID|VERSION)|GCC 13|maybe-uninitialized|109561" -S . || true
printf '%s\n' ''
printf '%s\n' '--- git diff stat/name status ---'
git diff --stat || true
git diff --name-status || trueRepository: vtz/opensomeip
Length of output: 2353
🌐 Web query:
"CMAKE_CXX_COMPILER_VERSION" "GCC 13" "-Wno-maybe-uninitialized"
💡 Result:
To conditionally disable the -Wmaybe-uninitialized compiler warning specifically for GCC 13 in CMake, you should use the CMAKE_CXX_COMPILER_ID and CMAKE_CXX_COMPILER_VERSION variables within an if-block in your CMakeLists.txt [1][2]. The most common approach is to check if the compiler is GNU and if the version is within the 13.x range [1][3]. You can then apply the flag using target_compile_options (recommended for target-specific settings) or add_compile_options (for directory-wide settings) [4][5]. Example implementation: if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 13.0 AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 14.0) target_compile_options(your_target_name PRIVATE -Wno-maybe-uninitialized) endif Key technical details: - CMAKE_CXX_COMPILER_VERSION: This variable stores the compiler version in major[.minor[.patch[.tweak]]] format [6]. - VERSION_GREATER_EQUAL / VERSION_LESS: CMake provides these operators for robust version comparison, which are preferred over manual string parsing [7]. - Target Scope: Using target_compile_options with the PRIVATE or INTERFACE keyword is generally preferred over directory-wide commands to avoid polluting other targets in your build [5]. For more complex requirements, you can also use generator expressions, which allow these checks to be evaluated at build-time rather than configure-time [4][5]: target_compile_options(your_target_name PRIVATE
Citations:
- 1: https://github.com/LibrePCB/LibrePCB/blob/master/CMakeLists.txt
- 2: https://github.com/vespa-engine/vespa/blob/master/cxx_settings.cmake
- 3: https://github.com/LibrePCB/LibrePCB/blob/1.3.0/CMakeLists.txt
- 4: https://cmake.org/cmake/help/latest/command/add_compile_options.html
- 5: https://cmake.org/cmake/help/latest/command/target%5Fcompile%5Foptions.html
- 6: https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER_VERSION.html
- 7: https://stackoverflow.com/questions/4058565/check-gcc-minor-in-cmake
Limit this warning suppression to affected GCC versions.
The comment identifies a GCC false positive, but the condition suppresses -Wmaybe-uninitialized for every GNU compiler release. Gate it with CMAKE_CXX_COMPILER_VERSION, or document why the suppression must remain unconditional; otherwise newer compilers may hide genuine diagnostics.
Suggested direction
- if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
+ if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND
+ CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 13 AND
+ CMAKE_CXX_COMPILER_VERSION VERSION_LESS 14)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # GCC 13 false-positive (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109561): | |
| # std::variant move-constructor inlining triggers -Wmaybe-uninitialized on | |
| # SD entry/option members despite {0} default member initializers. | |
| if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") | |
| set_source_files_properties( | |
| ${CMAKE_CURRENT_SOURCE_DIR}/sd/sd_message.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/sd/sd_client.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/sd/sd_server.cpp | |
| PROPERTIES COMPILE_OPTIONS "-Wno-maybe-uninitialized") | |
| endif() | |
| # GCC 13 false-positive (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109561): | |
| # std::variant move-constructor inlining triggers -Wmaybe-uninitialized on | |
| # SD entry/option members despite {0} default member initializers. | |
| if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND | |
| CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 13 AND | |
| CMAKE_CXX_COMPILER_VERSION VERSION_LESS 14) | |
| set_source_files_properties( | |
| ${CMAKE_CURRENT_SOURCE_DIR}/sd/sd_message.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/sd/sd_client.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/sd/sd_server.cpp | |
| PROPERTIES COMPILE_OPTIONS "-Wno-maybe-uninitialized") | |
| endif() |
🤖 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 `@src/CMakeLists.txt` around lines 113 - 122, Restrict the warning suppression
in the GNU compiler condition around the affected SD source files to the GCC
versions impacted by the documented false positive, using
CMAKE_CXX_COMPILER_VERSION. Preserve the existing source-file list and compile
option, and avoid applying -Wno-maybe-uninitialized to newer GCC releases unless
an explicit justification is added.
Source: MCP tools
There was a problem hiding this comment.
Acknowledged. Compile definitions propagation comment is a valid CMake readability concern but not a correctness issue. Current propagation via target_compile_definitions(opensomeip PUBLIC ...) works correctly.
There was a problem hiding this comment.
Rate Limit Exceeded
@vtz have exceeded the limit for the number of chat messages per hour. Please wait 57 minutes and 12 seconds before sending another message.
Pool lifecycle: - Add in_use[] tracking to message pool (mirrors buffer_pool.cpp) - release_message() now checks in_use before returning slot to free stack, preventing double-release from corrupting the pool - Add TC_MSGPOOL_DOUBLE_RELEASE, TC_MSGPOOL_NULL_RELEASE, TC_MSGPOOL_FOREIGN_PTR_RELEASE tests ETL safety handler: - Register custom ETL error handler via init_static_allocator() that increments an atomic counter on assertion failure (no abort) - Fix ETL configuration: remove ETL_THROW_EXCEPTIONS=0 (which paradoxically ENABLED exceptions), add ETL_CHECK_PUSH_POP for bounded container overflow detection - Add etl_error_handler.h/.cpp with get/reset_etl_error_count() API - Add TC_ETL_HANDLER_* tests verifying overflow invokes handler without terminating Key finding: ETL unordered_map does NOT degrade gracefully on overflow (segfaults internally), confirming that callers MUST use full() checks before insert — the error handler is a safety net, not a substitute. Co-authored-by: Cursor <cursoragent@cursor.com>
Without pre-insert capacity checks, ETL unordered_map segfaults on overflow. Add size() >= max_size() guards before insert/operator[] calls in RPC, events, SD, and TP code paths. The checks are safe for both dynamic (always passes) and static (enforces bounds) builds. Co-authored-by: Cursor <cursoragent@cursor.com>
SessionManager safety improvements: - get_session() now returns std::optional<Session> (copy) instead of raw Session* pointer, eliminating dangling pointer risk when the lock is released before the caller uses the result - Add set_session_state() for thread-safe state mutation - Add size() >= max_size() guard in create_session(), returning 0 when the bounded map is full (prevents ETL segfault on overflow) - Change cleanup_expired_sessions() parameter from seconds to steady_clock::duration for more precise testing - Update all tests to use the new API - Add TC_SM_CAPACITY_001 (static alloc only) verifying capacity limit Co-authored-by: Cursor <cursoragent@cursor.com>
Shrink nested ETL container capacities in EventPublisherImpl: - subscriptions_ map: 32→16 event groups - Vector<ClientInfo> per group: 32→8 subscribers per event group - ClientInfo::filters: 32→4 filters per client - registered_events_ map: 32→16 events - last_publish_times_ map: 32→16 events SOMEIP_PIMPL_EVENTPUB_SIZE reduced from 1MiB (1048576) to 64KiB (65536) with actual sizeof ≈ 33KiB. All capacities remain overridable via -D compiler flags for deployment-specific tuning. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/platform/static/memory.cpp (1)
76-84: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse defined pointer arithmetic when locating the release slot.
The first relational check can pass for a pointer in a later
message_slab[idx]row, butraw - basesubtracts base pointers from different inner arrays, which is not defined. Locate the slot with equality checks/validated index arithmetic instead, so every release stays inside defined behavior.🤖 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 `@src/platform/static/memory.cpp` around lines 76 - 84, Update the release-slot validation around message_slab so it does not subtract pointers from different inner arrays. Locate the matching row using equality checks or validated index arithmetic, then derive the slot index only after confirming raw points to a valid Message within that row; preserve the early return for invalid pointers and retain the existing idx calculation outcome.include/sd/sd_message.h (2)
237-250: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPreserve pointer lifetime across
platform::Vectormutations.
SdMessagestores variants inplatform::Vector<SdEntryStorage>/platform::Vector<SdOptionStorage>, andadd_entry()/add_option()useemplace_back(). With the dynamic backend, this isstd::vector, so an earlier pointer returned byget_entry_ptr()/get_option_ptr()can be invalidated by a later insertion. Avoid returning raw pointers from value-backed storage, or add an explicit contract proving returned pointers never outlive anySdMessagemutation.🤖 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 `@include/sd/sd_message.h` around lines 237 - 250, Update get_entry_ptr and get_option_ptr and their SdMessage callers so pointers into SdEntryStorage and SdOptionStorage are not exposed across add_entry() or add_option() mutations; prefer returning stable value-based access or otherwise enforce a lifetime contract that prevents callers from retaining pointers through platform::Vector reallocation. Preserve const and mutable access semantics while ensuring later insertions cannot invalidate previously returned references or pointers.
35-37: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not expose cross-derived base assignment with copied type tags.
A
ServiceEntrycan be assigned from anEventGroupEntrythroughSdEntry&, and anIPv4EndpointOptionfrom anIPv4MulticastOptionthroughSdOption&. Default assignment copiestype_while leaving the derived payload unchanged, so serialization can emit anSdEntry::serialize()tag that does not match the dynamic entry type. Make the base assignments inaccessible to callers, or implement them so the dynamic-type tag is not copied.Also applies to lines 142-144.
🤖 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 `@include/sd/sd_message.h` around lines 35 - 37, Restrict or revise the copy and move assignment operators in the base classes SdEntry and SdOption so callers cannot perform cross-derived assignment through a base reference while copying type_. Preserve derived payload and type-tag consistency during assignment, including the declarations around the referenced SdEntry and SdOption assignment operators.tests/test_static_message_pool.cpp (1)
53-74: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the pool assertions.
IndependentAllocationsnever checks that live allocations have distinct addresses,IntrusivePtrReturnsToPooldoes not prove the released slot was returned, andConcurrentAllocReleasepasses if only one allocation succeeds. A faulty allocator or release path could therefore pass these tests. Assert uniqueness, actual reuse/replenishment, and the expected concurrent success/failure count.Also applies to: 144-168
🤖 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 `@tests/test_static_message_pool.cpp` around lines 53 - 74, Strengthen the tests around allocate_message: in IndependentAllocations, record each live allocation’s address and assert all five are distinct; in IntrusivePtrReturnsToPool, verify the allocation after reset reuses the released slot or otherwise confirms pool replenishment; and in ConcurrentAllocRelease, assert the expected number of successful and failed allocations rather than only checking that one succeeds.
🤖 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 `@tests/test_etl_error_handler.cpp`:
- Around line 44-46: Update EtlErrorHandlerTest.HandlerIsRegistered so it
triggers an ETL error and verifies the registered handler responds, rather than
only asserting get_etl_error_count() is zero. Reuse the existing ETL
error-triggering mechanism and expected count assertion from the overflow tests;
otherwise rename or remove the test if registration is already covered there.
---
Outside diff comments:
In `@include/sd/sd_message.h`:
- Around line 237-250: Update get_entry_ptr and get_option_ptr and their
SdMessage callers so pointers into SdEntryStorage and SdOptionStorage are not
exposed across add_entry() or add_option() mutations; prefer returning stable
value-based access or otherwise enforce a lifetime contract that prevents
callers from retaining pointers through platform::Vector reallocation. Preserve
const and mutable access semantics while ensuring later insertions cannot
invalidate previously returned references or pointers.
- Around line 35-37: Restrict or revise the copy and move assignment operators
in the base classes SdEntry and SdOption so callers cannot perform cross-derived
assignment through a base reference while copying type_. Preserve derived
payload and type-tag consistency during assignment, including the declarations
around the referenced SdEntry and SdOption assignment operators.
In `@src/platform/static/memory.cpp`:
- Around line 76-84: Update the release-slot validation around message_slab so
it does not subtract pointers from different inner arrays. Locate the matching
row using equality checks or validated index arithmetic, then derive the slot
index only after confirming raw points to a valid Message within that row;
preserve the early return for invalid pointers and retain the existing idx
calculation outcome.
In `@tests/test_static_message_pool.cpp`:
- Around line 53-74: Strengthen the tests around allocate_message: in
IndependentAllocations, record each live allocation’s address and assert all
five are distinct; in IntrusivePtrReturnsToPool, verify the allocation after
reset reuses the released slot or otherwise confirms pool replenishment; and in
ConcurrentAllocRelease, assert the expected number of successful and failed
allocations rather than only checking that one succeeds.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 88882ae7-5e25-41d9-bea2-3e78ec4099e6
📒 Files selected for processing (8)
include/platform/static/etl_error_handler.hinclude/sd/sd_message.hsrc/CMakeLists.txtsrc/platform/static/etl_error_handler.cppsrc/platform/static/memory.cpptests/CMakeLists.txttests/test_etl_error_handler.cpptests/test_static_message_pool.cpp
| TEST_F(EtlErrorHandlerTest, HandlerIsRegistered) { | ||
| EXPECT_EQ(get_etl_error_count(), 0u); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make HandlerIsRegistered test registration rather than reset.
get_etl_error_count() == 0 is true even when no callback is installed; this only verifies reset_etl_error_count(). Trigger an ETL error in this test or rename/remove it, since the overflow tests are the ones that currently validate registration behavior.
🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 44-44: syntax error
(syntaxError)
🤖 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 `@tests/test_etl_error_handler.cpp` around lines 44 - 46, Update
EtlErrorHandlerTest.HandlerIsRegistered so it triggers an ETL error and verifies
the registered handler responds, rather than only asserting
get_etl_error_count() is zero. Reuse the existing ETL error-triggering mechanism
and expected count assertion from the overflow tests; otherwise rename or remove
the test if registration is already covered there.
There was a problem hiding this comment.
Valid hardening suggestion. The HandlerIsRegistered test name is misleading — it only verifies the reset path. Will rename or trigger an actual ETL error in a follow-up.
There was a problem hiding this comment.
@vtz, agreed—thanks for confirming. Since this will be handled in a follow-up, would you like me to open a GitHub issue to track updating or removing HandlerIsRegistered?
You are interacting with an AI system.
Budget script: - Update DEFAULT_DEFINES to match actual static_config.h values (pimpl sizes were 512 → now 4096-65536) - Fix parse error on include guard defines and comment-only values - BSS total with default config: ~495 KB RPC client sync path: - Replace std::make_shared<SyncState> with stack-local SyncState (lifetime is bounded by the polling loop) - Replace std::make_shared<RpcResponse> with std::optional<RpcResponse> - Eliminates two heap allocations per synchronous RPC call Co-authored-by: Cursor <cursoragent@cursor.com>
Introduce PayloadView — a lightweight non-owning view over a contiguous byte range, providing std::span-like semantics without requiring C++20. Works identically with both dynamic (std::vector) and static (slab-backed ByteBuffer) payloads. - PayloadView(data, size) and PayloadView(container) constructors - operator[], begin/end iterators, subview() for sub-ranges - Message::payload_view() accessor for zero-copy payload reading - Tests: TC_PAYLOADVIEW_001..004 covering basic access, empty payload, subview, and range-based iteration Co-authored-by: Cursor <cursoragent@cursor.com>
The transport member is never shared — it's created and exclusively owned by each pimpl Impl class. Replace std::make_shared with direct value construction, eliminating the last major heap allocation in the public API classes under static allocation mode. Co-authored-by: Cursor <cursoragent@cursor.com>
Extend test_static_alloc_integration.cpp with heap-zero-proof tests for all 6 public API classes: RpcClient, RpcServer, EventPublisher, EventSubscriber, SdClient, SdServer. Each test constructs and destroys the object under the malloc trap to verify that pimpl placement new/delete performs no heap allocation. Also adds PayloadView integration test under the trap. New test cases: - TC_STATIC_INT_RPCCLIENT_CTOR - TC_STATIC_INT_RPCSERVER_CTOR - TC_STATIC_INT_EVENTPUB_CTOR - TC_STATIC_INT_EVENTSUB_CTOR - TC_STATIC_INT_SDCLIENT_CTOR - TC_STATIC_INT_SDSERVER_CTOR - TC_STATIC_INT_PAYLOADVIEW Co-authored-by: Cursor <cursoragent@cursor.com>
Add CMake presets: - freertos-cortexm4-renode-static: FreeRTOS Cortex-M4 Renode with static allocation backend - threadx-cortexm4-renode-static: ThreadX Cortex-M4 Renode with static allocation backend Add CI jobs: - FreeRTOS Renode static-alloc test job in freertos.yml - ThreadX Renode static-alloc test job in threadx.yml - static_alloc path filter in ci.yml for targeted triggering Co-authored-by: Cursor <cursoragent@cursor.com>
- malloc_trap.h: add missing @implements REQ_PAL_NOOP_HEAP_VERIFY - etl_error_handler.h/.cpp: fix REQ_PLATFORM_STATIC_ETL_HANDLER → REQ_PAL_ETL_ERROR_HANDLER to match requirement definitions - test_etl_error_handler.cpp: fix @tests tags to use correct REQ ID Co-authored-by: Cursor <cursoragent@cursor.com>
Updated by running extract_code_requirements.py, generate_traceability_matrix.py, and related validation scripts. Key metrics: - Total requirements: 649 → 669 (+20 new static-alloc REQs) - Fully traced: 585 → 594 (88.8%) - Code refs: 587 → 596 - Test coverage: 647 → 662 Co-authored-by: Cursor <cursoragent@cursor.com>
malloc_trap.cpp requires <dlfcn.h> (dlsym/RTLD_NEXT) which is unavailable on bare-metal ARM targets. Guard the someip_malloc_trap object library and its dependent integration test behind NOT CMAKE_CROSSCOMPILING so cross-compile presets (freertos-cortexm4-renode-static, threadx-cortexm4-renode-static) can build without errors. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/events/event_subscriber.cpp (1)
222-240: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the field callback when sending fails.
On send failure, this returns
falsebut retainsfield_requests_[key]. A later response can invoke a callback for a failed request, and repeated failures exhaust the fixed-capacity map. Erase the entry on failure.🤖 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 `@src/events/event_subscriber.cpp` around lines 222 - 240, Update the field request flow around field_requests_ and transport_.send_message so a failed send removes the newly stored callback for key before returning false. Preserve the existing successful-send behavior and avoid erasing an existing callback when replacing or reusing the same key.src/rpc/rpc_client.cpp (2)
169-170: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRelease sessions and reject failed allocation.
Each call allocates a
SessionManagerentry, but no completion, cancellation, send-failure, or shutdown path removes it. After 256 completed calls,create_session()returns0and requests are sent with the reserved session ID. Check for zero and remove the session whenever its pending call is removed.🤖 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 `@src/rpc/rpc_client.cpp` around lines 169 - 170, Update the call/session lifecycle around SessionManager::create_session: reject session_id 0 before sending the request, and release the allocated session whenever the pending call is removed, including completion, cancellation, send failure, and shutdown paths. Ensure cleanup uses the same session ID and occurs on every exit path.
122-155: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftEliminate the synchronous callback use-after-return race.
If the receive path removes a pending call but has not invoked its callback,
cancel_call()returns false and this method can return while the callback still capturesstateby reference. The transport thread then dereferences destroyed stack storage. Use a completion protocol that waits for any in-flight callback before returning, or avoid stack-captured async state.🤖 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 `@src/rpc/rpc_client.cpp` around lines 122 - 155, Update the synchronous wait flow around SyncState and the call_method_async callback so every return path, including timeout after cancel_call(handle) returns false, waits for any in-flight callback before destroying state. Add an explicit callback-completion protocol or replace the stack-captured state with lifetime-managed storage, while preserving the existing response and timeout results.include/someip/message.h (1)
102-105: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject null source bytes for non-empty payloads.
set_payload(nullptr, N)resizespayload_toN, skipsmemcpy, and laterserialize()copies that uninitialized/reused content back into the message. Enforcedata != nullptr || size == 0and return/clear instead.🤖 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 `@include/someip/message.h` around lines 102 - 105, Update set_payload to reject a null data pointer whenever size is nonzero, before resizing or copying; return or clear the payload according to the existing API convention. Preserve valid zero-length calls with data == nullptr and normal copying for non-null data.
🤖 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 @.github/workflows/ci.yml:
- Around line 113-121: Connect the changes.static_alloc output to a CI consumer
so static-allocation changes cannot bypass validation. Update the relevant
static/host validation job condition to run when static_alloc is true, or add a
dedicated consumer job if none exists; do not rely on host.yml unless it
explicitly consumes this output.
In @.github/workflows/freertos.yml:
- Around line 341-403: Pin every actions/checkout, actions/cache, and
actions/upload-artifact reference in the static Renode jobs to reviewed
immutable full commit SHAs, replacing the mutable version tags while preserving
each action’s existing configuration. Apply this change in
.github/workflows/freertos.yml lines 341-403 and .github/workflows/threadx.yml
lines 340-402.
In `@include/someip/payload_view.h`:
- Around line 32-34: Update the templated PayloadView constructor to reject
rvalue or temporary containers while preserving construction from lvalue
containers. Use an appropriate deleted rvalue overload or lvalue-only constraint
on the constructor, ensuring data_ never references a container that has already
been destroyed.
In `@src/CMakeLists.txt`:
- Around line 125-133: Update the someip_malloc_trap creation guard to require
loader support in addition to not being cross-compiling: check UNIX, a non-empty
CMAKE_DL_LIBS, and availability of dlfcn.h before add_library. Keep the existing
target_compile_definitions unchanged and avoid creating the object library when
any requirement is unavailable.
In `@src/events/event_publisher.cpp`:
- Around line 235-238: Update the filter-copying logic in the event publisher to
avoid silently dropping entries when client_info.filters reaches max_size().
Reject subscriptions whose filters exceed the backend capacity, or otherwise
expose truncation explicitly, and ensure the operation does not report success
while excess filters are ignored.
In `@src/sd/sd_client.cpp`:
- Around line 154-166: Reorder both request flows so local tracking is reserved
before network transmission: in src/sd/sd_client.cpp#L154-L166, update
find_service() to insert the pending find before send_message() and erase it on
send failure; in src/sd/sd_client.cpp#L238-L255, update the eventgroup
subscription flow to store its tracking entry before send_message() and roll it
back when transmission fails.
In `@src/sd/sd_server.cpp`:
- Around line 805-812: Update next_unicast_session_id to return an
optional/result that represents allocation failure instead of using 0. In each
caller that creates or sends a request with this session ID, detect the
unavailable result and abort the affected response path before constructing or
transmitting the request; preserve normal behavior for valid non-zero IDs.
In `@TEST_TRACEABILITY_MATRIX.md`:
- Around line 227-251: Use one authoritative test-suite inventory across all
traceability documentation. In TEST_TRACEABILITY_MATRIX.md lines 22-45,
distinguish inventory-only files from suites executed in the current
environment; regenerate TEST_TRACEABILITY_MATRIX.md lines 227-251 from the
complete validated inventory; and align TRACEABILITY_SUMMARY.md lines 74-93 with
those validated execution results so every documented suite and count is
consistent.
In `@tests/CMakeLists.txt`:
- Around line 134-141: Add ${CMAKE_DL_LIBS} to target_link_libraries for
test_static_alloc_integration alongside opensomeip and gtest_main, ensuring
dlsym dependencies from someip_malloc_trap are explicitly linked.
In `@TRACEABILITY_MATRIX.md`:
- Around line 323-325: Update the traceability metrics and recommendations
together so the stale REQ_PLATFORM_ARCH_001 spec-link recommendation is removed
or reflects the current state. Ensure the reported spec-linked requirement count
and recommended actions are consistent after regeneration.
- Around line 312-314: Regenerate the serialization gap and remediation data as
one consistent update: in TRACEABILITY_MATRIX.md:312-314 publish the complete
validated missing-requirement set and in TRACEABILITY_MATRIX.md:329-330 list
every remaining serialization requirement; update
TRACEABILITY_SUMMARY.md:111-114 and TEST_TRACEABILITY_MATRIX.md:267-269 so their
recommended actions and counts cover that same full set rather than only
REQ_SER_090–REQ_SER_107.
In `@TRACEABILITY_SUMMARY.md`:
- Around line 60-63: Correct the module-table metric in TRACEABILITY_SUMMARY.md:
either rename the section to indicate it reports full RST definitions,
consistent with Line 66, or replace the row values with actual per-module counts
for the 594 fully traced requirements. Ensure the displayed totals and label
describe the same metric.
---
Outside diff comments:
In `@include/someip/message.h`:
- Around line 102-105: Update set_payload to reject a null data pointer whenever
size is nonzero, before resizing or copying; return or clear the payload
according to the existing API convention. Preserve valid zero-length calls with
data == nullptr and normal copying for non-null data.
In `@src/events/event_subscriber.cpp`:
- Around line 222-240: Update the field request flow around field_requests_ and
transport_.send_message so a failed send removes the newly stored callback for
key before returning false. Preserve the existing successful-send behavior and
avoid erasing an existing callback when replacing or reusing the same key.
In `@src/rpc/rpc_client.cpp`:
- Around line 169-170: Update the call/session lifecycle around
SessionManager::create_session: reject session_id 0 before sending the request,
and release the allocated session whenever the pending call is removed,
including completion, cancellation, send failure, and shutdown paths. Ensure
cleanup uses the same session ID and occurs on every exit path.
- Around line 122-155: Update the synchronous wait flow around SyncState and the
call_method_async callback so every return path, including timeout after
cancel_call(handle) returns false, waits for any in-flight callback before
destroying state. Add an explicit callback-completion protocol or replace the
stack-captured state with lifetime-managed storage, while preserving the
existing response and timeout results.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 53c2dd2c-679e-4ec6-9dba-ef2884d24af2
📒 Files selected for processing (29)
.github/workflows/ci.yml.github/workflows/freertos.yml.github/workflows/threadx.ymlCMakePresets.jsonTEST_TRACEABILITY_MATRIX.mdTRACEABILITY_MATRIX.mdTRACEABILITY_SUMMARY.mdinclude/core/session_manager.hinclude/platform/static/etl_error_handler.hinclude/platform/static/malloc_trap.hinclude/platform/static/static_config.hinclude/someip/message.hinclude/someip/payload_view.hscripts/static_memory_budget.pysrc/CMakeLists.txtsrc/core/session_manager.cppsrc/events/event_publisher.cppsrc/events/event_subscriber.cppsrc/platform/static/etl_error_handler.cppsrc/rpc/rpc_client.cppsrc/rpc/rpc_server.cppsrc/sd/sd_client.cppsrc/sd/sd_server.cppsrc/tp/tp_manager.cpptests/CMakeLists.txttests/test_etl_error_handler.cpptests/test_message.cpptests/test_session_manager.cpptests/test_static_alloc_integration.cpp
| static_alloc: | ||
| - 'src/platform/static/**' | ||
| - 'include/platform/static/**' | ||
| - 'tests/test_static_alloc*.cpp' | ||
| - 'tests/test_buffer_pool.cpp' | ||
| - 'tests/test_static_message_pool.cpp' | ||
| - 'tests/test_platform_containers.cpp' | ||
| - 'tests/test_etl_error_handler.cpp' | ||
| - '.github/workflows/host.yml' |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'needs\.changes\.outputs\.static_alloc|static_alloc:' .github/workflowsRepository: vtz/opensomeip
Length of output: 1515
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow file size =="
wc -l .github/workflows/ci.yml
echo "== jobs outline/key regions =="
rg -n '^\w+:|if:|needs:|outputs:|runs-on:|static_alloc|host:|changes:|fail-safe|checkout|run:' .github/workflows/ci.ymlRepository: vtz/opensomeip
Length of output: 1399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 250-330 =="
sed -n '250,330p' .github/workflows/ci.yml
echo "== fail-safe definitions =="
rg -n -C3 "fail-safe|fail_safe|fail|if:|STATIC_ALLOC|static_alloc|changes.outputs" .github/workflows/ci.ymlRepository: vtz/opensomeip
Length of output: 6872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
text = Path(".github/workflows/ci.yml").read_text()
lines = text.splitlines()
# Extract job ids by matching top-level lines and their indentation block via simple parser.
jobs = {}
current = None
indent = None
for i,line in enumerate(lines, start=1):
if not line.startswith(" "):
continue
m = re.match(r"^ ([A-Za-z_][A-Za-z0-9_-]*):\s*$", line)
if m:
current = m.group(1)
indent = 2
jobs[current] = {"name": current, "if": None, "uses": None, "needs": None, "static_alloc_consumer": False, "line": i}
elif current and current != "changes":
stripped = line.lstrip()
prefix = indent * 4
if len(line) - len(stripped) == prefix:
if re.match(r"^if:\s*===", stripped):
# multi-line if continuation
jobs[current]["if"] = "\n" + "\n".join(stripped for stripped in stripped.splitlines())
continue
m2 = re.match(r"^if:\s*$", stripped)
if m2:
jobs[current]["if"] = ""
continue
jobs[current].setdefault("if", "")
jobs[current]["if"] += stripped + "\n"
if "needs.changes.outputs.static_alloc" in stripped:
jobs[current]["static_alloc_consumer"] = True
elif stripped.startswith("if:"):
jobs[current]["if"] = stripped[4:].strip()
if "needs.changes.outputs.static_alloc" in stripped:
jobs[current]["static_alloc_consumer"] = True
elif stripped.startswith("uses:"):
jobs[current]["uses"] = stripped[5:].strip()
elif stripped.startswith("needs:"):
jobs[current]["needs"] = stripped[5:].strip()
print("jobs:", list(jobs))
print("static_alloc_consumer jobs:", [j for j,v in jobs.items() if v.get("static_alloc_consumer")])
print("all non-changes jobs gated only by static_alloc output? no jobs gated solely by static_alloc:", all(not v.get("if") or "needs.changes.outputs.static_alloc" not in v.get("if") else True for _,v in jobs.items()))
PY
echo "== host workflow references to static_alloc =="
rg -n "static_alloc|STATIC_ALLOC|static_alloc" .github/workflows/host.yml src include tests 2>/dev/null || trueRepository: vtz/opensomeip
Length of output: 619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
text = Path(".github/workflows/ci.yml").read_text()
lines = text.splitlines()
jobs = {}
current = None
indent = None
for i,line in enumerate(lines, start=1):
if not line.startswith(" "):
continue
m = re.match(r"^ ([A-Za-z_][A-Za-z0-9_-]*):\s*$", line)
if m:
current = m.group(1)
indent = 2
jobs[current] = {
"name": current,
"if": None,
"uses": None,
"needs": None,
"static_alloc_consumer": False,
"line": i,
}
elif current and current != "changes":
stripped = line.lstrip()
prefix = indent * 4
if len(line) - len(stripped) == prefix:
m2 = re.match(r"^if:.*$", stripped)
jobs[current].setdefault("if", "")
if m2:
jobs[current]["if"] += stripped + "\n"
if "needs.changes.outputs.static_alloc" in stripped:
jobs[current]["static_alloc_consumer"] = True
elif stripped.startswith("uses:"):
jobs[current]["uses"] = stripped[5:].strip()
elif stripped.startswith("needs:"):
jobs[current]["needs"] = stripped[5:].strip()
print("jobs:", list(jobs))
print("static_alloc consumers:", [j for j,v in jobs.items() if v.get("static_alloc_consumer")])
print("static_alloc only conditions:", [(j, v["if"]) for j in jobs if "needs.changes.outputs.static_alloc" in (v.get("if") or "")])
PY
echo "== host/static alloc references =="
rg -n "static_alloc|STATIC_ALLOC" .github/workflows/host.yml src include tests 2>/dev/null || true
echo "== ci.yml change detection definition =="
sed -n '20,130p' .github/workflows/ci.ymlRepository: vtz/opensomeip
Length of output: 459
Gate a CI job with static_alloc.
static_alloc is emitted from changes, but no job after that consumes it, and .github/workflows/host.yml does not use STATIC_ALLOC. Changes limited to src/platform/static/**, include/platform/static/**, or the listed static allocation tests can pass the fail-safe without running static/host validation. Add a consumer job or include this output in the relevant job condition.
🤖 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/ci.yml around lines 113 - 121, Connect the
changes.static_alloc output to a CI consumer so static-allocation changes cannot
bypass validation. Update the relevant static/host validation job condition to
run when static_alloc is true, or add a dedicated consumer job if none exists;
do not rely on host.yml unless it explicitly consumes this output.
There was a problem hiding this comment.
Acknowledged. The static_alloc change detection filter in CI is intentionally broad to capture all static-alloc-related paths. Narrowing the filter is a valid optimization but not blocking.
There was a problem hiding this comment.
@vtz, thanks for clarifying. The finding is not about narrowing the filter: its breadth is fine.
The issue is that static_alloc currently only affects the fail-safe check; no downstream job condition consumes it. Consequently, a change matching only this category can still skip host/static validation. Please add needs.changes.outputs.static_alloc == 'true' to the appropriate validation job condition (likely host) to ensure those changes run CI.
You are interacting with an AI system.
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Install ccache | ||
| run: sudo apt-get update -qq && sudo apt-get install -y ccache | ||
|
|
||
| - name: Cache ccache | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: ~/.cache/ccache | ||
| key: ccache-${{ runner.os }}-freertos-renode-cortexm4-static-${{ github.sha }} | ||
| restore-keys: | | ||
| ccache-${{ runner.os }}-freertos-renode-cortexm4-static- | ||
|
|
||
| - name: Cache ARM toolchain apt packages | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: /var/cache/apt/archives | ||
| key: apt-arm-toolchain-${{ runner.os }} | ||
|
|
||
| - name: Install ARM toolchain | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y gcc-arm-none-eabi libnewlib-arm-none-eabi | ||
|
|
||
| - name: Cache CMake FetchContent | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: build/freertos-cortexm4-renode-static/_deps | ||
| key: fetchcontent-freertos-renode-static-${{ hashFiles('CMakeLists.txt') }} | ||
| restore-keys: | | ||
| fetchcontent-freertos-renode-static- | ||
|
|
||
| - name: Cache Renode | ||
| id: renode-cache | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: /opt/renode | ||
| key: renode-${{ env.RENODE_VERSION }}-linux | ||
|
|
||
| - name: Install Renode | ||
| if: steps.renode-cache.outputs.cache-hit != 'true' | ||
| run: | | ||
| wget -q "https://github.com/renode/renode/releases/download/v${RENODE_VERSION}/renode-${RENODE_VERSION}.linux-portable.tar.gz" | ||
| sudo mkdir -p /opt/renode | ||
| sudo tar xzf "renode-${RENODE_VERSION}.linux-portable.tar.gz" -C /opt/renode --strip-components=1 | ||
|
|
||
| - name: Add Renode to PATH | ||
| run: sudo ln -sf /opt/renode/renode /usr/local/bin/renode | ||
|
|
||
| - name: Build & run FreeRTOS static-alloc tests on Renode | ||
| run: | | ||
| cmake --preset freertos-cortexm4-renode-static -S "$GITHUB_WORKSPACE" \ | ||
| -DCMAKE_C_COMPILER_LAUNCHER=ccache \ | ||
| -DCMAKE_CXX_COMPILER_LAUNCHER=ccache | ||
| cmake --build "$GITHUB_WORKSPACE/build/freertos-cortexm4-renode-static" -j"$(nproc)" | ||
| ./scripts/run_freertos_renode_test.sh \ | ||
| --skip-build \ | ||
| --build-dir build/freertos-cortexm4-renode-static \ | ||
| --timeout 60 \ | ||
| --junit-output /tmp/freertos_renode_static_junit.xml | ||
|
|
||
| - name: Upload Renode test results | ||
| uses: actions/upload-artifact@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP '^\s*uses:\s+actions/(checkout|cache|upload-artifact)@(?![0-9a-f]{40}\b)' \
.github/workflows/freertos.yml .github/workflows/threadx.yml
# Expected: no output for the newly added static Renode jobs.Repository: vtz/opensomeip
Length of output: 3386
Pin the static CI actions to immutable commit hashes.
The new FreeRTOS and ThreadX static Renode jobs still use mutable GitHub Action tags for actions/checkout, actions/cache, and actions/upload-artifact; pin these occurrences to reviewed full SHAs to avoid code changes through tag rewrites.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 341-341: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 341-341: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 347-347: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 355-355: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 366-366: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 375-375: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 403-403: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
📍 Affects 2 files
.github/workflows/freertos.yml#L341-L403(this comment).github/workflows/threadx.yml#L340-L402
🤖 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/freertos.yml around lines 341 - 403, Pin every
actions/checkout, actions/cache, and actions/upload-artifact reference in the
static Renode jobs to reviewed immutable full commit SHAs, replacing the mutable
version tags while preserving each action’s existing configuration. Apply this
change in .github/workflows/freertos.yml lines 341-403 and
.github/workflows/threadx.yml lines 340-402.
Source: Linters/SAST tools
There was a problem hiding this comment.
Acknowledged. GitHub Actions pinning to SHA hashes is a repo-wide CI hardening concern unrelated to this PR's static allocation scope. Tracked separately.
There was a problem hiding this comment.
@vtz, acknowledged. Since this is tracked separately and outside the static-allocation rollout’s scope, I’ll leave it as a non-blocking follow-up rather than requiring changes in this PR.
You are interacting with an AI system.
| template <typename Container> | ||
| explicit PayloadView(const Container& c) noexcept | ||
| : data_(c.data()), size_(c.size()) {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'include/someip/payload_view\.h|ByteBuffer|payload_view' || true
echo
echo "payload_view.h outline:"
if [ -f include/someip/payload_view.h ]; then
ast-grep outline include/someip/payload_view.h || true
echo
echo "payload_view.h contents:"
cat -n include/someip/payload_view.h
fi
echo
echo "ByteBuffer definitions/usages:"
rg -n "struct ByteBuffer|class ByteBuffer|class ByteBuffer|using ByteBuffer|namespace ByteBuffer|ByteBuffer" -g '*.h' -g '*.hpp' -g '*.cc' -g '*.cpp' . | head -200Repository: vtz/opensomeip
Length of output: 20412
🌐 Web query:
C++ constexpr const T& parameter rvalue argument object lifetime
💡 Result:
In C++, binding a temporary object (rvalue) to a const T& function parameter does not extend the temporary's lifetime beyond the full-expression containing the function call [1][2][3]. While a const reference in a local scope (e.g., const T& ref = T;) extends the lifetime of the temporary to the scope of that reference [4][5], this rule specifically excludes function parameters [1][2][3]. When a temporary is bound to a reference parameter, the temporary persists only until the completion of the full-expression that contains the function call [1][2][3]. Consequently, if a function returns a reference to one of its arguments that was bound to a temporary, the returned reference becomes dangling immediately after the function returns [6][2]. This behavior remains the same regardless of whether the function is declared constexpr [7]. During constant evaluation, the C++ abstract machine manages the lifetime of temporaries created within that evaluation [7]. If a constexpr function is evaluated at compile-time, the lifetime of temporaries bound to its reference parameters will align with the duration of that specific constant-evaluation [7]. However, this does not grant the reference a persistent lifetime outside of that evaluation context [7]. Because constexpr functions must be valid for both compile-time and runtime execution, the compiler enforces the same lifetime rules for reference parameters in both contexts [7]. Therefore, code that relies on lifetime extension via a reference parameter will lead to undefined behavior at runtime, and such expressions are generally not usable in a constant expression if they result in dangling references [6][2][7]. Top results: [1], [2], [7], [3]
Citations:
- 1: https://stackoverflow.com/questions/10190677/will-a-reference-bound-to-a-function-parameter-prolong-the-lifetime-of-that-temp
- 2: https://stackoverflow.com/questions/65642004/side-effects-of-passing-temporary-as-const-t-and-binding-to-class-reference
- 3: https://stackoverflow.com/questions/41928634/c11-scoping-and-lifetime-of-temporary-bound-to-a-const-reference-gcc
- 4: https://sinhak.dev/posts/const-reference-to-temporary-objects/
- 5: https://stackoverflow.com/questions/39718268/why-do-const-references-extend-the-lifetime-of-rvalues
- 6: https://stackoverflow.com/questions/55567962/why-doesnt-a-const-reference-extend-the-life-of-a-temporary-object-passed-via-a
- 7: https://stackoverflow.com/questions/62514315/constant-function-when-its-parameter-is-a-reference-to-literal-type
Prevent construction from temporary containers.
PayloadView(platform::ByteBuffer{...}) compiles, but the temporary argument ends at the end of the expression, so the stored data_ immediately points outside a valid container. Delete or overload for rvalue containers, or constrain construction to lvalue containers.
🤖 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 `@include/someip/payload_view.h` around lines 32 - 34, Update the templated
PayloadView constructor to reject rvalue or temporary containers while
preserving construction from lvalue containers. Use an appropriate deleted
rvalue overload or lvalue-only constraint on the constructor, ensuring data_
never references a container that has already been destroyed.
There was a problem hiding this comment.
Fixed in 1024e55. operator[] now includes assert(i < size_) in debug builds (fires on OOB access). In release builds, the contract matches std::span::operator[] — no bounds check. Class-level documentation updated to describe the span-like semantics.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- current PayloadView outline ---'
ast-grep outline include/someip/payload_view.h --items all
echo
echo '--- current constructor and element-access section ---'
nl -ba include/someip/payload_view.h | sed -n '20,70p'
echo
echo '--- cited commit metadata and payload_view diff ---'
git show --no-ext-diff --format=fuller --stat 1024e55 --
git show --no-ext-diff --format= -- include/someip/payload_view.h 1024e55 || trueLength of output: 1246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- current include/someip/payload_view.h (lines 20-70) ---'
sed -n '20,70p' include/someip/payload_view.h
echo
echo '--- constructor declarations and deleted overloads ---'
rg -n -C 3 'PayloadView\s*\(|=\s*delete|requires|enable_if' include/someip/payload_view.h
echo
echo '--- cited commit: relevant header patch ---'
git show --no-ext-diff --format= --find-renames 1024e55 -- include/someip/payload_view.h || true
echo
echo '--- current commit containing payload_view changes ---'
git log -1 --oneline -- include/someip/payload_view.hLength of output: 4306
@vtz, verified against 1024e55: the change adds the operator[] debug assertion and documents span-like bounds behavior, but PayloadView still has the unconstrained constructor:
template <typename Container>
explicit PayloadView(const Container& c) noexceptThat constructor still accepts temporaries, so PayloadView(platform::ByteBuffer{...}) creates a dangling view. The original lifetime finding remains unresolved; please reject rvalue containers (for example, with a deleted Container&& overload) while retaining lvalue construction.
You are interacting with an AI system.
| # Malloc trap object library (linked into verification tests, not into main lib). | ||
| # Only built on hosted platforms — requires <dlfcn.h> (unavailable on bare-metal). | ||
| if(NOT CMAKE_CROSSCOMPILING) | ||
| add_library(someip_malloc_trap OBJECT | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/platform/static/malloc_trap.cpp) | ||
| target_compile_definitions(someip_malloc_trap PRIVATE SOMEIP_MALLOC_TRAP) | ||
| endif() | ||
| endif() | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'dlfcn|CMAKE_CROSSCOMPILING|CMAKE_DL_LIBS|WIN32|UNIX' src tests CMakeLists.txtRepository: vtz/opensomeip
Length of output: 13538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the malloc trap implementation and the CMake section around the guard.
wc -l src/platform/static/malloc_trap.cpp src/CMakeLists.txt tests/CMakeLists.txt
printf '\n--- src/platform/static/malloc_trap.cpp first 80 lines ---\n'
sed -n '1,90p' src/platform/static/malloc_trap.cpp
printf '\n--- src/CMakeLists.txt malloc trap section ---\n'
sed -n '118,136p' src/CMakeLists.txt
printf '\n--- tests/CMakeLists.txt malloc trap tests references ---\n'
sed -n '130,139p' tests/CMakeLists.txt
# Deterministically identify malloc_trap inclusion condition from src/CMakeLists.txt and report how non-Unix native targets would match it.
python3 - <<'PY'
from pathlib import Path
text = Path('src/CMakeLists.txt').read_text()
needle = '\tif(NOT CMAKE_CROSSCOMPILING)\n'
idx = text.index('{}{}{}\n'.format(needle, 'add_library(someip_malloc_trap OBJECT', f'\t\t${{CMAKE_CURRENT_SOURCE_DIR}}/platform/static/malloc_trap.cpp)'))
start = text.rfind('\n', 0, idx - len(needle)) + 1
block = text[idx:idx + 200]
print('has_malloc_trap_block=', 'add_library(someip_malloc_trap OBJECT' in block)
print('current_block=\n' + '\n'.join(block.splitlines()[:12]))
print('matches_windows_host_scenario=', 'src/platform/static/malloc_trap.cpp' in block and 'if(NOT CMAKE_CROSSCOMPILING)' in block)
PY
# Inspect platform definitions in tests to see whether malloc_trap tests are skipped on WIN32 even if target existed.
rg -n -C3 'TARGET someip_malloc_trap|test_static_alloc_integration|WIN32' tests/CMakeLists.txt src tests/platform include platform || trueRepository: vtz/opensomeip
Length of output: 5214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- malloc_trap.cpp header guards ---'
sed -n '1,48p' src/platform/static/malloc_trap.cpp
printf '%s\n' '--- tests malloc trap inclusion context ---'
sed -n '130,140p' tests/CMakeLists.txt
printf '%s\n' '--- CMake configuration for tests ---'
sed -n '56,140p' tests/CMakeLists.txtRepository: vtz/opensomeip
Length of output: 6456
Gate the malloc trap on loader support, not only cross-compilation.
malloc_trap.cpp uses <dlfcn.h>/dlsym, but the object library is created whenever CMAKE_CROSSCOMPILING is false. A native Windows build can therefore attempt to compile the trap target without that header. Add UNIX/CMAKE_DL_LIBS/dlfcn.h checks before creating someip_malloc_trap.
Suggested direction
-if(NOT CMAKE_CROSSCOMPILING)
+if(UNIX AND NOT CMAKE_CROSSCOMPILING)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Malloc trap object library (linked into verification tests, not into main lib). | |
| # Only built on hosted platforms — requires <dlfcn.h> (unavailable on bare-metal). | |
| if(NOT CMAKE_CROSSCOMPILING) | |
| add_library(someip_malloc_trap OBJECT | |
| ${CMAKE_CURRENT_SOURCE_DIR}/platform/static/malloc_trap.cpp) | |
| target_compile_definitions(someip_malloc_trap PRIVATE SOMEIP_MALLOC_TRAP) | |
| endif() | |
| endif() | |
| # Malloc trap object library (linked into verification tests, not into main lib). | |
| # Only built on hosted platforms — requires <dlfcn.h> (unavailable on bare-metal). | |
| if(UNIX AND NOT CMAKE_CROSSCOMPILING) | |
| add_library(someip_malloc_trap OBJECT | |
| ${CMAKE_CURRENT_SOURCE_DIR}/platform/static/malloc_trap.cpp) | |
| target_compile_definitions(someip_malloc_trap PRIVATE SOMEIP_MALLOC_TRAP) | |
| endif() | |
| endif() |
🤖 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 `@src/CMakeLists.txt` around lines 125 - 133, Update the someip_malloc_trap
creation guard to require loader support in addition to not being
cross-compiling: check UNIX, a non-empty CMAKE_DL_LIBS, and availability of
dlfcn.h before add_library. Keep the existing target_compile_definitions
unchanged and avoid creating the object library when any requirement is
unavailable.
There was a problem hiding this comment.
Acknowledged. dlfcn.h guard (NOT CMAKE_CROSSCOMPILING) is already in place in src/CMakeLists.txt. The unused #include <dlfcn.h> suggestion applies to test code, not the library itself — no change needed.
There was a problem hiding this comment.
@vtz, thanks for clarifying. The concern is not an unused test include: NOT CMAKE_CROSSCOMPILING only excludes cross builds. A native Windows build also satisfies that condition, so it would still attempt to compile src/platform/static/malloc_trap.cpp, which includes <dlfcn.h>.
The guard should additionally restrict the trap to platforms that provide the POSIX dynamic-loader API (for example, UNIX), unless Windows static-allocation builds are intentionally unsupported.
You are interacting with an AI system.
| ### Test Execution Results (Current Environment) | ||
|
|
||
| | Test Suite | Tests | Passing | Notes | | ||
| |------------|-------|---------|-------| | ||
| | Message Tests | 23 | 23 | | | ||
| | Serialization Tests | 49 | 49 | | | ||
| | SD Tests | 52 | 52 | | | ||
| | TP Tests | 23 | 23 | | | ||
| | TCP Transport Tests | 16 | 16 | | | ||
| | UDP Transport Tests | 27 | 27 | | | ||
| | Platform Threading | 21 | 21 | | | ||
| | E2E Tests | 11 | 11 | | | ||
| | RPC Tests | 8 | 8 | | | ||
| | Events Tests | 14 | 14 | | | ||
| | PAL FreeRTOS Mock | 22 | 22 | | | ||
| | PAL ThreadX Mock | 22 | 22 | | | ||
| | PAL Zephyr Mock | 22 | 22 | | | ||
| | SD Tests | 57 | 57 | | | ||
| | Serialization Tests | 37 | 37 | | | ||
| | E2E Tests | 31 | 31 | | | ||
| | Message Tests | 27 | 27 | | | ||
| | TP Tests | 25 | 25 | | | ||
| | Platform Containers | 21 | 21 | | | ||
| | Session Manager Tests | 19 | 19 | | | ||
| | TCP Transport Tests | 17 | 17 | | | ||
| | Static Message Pool | 16 | 16 | | | ||
| | Platform Threading | 15 | 15 | | | ||
| | Buffer Pool Tests | 14 | 14 | | | ||
| | Static Alloc Integration | 14 | 14 | | | ||
| | Events Tests | 10 | 10 | | | ||
| | UDP Transport Tests | 6 | 6 | | | ||
| | ETL Error Handler | 4 | 4 | | | ||
| | System Tests | 4 | 4 | | | ||
| | PAL FreeRTOS Mock | 1 | 1 | | | ||
| | PAL ThreadX Mock | 1 | 1 | | | ||
| | PAL Zephyr Mock | 1 | 1 | | | ||
| | PAL Static Alloc Mock | 1 | 1 | | | ||
| | RPC Tests | 1 | 1 | | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use one authoritative test-suite inventory for all traceability tables.
The documented file inventory and execution summaries list different suites and can misrepresent what was actually run.
- TEST_TRACEABILITY_MATRIX.md#L227-L251: regenerate execution results from the complete suite inventory.
- TRACEABILITY_SUMMARY.md#L74-L93: align the summary suite list with the validated execution results.
- TEST_TRACEABILITY_MATRIX.md#L22-L45: distinguish inventory-only files from suites executed in the current environment.
📍 Affects 2 files
TEST_TRACEABILITY_MATRIX.md#L227-L251(this comment)TRACEABILITY_SUMMARY.md#L74-L93TEST_TRACEABILITY_MATRIX.md#L22-L45
🤖 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 `@TEST_TRACEABILITY_MATRIX.md` around lines 227 - 251, Use one authoritative
test-suite inventory across all traceability documentation. In
TEST_TRACEABILITY_MATRIX.md lines 22-45, distinguish inventory-only files from
suites executed in the current environment; regenerate
TEST_TRACEABILITY_MATRIX.md lines 227-251 from the complete validated inventory;
and align TRACEABILITY_SUMMARY.md lines 74-93 with those validated execution
results so every documented suite and count is consistent.
There was a problem hiding this comment.
Acknowledged. Traceability matrix / test inventory reconciliation is documentation hygiene, not a functional issue. Will address in a follow-up docs PR.
There was a problem hiding this comment.
@vtz, understood—this is documentation hygiene and can be handled separately from the static-allocation implementation. Would you like me to open a follow-up GitHub issue to track the traceability inventory reconciliation?
You are interacting with an AI system.
| # Malloc trap integration tests — host-only (requires dlsym/dlfcn.h) | ||
| if(TARGET someip_malloc_trap) | ||
| add_executable(test_static_alloc_integration test_static_alloc_integration.cpp | ||
| $<TARGET_OBJECTS:someip_malloc_trap>) | ||
| target_link_libraries(test_static_alloc_integration opensomeip gtest_main) | ||
| add_test(NAME StaticAllocIntegrationTest COMMAND test_static_alloc_integration) | ||
| set_tests_properties(StaticAllocIntegrationTest PROPERTIES TIMEOUT 30) | ||
| endif() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate relevant files =="
git ls-files | grep -E '(^|/)(CMakeLists\.txt|malloc_trap|test_static_alloc_integration)' | sed -n '1,120p'
echo
echo "== Inspect tests/CMakeLists.txt around target =="
if [ -f tests/CMakeLists.txt ]; then
wc -l tests/CMakeLists.txt
sed -n '120,150p' tests/CMakeLists.txt | cat -n
fi
echo
echo "== Search for someip_malloc_trap and malloc_trap usages =="
rg -n "someip_malloc_trap|malloc_trap|dlsym|dlfcn|CMAKE_DL_LIBS|FindDL|FindThreads" -S . | sed -n '1,200p'Repository: vtz/opensomeip
Length of output: 7523
Link the platform dynamic-loader library.
someip_malloc_trap calls dlsym, but test_static_alloc_integration does not link ${CMAKE_DL_LIBS} and object files do not propagate the dynamic loader dependency. Add ${CMAKE_DL_LIBS} to this executable so the link works on platforms where loader symbols are in a separate library.
Suggested fix
- target_link_libraries(test_static_alloc_integration opensomeip gtest_main)
+ target_link_libraries(test_static_alloc_integration PRIVATE
+ opensomeip gtest_main ${CMAKE_DL_LIBS})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Malloc trap integration tests — host-only (requires dlsym/dlfcn.h) | |
| if(TARGET someip_malloc_trap) | |
| add_executable(test_static_alloc_integration test_static_alloc_integration.cpp | |
| $<TARGET_OBJECTS:someip_malloc_trap>) | |
| target_link_libraries(test_static_alloc_integration opensomeip gtest_main) | |
| add_test(NAME StaticAllocIntegrationTest COMMAND test_static_alloc_integration) | |
| set_tests_properties(StaticAllocIntegrationTest PROPERTIES TIMEOUT 30) | |
| endif() | |
| # Malloc trap integration tests — host-only (requires dlsym/dlfcn.h) | |
| if(TARGET someip_malloc_trap) | |
| add_executable(test_static_alloc_integration test_static_alloc_integration.cpp | |
| $<TARGET_OBJECTS:someip_malloc_trap>) | |
| target_link_libraries(test_static_alloc_integration PRIVATE | |
| opensomeip gtest_main ${CMAKE_DL_LIBS}) | |
| add_test(NAME StaticAllocIntegrationTest COMMAND test_static_alloc_integration) | |
| set_tests_properties(StaticAllocIntegrationTest PROPERTIES TIMEOUT 30) | |
| endif() |
🤖 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 `@tests/CMakeLists.txt` around lines 134 - 141, Add ${CMAKE_DL_LIBS} to
target_link_libraries for test_static_alloc_integration alongside opensomeip and
gtest_main, ensuring dlsym dependencies from someip_malloc_trap are explicitly
linked.
There was a problem hiding this comment.
Acknowledged. malloc_trap conditional linking for test_static_alloc_integration is already handled via $<TARGET_OBJECTS:someip_malloc_trap>. The ASSERT_* trap-arming concern was fixed in a prior commit (f84dd7e) with explicit null checks + FAIL() after disarming.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== someip_malloc_trap definition and loader-library references =='
rg -n -C 8 'someip_malloc_trap|CMAKE_DL_LIBS|dlsym|dlfcn' \
CMakeLists.txt src/CMakeLists.txt tests/CMakeLists.txt \
include src tests 2>/dev/null || true
echo
echo '== CMake target outline =='
ast-grep outline src/CMakeLists.txt --items all 2>/dev/null || trueLength of output: 10503
@vtz, thanks for the clarification on the separate trap-arming fix. The loader-linkage finding still applies, though: $<TARGET_OBJECTS:someip_malloc_trap> adds the object file to the executable, but it does not link a dynamic-loader library.
The current someip_malloc_trap target has no declared link dependency, while src/platform/static/malloc_trap.cpp directly calls dlsym. test_static_alloc_integration currently links only opensomeip and gtest_main, and no ${CMAKE_DL_LIBS} reference exists. Please retain the explicit ${CMAKE_DL_LIBS} dependency for portability.
You are interacting with an AI system.
| - 73 requirements have no `@implements` annotation in code (some may be implemented but unannotated) | ||
| - 7 requirements have no `@tests` annotation | ||
| - Serialization module has the most unimplemented requirements (23 missing) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Regenerate the serialization gap and remediation data together.
The documents report 23 missing serialization requirements but recommend only the 18-ID range REQ_SER_090–REQ_SER_107.
- TRACEABILITY_MATRIX.md#L312-L314: publish the complete missing-requirement set.
- TRACEABILITY_SUMMARY.md#L111-L114: align the short-term action with all missing requirements.
- TRACEABILITY_MATRIX.md#L329-L330: list every remaining serialization requirement.
- TEST_TRACEABILITY_MATRIX.md#L267-L269: keep the recommended improvements consistent with the validated count.
📍 Affects 3 files
TRACEABILITY_MATRIX.md#L312-L314(this comment)TRACEABILITY_SUMMARY.md#L111-L114TRACEABILITY_MATRIX.md#L329-L330TEST_TRACEABILITY_MATRIX.md#L267-L269
🤖 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 `@TRACEABILITY_MATRIX.md` around lines 312 - 314, Regenerate the serialization
gap and remediation data as one consistent update: in
TRACEABILITY_MATRIX.md:312-314 publish the complete validated
missing-requirement set and in TRACEABILITY_MATRIX.md:329-330 list every
remaining serialization requirement; update TRACEABILITY_SUMMARY.md:111-114 and
TEST_TRACEABILITY_MATRIX.md:267-269 so their recommended actions and counts
cover that same full set rather than only REQ_SER_090–REQ_SER_107.
There was a problem hiding this comment.
Acknowledged. Traceability serialization gap count inconsistency is a documentation refresh issue. Tracked for follow-up.
There was a problem hiding this comment.
@vtz, acknowledged. I’ll leave this finding unresolved for the follow-up documentation refresh. Would you like me to open a GitHub issue capturing the affected traceability documents and acceptance criteria?
You are interacting with an AI system.
| 1. Add `@implements` / `@satisfies` annotations to 73 unannotated requirements | ||
| 2. Add `@tests` annotations for the 7 remaining requirements without test coverage | ||
| 3. Add `:satisfies:` field to REQ_PLATFORM_ARCH_001 (only spec-link gap) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Remove or update the stale spec-link recommendation.
This section reports 669/669 spec-linked requirements, while the recommendation still says REQ_PLATFORM_ARCH_001 lacks :satisfies:. Regenerate the metrics and recommendations together so the reported gap and action agree.
🤖 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 `@TRACEABILITY_MATRIX.md` around lines 323 - 325, Update the traceability
metrics and recommendations together so the stale REQ_PLATFORM_ARCH_001
spec-link recommendation is removed or reflects the current state. Ensure the
reported spec-linked requirement count and recommended actions are consistent
after regeneration.
There was a problem hiding this comment.
Acknowledged. Stale spec-link recommendation will be regenerated in a follow-up documentation pass.
There was a problem hiding this comment.
@vtz, acknowledged. The finding will remain open until the regenerated metrics and recommendation are updated together. Would you like me to open a follow-up issue to track the documentation pass?
You are interacting with an AI system.
| | Platform (REQ_PLATFORM_*, REQ_PAL_*) | 73 | | ||
| | Transport (REQ_TRANSPORT_*) | 42 | | ||
| | PAL Abstractions (REQ_PAL_*) | 35 | | ||
| | Platform Backends (REQ_PLATFORM_*) | 17 | | ||
| | Compatibility (REQ_COMPAT_*) | 17 | | ||
| | Architecture (REQ_ARCH_*) | 7 | | ||
| | Architecture (REQ_ARCH_*) | 8 | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Correct the module-table metric label.
These rows sum to 673 and Line 66 says they represent full RST definitions, not the 594 fully traced requirements. Rename the section or publish actual per-module fully traced counts.
🤖 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 `@TRACEABILITY_SUMMARY.md` around lines 60 - 63, Correct the module-table
metric in TRACEABILITY_SUMMARY.md: either rename the section to indicate it
reports full RST definitions, consistent with Line 66, or replace the row values
with actual per-module counts for the 594 fully traced requirements. Ensure the
displayed totals and label describe the same metric.
There was a problem hiding this comment.
Acknowledged. Module-table metric label inconsistency is a documentation issue. Tracked for follow-up.
There was a problem hiding this comment.
@vtz, acknowledged. Thanks for tracking this documentation consistency fix for follow-up.
You are interacting with an AI system.
- session_manager.cpp: add missing #include <optional>
- rpc_client.cpp: guard optional access with has_value() check
- thread_impl.h: replace ctx_ = {} with ctx_ = nullptr to fix
ambiguous operator= with etl::inplace_function; add specialized
store_callable overload for member-function-pointer + object
pattern to keep lambda capture within inplace_function capacity
Co-authored-by: Cursor <cursoragent@cursor.com>
The general store_callable wrapped all callables in a
lambda+tuple, exceeding etl::inplace_function's 32-byte capture
limit on 32-bit ARM. Add a zero-arg overload that assigns the
callable directly to ctx_, avoiding the tuple wrapper for the
common Thread([this]() { ... }) pattern used in sd_client,
sd_server, and event_publisher.
Co-authored-by: Cursor <cursoragent@cursor.com>
Resolve all remaining Renode static-alloc CI failures and strengthen
the no-heap backend for both FreeRTOS and ThreadX bare-metal targets.
Renode / cross-compile fixes:
- Migrate tests/shared/*.inc from std::vector/std::string to
platform::ByteBuffer / platform::String for ETL compatibility
- Add char8_t polyfill for GCC <11 in C++17 mode (ETL string_view.h)
- Add __sync_synchronize stub in BSP startup for bare-metal libstdc++
- Use placement-new for pool mutexes to avoid static-init-order fiasco
with FreeRTOS xSemaphoreCreateMutex
- Remove ${PROJECT_SOURCE_DIR} from Renode test include paths to avoid
<version> header collision on case-insensitive filesystems
- Propagate pool-sizing cache variables as target_compile_definitions
instead of CMAKE_CXX_FLAGS (prevents wiping ARM toolchain flags)
- Make FreeRTOS configTOTAL_HEAP_SIZE overridable via
SOMEIP_FREERTOS_HEAP_SIZE
- Add SOMEIP_MAX_TP_REASSEMBLY_SIZE to control per-byte reception
bitvector capacity in TpReassemblyBuffer (ETL vector<bool> is not
bit-packed — 16384 bytes per entry caused 66 KB stack overflow)
- Tune Renode preset pool sizes, TP segments, map capacity, heap,
and task stack sizes for STM32F407 256 KB SRAM budget
Host / API hardening:
- MallocTrapGuard RAII in test_static_alloc_integration.cpp
- Assert-on-uninitialized in allocate_message / acquire_buffer (debug)
- Document init_static_allocator() precondition in memory_impl.h
- TpSegmentVector type alias sized by SOMEIP_MAX_TP_SEGMENTS
- SD add_entry / add_option return bool; deserialize checks capacity
- ThreadX thread_impl.h: ctx_=nullptr instead of ctx_={}, add
store_callable overloads to stay within inplace_function capture size
- Keep malloc_trap host-only (NOT CMAKE_CROSSCOMPILING)
Validated: host static ctest (21/21), host dynamic ctest (15/15),
FreeRTOS Renode static (133/133), ThreadX Renode static (121/121).
Co-authored-by: Cursor <cursoragent@cursor.com>
- Add --build-dir argument to run_freertos_renode_test.sh and run_threadx_renode_test.sh so CI can pass the static-alloc build directory (the workflow already passes --build-dir but the scripts lacked the case branch, causing "Unknown argument" failures). - Wrap the char8_t polyfill in containers_impl.h with #pragma GCC diagnostic ignored "-Wc++20-compat" so host builds with -Werror don't reject the typedef. Co-authored-by: Cursor <cursoragent@cursor.com>
- Rename kMaxTpReassemblySize/kMaxTpSegments to UPPER_CASE per project readability-identifier-naming convention - Add NOLINT for macro-usage on the fallback #define (must be a preprocessor macro since it comes from -D flags) - Remove unused containers.h include from tp_segmenter.cpp Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/rpc/rpc_client.cpp (2)
122-159: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftPrevent the synchronous-response callback from outliving
SyncState.A response can erase and copy the callback just before Line 145, causing
cancel_call()to returnfalse;call_method_sync()then returns and destroysstatebeforeon_message_received()invokes the copied callback. The callback dereferences the destroyed stack object at Lines 130-133.Use client-owned/bounded response state or add a completion handoff that guarantees no callback can begin after this frame is destroyed.
🤖 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 `@src/rpc/rpc_client.cpp` around lines 122 - 159, Update call_method_sync and its SyncState/callback lifecycle so the asynchronous response callback cannot access destroyed stack state after timeout or cancellation. Use client-owned bounded response state or an explicit completion handoff that waits for any copied callback to finish before returning, while preserving normal response and timeout results.
173-195: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAbort when bounded session allocation fails.
SessionManager::create_session()returns0when its session container is full, but this path still records and sends an RPC using session ID0. Concurrent calls can then share an untracked session ID and be matched incorrectly.Proposed fix
const uint16_t session_id = session_manager_.create_session(client_id_); + if (session_id == 0) { + return 0; + }🤖 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 `@src/rpc/rpc_client.cpp` around lines 173 - 195, Check the result of SessionManager::create_session() in the RPC call flow before constructing the request or PendingCall; if it returns 0, abort immediately with the existing failure result (0) and do not record or send the call. Preserve normal processing for valid session IDs.src/sd/sd_message.cpp (1)
609-647: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBound each option to the declared options array.
Known-option parsing validates against
data.size(), notoptions_end. A malformed length can therefore make a known option consume trailing bytes outside the declared options array and still deserialize successfully. Validate the complete wire size (3 + option_len) againstoptions_endbefore dispatching.Proposed fix
while (offset < options_end) { - if (offset + 4 > data.size()) { + if (offset + 4 > options_end) { return false; } + + const uint16_t option_len = static_cast<uint16_t>( + (static_cast<uint32_t>(data[offset]) << 8U) | + static_cast<uint32_t>(data[offset + 1])); + if (offset + 3U + option_len > options_end) { + return false; + } // Options start with length(2) + type(1) + reserved(1).🤖 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 `@src/sd/sd_message.cpp` around lines 609 - 647, In the option-parsing loop, validate each option’s complete wire size (3-byte header plus the declared option length) does not exceed options_end before dispatching to any known option deserializer. Use the length field at the current offset and preserve the existing data.size() bounds check, rejecting malformed options that extend beyond the declared options array.src/platform/static/buffer_pool.cpp (1)
104-124: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPool mutex is dereferenced before/without checking initialization in four places across two files.
pool_mutex_ptrstaysnullptruntilinit_buffer_pool()/init_static_allocator()runs, and each of these functions constructs aScopedLockfrom*pool_mutex_ptreither before the preconditionassertor with no guard at all — turning a documented, diagnosable precondition violation into an undiagnosed null-pointer crash.
src/platform/static/buffer_pool.cpp#L104-L124: moveassert(pool_initialized && ...)aboveScopedLock lk(*pool_mutex_ptr);inacquire_buffer().src/platform/static/buffer_pool.cpp#L126-L142: add the samepool_initializedassert beforeScopedLock lk(*pool_mutex_ptr);inrelease_buffer(), or document why it's safe to omit.src/platform/static/memory.cpp#L67-L81: moveassert(pool_initialized && ...)aboveScopedLock lk(*pool_mutex_ptr);inallocate_message().src/platform/static/memory.cpp#L83-L106: add the same guard beforeScopedLock lk(*pool_mutex_ptr);inrelease_message(), or document why it's safe to omit.🛡️ Proposed fix for acquire_buffer / allocate_message
BufferSlot* acquire_buffer(size_t requested_size) { if (requested_size == 0) { requested_size = 1; } - ScopedLock lk(*pool_mutex_ptr); assert(pool_initialized && "init_static_allocator() must be called before acquire_buffer()"); + ScopedLock lk(*pool_mutex_ptr);MessagePtr allocate_message() { - ScopedLock lk(*pool_mutex_ptr); assert(pool_initialized && "init_static_allocator() must be called before allocate_message()"); + ScopedLock lk(*pool_mutex_ptr);🤖 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 `@src/platform/static/buffer_pool.cpp` around lines 104 - 124, Move the pool_initialized assertion before lock construction in acquire_buffer and allocate_message, and add the same precondition guard before locking in release_buffer and release_message. Apply these changes at src/platform/static/buffer_pool.cpp lines 104-124 and 126-142, and src/platform/static/memory.cpp lines 67-81 and 83-106; all four functions must validate initialization before dereferencing pool_mutex_ptr.
🤖 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 `@include/platform/static/static_config.h`:
- Around line 105-115: Add compile-time range validation for
SOMEIP_MAX_TP_SEGMENTS and SOMEIP_MAX_TP_REASSEMBLY_SIZE, matching the valid
capacities required by the static backend’s platform::Vector usage. Since
SOMEIP_MAX_SD_OPTIONS has no static vector consumer, remove its
compiler-override documentation or add an appropriate range check only if it is
used for a capacity.
In `@tests/freertos/test_freertos_core.cpp`:
- Around line 184-208: Update the test around the
allocate_message/serialize/deserialize round trip to instrument FreeRTOS
allocation calls using a test-local counter or trap. Assert that no
pvPortMalloc/free allocation calls occur during the message operations, while
retaining the existing heap-growth assertion as a secondary check; integrate the
instrumentation with the test setup and cleanup so it does not affect other
tests.
In `@tests/test_sd.cpp`:
- Around line 1928-1945: Update SdTest.AddEntryReturnsBool to populate the
message’s entry and option containers to their configured capacities, asserting
each insertion succeeds, then attempt one additional insertion for each
container and assert add_entry and add_option return false. Preserve the
existing test setup and use the container capacity definitions or established
test symbols rather than hardcoding an unrelated limit.
In `@tests/threadx/test_threadx_core.cpp`:
- Around line 174-184: Extend the static roundtrip assertions after
decoded.get_payload() in tests/threadx/test_threadx_core.cpp lines 174-184 and
tests/freertos/test_freertos_core.cpp lines 193-202 to compare every decoded
payload byte with payload, not just the size; preserve the existing size checks
and ensure both tests validate identical contents.
---
Outside diff comments:
In `@src/platform/static/buffer_pool.cpp`:
- Around line 104-124: Move the pool_initialized assertion before lock
construction in acquire_buffer and allocate_message, and add the same
precondition guard before locking in release_buffer and release_message. Apply
these changes at src/platform/static/buffer_pool.cpp lines 104-124 and 126-142,
and src/platform/static/memory.cpp lines 67-81 and 83-106; all four functions
must validate initialization before dereferencing pool_mutex_ptr.
In `@src/rpc/rpc_client.cpp`:
- Around line 122-159: Update call_method_sync and its SyncState/callback
lifecycle so the asynchronous response callback cannot access destroyed stack
state after timeout or cancellation. Use client-owned bounded response state or
an explicit completion handoff that waits for any copied callback to finish
before returning, while preserving normal response and timeout results.
- Around line 173-195: Check the result of SessionManager::create_session() in
the RPC call flow before constructing the request or PendingCall; if it returns
0, abort immediately with the existing failure result (0) and do not record or
send the call. Preserve normal processing for valid session IDs.
In `@src/sd/sd_message.cpp`:
- Around line 609-647: In the option-parsing loop, validate each option’s
complete wire size (3-byte header plus the declared option length) does not
exceed options_end before dispatching to any known option deserializer. Use the
length field at the current offset and preserve the existing data.size() bounds
check, rejecting malformed options that extend beyond the declared options
array.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7fae9e7-a561-4f96-99ba-6424dc124d48
📒 Files selected for processing (31)
CMakeLists.txtCMakePresets.jsonbsp/stm32f407_renode/FreeRTOSConfig.hbsp/stm32f407_renode/startup.cinclude/platform/freertos/thread_impl.hinclude/platform/static/containers_impl.hinclude/platform/static/memory_impl.hinclude/platform/static/static_config.hinclude/platform/threadx/thread_impl.hinclude/sd/sd_message.hinclude/tp/tp_segmenter.hinclude/tp/tp_types.hsrc/CMakeLists.txtsrc/core/session_manager.cppsrc/platform/static/buffer_pool.cppsrc/platform/static/memory.cppsrc/rpc/rpc_client.cppsrc/sd/sd_message.cppsrc/tp/tp_manager.cppsrc/tp/tp_segmenter.cpptests/freertos/CMakeLists.txttests/freertos/test_freertos_core.cpptests/shared/test_e2e_common.inctests/shared/test_message_common.inctests/shared/test_serializer_common.inctests/shared/test_tp_common.inctests/test_sd.cpptests/test_static_alloc_integration.cpptests/test_tp.cpptests/threadx/CMakeLists.txttests/threadx/test_threadx_core.cpp
💤 Files with no reviewable changes (2)
- tests/freertos/CMakeLists.txt
- tests/threadx/CMakeLists.txt
| #ifndef SOMEIP_MAX_TP_SEGMENTS | ||
| #define SOMEIP_MAX_TP_SEGMENTS 64 | ||
| #endif | ||
|
|
||
| #ifndef SOMEIP_MAX_TP_REASSEMBLY_SIZE | ||
| #define SOMEIP_MAX_TP_REASSEMBLY_SIZE 2048 | ||
| #endif | ||
|
|
||
| #ifndef SOMEIP_MAX_SD_OPTIONS | ||
| #define SOMEIP_MAX_SD_OPTIONS 32 | ||
| #endif |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the new TP/SD capacity macros are used downstream (array sizes, index types).
rg -nP 'SOMEIP_MAX_TP_SEGMENTS|SOMEIP_MAX_TP_REASSEMBLY_SIZE|SOMEIP_MAX_SD_OPTIONS' include/tp include/sd -A3 -B3Repository: vtz/opensomeip
Length of output: 1385
🏁 Script executed:
#!/bin/bash
# Inspect the definitions/usages around TP/SD capacity symbols in the relevant headers.
set -eo pipefail
printf '--- files ---\n'
git ls-files include/tp include/sd include/platform/static/static_config.h | sed 'N;s/\n/\t/'
printf '\n--- static_config.h relevant lines ---\n'
sed -n '1,180p' include/platform/static/static_config.h
printf '\n--- tp_types.h relevant lines ---\n'
sed -n '1,220p' include/tp/tp_types.h
printf '\n--- sd types candidate usages ---\n'
rg -n 'SOMEIP_MAX_SD_ENTRIES|SOMEIP_MAX_SD_OPTIONS|SD_OPTIONS|sd_options|sd_options' include/sd include platform -A4 -B4 || trueRepository: vtz/opensomeip
Length of output: 243
🏁 Script executed:
#!/bin/bash
# Inspect the definitions/usages around TP/SD capacity symbols in the relevant headers.
set -eo pipefail
printf '%s\n' '--- static_config.h relevant lines ---'
sed -n '1,180p' include/platform/static/static_config.h
printf '%s\n' '--- tp_types.h relevant lines ---'
sed -n '1,220p' include/tp/tp_types.h
printf '%s\n' '--- sd types candidate usages ---'
rg -n 'SOMEIP_MAX_SD_ENTRIES|SOMEIP_MAX_SD_OPTIONS|SD_OPTIONS|sd_options|sd_options|max_options|max_options' include/sd include platform -A4 -B4 || trueRepository: vtz/opensomeip
Length of output: 12416
🏁 Script executed:
#!/bin/bash
# Search the full repository for platform::Vector usage and SD options usage to determine allocation/index widths.
set -eo pipefail
printf '%s\n' '--- files ---'
git ls-files | rg '(^include/platform/|^include/tp/|^include/sd/)' || true
printf '%s\n' '--- platform::Vector definitions/usages ---'
rg -n 'template<.*class.*capacity|class Vector|struct Vector|platform::Vector|Vector<.*Platform|capacity_|capacity' include platform -A5 -B5 || true
printf '%s\n' '--- SOMEIP_MAX_SD_OPTIONS direct usages ---'
rg -n 'SOMEIP_MAX_SD_OPTIONS' . -A4 -B4 || true
printf '%s\n' '--- SD options related names ---'
rg -n 'SD_OPTION|sd_option|sd_options|Option.*Capacity|option.*Capacity|max.*option' include include/sd include/tp platform 2>/dev/null -A3 -B3 || trueRepository: vtz/opensomeip
Length of output: 34750
🏁 Script executed:
#!/bin/bash
# Inspect platform Vector definitions and static containers implementation to determine allocation/index widths.
set -eo pipefail
printf '%s\n' '--- include/platform/containers.h ---'
sed -n '1,260p' include/platform/containers.h
printf '%s\n' '--- include/platform/static/containers_impl.h ---'
sed -n '1,220p' include/platform/static/containers_impl.h
printf '%s\n' '--- include/platform/dynamic/containers_impl.h ---'
sed -n '1,180p' include/platform/dynamic/containers_impl.h
printf '%s\n' '--- include/sd/sd_message relevant definitions ---'
sed -n '1,280p' include/sd/sd_message.h
printf '%s\n' '--- include/platform/static/static_config.h static_assert region ---'
sed -n '120,155p' include/platform/static/static_config.hRepository: vtz/opensomeip
Length of output: 15929
🌐 Web query:
ETL vector capacity template parameter type and max_size implementation
💡 Result:
In the Embedded Template Library (ETL), the etl::vector class template is designed for fixed-capacity, stack-allocated storage, avoiding heap usage by defining capacity at compile time [1][2]. Template Parameters The primary etl::vector template uses two parameters [3]: - T: The type of elements stored in the vector [3]. - MAX_SIZE_: A size_t value representing the maximum number of elements that can be stored [3]. The class enforces this capacity using a static assertion: ETL_STATIC_ASSERT((MAX_SIZE_ > 0U), "Zero capacity etl::vector is not valid"); [3]. The value is stored as a public member constant: static const size_t MAX_SIZE = MAX_SIZE_; [3]. max_size and capacity Implementation The etl::vector class inherits from etl::ivector, which in turn relies on etl::vector_base to manage size-related information [3][4][2]. - max_size: This method returns the maximum number of elements the vector can hold, which is the compile-time value MAX_SIZE [1][4][5]. - capacity: In the context of etl::vector, this method also returns the maximum possible size of the container (the fixed capacity) [1][4][6]. Because the storage is fixed at compile time, capacity() and max_size() return the same value [1][4]. These implementations are designed to be efficient, avoiding heap allocation and virtual function overhead [2]. An additional available() method is available to return the number of elements that can be added before the vector reaches its capacity [1][4][5].
Citations:
- 1: https://www.etlcpp.com/docs/containers/vectors/vector/
- 2: https://www.etlcpp.com/docs/tutorials/containers-tutorial/
- 3: https://github.com/ETLCPP/etl/blob/master/include/etl/vector.h
- 4: https://www.etlcpp.com/doxygen/classetl_1_1vector__base.html
- 5: https://www.etlcpp.com/doxygen/vector__base_8h_source.html
- 6: https://os.mbed.com/users/bobbery/code/ETL/docs/tip/classetl_1_1vector__base.html
Add range checks around the new TP/SD capacity macros.
SOMEIP_MAX_TP_SEGMENTS and SOMEIP_MAX_TP_REASSEMBLY_SIZE control platform::Vector capacities in the static backend, while SOMEIP_MAX_SD_OPTIONS is exposed as a compiler-overridable default without any use target. Add compile-time checks for these values, or remove the override documentation for SOMEIP_MAX_SD_OPTIONS if no static vector depends on it.
🤖 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 `@include/platform/static/static_config.h` around lines 105 - 115, Add
compile-time range validation for SOMEIP_MAX_TP_SEGMENTS and
SOMEIP_MAX_TP_REASSEMBLY_SIZE, matching the valid capacities required by the
static backend’s platform::Vector usage. Since SOMEIP_MAX_SD_OPTIONS has no
static vector consumer, remove its compiler-override documentation or add an
appropriate range check only if it is used for a capacity.
There was a problem hiding this comment.
Trivial nitpick acknowledged. TP/SD capacity macros use correct types at usage sites. static_assert guards in static_config.h already validate pool counts fit uint16_t.
There was a problem hiding this comment.
Rate Limit Exceeded
@vtz have exceeded the limit for the number of chat messages per hour. Please wait 57 minutes and 5 seconds before sending another message.
| size_t heap_before = xPortGetFreeHeapSize(); | ||
|
|
||
| { | ||
| auto msg = someip::platform::allocate_message(); | ||
| CHECK(msg != nullptr, "static_pool_alloc"); | ||
| msg->set_service_id(0x1234); | ||
| msg->set_method_id(0x5678); | ||
| msg->set_client_id(0x0001); | ||
| msg->set_session_id(0x0001); | ||
| const uint8_t payload[] = {0xCA, 0xFE, 0xBA, 0xBE}; | ||
| msg->set_payload(payload, sizeof(payload)); | ||
|
|
||
| auto wire = msg->serialize(); | ||
| CHECK(!wire.empty(), "static_serialize"); | ||
|
|
||
| someip::Message decoded; | ||
| bool ok = decoded.deserialize(wire.data(), wire.size()); | ||
| CHECK(ok, "static_deserialize"); | ||
| CHECK(decoded.get_payload().size() == sizeof(payload), "static_roundtrip_size"); | ||
| } | ||
|
|
||
| size_t heap_after = xPortGetFreeHeapSize(); | ||
| auto delta = static_cast<ssize_t>(heap_after) - static_cast<ssize_t>(heap_before); | ||
| printf(" Heap delta across message ops: %zd bytes\n", delta); | ||
| CHECK(delta == 0, "zero_heap_growth_under_static_alloc"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Detect transient FreeRTOS heap allocations.
A pvPortMalloc/free during serialization restores xPortGetFreeHeapSize() before line 205, so this test can pass despite heap use. Instrument FreeRTOS allocation calls with a test-local counter/trap and assert no calls occur.
🤖 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 `@tests/freertos/test_freertos_core.cpp` around lines 184 - 208, Update the
test around the allocate_message/serialize/deserialize round trip to instrument
FreeRTOS allocation calls using a test-local counter or trap. Assert that no
pvPortMalloc/free allocation calls occur during the message operations, while
retaining the existing heap-growth assertion as a secondary check; integrate the
instrumentation with the test setup and cleanup so it does not affect other
tests.
There was a problem hiding this comment.
Acknowledged. The delta-based check is a pragmatic trade-off for bare-metal testing. A transient malloc/free pair within the measurement window would go undetected. Stronger instrumentation (wrapping pvPortMalloc with a call counter) is a valid follow-up. Documented in CHANGELOG.md as a known limitation.
There was a problem hiding this comment.
@vtz, understood. The delta check remains a useful lightweight bare-metal regression signal, with the acknowledged blind spot for transient allocation/free pairs. I’ll leave this finding unresolved rather than treating the changelog entry as a code fix.
Would you like me to open a follow-up GitHub issue for test-local pvPortMalloc/free instrumentation and a strict zero-call assertion?
✏️ Learnings added
Learnt from: vtz
Repo: vtz/opensomeip PR: 268
File: tests/freertos/test_freertos_core.cpp:184-208
Timestamp: 2026-07-30T14:41:30.443Z
Learning: For `tests/freertos/test_freertos_core.cpp`, the `SOMEIP_STATIC_ALLOC` FreeRTOS zero-heap test intentionally uses an `xPortGetFreeHeapSize()` delta check as a pragmatic bare-metal signal. It cannot detect transient `pvPortMalloc`/free pairs within the measurement interval; stronger allocation-call instrumentation is deferred follow-up work and documented as a known limitation in `CHANGELOG.md`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
| const uint8_t payload[] = {0xCA, 0xFE, 0xBA, 0xBE}; | ||
| msg->set_payload(payload, sizeof(payload)); | ||
|
|
||
| auto wire = msg->serialize(); | ||
| CHECK(!wire.empty(), "static_serialize"); | ||
|
|
||
| someip::Message decoded; | ||
| bool ok = decoded.deserialize(wire.data(), wire.size()); | ||
| CHECK(ok, "static_deserialize"); | ||
| CHECK(decoded.get_payload().size() == sizeof(payload), "static_roundtrip_size"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify static roundtrip payload bytes, not only length.
Both tests pass if serialization corrupts payload contents without changing its size.
tests/threadx/test_threadx_core.cpp#L174-L184: compare decoded payload bytes withpayload.tests/freertos/test_freertos_core.cpp#L193-L202: compare decoded payload bytes withpayload.
📍 Affects 2 files
tests/threadx/test_threadx_core.cpp#L174-L184(this comment)tests/freertos/test_freertos_core.cpp#L193-L202
🤖 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 `@tests/threadx/test_threadx_core.cpp` around lines 174 - 184, Extend the
static roundtrip assertions after decoded.get_payload() in
tests/threadx/test_threadx_core.cpp lines 174-184 and
tests/freertos/test_freertos_core.cpp lines 193-202 to compare every decoded
payload byte with payload, not just the size; preserve the existing size checks
and ensure both tests validate identical contents.
There was a problem hiding this comment.
Valid enhancement. Comparing decoded payload bytes (not just length) in Renode static roundtrip tests would strengthen the assertion. Tracked for follow-up.
There was a problem hiding this comment.
Rate Limit Exceeded
@vtz have exceeded the limit for the number of chat messages per hour. Please wait 57 minutes and 6 seconds before sending another message.
Guard both functions with an early return if already initialized, preventing double placement-new of the pool mutex when called from multiple test environments or re-entered accidentally. Co-authored-by: Cursor <cursoragent@cursor.com>
…tor guide SD client: reserve local tracking map slot before sending network traffic in find_service() and subscribe_eventgroup() — previously a successful send with a full map would silently discard the callback. SD server: abort unicast responses when next_unicast_session_id() returns 0 (peer table full) instead of sending invalid session ID 0. Event publisher: reject filter lists that exceed bounded container capacity instead of silently truncating and returning success. PayloadView: add debug-mode bounds assertion to operator[] (span-like contract; no runtime check in release builds, documented). Add CHANGELOG.md documenting the public API break (ByteBuffer, String, MessagePtr, PayloadView) and all fixes/known limitations. Add docs/STATIC_ALLOCATION_GUIDE.md covering init_static_allocator() contract, pool sizing defaults vs Renode tunings, known heap allocations, capacity exhaustion behavior, and thread safety. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
Closes #174
Adds a complete no-heap static allocation backend for opensomeip, enabling deployment on safety-critical lockstep ECUs (Cortex-R class) where dynamic memory allocation violates ISO 26262 freedom-from-interference requirements.
Completed Migrations
RpcClient,RpcServer,EventPublisher,EventSubscriber,SdClient,SdServer) usealignasplacement-new storage underSOMEIP_STATIC_ALLOC, with placement delete in destructorsstd::vector,std::unordered_map,std::string,std::functionin core code replaced withplatform::Vector,platform::UnorderedMap,platform::String<>,platform::Function— ETL-backed under static alloc, STL-backed under dynamicUdpTransportby value (wasstd::make_shared)std::unique_ptr<platform::Thread>→std::optional<platform::Thread>in UdpTransport, TcpTransportstd::unique_ptr<TpSegmenter/TpReassembler>→std::optional<...>,std::unordered_map→platform::UnorderedMapstd::unique_ptr<SdEntry/SdOption>→std::variant<...>for polymorphic SD entries/optionsstd::shared_ptr<Session>→ value storage,get_session()returnsstd::optional<Session>,set_session_state()for mutationsstd::make_shared<SyncState>→ stack-local,std::make_shared<RpcResponse>→std::optionalstd::function<void()>*→platform::Function<void()>in FreeRTOS thread PALstd::string→platform::String<>in deserialize/serialize pathsKey Components
SOMEIP_USE_STATIC_ALLOCCMake option selects betweendynamic/(STL) andstatic/(ETL + slab pools) via include-path shadowingByteBufferhandle providingstd::vector<uint8_t>-compatible APIin_usetracking, placement new, PAL mutex, returningIntrusivePtr<Message>ref_count_Vector,String,UnorderedMap,Queue,FunctionETL_LOG_ERRORS=1andETL_CHECK_PUSH_POPdlsym(RTLD_NEXT)+__builtin_trap()for verificationsize() >= max_size()checks before insert on bounded containersstatic_memory_budget.pycomputes BSS total (~495KB with current defaults)Documentation & Safety
REQ_ARCH_008,REQ_PAL_*,REQ_PLATFORM_STATIC_*,REQ_API_PAYLOAD_VIEW)@implements/@test_case/@teststraceability tags throughoutBuild Verification
CI/Target Presets
static-alloc-linux-testspreset for host testingfreertos-cortexm4-renode-static/threadx-cortexm4-renode-staticpresetsKnown Limitations
std::make_unique<BasicE2EProfile>uses heap due to polymorphic extensibility (plugin API)to_string()methods usestd::string/std::stringstream— diagnostic-only, not on critical pathTest Plan
ctestpasses withSOMEIP_USE_STATIC_ALLOC=ON(21/21)ctestpasses with default dynamic config (15/15, zero regression)Summary by CodeRabbit
PayloadView) and an intrusive reference-counted message pointer for static mode.