Skip to content

fix: harden SIMD feature detection, bounds checks, SWIG bindings, and add inspection tools - #5

Closed
korrayt wants to merge 1 commit into
EpicGames:mainfrom
korrayt:fix/riglogic-stability-and-optimizations
Closed

korrayt wants to merge 1 commit into
EpicGames:mainfrom
korrayt:fix/riglogic-stability-and-optimizations

Conversation

@korrayt

@korrayt korrayt commented Oct 10, 2026 •

Copy link
Copy Markdown

Summary

Hardens RigLogic runtime stability across hypervisors, malformed DNA streams, Python SWIG bindings, and vectorized solvers, and introduces missing inspection tools.

Linked issue

Fixes multiple stability edge cases and crashes reported on hypervisors, SWIG string passing, and DNA binary stream validation.

What changed and why

  • trimd (AVX/AVX2/AVX512/FMA3/F16C Detection): Guarded x86 SIMD feature detection with OSXSAVE (_xgetbv(0)) to verify that the OS has enabled AVX and AVX-512 state saving before executing extended instructions. Prevents xC000001D (STATUS_ILLEGAL_INSTRUCTION) crashes on hypervisors and virtualized environments where the CPU supports AVX but the OS disables it.
  • riglogic (SWIG Python Bindings): Added spyus/CharPtr.i into RigLogic.i to handle null string conversions gracefully and prevent NULL-pointer dereference segmentation faults when None is passed to string parameters in Python.
  • dna (Stream Bounds Validation): Hardened FilteredBinaryInputArchive bounds checks in processSubset,
    emoveByIndex, and RawVertexSkinWeights against corrupted or truncated DNA files.
  • dna (Coordinate Converter Optimization): Optimized CoordinateSystemConverter to avoid redundant (N \times \text{cols})$ re-allocations of
    ewOutputIndices on each column iteration.
  • ml (Generic Vector Templates): Provided complete generic vector templates for SigmoidActivationFunction and TanHActivationFunction across arbitrary SIMD vector widths.
  • rbf (Solver Vectorization & Widen Counters): Vectorized and unrolled dot product calculations in InterpolativeRBFSolver for enhanced throughput; widened RBFSolver loop counter to std::size_t to eliminate potential integer truncation on large weight sets.
  • python (UTF-8 File Encoding): Enforced UTF-8 encoding in insert_copyright.py to prevent Windows charmap/CP1252 decoding failures.
  • tools (Inspection CLI & Python Inspector): Added ools/CMakeLists.txt, DnaInfo C++ command-line tool, and a standalone pure-Python dna_tool.py for headless DNA introspection.

Test plan

  • Validated with CTest suite across all build configurations (Release, Debug, RelWithDebInfo); all 4 test targets passed (dna-tests,
    iglogic-tests, rimd-tests, pydna-tests).
  • Verified Python SWIG bindings with est_riglogic.py; confirmed None strings no longer crash the interpreter.
  • Tested DnaInfo CLI and dna_tool.py against sample .dna asset files.

Checklist

… add inspection tools

- trimd: Guard AVX/AVX2/AVX512/FMA3/F16C with OSXSAVE and XCR0 verification to prevent illegal instruction crashes (0xC000001D) on hypervisors and VMs
- riglogic: Include spyus/CharPtr.i in RigLogic.i to prevent NULL-dereference segfaults when None is passed to string parameters
- dna: Harden FilteredBinaryInputArchive bounds checks in processSubset, removeByIndex, and RawVertexSkinWeights against malformed stream corruption
- dna: Optimize CoordinateSystemConverter to avoid redundant O(N * cols) writes of newOutputIndices on each column
- ml: Provide complete generic vector templates for SigmoidActivationFunction and TanHActivationFunction across all SIMD widths
- rbf: Vectorize and unroll dot products in InterpolativeRBFSolver; widen RBFSolver loop counter to size_t to prevent truncation
- python: Enforce utf-8 encoding in insert_copyright.py to prevent Windows charmap decode errors
- tools: Add missing tools/CMakeLists.txt, DnaInfo C++ CLI, and standalone pure-Python dna_tool.py inspector

Signed-off-by: Koray Tasan <korrayt@users.noreply.github.com>
@korrayt korrayt changed the title fix: harden SIMD feature detection, bounds checks, SWIG bindings, and… fix: harden SIMD feature detection, bounds checks, SWIG bindings, and add inspection tools Oct 11, 2026
@andrean-3lateral

Copy link
Copy Markdown

Hi @korrayt, thanks for taking the time to go through the codebase and for the detailed PR. I've reviewed each change.

