[PATCH v5 0/29] MR8955: ntdll: NtSetEventBoostPriority stub regression fixes for bug #58688
NtSetEventBoostPriority stub regression fixes for: https://bugs.winehq.org/show_bug.cgi?id=58688
Regressive commit: ed9f31120b68e7d684c1544c05d94c38b25cb759
Other stubs were also changed by regress commit and may still be broken.
I'll test the other stubs and fix if necessary.
In the meantime I'm submitting this merge for review in case I missed anything that may be needed for the other stub fixes.
-- v5: Call NtSetEvent from NtSetEventBoostPriority unconditionally
This merge request has too many patches to be relayed via email. Please visit the URL below to see the contents of the merge request. https://gitlab.winehq.org/wine/wine/-/merge_requests/8955
On Tue Sep 16 18:52:19 2025 +0000, Elizabeth Figura wrote:
I have another pending patch that calls NetSetEvent from
NtSetEventBoostPriority but the condition under which NetSetEvent should be called is still unclear to me. Unconditionally, by my understanding.
Looking deeper at Xenia Canary SetBoostPriority() calls:
https://github.com/xenia-canary/xenia-canary/blob/canary_experimental/src/xe...:
`// SetEvent, but if there is a waiter we immediately transfer
execution to it`
This comment seems inline with @mzent assumptions that threads
awaiting the event are made immediately runnable. From a user space perspective, that's how NtSetEvent behaves too. If there's a difference it's in some slight nuances of how threads are prioritized by the scheduler, and that behaviour doesn't necessarily match between Windows and the host system, nor is it probably configurable.
I realize my commits should have been to a dedicated branch instead of master.
There's nothing wrong with using master on your local fork. It's not special.
Worse, I pulled commits by others to resolve Gitlab pipeline errors.
In hindsight that seems unnecessary and not advised?
It's not necessary in general.
Should I start over with a cleaner merge request once a solution ready?
No need for that; in fact it's better to avoid it, and keep this merge request in one place.
Latest pushed commit unconditionally calls NtSetEvent from NtSetEventBoostPriority.
It seems to me we should just add a quick test that it behaves like NtSetEvent(), and call NtSetEvent() from it if so.
This is the part that was unclear. By "quick test" I thought perhaps the suggestion was to query thread state info to somehow determine if behavior matched NtSetEvent.
Should a NtSetEventBoostPriority test be added to ./dlls/ntdll/tests/sync.c?
Xenia Canary still seems happy enough with this slightly more fleshed out implementation to the extent that the crash reported by the bug is still resolved. The game .iso still plays audio but with black screen instead of video which still seems to be a separate bug.
On Wed Sep 17 20:03:44 2025 +0000, Stian Low wrote:
Latest pushed commit unconditionally calls NtSetEvent from NtSetEventBoostPriority.
It seems to me we should just add a quick test that it behaves like
NtSetEvent(), and call NtSetEvent() from it if so. This is the part that was unclear. By "quick test" I thought perhaps the suggestion was to query thread state info to somehow determine if behavior matched NtSetEvent. Should a NtSetEventBoostPriority test be added to ./dlls/ntdll/tests/sync.c? Xenia Canary still seems happy enough with this slightly more fleshed out implementation to the extent that the crash reported by the bug is still resolved. The game .iso still plays audio but with black screen instead of video which still seems to be a separate bug.
Sorry for delays with this merge request. Gitlab has started blocking my account very often again and I have to wait before I'm able to log back in. My alt account is able to login fine so its definitely account specific. My alt account also eventually gets blocked so maybe it's ExpressVPN related. Previously changing ExpressVPN locations seemed to help but none have been effective yet. Please let me know if there's anything that may help resolve this account blocking problem. It happens as simply as visiting my merge request link posted for the bug report.
On Wed Sep 17 20:14:11 2025 +0000, Stian Low wrote:
Sorry for delays with this merge request. Gitlab has started blocking my account very often again and I have to wait before I'm able to log back in. My alt account is able to login fine so its definitely account specific. My alt account also eventually gets blocked so maybe it's ExpressVPN related. Previously changing ExpressVPN locations seemed to help but none have been effective yet. Please let me know if there's anything that may help resolve this account blocking problem. It happens as simply as visiting my merge request link posted for the bug report.
Yes it should be tested similarly to how `NtSetEvent` is.
Also feel free to force push your branch once you have rebased your changes back on top of master to clean this up a bit.
You could do the entire implementation in a commit named something like
ntdll: Implement NtSetEventBoostPriority().
and then for the tests
ntdll/tests: Add tests for NtSetEventBoostPriority().
or something like that, then this MR becomes a bit more reviewable.
I am not sure about the black video screen playback in xenia canary, but that might very well be another bug independent of this one.
On Wed Sep 17 20:21:22 2025 +0000, Marc-Aurel Zent wrote:
Yes it should be tested similarly to how `NtSetEvent` is. Also feel free to force push your branch once you have rebased your changes back on top of master to clean this up a bit. You could do the entire implementation in a commit named something like
ntdll: Implement NtSetEventBoostPriority().
and then for the tests
ntdll/tests: Add tests for NtSetEventBoostPriority().
or something like that, then this MR becomes a bit more reviewable. I am not sure about the black video screen playback in xenia canary, but that might very well be another bug independent of this one.
Right, it could be as simple as creating an unsignaled event and then checking that it changes the event to signaled state.
On Wed Sep 17 20:28:32 2025 +0000, Elizabeth Figura wrote:
Right, it could be as simple as creating an unsignaled event and then checking that it changes the event to signaled state.
Simple test cases have been added but `./dlls/ntdll/tests` fails when running `make test` with these logs:
``` 016c:err:seh:call_seh_handlers invalid frame 000000000022FD88 (00007FFFFE102000-00007FFFFE1FFD20) 016c:err:seh:NtRaiseException Exception frame is not in stack limits => unable to dispatch exception. make[1]: *** [Makefile:235094: dlls/ntdll/tests/i386-windows/exception.ok] Error 5 make[1]: Leaving directory '/home/any/tmp/wine_stianlow_new_wow64_build_000' make: *** [Makefile:36: test] Error 2 ```
It fails even without any of my changes so there may be existing bugs for ntdll tests.
I'm running the tests using new wow64 and have not tested shared wow64.
Other tests like ntdsapi succeed without issues.
ntdll has a mix of i386 and x86_64 functions.
i386 test_prot_fault() and x86_64 test_debugger() seem to have issues. When I comment these out the tests succeed.
If I might be doing something wrong please let me know. Otherwise I'm working to fix these issues to include with this merge request unless they should be handled separately.
On Thu Sep 18 13:32:42 2025 +0000, Stian Low wrote:
Simple test cases have been added but `./dlls/ntdll/tests` fails when running `make test` with these logs:
016c:err:seh:call_seh_handlers invalid frame 000000000022FD88 (00007FFFFE102000-00007FFFFE1FFD20) 016c:err:seh:NtRaiseException Exception frame is not in stack limits => unable to dispatch exception. make[1]: *** [Makefile:235094: dlls/ntdll/tests/i386-windows/exception.ok] Error 5 make[1]: Leaving directory '/home/any/tmp/wine_stianlow_new_wow64_build_000' make: *** [Makefile:36: test] Error 2It fails even without any of my changes so there may be existing bugs for ntdll tests. I'm running the tests using new wow64 and have not tested shared wow64. Other tests like ntdsapi succeed without issues. ntdll has a mix of i386 and x86_64 functions. i386 test_prot_fault() and x86_64 test_debugger() seem to have issues. When I comment these out the tests succeed. If I might be doing something wrong please let me know. Otherwise I'm working to fix these issues to include with this merge request unless they should be handled separately.
Commenting this line allow i386 test_prot_fault() to succeed: ``` modified dlls/ntdll/tests/exception.c @@ -550,9 +550,9 @@ static const struct exception { { 0x06, 0x31, 0xc0, 0x8e, 0xc0, 0x26, 0xa1, 0, 0, 0, 0, 0x07, 0xc3 }, /* push %es; xor %eax,%eax; mov %ax,%es; mov %es:(0),%ax; pop %es; ret */ 5, 6, TRUE, STATUS_ACCESS_VIOLATION, 2, { 0, 0xffffffff } }, - { { 0x0f, 0xa8, 0x31, 0xc0, 0x8e, 0xe8, 0x65, 0xa1, 0, 0, 0, 0, 0x0f, 0xa9, 0xc3 }, - /* push %gs; xor %eax,%eax; mov %ax,%gs; mov %gs:(0),%ax; pop %gs; ret */ - 6, 6, FALSE, STATUS_ACCESS_VIOLATION, 2, { 0, 0xffffffff } }, + /* { { 0x0f, 0xa8, 0x31, 0xc0, 0x8e, 0xe8, 0x65, 0xa1, 0, 0, 0, 0, 0x0f, 0xa9, 0xc3 }, */ + /* /* push %gs; xor %eax,%eax; mov %ax,%gs; mov %gs:(0),%ax; pop %gs; ret */ */ + /* 6, 6, FALSE, STATUS_ACCESS_VIOLATION, 2, { 0, 0xffffffff } }, */
```
Switching param 4 wow64_broken from FALSE to TRUE does not fix so its something else.
On Thu Sep 18 14:19:47 2025 +0000, Stian Low wrote:
Commenting these exceptions from the list allows i386 test_prot_fault() to succeed:
modified dlls/ntdll/tests/exception.c @@ -550,9 +550,9 @@ static const struct exception { { 0x06, 0x31, 0xc0, 0x8e, 0xc0, 0x26, 0xa1, 0, 0, 0, 0, 0x07, 0xc3 }, /* push %es; xor %eax,%eax; mov %ax,%es; mov %es:(0),%ax; pop %es; ret */ 5, 6, TRUE, STATUS_ACCESS_VIOLATION, 2, { 0, 0xffffffff } }, - { { 0x0f, 0xa8, 0x31, 0xc0, 0x8e, 0xe8, 0x65, 0xa1, 0, 0, 0, 0, 0x0f, 0xa9, 0xc3 }, - /* push %gs; xor %eax,%eax; mov %ax,%gs; mov %gs:(0),%ax; pop %gs; ret */ - 6, 6, FALSE, STATUS_ACCESS_VIOLATION, 2, { 0, 0xffffffff } }, + /* { { 0x0f, 0xa8, 0x31, 0xc0, 0x8e, 0xe8, 0x65, 0xa1, 0, 0, 0, 0, 0x0f, 0xa9, 0xc3 }, */ + /* /\* push %gs; xor %eax,%eax; mov %ax,%gs; mov %gs:(0),%ax; pop %gs; ret *\/ */ + /* 6, 6, FALSE, STATUS_ACCESS_VIOLATION, 2, { 0, 0xffffffff } }, */Switching param 4 wow64_broken from FALSE to TRUE does not fix so its something else I'm still looking into.
You don't really need exception tests, you can run just sync ones, with "make x86_64-windows/sync.ok".
On Thu Sep 18 14:24:06 2025 +0000, Nikolay Sivov wrote:
You don't really need exception tests, you can run just sync ones, with "make x86_64-windows/sync.ok".
Thanks @nsivov
"make x86_64-windows/sync.ok" works so I'll push and clean up this merge.
Should a bug be filed for exception tests?
-
Elizabeth Figura (＠zfigura) -
Marc-Aurel Zent (＠mzent)
-
Nikolay Sivov (＠nsivov)
-
Stian Low (＠stianlow)