MagicBeanDev/Hookshot
Archived
Watch
6
0
Fork
You've already forked Hookshot
 0
Code Issues 125 Pull requests 205 Activity

fix(host): BroadcastConditionVariable and the timed semaphore acquires reach the HLE's objects #2765

Open
github-sync wants to merge 3 commits from github/pr/2765 into main
pull from: github/pr/2765
merge into: MagicBeanDev:main
Conversation 4 Commits 3 Files changed 2 +284 -1
github-sync commented 2026-11-12 12:37:00 +00:00
Owner

@klimovich008 opened this pull request on GitHub · 2026-11-12 12:01 UTC · original

The HLE replaces the nn::os condition variable and semaphore functions with host objects, but a few siblings were left to the game's own SDK code. That code acts on guest words the HLE never writes, and signals kernel keys that no HLE waiter sleeps on. Some of these siblings have callers.

The problem

  • BroadcastConditionVariable woke nobody.
    • The SDK's version signals the kernel key at cv+4 (svcSignalProcessWideKey). The HLE's WaitConditionVariable sleeps on its host object until the object's generation moves, and only the HLE's Signal moves it.
    • It is called by the movie framework's reply and buffer queues, and by NVIDIA's OS layer.
    • Their waits are untimed, so a waiter only got out if some other Signal reached the same condition variable.
    • This could explain movie hangs. It is not reproduced here.
  • TimedAcquireSemaphore never saw an HLE release. The C library's timed semaphore wait and NVIDIA's OS layer call it. The light-semaphore timed and try acquires have the same gap but no direct callers.

The change

  • Broadcast (host/nn_os_sync.c):
    • Under cv->lk, it moves the generation and wakes all waiters.
    • As in Horizon, where a broadcast moves every waiter onto the mutex, each waiter that is not yet owed the mutex is owed it. This is the same hand-off a Signal makes (cv_reserve): one mutex for all of them, at most 8 owed. HOS_NO_HANDOFF=1 turns it off, as for Signal.
  • TimedAcquireSemaphore, TimedAcquireLightSemaphore, TryAcquireLightSemaphore: these take the HLE's count through sema_acquire_for, which waits up to the timeout. A timeout of 0 or less tries once.
  • wait_until: sleeps in steps of at most 0x7FFFFFFF ms, because a finite wait of 0xFFFFFFFF ms is INFINITE on Windows. Its callers already wait again until their deadline.
  • The timed acquire's waits are noted for HOS_CHAIN, like sema_acquire's. Its deadline saturates for a huge TimeSpan.

How these were found: the image's exports were compared with the HLE's list, per sync object type. Siblings still left to the SDK that have direct callers:

  • StopTimerEvent (the C library's timers);
  • RegisterSystemWorkerHandler.

The HLE timer has no separate "signalled" state, so a correct Stop needs a model change; it is left for a follow-up.

Tested

  • tests/test_os_lifted_siblings.py (new) runs the real code cut from the source (cv_enter, cv_leave, cv_reserve, the Broadcast HLE, sema_acquire_for) over counting stubs and a fake clock.
    • Broadcast: with no waiters, three, one already owed plus two, ten (8 owed), and mixed mutexes. Each case is run with HOS_NO_HANDOFF unset, 1 and 0.
    • Semaphore: a count already there, a zero or negative timeout, a release inside the timeout, no release, a release after the deadline, a timeout so large the deadline would wrap, and the HOS_CHAIN notes.
    • Broken variants must fail: one wake instead of all, no generation move, one reservation, a single wait, taking a count of 0, no decrement.
    • A line check pins the three acquire HLEs to sema_acquire_for.
    • The test fails on main, which has no Broadcast HLE.
  • tests/run_all.py: 425 passed. The 5 failures are this PC's toolchain and fail the same way on main:
    • nx2/test_nup_paths.py: symlinks on Windows;
    • test_spawn_emit, test_fpx_sticky: <algorithm> not found;
    • test_pipeline_play_cache, test_prepare_stall: NOMINMAX redefined.
  • Syntax compile of every host/*.c; a gpt-6.1-sol review (three rounds, last clean).

In game

Boot to title and prologue movie OK; Broadcast is not reached in this scene, so the movie-hang link is not shown.

  • Windows 11, Ryzen 7 5700X3D, RX 9070 XT, AMD driver 26.10.2; base main 46780503. A new game from an empty save, the prologue movie, 190 s a run.
  • Without the change, the movie played to its end (154.2 s).
  • With the change, the movie was at 137.8 s, 0.02 s ahead of the wall clock, when the 190 s stop came. That run's load took longer, so the stop cut the movie before its end; it was not stuck.
  • A local log in the changed build printed every Broadcast call: none in that run.
  • No FATAL, device lost or Vulkan validation (VUID) line in either run.
  • n = 2 of the planned 4. The PC's memory guard refused the third run, and since this scene never calls Broadcast, more runs of it could not show anything new.

Not tested

  • A scene that reaches Broadcast in game, so whether this fixes a movie hang is not shown.
  • Linux.

🤖 Generated with Claude Code

> [@klimovich008](https://github.com/klimovich008) opened this pull request on GitHub · 2026-11-12 12:01 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2765) The HLE replaces the nn::os condition variable and semaphore functions with host objects, but a few siblings were left to the game's own SDK code. That code acts on guest words the HLE never writes, and signals kernel keys that no HLE waiter sleeps on. Some of these siblings have callers. ## The problem - **`BroadcastConditionVariable`** woke nobody. - The SDK's version signals the kernel key at cv+4 (`svcSignalProcessWideKey`). The HLE's `WaitConditionVariable` sleeps on its host object until the object's generation moves, and only the HLE's Signal moves it. - It is called by the movie framework's reply and buffer queues, and by NVIDIA's OS layer. - Their waits are untimed, so a waiter only got out if some other Signal reached the same condition variable. - This could explain movie hangs. It is not reproduced here. - **`TimedAcquireSemaphore`** never saw an HLE release. The C library's timed semaphore wait and NVIDIA's OS layer call it. The light-semaphore timed and try acquires have the same gap but no direct callers. ## The change - **Broadcast (`host/nn_os_sync.c`):** - Under `cv->lk`, it moves the generation and wakes all waiters. - As in Horizon, where a broadcast moves every waiter onto the mutex, each waiter that is not yet owed the mutex is owed it. This is the same hand-off a Signal makes (`cv_reserve`): one mutex for all of them, at most 8 owed. `HOS_NO_HANDOFF=1` turns it off, as for Signal. - **`TimedAcquireSemaphore`, `TimedAcquireLightSemaphore`, `TryAcquireLightSemaphore`:** these take the HLE's count through `sema_acquire_for`, which waits up to the timeout. A timeout of 0 or less tries once. - **`wait_until`:** sleeps in steps of at most 0x7FFFFFFF ms, because a finite wait of 0xFFFFFFFF ms is `INFINITE` on Windows. Its callers already wait again until their deadline. - The timed acquire's waits are noted for `HOS_CHAIN`, like `sema_acquire`'s. Its deadline saturates for a huge TimeSpan. How these were found: the image's exports were compared with the HLE's list, per sync object type. Siblings still left to the SDK that have direct callers: - `StopTimerEvent` (the C library's timers); - `RegisterSystemWorkerHandler`. The HLE timer has no separate "signalled" state, so a correct Stop needs a model change; it is left for a follow-up. ## Tested - `tests/test_os_lifted_siblings.py` (new) runs the real code cut from the source (`cv_enter`, `cv_leave`, `cv_reserve`, the Broadcast HLE, `sema_acquire_for`) over counting stubs and a fake clock. - **Broadcast:** with no waiters, three, one already owed plus two, ten (8 owed), and mixed mutexes. Each case is run with `HOS_NO_HANDOFF` unset, `1` and `0`. - **Semaphore:** a count already there, a zero or negative timeout, a release inside the timeout, no release, a release after the deadline, a timeout so large the deadline would wrap, and the `HOS_CHAIN` notes. - **Broken variants must fail:** one wake instead of all, no generation move, one reservation, a single wait, taking a count of 0, no decrement. - A line check pins the three acquire HLEs to `sema_acquire_for`. - The test fails on main, which has no Broadcast HLE. - `tests/run_all.py`: 425 passed. The 5 failures are this PC's toolchain and fail the same way on main: - `nx2/test_nup_paths.py`: symlinks on Windows; - `test_spawn_emit`, `test_fpx_sticky`: `<algorithm>` not found; - `test_pipeline_play_cache`, `test_prepare_stall`: `NOMINMAX` redefined. - Syntax compile of every `host/*.c`; a gpt-6.1-sol review (three rounds, last clean). ## In game Boot to title and prologue movie OK; Broadcast is not reached in this scene, so the movie-hang link is not shown. - Windows 11, Ryzen 7 5700X3D, RX 9070 XT, AMD driver 26.10.2; base main 46780503. A new game from an empty save, the prologue movie, 190 s a run. - Without the change, the movie played to its end (154.2 s). - With the change, the movie was at 137.8 s, 0.02 s ahead of the wall clock, when the 190 s stop came. That run's load took longer, so the stop cut the movie before its end; it was not stuck. - A local log in the changed build printed every Broadcast call: none in that run. - No FATAL, device lost or Vulkan validation (VUID) line in either run. - n = 2 of the planned 4. The PC's memory guard refused the third run, and since this scene never calls Broadcast, more runs of it could not show anything new. ## Not tested - A scene that reaches Broadcast in game, so whether this fixes a movie hang is not shown. - Linux. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- github-sync:item:2765 -->
The HLE replaces the nn::os condition variable and semaphore functions with host objects, but left some siblings to the
game's own SDK code, which acts on guest words the HLE never writes and signals kernel keys no HLE waiter sleeps on.
- BroadcastConditionVariable woke nobody (the movie framework and NVIDIA's OS layer call it, with untimed waiters). It
  now moves the generation and wakes all, and each waiter not yet owed the mutex is owed it (the EvilOOT hand-off;
  HOS_NO_HANDOFF=1: none).
- TimedAcquireSemaphore, TimedAcquireLightSemaphore and TryAcquireLightSemaphore take the HLE's count
  (sema_acquire_for). Its deadline saturates, and its waits are noted for HOS_CHAIN.
- wait_until sleeps in bounded steps: a finite 0xFFFFFFFF ms is INFINITE on Windows.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
github-sync commented 2026-11-12 14:47:44 +00:00
Author
Owner

@Joey-Magnus reviewed on GitHub · 2026-11-12 14:35 UTC · original

I merged this into current main. tests/test_os_lifted_siblings.py and tests/test_multiwait_wake.py pass. The full tests/run_all.py passes except tests/test_lift_cache.py, which fails on main here too, and test_recorder_sync, which timed out under load.

The code looks right. The Broadcast HLE (host/nn_os_sync.c) bumps gen under cv->lk and wakes all waiters, makes the same hand-off Signal does (capped at cv_reserve's 8), and respects HOS_NO_HANDOFF. sema_acquire_for saturates the deadline and tries once for a timeout of 0 or less, and the 0x7FFFFFFF ms cap in wait_until avoids INFINITE.

My concern is reach. Until now, every Broadcast caller (ALooper, LiveDataSource, NvOs) slept until some unrelated Signal. Now they wake on time. That's the fix, but it changes timing on every movie and NVN OS-layer path, and nothing has been run in game. Please boot to the title, play a scene with a movie (the opening, or one of the reported hangs), and add that run to the description. Then I'd merge it.

> [@Joey-Magnus](https://github.com/Joey-Magnus) reviewed on GitHub · 2026-11-12 14:35 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2765#pullrequestreview-5876249386) I merged this into current main. tests/test_os_lifted_siblings.py and tests/test_multiwait_wake.py pass. The full tests/run_all.py passes except tests/test_lift_cache.py, which fails on main here too, and test_recorder_sync, which timed out under load. The code looks right. The Broadcast HLE (host/nn_os_sync.c) bumps gen under cv->lk and wakes all waiters, makes the same hand-off Signal does (capped at cv_reserve's 8), and respects HOS_NO_HANDOFF. sema_acquire_for saturates the deadline and tries once for a timeout of 0 or less, and the 0x7FFFFFFF ms cap in wait_until avoids INFINITE. My concern is reach. Until now, every Broadcast caller (ALooper, LiveDataSource, NvOs) slept until some unrelated Signal. Now they wake on time. That's the fix, but it changes timing on every movie and NVN OS-layer path, and nothing has been run in game. Please boot to the title, play a scene with a movie (the opening, or one of the reported hangs), and add that run to the description. Then I'd merge it. <!-- github-sync:r:5876249386 -->
github-sync commented 2026-11-12 16:56:04 +00:00
Author
Owner

@hckrman101 commented on host/nn_os_sync.c line 936 on GitHub · 2026-11-12 16:33 UTC · original

nit: this saturation is right. The other timed waits in this file (TimedWaitConditionVariable, event_wait, the queue) still wrap on a huge TimeSpan. A shared helper would cover them all (could be a follow-up).

> [@hckrman101](https://github.com/hckrman101) commented on `host/nn_os_sync.c` line 936 on GitHub · 2026-11-12 16:33 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2765#discussion_r4553286100) nit: this saturation is right. The other timed waits in this file (`TimedWaitConditionVariable`, `event_wait`, the queue) still wrap on a huge TimeSpan. A shared helper would cover them all (could be a follow-up). <!-- github-sync:rc:4553286100 -->
github-sync commented 2026-11-12 16:56:04 +00:00
Author
Owner

@hckrman101 reviewed on GitHub · 2026-11-12 16:33 UTC · original

Thanks, this is a real gap and the code holds up. I went through it for lost and spurious wake-ups and the timeout arithmetic, and agree with Joey-Magnus's read:

  • Broadcast: gen moves under cv->lk and the wake-all comes after, so a waiter that snapshotted the old generation can't miss it. The reservations balance: one per waiter counted at the broadcast, capped at 8, and every one of those waiters leaves under the same lock (a timed waiter that is timing out included), so nothing stays owed to nobody. Mixed mutexes and HOS_NO_HANDOFF=1 fall back the same way Signal does.
  • sema_acquire_for: it checks the count under the lock, loops on spurious and vcore wake-ups, takes a release that races the deadline, and a timeout of 0 or less tries once. The saturated deadline can't wrap.
  • wait_until: clamping to 0x7FFFFFFF ms fixes both platforms. Windows treats 0xFFFFFFFF as INFINITE, and so does the POSIX shim (ms == INFINITE). All four callers loop back to their deadline.
  • CI is green and the branch merges cleanly.

What's missing is the run in the game, the same thing Joey-Magnus asked for. Every Broadcast caller (the movie framework's queues, NVIDIA's OS layer) used to sleep until some unrelated Signal and now wakes on time. That is the fix, but it changes timing on paths nothing here has run. Before this goes in, please:

  1. Boot to the title and play at least one scene with a movie (the opening, or one where a hang was reported). Say whether it plays through on Windows and, if you can, on Linux.
  2. Show that the new HLEs are actually reached in that run, e.g. with a temporary count of Broadcast and TimedAcquireSemaphore calls, or HOS_CHAIN notes on a movie condition variable. If a hang reproduces on main, show it gone with the change.

A question and two small nits:

  • The plain TryAcquireSemaphore (not the light one) isn't HLE'd either. Is it absent from the image's imports, or was it left out on purpose? If it is imported, it has the same gap as the light try.
  • nit: TimedWaitConditionVariable, event_wait and the queue's timed paths still compute hos_now_ns() + timeout without saturating, so a huge TimeSpan wraps negative and times out at once. A small shared deadline_after(ns) used by all of them would fix that. It's older than this PR, so a follow-up is fine.
  • nit: the fixture stubs wait_until, so the new clamp isn't covered. A line check on the 0x7FFFFFFF cap would pin it.

Overlap: #2708 touches the code right next to this (the Signal early-out when nothing waits, and an atomic cv_waiting) but doesn't conflict textually. Either order works. Once both are in, Broadcast could take the same early-out for consistency. #2709 (spin) and #2487 (POSIX futex waiter count) don't touch this code, and #2487's wake skip is safe for the wake-all here.

> [@hckrman101](https://github.com/hckrman101) reviewed on GitHub · 2026-11-12 16:33 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2765#pullrequestreview-5878211323) Thanks, this is a real gap and the code holds up. I went through it for lost and spurious wake-ups and the timeout arithmetic, and agree with Joey-Magnus's read: - **Broadcast:** `gen` moves under `cv->lk` and the wake-all comes after, so a waiter that snapshotted the old generation can't miss it. The reservations balance: one per waiter counted at the broadcast, capped at 8, and every one of those waiters leaves under the same lock (a timed waiter that is timing out included), so nothing stays owed to nobody. Mixed mutexes and `HOS_NO_HANDOFF=1` fall back the same way Signal does. - **`sema_acquire_for`:** it checks the count under the lock, loops on spurious and vcore wake-ups, takes a release that races the deadline, and a timeout of 0 or less tries once. The saturated deadline can't wrap. - **`wait_until`:** clamping to 0x7FFFFFFF ms fixes both platforms. Windows treats 0xFFFFFFFF as `INFINITE`, and so does the POSIX shim (`ms == INFINITE`). All four callers loop back to their deadline. - CI is green and the branch merges cleanly. What's missing is the run in the game, the same thing Joey-Magnus asked for. Every Broadcast caller (the movie framework's queues, NVIDIA's OS layer) used to sleep until some unrelated Signal and now wakes on time. That is the fix, but it changes timing on paths nothing here has run. Before this goes in, please: 1. Boot to the title and play at least one scene with a movie (the opening, or one where a hang was reported). Say whether it plays through on Windows and, if you can, on Linux. 2. Show that the new HLEs are actually reached in that run, e.g. with a temporary count of Broadcast and TimedAcquireSemaphore calls, or `HOS_CHAIN` notes on a movie condition variable. If a hang reproduces on main, show it gone with the change. A question and two small nits: - The plain `TryAcquireSemaphore` (not the light one) isn't HLE'd either. Is it absent from the image's imports, or was it left out on purpose? If it is imported, it has the same gap as the light try. - nit: `TimedWaitConditionVariable`, `event_wait` and the queue's timed paths still compute `hos_now_ns() + timeout` without saturating, so a huge TimeSpan wraps negative and times out at once. A small shared `deadline_after(ns)` used by all of them would fix that. It's older than this PR, so a follow-up is fine. - nit: the fixture stubs `wait_until`, so the new clamp isn't covered. A line check on the `0x7FFFFFFF` cap would pin it. Overlap: #2708 touches the code right next to this (the Signal early-out when nothing waits, and an atomic `cv_waiting`) but doesn't conflict textually. Either order works. Once both are in, Broadcast could take the same early-out for consistency. #2709 (spin) and #2487 (POSIX futex waiter count) don't touch this code, and #2487's wake skip is safe for the wake-all here. <!-- github-sync:r:5878211323 -->
The fixture stubs wait_until, so a line check now pins its 0x7FFFFFFF ms cap (hckrman101's review). The comments in
host/nn_os_sync.c and the test's docstring name the Broadcast and timed-acquire callers by role (the movie framework's
reply and buffer queues, NVIDIA's OS layer, the C library) rather than by the image's symbols, as the description does.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
github-sync commented 2026-11-12 17:59:05 +00:00
Author
Owner

@klimovich008 commented on GitHub · 2026-11-12 17:32 UTC · original

Thanks both. The in-game run is now in the description (In game). On Windows (RX 9070 XT, base main 46780503) the build boots to the title and plays the prologue movie from a new game:

  • Without the change, the movie played to its end (154.2 s).
  • With it, the movie was at 137.8 s and on time (0.02 s ahead of the wall clock) when the 190 s stop came. That run's load was longer, so the stop came before the movie's end.
  • No FATAL, device lost or VUID in either run. That's n = 2: the PC's memory guard refused the remaining runs.

hckrman101's points:

  1. Platforms: Windows only; Linux was not run.
  2. Reach: a local log of every Broadcast and timed/try semaphore call in the changed build printed none in that run, so the prologue reaches neither. The run shows the change is harmless on the boot, title and prologue path, but not the movie-hang link. I haven't found a scene that reaches Broadcast, or a hang that reproduces on main.
  • TryAcquireSemaphore: the non-light one is already HLE'd on main (host/nn_os_sync.c:1325); only the light try was left to the SDK.
  • Deadlines (nit): agreed. TimedWaitConditionVariable (:1293) and the event and queue timed paths (:696, :1012, :1201) still add the timeout without saturating. They predate this PR; I'll leave them for a follow-up with a shared deadline helper, as you suggest.
  • Clamp coverage (nit): 973970a7 adds a line check on wait_until's 0x7FFFFFFF cap. It fails with the cap removed. The same commit names the callers in comments by role rather than by symbol, as the description does.
  • Overlap with #2708: agreed. The same no-waiter early return for Broadcast is a follow-up once both are in.

🤖 Generated with Claude Code

> [@klimovich008](https://github.com/klimovich008) commented on GitHub · 2026-11-12 17:32 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2765#issuecomment-6831042327) Thanks both. The in-game run is now in the description (**In game**). On Windows (RX 9070 XT, base main 46780503) the build boots to the title and plays the prologue movie from a new game: - Without the change, the movie played to its end (154.2 s). - With it, the movie was at 137.8 s and on time (0.02 s ahead of the wall clock) when the 190 s stop came. That run's load was longer, so the stop came before the movie's end. - No FATAL, device lost or VUID in either run. That's n = 2: the PC's memory guard refused the remaining runs. hckrman101's points: 1. **Platforms:** Windows only; Linux was not run. 2. **Reach:** a local log of every Broadcast and timed/try semaphore call in the changed build printed none in that run, so the prologue reaches neither. The run shows the change is harmless on the boot, title and prologue path, but not the movie-hang link. I haven't found a scene that reaches Broadcast, or a hang that reproduces on main. - **`TryAcquireSemaphore`:** the non-light one is already HLE'd on main (`host/nn_os_sync.c:1325`); only the light try was left to the SDK. - **Deadlines (nit):** agreed. `TimedWaitConditionVariable` (`:1293`) and the event and queue timed paths (`:696`, `:1012`, `:1201`) still add the timeout without saturating. They predate this PR; I'll leave them for a follow-up with a shared deadline helper, as you suggest. - **Clamp coverage (nit):** 973970a7 adds a line check on `wait_until`'s `0x7FFFFFFF` cap. It fails with the cap removed. The same commit names the callers in comments by role rather than by symbol, as the description does. - **Overlap with #2708:** agreed. The same no-waiter early return for Broadcast is a follow-up once both are in. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- github-sync:c:6831042327 -->
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
Labels
Clear labels   
accessibility
Barrier affecting people with disabilities

  
autorelease: pending

   
autorelease: tagged

   
bug
Something isn't working

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
testing wanted

   
wontfix
This will not be worked on

No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
MagicBeanDev/Hookshot!2765
No description provided.