What we could take

  • AVX/AVX2/AVX-512 detection without OSXSAVE/XCR0 checks - this is a real gap, we could incorporate the patch in our codebase.
  • encoding='utf-8' in both insert_copyright.py files - correct, the generated wrapper can contain non-ASCII doxygen text
  • Rejecting mismatched skin-weight arrays in RawVertexSkinWeights - this one is a real bug, confirmed under AddressSanitizer. The weights and jointIndices arrays are serialized with independent length prefixes, so a corrupted stream can make them unequal, and under an active LOD constraint the joint filter then walks the shorter array out of bounds. We do have a malformed data path in place though, and this should go through that as well.
  • std::size_t loop counter in RBFSolver::normalizeAndCutOff - fine as a type cleanup. The manual 4-wide unroll around it we'd rather not take (see below).

What we can't merge, and why

  1. %include <spyus/CharPtr.i> in RigLogic.i - the riglogic module has no const char* parameters, so this is a no-op. The None/wrong-type crashes were fixed in 13.5.0 in DNA.i (where the string parameters actually live) together with the module capsule publish in RigLogic.i. If you can still reproduce a crash on main, please open an issue with the call.
  2. Generic primary templates for SigmoidActivationFunction / TanHActivationFunction - the primary template is intentionally declared but not defined so that a missing width specialization is a compile error. Defining it would hide that behind a scalar store/loop/load fallback. In the current build it would also be dead code, since the 4-wide specialization covers everything LayerEvaluator.h instantiates.
  3. 4-wide manual unroll in InterpolativeRBFSolver::solve - this changes the floating-point summation order, so pose weights differ in the low bits from current builds and from the UE evaluator. It's a hot path, so any change here needs an A/B benchmark against the rig suite plus a check that results stay within the regression tolerance. The compiler already vectorizes this loop. If we revisit it, it will go through the TRiMD vector types with measurements, not a hand unroll.
  4. DNA processSubset and removeByIndex assert replacements - the stream is bounded, so none of these is a memory-safety issue today. The early return in processSubset also leaves the stream position where the old path advanced it, and the silent skip in removeByIndex hides a malformed state rather than flagging it.
  5. CoordinateSystemConverter first-column-only index write - correct, but it's a cold conversion path and unmeasured, so we'd rather not carry it as part of a mixed PR.
  6. New tools/ directory - dna_tool.py doesn't match the file format: a DNA file starts with a 3-byte DNA signature followed by big-endian 16-bit fields, while the tool assumes a 4-byte DNA\0 signature with little-endian fields, so it reports wrong values for real files (the unit tests only feed it synthetic bytes in the tool's own layout). The tools/ directory is also only built with RL_BUILD_TOOLS=ON, so the CTest runs in the test plan did not compile it. For inspecting DNA files, the Python wrapper and python/dna/examples/demo.py already cover this.

Contribution process

Please have a look at CONTRIBUTING.md; this PR skipped several steps it requires, and following them is what lets us merge the good parts quickly. Specifically:

  1. Open an issue first. Anything beyond a trivial fix needs a GitHub Issue before code, so a maintainer can weigh in on scope. None of the eight changes here had one. For the detection and skin-weights items, I'll open the issues myself and link them to the fixes.
  2. One logical change per PR. Eight unrelated changes in one squashed commit can't be reviewed or merged partially. Each accepted item above would have been a one-file PR we could have taken the same day.
  3. Tests for the behaviour you change. Every library change here landed without a test. A fix needs a test that fails before it and passes after.
  4. Performance note for hot-path changes. The RBF unroll touches the evaluation path and must come with before-and-after measurements on a representative platform. None were provided.
  5. PR description and commit message. The description should follow the What / Why / How / Testing structure and link the issue. The commit subject must be imperative and under 72 characters, with the issue referenced in the body. The DCO sign-off is present, thank you for that.
  6. Copyright headers on new files. New files should carry your own copyright line plus an SPDX identifier, not Epic's header. The new tools/ files were added under Epic's copyright, which we can't accept as written.
  7. AI assistance disclosure. Contributions that used AI tools are welcome, but the policy requires you to disclose which tools in the PR description, to review and test all generated code yourself, and to write the PR description in your own words. Several items here (the tools directory, the DNA header parser, the test plan) don't match what the code actually does, which is usually a sign that output wasn't verified. If AI tools were involved, please say so in future PRs and make sure you can explain and have run every line you submit.

I'll close this PR once the detection fix, the skin-weights fix and the UTF-8 change have landed, and I'll link the commits and issues here so the credit stays attached to your findings. If you'd like to resubmit any of the declined items, open an issue first describing the problem and the evidence, and we'll take it from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants