Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18804
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 2 New Failures, 4 Cancelled Jobs, 2 Unrelated FailuresAs of commit ff93cd1 with merge base 21d9c64 ( NEW FAILURES - The following jobs have failed:
CANCELLED JOBS - The following jobs were cancelled. Please retry:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
|
|
||
| // Safe lookup in remapped_ids that returns an error instead of throwing | ||
| // from std::unordered_map::at() in noexcept functions. | ||
| inline Result<uint32_t> remapId( |
There was a problem hiding this comment.
Use remapId + ET_UNWRAP because a macro could potentially have larger binary size bloat.
There was a problem hiding this comment.
Pull request overview
This PR hardens XNNPACK runtime compilation by replacing unchecked std::unordered_map::at() accesses (which can throw inside noexcept node-definition functions and trigger std::terminate()) with a safe remapId() helper that returns Result<uint32_t> and propagates an Error instead.
Changes:
- Add
remapId()helper to safely look up remapped tensor/value IDs without exceptions. - Update node-definition helpers to use
ET_UNWRAP(remapId(...))instead ofremapped_ids.at(...)when defining XNNPACK operators.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uint32_t key) { | ||
| auto it = map.find(key); | ||
| if (it == map.end()) { | ||
| ET_LOG(Error, "Remapped id not found for key %u", key); |
| auto cvt_input_id = ET_UNWRAP(remapId(remapped_ids, graph_node->input_id())); | ||
| auto cvt_output_id = ET_UNWRAP(remapId(remapped_ids, graph_node->output_id())); | ||
|
|
|
@claude review this PR |
|
Claude finished @JacobSzwejbka's task in 2m 42s —— View job PR Review: Fix unchecked map access in XNNPACK compiler
Summary: This PR replaces all CorrectnessThe core approach is sound. All ~74 No remaining Issues1. Commit message is stale — The commit says "replace remapped_ids.at() with safe REMAP_ID macro" but the implementation uses a 2. Lint failure (clang-format) — needs fix — CI reports formatting violations at several locations. The issues are minor whitespace/line-break differences. Running
3. Error message lacks node context — (echoing Copilot's review comment) The ET_LOG(Error, "Remapped id not found for key %u", key);When this fails, the caller is deep in one of ~30 auto fc_input1 = ET_UNWRAP(
remapId(remapped_ids, graph_node->input1_id()),
"input1_id remap failed for node %u", node->debug_handle());This is a quality-of-life improvement and not a blocker. Unrelated CI failures
OverallThe security fix is correct and well-structured. The The only blocking issue is the clang-format lint failure, which should be a straightforward fix with |
There was a problem hiding this comment.
Pull request overview
This PR hardens the XNNPACK runtime compiler against unchecked tensor-id remapping lookups that could previously throw std::out_of_range inside noexcept node-definition functions (leading to std::terminate()), by converting those lookups into Result-based error propagation.
Changes:
- Added a
remapId()helper that safely looks up remapped tensor IDs and returns anErroron missing keys rather than throwing. - Replaced
remapped_ids.at(...)usages across node-definition helpers withET_UNWRAP(remapId(...))to propagate errors cleanly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Safe lookup in remapped_ids that returns an error instead of throwing | ||
| // from std::unordered_map::at() in noexcept functions. | ||
| inline Result<uint32_t> remapId( | ||
| const std::unordered_map<uint32_t, uint32_t>& map, | ||
| uint32_t key) { | ||
| auto it = map.find(key); | ||
| if (it == map.end()) { | ||
| ET_LOG(Error, "Remapped id not found for key %u", key); | ||
| return Error::Internal; | ||
| } |
…acro Introduce REMAP_ID macro that uses find() + ET_CHECK_OR_RETURN_ERROR instead of std::unordered_map::at(), which throws std::out_of_range in noexcept functions causing std::terminate(). Applied across all ~30 node-definition functions in XNNCompiler. Authored-with: Claude
|
I think the key is enough to correlate with the flatbuffer, and don't need the additional context / can get noisy. |
GregoryComer
left a comment
GregoryComer
left a comment
There was a problem hiding this comment.
Looks good to me. One note - ET_UNWRAP relies on Clang/GCC-specific extensions and won't build with MSVC. I don't know what guarantees we want to make for MSVC, in general, so it might be fine. CC @JacobSzwejbka
Reverts #18804, which is breaking windows tests
Introduce remapId function that checks error instead of std::unordered_map::at(), which throws std::out_of_range in noexcept functions causing std::terminate(). Applied across all ~30 node-definition functions in XNNCompiler. Authored-with: Claude Co-authored-by: Github Executorch <github_executorch@arm.com>
Reverts pytorch#18804, which is breaking windows tests
Introduce remapId function that checks error instead of std::unordered_map::at(), which throws std::out_of_range in noexcept functions causing std::terminate(). Applied across all ~30 node-definition functions in XNNCompiler. retake #18804, for windows compatibility Co-authored-by: Github Executorch <github_executorch@arm.com>
Introduce remapId function that checks error instead of std::unordered_map::at(), which throws std::out_of_range in noexcept functions causing std::terminate(). Applied across all ~30 node-definition functions in XNNCompiler.
Authored-with: Claude