On Tue Oct 18 17:24:25 2022 +0000, Alexandre Julliard wrote:
> I have a feeling that the recent stream of ddraw and d3drm test failures
> are also related to this MR. Any ideas?
Do you mean the `d3drm.c:4776: Test failed: Cannot create IDirect3DRMDevice2 interface, hr 0x8007000e.` and the `ddraw7.c:15663: Test failed: Expected unsynchronised map for flags 0x1000.` kind of errors?
The first one seems to be caused by an OOM error. I've seen similar issues in the past when running the tests under llvmpipe, and when multiple D3D devices are initialized. The llvmpipe GL context is memory hungry and it quickly fills the 32-bit address space.
This doesn't disculp this changes in any way and it could be very well related, if for some reason the messages here are causing GL contexts to be re-created more often or at different times.
For the ddraw issue, I can't guess where it comes from, I believe I've seen the errors before but I'll have to investigate. I'll probably need to re-create the test environment to be sure, these tests tend to be annoying to run as they changes the display modes.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/944#note_11182
This series improves support for blocks which spread across several
non contiguous memory chunks.
Mingw/Gcc tends to emit quite a few of these, especially in 32bit mode.
Now the gaps (between two valid ranges) will no longer be understood
as being part of the block, hence potentially hiding valid information
lying in other blocks.
Note that native doesn't support blocks with multiple ranges of addresses
(MSVC stores the information quite differently), so these ranges will
be used internally but not exposed to DbgHelp's user.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/1091
--
v2: comctl32: Add helper for getting icon from HPROPSHEETPAGE.
comctl32: Add helper for getting title from HPROPSHEETPAGE.
comctl32: Add helper for loading dialog template from HPROPSHEETPAGE.
https://gitlab.winehq.org/wine/wine/-/merge_requests/1072
_mbctolower_l - converts multibyte uppercase character to lowercase character, by using specified locale that's passed in.
More information about these methods are available at:
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/mbctolowe…
More information about application crash:
https://bugs.winehq.org/show_bug.cgi?id=45273
--
v2: msvcrt: implement missing _mbsupr_s_l CRT function to fix ChessBase 14 crash
msvcrt: implement missing _mbslwr_s_l CRT function to fix ChessBase 14 crash
msvcrt: implement missing _mbctoupper_l CRT function to fix ChessBase 14 crash
msvcrt: implement missing _mbctolower_l CRT function to fix ChessBase 14 crash
https://gitlab.winehq.org/wine/wine/-/merge_requests/1090
On Tue Oct 18 12:28:34 2022 +0000, Rémi Bernon wrote:
> Can `current_modref` really still be NULL now if
> `LdrGetProcedureAddress` also sets it?
Seems like it, although I haven't verified it rigorously.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_11165
On Tue Oct 18 12:28:32 2022 +0000, Rémi Bernon wrote:
> I think you can simplify this a bit, by doing `if ((current_modref =
> get_modref(...)))`, after saving its previous value, and restoring it
> before leaving the CS.
> Then I'm not sure what side effects this could have, and maybe it'd be
> better to have this specific change separate from the rest?
How about
```c
RtlEnterCriticalSection( &loader_section );
prev = current_modref;
wm = get_modref( module );
if (wm) current_modref = wm;
if (!wm) ret = STATUS_DLL_NOT_FOUND;
else
{ ... }
current_modref = prev;
RtlLeaveCriticalSection( &loader_section );
```
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_11162
On Tue Oct 18 15:32:53 2022 +0000, Jinoh Kang wrote:
> Also note that the forward[1-3] DLLs are exactly designed to test
> transitively forwarded exports.
Correction:
The importer DLL references not only the final DLL but also every intermediate DLL in the forward chain. Intermediate DLLs do not gain any new dependencies, even if they are used in the resolution process of forwarded exports. Only the earliest import DLLs gain dependencies.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_11161
On Tue Oct 18 15:31:21 2022 +0000, Jinoh Kang wrote:
> > Then what happens here if `find_ordinal_export` finds another
> transitive forwarded export? Should we still keep the same `current_modref`?
> Yes, we should. The importer DLL references the eventual DLL the target
> object resides in, not any of the intermediate DLL in the forward chain.
> > Should we add the second forwarded module to the first one
> dependencies instead?
> No, we should only add the last non-forwarding module to the importer
> DLL's dependencies.
Also note that the forward[1-3] DLLs are exactly designed to test transitively forwarded exports.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_11158
> Then what happens here if `find_ordinal_export` finds another transitive forwarded export? Should we still keep the same `current_modref`?
Yes, we should. The importer DLL references the eventual DLL the target object resides in, not any of the intermediate DLL in the forward chain.
> Should we add the second forwarded module to the first one dependencies instead?
No, we should only add the last non-forwarding module to the importer DLL's dependencies.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_11157
With this commit the following functions were implemented:
- _mbsupr_s_l - converts multibyte string to uppercase, by using specified locale that's passed in.
- _mbslwr_s_l - converts multibyte string to lowercase, by using specified locale that's passed in.
- _mbctolower_l - converts multibyte uppercase character to lowercase character, by using specified locale that's passed in.
- _mbctoupper_l - converts multibyte lowercase character to uppercase character, by using specified locale that's passed in.
More information about these methods are available at:
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strupr-s-…
After applying the patch the crash of the ChessBase application was fixed.
More information:
https://bugs.winehq.org/show_bug.cgi?id=45273
--
v7: msvcrt: implement missing CRT functions to fix ChessBase 14 crash
https://gitlab.winehq.org/wine/wine/-/merge_requests/1080
@rbernon Thanks for your review! Right now I'm tentatively putting this MR on hold until I find a solution for a memory leak: a new (redundant) dependency edge is added every time the application calls `GetProcAddress()` on a forwarded ref.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_11151
This change is adding DWARF (CFI) unwind information to the
hand-written assembly of the `__wine_syscall_dispatcher` function.
This enables unwinding through the dispatcher from the Linux stack
into (and through) the Windows stack.
The general idea is that the `syscall_frame` struct contains the
content of the callee-save registers before the function call
(in particular the stack pointer and the return address). At any
point of the execution, we have a pointer into the `syscall_frame`
in $rcx, $rbp or $rsp.
For the CFI codes the general idea is that we are defining the
computations of the callee-save registers based on the
`syscall_frame` using DWARF’s `breg` instruction, rather than
relative to CFA.
This change adds a bunch of convenience macros, to (hopefully)
improve readability of the CFI instructions.
Note: Those change was used with great success for unwinding through
the dispatcher using a modified LLDB shown in the
[“how-wine-works-101”](https://werat.dev/blog/how-wine-works-101/)
blog post as well as for in the [Orbit profiler](https://github.com/google/orbit),
that has mixed-callstack unwinding support.
Test: Inspect callstacks reported by the Orbit profiler while
running some Windows targets using the modified wine, as well as
verify debugging reports correct callstacks when stepping with our
modified LLDB through the dispatcher itself (so that we are able
to unwind through the dispatcher at any instruction).
--
v6: ntdll: Add CFI unwind info to __wine_syscall_dispatcher (x86_64)
https://gitlab.winehq.org/wine/wine/-/merge_requests/1065
Using a dedicated exit jmpbuf and removing the need for assembly
routines.
When Wine handles an exception in unix code, we return to user mode by
jumping to the last syscall frame. This can leave some pthread cancel
cleanups registered, in the pthread internal linked list, and at the
same time later overwrite the stack frame they were registered for.
In the same way, jumping to the exit frame on thread exit or abort, can
also leave some cleanup handlers registered for invalid stack frames.
Depending on the implementation, calling pthread_exit will cause all the
registered pthread cleanup handlers to be called, possibly jumping back
to now overwritten stack frames and causing segmentation faults.
Exiting a pthread normally, by returning from its procedure, or calling
exit(0) for the main thread doesn't run pthread_exit and doesn't call
cleanup handlers, avoiding that situation.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52213
### Additional note:
For robustness, we should probably try to execute these cleanup handlers
when unwinding the stack frames, as we would otherwise leave pthread
objects in a potential problematic state (like a mutex locked, etc).
It is however hard to do so when the handlers are registered from some C
code: pthread C implementation is done by calling some internal pthread
functions to register the handlers, and they aren't registered as
standard unwind handlers.
Only pthread_cancel and pthread_exit can unwind and call / unregister
the C handlers, but interrupting that procedure, for instance calling
setjmp / longjmp from withing our own handler isn't supported.
From C++ code, pthread cleanup handlers are registered through C++ class
constructors / destructors, and it would then be possible to partially
unwind and call them at the same time.
--
v2: ntdll: Remove unnecessary signal_start_thread register spilling.
ntdll: Remove unnecessary arch specific exit frame pointer.
ntdll: Avoid calling pthread_exit on thread exit.
loader: Allow __wine_main return without emitting a warning.
https://gitlab.winehq.org/wine/wine/-/merge_requests/1088
This change is adding DWARF (CFI) unwind information to the
hand-written assembly of the `__wine_syscall_dispatcher` function.
This enables unwinding through the dispatcher from the Linux stack
into (and through) the Windows stack.
The general idea is that the `syscall_frame` struct contains the
content of the callee-save registers before the function call
(in particular the stack pointer and the return address). At any
point of the execution, we have a pointer into the `syscall_frame`
in $rcx, $rbp or $rsp.
For the CFI codes the general idea is that we are defining the
computations of the callee-save registers based on the
`syscall_frame` using DWARF’s `breg` instruction, rather than
relative to CFA.
This change adds a bunch of convenience macros, to (hopefully)
improve readability of the CFI instructions.
Note: Those change was used with great success for unwinding through
the dispatcher using a modified LLDB shown in the
[“how-wine-works-101”](https://werat.dev/blog/how-wine-works-101/)
blog post as well as for in the [Orbit profiler](https://github.com/google/orbit),
that has mixed-callstack unwinding support.
Test: Inspect callstacks reported by the Orbit profiler while
running some Windows targets using the modified wine, as well as
verify debugging reports correct callstacks when stepping with our
modified LLDB through the dispatcher itself (so that we are able
to unwind through the dispatcher at any instruction).
--
v5: ntdll: Add CFI unwind info to __wine_syscall_dispatcher (x86_64)
https://gitlab.winehq.org/wine/wine/-/merge_requests/1065
Rémi Bernon (@rbernon) commented about dlls/ntdll/loader.c:
> proc = find_ordinal_export( wm->ldr.DllBase, exports, exp_size,
> atoi(name+1) - exports->Base, load_path );
> } else
> proc = find_named_export( wm->ldr.DllBase, exports, exp_size, name, -1, load_path );
Then what happens here if `find_ordinal_export` finds another transitive forwarded export? Should we still keep the same `current_modref`? Should we add the second forwarded module to the first one dependencies instead?
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_11123
Rémi Bernon (@rbernon) commented about dlls/ntdll/loader.c:
> " If you are using builtin %s, try using the native one instead.\n",
> forward, debugstr_w(get_modref(module)->ldr.FullDllName.Buffer),
> debugstr_w(get_modref(module)->ldr.BaseDllName.Buffer) );
> + if (wm) LdrUnloadDll( wm->ldr.DllBase );
> + }
> + else if (current_modref)
> + {
> + add_module_dependency( current_modref->ldr.DdagNode, wm->ldr.DdagNode );
Maybe we can then always add the module dependency after it's been loaded?
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_11122
Rémi Bernon (@rbernon) commented about dlls/ntdll/loader.c:
>
> RtlEnterCriticalSection( &loader_section );
>
> - /* check if the module itself is invalid to return the proper error */
> - if (!get_modref( module )) ret = STATUS_DLL_NOT_FOUND;
> - else if ((exports = RtlImageDirectoryEntryToData( module, TRUE,
> - IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size )))
> + wm = get_modref( module );
> + if (!wm) ret = STATUS_DLL_NOT_FOUND;
> + else
> {
> - void *proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1, NULL )
> - : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL );
> - if (proc)
> + prev = current_modref;
> + current_modref = wm;
I think you can simplify this a bit, by doing `if ((current_modref = get_modref(...)))`, after saving its previous value, and restoring it before leaving the CS.
Then I'm not sure what side effects this could have, and maybe it'd be better to have this specific change separate from the rest?
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_11120
Using a dedicated exit jmpbuf and removing the need for assembly
routines.
When Wine handles an exception in unix code, we return to user mode by
jumping to the last syscall frame. This can leave some pthread cancel
cleanups registered, in the pthread internal linked list, and at the
same time later overwrite the stack frame they were registered for.
In the same way, jumping to the exit frame on thread exit or abort, can
also leave some cleanup handlers registered for invalid stack frames.
Depending on the implementation, calling pthread_exit will cause all the
registered pthread cleanup handlers to be called, possibly jumping back
to now overwritten stack frames and causing segmentation faults.
Exiting a pthread normally, by returning from its procedure, or calling
exit(0) for the main thread doesn't run pthread_exit and doesn't call
cleanup handlers, avoiding that situation.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52213
### Additional note:
For robustness, we should probably try to execute these cleanup handlers
when unwinding the stack frames, as we would otherwise leave pthread
objects in a potential problematic state (like a mutex locked, etc).
It is however hard to do so when the handlers are registered from some C
code: pthread C implementation is done by calling some internal pthread
functions to register the handlers, and they aren't registered as
standard unwind handlers.
Only pthread_cancel and pthread_exit can unwind and call / unregister
the C handlers, but interrupting that procedure, for instance calling
setjmp / longjmp from withing our own handler isn't supported.
From C++ code, pthread cleanup handlers are registered through C++ class
constructors / destructors, and it would then be possible to partially
unwind and call them at the same time.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/1088