Fix unchecked map access in xnnpack

[lucylq]

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

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19008

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 3 New Failures, 2 Unrelated Failures

As of commit 909b931 with merge base 8ed6e85 ():

NEW FAILURES - The following jobs have failed:

• pull / test-multimodal-linux (gemma3-4b) / linux-job (gh)
  RuntimeError: Command docker exec -t 8d5d8dcb390d91264e665ce36c994bc077175da856ff0875cf30a9cb696fdb0e /exec failed with exit code 139
• pull / unittest / macos / macos-job (gh)
  export/tests/test_target_recipes.py::TestTargetRecipes::test_mv2_model
• pull / unittest-editable / macos / macos-job (gh)
  export/tests/test_target_recipes.py::TestTargetRecipes::test_mv2_model

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• pull / unittest / windows / windows-job (gh) (trunk failure)
  ##[error]The operation was canceled.
• pull / unittest-editable / windows / windows-job (gh) (trunk failure)
  ##[error]The operation was canceled.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[github-actions]

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

[lucylq]

@claude review

[claude]

Claude finished @lucylq's task in 1m 14s —— View job

---

Review of PR #19008

• Read the diff and changed file
• Analyze the REMAP_ID macro and its usage
• Check all call sites for correctness
• Post review feedback

---

This is a well-motivated defensive fix. All ~30 node-definition functions are noexcept, which means the previous std::unordered_map::at() calls would trigger std::terminate() if a key was missing (since at() throws std::out_of_range). The REMAP_ID macro replaces this with a find()-based lookup that returns Error::Internal instead. The approach is sound.

Macro design

The REMAP_ID macro (XNNCompiler.cpp:656-666) is well-structured:

• _et_remap_it is scoped inside {} braces, so multiple invocations in the same function don't conflict.
• out_var is declared outside the braces (in function scope), giving each callsite a uniquely-named uint32_t variable.
• ET_CHECK_OR_RETURN_ERROR uses a plain return statement, which correctly exits the enclosing function even from within the nested braces.
• The static_cast<unsigned>(key) in the error message is portable for %u formatting.

The Windows compatibility fix (vs. the prior PR #18804) is the avoidance of GCC/Clang statement-expression extensions (({...})), which MSVC doesn't support. Using a block + separate variable declaration is the right portable approach.

One minor observation

The macro introduces a leading-underscore identifier _et_remap_it. Per the C++ standard, identifiers starting with underscore followed by a lowercase letter are reserved in the global namespace but allowed in local/block scope ([basic.scope]). Since this is always expanded inside function body blocks, it's technically fine -- though some linters or style guides may flag it. Not a blocking concern.

Completeness check

I verified all 34 REMAP_ID call sites across the file. Every previous remapped_ids.at(...) has been converted, including:

• All explicitly written node functions (convert, fully-connected, softmax, pooling, conv2d, etc.)
• The generic helpers defineGenericUnaryNode and defineGenericBinaryNode
• All macro-generated unary/binary node functions (which call through the generic helpers)

No remapped_ids.at( calls remain.

Summary

Looks good. The change is mechanical, consistent, and addresses a real crash risk on map lookup failure in noexcept contexts. The macro design is portable and correct.

---

[Copilot]

Pull request overview

Prevents std::terminate() in XNNPACK graph compilation by replacing unchecked std::unordered_map::at() lookups (which can throw inside noexcept node-definition functions) with a checked remap-id lookup that returns an Error instead. This improves robustness and Windows/MSVC compatibility for malformed or unexpected serialized graphs.

Changes:

• Added a REMAP_ID helper macro to safely look up remapped tensor IDs and return Error::Internal when missing.
• Replaced remapped_ids.at(...) usages across many node-definition helpers (convert/conv/pooling/unary/binary/etc.) with the checked lookup.
• Updated generic unary/binary node-definition helpers to use the new safe remapping path.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

REMAP_ID is a macro that expands to multiple statements and evaluates key (and map) more than once. That makes it easy to misuse later (e.g., as the body of an if without braces, or with a non-trivial key expression), and it can also lead to duplicate evaluation costs/side effects. Consider capturing key/map into temporaries inside the macro, or replacing this with a small static inline helper (returning Error and writing to an out-param) and using ET_CHECK_OK_OR_RETURN_ERROR(...) at call sites for type-safety and single evaluation.

Suggested change

	// Clang, and GCC (no statement-expression extension).
	#define REMAP_ID(map, key, out_var) \
	uint32_t out_var = 0; \
	{ \
	const auto _et_remap_it = (map).find(key); \
	ET_CHECK_OR_RETURN_ERROR( \
	_et_remap_it != (map).end(), \
	Internal, \
	"Remapped id not found for key %u", \
	static_cast<unsigned>(key)); \
	out_var = _et_remap_it->second; \
	}
	// Clang, and GCC (no statement-expression extension). Evaluates `map` and
	// `key` exactly once and expands to a single declaration statement.
	#define REMAP_ID(map, key, out_var) \
	uint32_t out_var = [&]() -> uint32_t { \
	auto&& _et_remap_map = (map); \
	auto _et_remap_key = (key); \
	const auto _et_remap_it = _et_remap_map.find(_et_remap_key); \
	ET_CHECK_OR_RETURN_ERROR( \
	_et_remap_it != _et_remap_map.end(), \
	Internal, \
	"Remapped id not found for key %u", \
	static_cast<unsigned>(_et_remap_key)); \
	return _et_remap_it->second; \
	}()