Fix: retain callable chip buffers by registration#1295
Conversation
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
|
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.
1 participant
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