Fix: retain callable chip buffers by registration#1295
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCallableArtifacts and upload_and_collect_child_addrs are extended to expose chip-callable buffer device address, hash, and size. Onboard and sim DeviceRunnerBase implementations replace a separate orch-SO dedup pool with refcounted lifetime management of the chip-callable buffer, threading chip_buffer_hash/chip_dev through registration and adding release_chip_callable_buffer. Callable-registration glue and runtime maker call sites are updated accordingly. ChangesChipCallable Buffer Unification
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the device runner to manage orchestration shared objects (SOs) directly within the refcounted chip callable buffers, eliminating the separate orch_so_dedup_ cache. It introduces a release mechanism for these buffers and integrates RAII guards in the C API to prevent memory leaks on registration failures. The review feedback recommends adding validation checks to ensure callable->child_count() is non-negative before layout computation and memory operations to prevent potential signed-to-unsigned conversion overflows.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (callable == nullptr || upload_fn == nullptr || out == nullptr) return -1; | ||
| out->clear(); |
There was a problem hiding this comment.
When using a signed integer for a count parameter (such as callable->child_count()), always validate that the count is non-negative (e.g., callable->child_count() >= 0) before performing memory operations or casting to size_t to prevent potential signed-to-unsigned conversion overflows or negative bounds bypasses.
| if (callable == nullptr || upload_fn == nullptr || out == nullptr) return -1; | |
| out->clear(); | |
| if (callable == nullptr || upload_fn == nullptr || out == nullptr) return -1; | |
| if (callable->child_count() < 0) return -1; | |
| out->clear(); |
References
- When using a signed integer for a count or size parameter in functions that perform bounds checks and memory operations, always validate that the count is non-negative to prevent bypassing bounds checks and causing out-of-bounds writes or signed-to-unsigned conversion overflows.
| if (callable == nullptr) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Validate that callable->child_count() is non-negative before proceeding with layout computation and memory allocation to prevent potential signed-to-unsigned conversion overflows or negative bounds bypasses.
if (callable == nullptr) {
return 0;
}
if (callable->child_count() < 0) {
LOG_ERROR("upload_chip_callable_buffer: negative child_count=%d", callable->child_count());
return 0;
}References
- When using a signed integer for a count or size parameter in functions that perform bounds checks and memory operations, always validate that the count is non-negative to prevent bypassing bounds checks and causing out-of-bounds writes or signed-to-unsigned conversion overflows.
| if (callable == nullptr) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Validate that callable->child_count() is non-negative before proceeding with layout computation and memory allocation to prevent potential signed-to-unsigned conversion overflows or negative bounds bypasses.
| if (callable == nullptr) { | |
| return 0; | |
| } | |
| if (callable == nullptr) { | |
| return 0; | |
| } | |
| if (callable->child_count() < 0) { | |
| LOG_ERROR("upload_chip_callable_buffer: negative child_count=%d", callable->child_count()); | |
| return 0; | |
| } |
References
- When using a signed integer for a count or size parameter in functions that perform bounds checks and memory operations, always validate that the count is non-negative to prevent bypassing bounds checks and causing out-of-bounds writes or signed-to-unsigned conversion overflows.
Keep one refcounted ChipCallable buffer lease per registered callable id. Use the retained chip storage slice as the device orch SO. This removes the second orch SO upload for registered callables. Release the retained chip buffer on unregister and registration failure. Fixes hw-native-sys#1082
puddingfjz
force-pushed
the
fix/issue-1082-unify-callable-buffer
branch
from
cbc5945 to
c8f12ef
Compare
ChaoWao
left a comment
ChaoWao
left a comment
There was a problem hiding this comment.
Mechanism is correct and faithfully implements #1082 (both the double-upload and the leak-until-finalize). Verified the layout assumption (ChipCallable::binary_data() == storage_, orch SO is the leading slice) and traced every register/unregister failure path — the refcount balance holds on all of them. Leaving 3 non-blocking comments inline.
| CallableState state = std::move(it->second); | ||
| callables_.erase(it); | ||
| aicpu_seen_callable_ids_.erase(callable_id); | ||
| release_chip_callable_buffer(state.chip_buffer_hash); |
There was a problem hiding this comment.
Should fix — no regression test for this lifetime/refcount fix.
Both bugs #1082 describes are silent ("Neither is a crash today"), and every existing test passes with or without the fix: the prepared_callable ST asserts only functional correctness and the monotonic aicpu_dlopen_count (never decremented on unregister), so it cannot observe a chip-buffer leak; test_chip_callable_upload_immutable.cpp is a buffer-immutability contract test unrelated to refcounting.
If a future edit drops this release_chip_callable_buffer call, or the chip_buffer_guard/dismiss() → CallableState ownership handoff regresses, nothing goes red. Per .claude/rules/discipline.md §3 a bugfix should leave a barrier behind.
Cheapest lock-in: a cpput test on DeviceRunnerBase — register a callable, assert chip_callable_buffers_.size() == 1; unregister, assert 0; register identical bytes at two cids, assert the dedup refcount, unregister one → still present, unregister the other → freed. (Alternatively expose a live-buffer counter mirroring aicpu_dlopen_count and assert delta == 0 after register+unregister in the existing ST.)
| out->clear(); | ||
| if (callable->child_count() <= 0) return 0; | ||
|
|
||
| const ChipCallableLayout layout = compute_chip_callable_layout(callable); |
There was a problem hiding this comment.
Consider (minor). compute_chip_callable_layout(callable).content_hash is computed here, and upload_chip_callable_buffer recomputes the same layout/hash internally to key chip_callable_buffers_. It's a once-per-registration FNV pass over the whole buffer — negligible, and the two-computation split keeps the upload_fn pointer signature clean. Fine to leave; noting only for completeness.
| } | ||
|
|
||
| CallableState state; | ||
| state.hash = hash; |
There was a problem hiding this comment.
Consider (informational). With orch_so_dedup_ gone, state.hash (the orch ELF build-id) is no longer used for any buffer lookup — its only remaining consumer is the callable_hash() getter (unchanged by this PR, still used elsewhere). Not dead code, but the field's role has narrowed to just that accessor; worth a note so a future reader doesn't assume it still drives dedup.
2 participants
Keep one refcounted ChipCallable buffer lease per registered callable id.
Use the retained chip storage slice as the device orch SO.
This removes the second orch SO upload for registered callables.
Release the retained chip buffer on unregister and registration failure.
Fixes #1082