On Sun Feb 16 17:41:01 2025 +0000, Brendan Shanks wrote:
> Another possibility is to use `libbsdxml`, a renamed expat that FreeBSD
> includes (although the man page says that “The libbsdxml library is
> intended to be used within the FreeBSD base system only. Use of the
> libbsdxml library for other purposes is not supported and discouraged.”
> But expat’s API is quite stable, and avahi also has the option to use bsdxml.)
> Otherwise, maybe just depend on a system-installed expat or libxml2 on FreeBSD?
Assuming this is desirable to read this file to begin with (e.g. if it's going to stay available in the future, if structure is stable, etc), we could potentially convert to WCHARs, and then reused much of existing code from manifest parsing. External library for something like seems heavy.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7339#note_94835
On Sun Feb 16 16:38:40 2025 +0000, eric pouech wrote:
> this likely could be folded in code line 510, by inserting between 510 & 511:
> if (return_code != NO_ERROR) dirStack = NULL;
Thank you. I knew this was non-optimal.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94834
On Sun Feb 16 16:37:29 2025 +0000, eric pouech wrote:
> actually, we could (later on) have other values as return code...
> so IMO this should rather be written as:
> `if (return_code != NO_ERROR || (num_empty && !num_with_data))`
> ` errorlevel = ERROR_INVALID_FUNCTION;`
> `else`
> ` errorlevel = NO_ERROR;`
Thank you. I was hoping for clarification on this one.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94833
On Sun Feb 16 16:38:49 2025 +0000, eric pouech wrote:
> nitpick: IMO it's more readable to return a RETURN_CODE, and pass a
> DIRECTORY_STACK\*\* to update the pointer (you likely did it this way to
> minimize change, but let's take the opportunity of a change to sanitize
> a bit the current spaghetti code)
It is *always* my philosophy to minimize changes to existing, working code. Some projects emphasize this. I also come from a working assumption, perhaps erroneously, that existing code is functional and already meets project standards. No one likes to come into a project to add contributions and then have to get out the mop bucket to clean up legacy technical debt. I'm speaking generally here and not specifically about this particular part of code.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94832
On Sun Feb 16 17:01:28 2025 +0000, eric pouech wrote:
> globally it looks better
> a couple of stylistic remarks, but you need to go further for not using
> errorlevel inside the function and its helper
> perhaps, it's worthwhile to split the patch in two:
> * first one to no longer use error level internally, adapt
> list_directory prototype... (in the details, it's fine to use return
> errorlevel = ERROR\_\* while parsing the arguments in case of error, but
> when this is done all the rest of the code should use only the
> return_code (and set errorlevel when exiting WCMD_dir))
> * the second one to plug in the ctrl-c handling and breaking from the
> various loops
I only ever intended to do the second part. I've been doing the rest per your suggestions out of goodwill.
I'll continue to fix up the first part per your suggestions but I don't have the energy to split this into separate patches. I'll address the remaining issues that you have raised today but that's going to be it from me I'm afraid on this.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94831
This introduces a faster implementation of signal and wait operations on NT
events, semaphores, and mutexes, which improves performance to native levels for
a wide variety of games and other applications.
The goal here is similar to the long-standing out-of-tree "esync" and "fsync"
patch sets, but without the flaws that make those patch sets not upstreamable.
The Linux "ntsync" driver is not currently released. It has been accepted into
the trunk Linux tree for 6.14, so barring any extraordinary circumstances, the
API is frozen and it will be released in its current form in about 2 months.
Since it has passed all relevant reviewers on the kernel side, and the API is
all but released, it seems there is no reason any more not to submit the Wine
side to match.
Some important notes:
* This patch set does *not* include any way to disable ntsync support, since
that kind of configuration often seems to be dispreferred where not necessary.
In essence, ntsync should just work everywhere.
Probably the easiest way to effectively disable ntsync, for the purposes of
testing, is to chmod the /dev/ntsync device to prevent its being opened.
Regardless, a Wine switch to disable ntsync can be added simply enough. Note
that it should probably not take the form of a registry key, however, since it
needs to be easily accessible from the server itself.
* It is, generally speaking, not possible for only some objects, or some
processes, to have backing ntsync objects, while others use the old server
path. The esync/fsync patch sets explicitly protected against this by making
sure every process had a consistent view of whether esync was enabled. This is
not provided here, since no switch is provided to toggle ntsync, and it should
not be possible to get into such an inconsistent state without gross
misconfiguration.
* Similarly, no diagnostic messages are provided to note that ntsync is in use,
or not in use. These messages are part of esync/fsync, as well as part of
ntsync "testing" trees unofficially distributed. However, if ntsync is working
correctly, no message should be necessary.
The basic structure is:
* Each type of server object which can be waited on by the client (including
events, semaphores, mutexes, but also other types such as processes, files)
must store an "inproc_sync" object.
This "inproc_sync" object is a full server object (note that this differs from
esync/fsync). A vector and server request is introduced to retrieve an NT
handle to this object from an arbitrary NT handle.
Since the actual ntsync objects are simply distinct file descriptions, we then
call get_handle_fd from the client to retrieve an fd to the object, and then
perform ioctls on it.
* Objects signaled by the server (processes, files, etc) perform ntsync ioctls
on that object. The backing object in all such cases is simply an event.
* Signal and wait operations on the client side attempt to defer to an
"inproc_\*" function, falling back to the server implementation if it returns
STATUS_NOT_IMPLEMENTED. This mirrors how in-process synchronization objects
(critical sections, SRW locks, etc) used to be implemented—attempting to use
an architecture-specific "fast_\*" function and falling back if it returned
STATUS_NOT_IMPLEMENTED.
* The inproc_sync handles, once retrieved, are cached per-process. This caching
takes a similar form to the fd cache. It does not reuse the same
infrastructure, however.
The primary reason for this is that the fd cache is designed to fit within a
64-bit value and uses 64-bit atomic operations to ensure consistency. However,
we need to store more than 64 bits of information. [We also need to modify
them after caching, in order to correctly implement handle closing—see below.]
The secondary reason is that retrieving the ntsync fd from the inproc_sync
handle itself uses the fd cache.
* In order to keep the Linux driver simple, it does not implement access flags
(EVENT_MODIFY_STATE etc.) Instead, the flags are cached locally and validated
there. This too mirrors the fd cache. Note that this means that a malicious
process can now modify objects it should not be able modify—which is less true
than it is with wineserver—but this is no different from the way other objects
(notably fds) are handled, and would require manual syscalls.
* In order to achieve correct behaviour related to closing objects while they
are used, this patch set essentially relies on refcounting. This is broadly
true of the server as well, but because we need to avoid server calls when
performing object operations, significantly more care must be taken.
In particular, because waits need to be interruptable by signals and then be
restarted, we need the backing ntsync object to remain valid until all users
are done with it. On a process level, this is achieved by letting multiple
processes own handles to the underlying inproc_sync server object.
On a thread level, multiple simultaneous calls need to refcount the process's
local handle. This refcount is stored in the sync object cache. When it
reaches zero, the cache is cleared.
Punting this behaviour to the Linux driver would have introduced a great deal
more complexity, which is best kept in userspace and out of the kernel.
* The cache is, as such, treated as a cache. The penultimate commit, which
introduces client support but does not yet cache the objects, effectively
illustrates this by never actually caching anything, and retrieving a new NT
handle and fd every time.
* Certain waits, on internal handles (such as async, startup_info, completion),
are delegated to the server even when ntsync is used. Those server objects do
not create an underlying ntsync object.
--
v3: ntdll: Cache in-process synchronization objects.
ntdll: Use server_wait_for_object() when waiting on only the queue object.
ntdll: Use in-process synchronization objects.
ntdll: Introduce a helper to wait on an internal server handle.
server: Allow creating an event object for client-side user APC signaling.
server: Introduce select_inproc_queue and unselect_inproc_queue requests.
server: Add a request to retrieve the in-process synchronization object from a handle.
server: Create in-process synchronization objects for fd-based objects.
server: Create in-process synchronization objects for timers.
server: Create in-process synchronization objects for threads.
server: Create in-process synchronization objects for message queues.
server: Create in-process synchronization objects for jobs.
server: Create in-process synchronization objects for processes.
server: Create in-process synchronization objects for keyed events.
server: Create in-process synchronization objects for device managers.
server: Create in-process synchronization objects for debug objects.
server: Create in-process synchronization objects for console servers.
server: Create in-process synchronization objects for consoles.
server: Create in-process synchronization objects for completion ports.
server: Create in-process synchronization objects for mutexes.
server: Create in-process synchronization objects for semaphores.
server: Create in-process synchronization objects for events.
server: Add an object operation to retrieve an in-process synchronization object.
ntdll: Retrieve and cache an ntsync device in wait calls.
ntdll: Add stub functions for in-process synchronization.
ntdll: Add some traces to synchronization methods.
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/7226
On Sun Feb 16 16:37:14 2025 +0000, eric pouech wrote:
> this shouldn't set errorlevel but return_code
> and likely using ERROR_FILE_NOT_FOUND to mark that listing was empty
My head is starting to spin here. That is existing code that I haven't touched. I'll fix along with the other issues though...
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94829
Normally Cyberpunk 2077 spawns #cpu_threads of dispatcher threads.
However with Freebsd, since create_logical_proc_info was a stub,
the game would instead spawn a single dispatcher thread, causing
the game to just crawl.
Working around the issue by adding a barebone implementation of
create_logical_proc_info for FreeBSD so that the game would at
least get a thread count through ntdll.
--
v2: Fix Cyberpunk 2077 spawning only one dispatcher thread in FreeBSD
https://gitlab.winehq.org/wine/wine/-/merge_requests/5213
Another possibility is to use `libbsdxml`, a renamed expat that FreeBSD includes (although the man page says that “The libbsdxml library is intended to be used within the FreeBSD base system only. Use of the libbsdxml library for other purposes is not supported and discouraged.” But expat’s API is quite stable, and avahi also has the option to use bsdxml.)
Otherwise, maybe just depend on a system-installed expat or libxml2 on FreeBSD?
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7339#note_94826
globally it looks better
a couple of stylistic remarks, but you need to go further for not using errorlevel inside the function and its helper
perhaps, it's worthwhile to split the patch in two:
* first one to no longer use error level internally, adapt list_directory prototype... (in the details, it's fine to use return errorlevel = ERROR\_\* while parsing the arguments in case of error, but when this is done all the rest of the code should use only the return_code (and set errorlevel when exiting WCMD_dir))
* the second one to plug in the ctrl-c handling and breaking from the various loops
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94825
eric pouech (@epo) commented about programs/cmd/directory.c:
> * FIXME: Assumes 24-line display for the /P qualifier.
> */
>
> -static DIRECTORY_STACK *WCMD_list_directory (DIRECTORY_STACK *inputparms, int level) {
> +static DIRECTORY_STACK *WCMD_list_directory (DIRECTORY_STACK *inputparms, int level, RETURN_CODE *return_code) {
nitpick: IMO it's more readable to return a RETURN_CODE, and pass a DIRECTORY_STACK\*\* to update the pointer (you likely did it this way to minimize change, but let's take the opportunity of a change to sanitize a bit the current spaghetti code)
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94822
eric pouech (@epo) commented about programs/cmd/directory.c:
> thisDir = tempDir;
> }
> }
> + /* Free remaining structures if we broke out of previous loop due to error. */
> + while (dirStack != NULL) {
this likely could be folded in code line 510, by inserting between 510 & 511:
if (return_code != NO_ERROR) dirStack = NULL;
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94821
eric pouech (@epo) commented about programs/cmd/directory.c:
> }
>
> /* Trailer Information */
> - if (trailerReqd) {
> + if (trailerReqd && return_code == NO_ERROR) {
> WCMD_dir_trailer(prevEntry->dirName);
> }
>
> - if (num_empty && !num_with_data)
> + if (return_code == STATUS_CONTROL_C_EXIT)
actually, we could (later on) have other values as return code...
so IMO this should rather be written as:
`if (return_code != NO_ERROR || (num_empty && !num_with_data))`
` errorlevel = ERROR_INVALID_FUNCTION;`
`else`
` errorlevel = NO_ERROR;`
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94820
eric pouech (@epo) commented about programs/cmd/directory.c:
> /* Clear any errors from previous invocations, and process it */
> errorlevel = NO_ERROR;
> prevEntry = thisEntry;
> - thisEntry = WCMD_list_directory (thisEntry, 0);
> + thisEntry = WCMD_list_directory (thisEntry, 0, &return_code);
> if (errorlevel)
this should use return_code not error level
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94819
eric pouech (@epo) commented about programs/cmd/directory.c:
> if ((file_total + dir_total == 0) && (level == 0)) {
> SetLastError (ERROR_FILE_NOT_FOUND);
> WCMD_print_error ();
> errorlevel = ERROR_INVALID_FUNCTION;
this shouldn't set errorlevel but return_code
and likely using ERROR_FILE_NOT_FOUND to mark that listing was empty
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94818
eric pouech (@epo) commented about programs/cmd/directory.c:
> WCHAR fname[MAX_PATH];
> WCHAR ext[MAX_PATH];
> unsigned num_empty = 0, num_with_data = 0;
> + RETURN_CODE return_code = NO_ERROR;
>
> errorlevel = NO_ERROR;
this should not be necessary (in fact starting from here up the exit of the function, you should only deal with return_code... errorlevel should be set upon exiting)
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94817
On Sun Feb 16 07:21:41 2025 +0000, Brian Wright wrote:
> Bah. There were too many changes to X11DRV_ConfigureNotify for printf
> debugging, and I wouldn't know what to change that also keeps other
> programs from breaking.
> I can provide detailed logs for 9.21 (or any interim versions), but
> that's about all I can do
For the record, all of these `ConfigureNotify` events were a hack to let Wine know where on screen the window is, which happened to work way better than it should and it resulted in more more reliable behavior than Wine's built in XEmbed support at the time. Ideally, yabridge would just use XEmbed instead and Wine would keep track of the screen coordinates.
If you enable yabridge's XEmbed support then it stops sending these `ConfigureNotify` messages and it will use XEmbed instead. But I also have not yet been able to get that to work as expected with the latest Wine version. I spent a couple hours debugging Wine's X11 driver last Friday but I haven't figured out what goes wrong or what's missing yet.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6569#note_94810
Make both method calls;
1. Consistently written with parameter names as per spec.
2. Fix parameter validation to be consistent with spec.
3. Fix szNameBuf parameter semantics as per spec.
4. Fix szNameBuf casing str search as per spec.
5. Factor out common code into TLB_ helpers vastly improves readability.
--
v25: dlls/oleaut32: Replace infinite loops with for loops
dlls/oleaut32: Use consistent SegDir field names
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/7286
On Sun Feb 16 08:08:14 2025 +0000, Joe Souza wrote:
> Looking through the existing code, especially WCMD_directory, there
> seems to be no distinction between errorlevel and the RETURN_CODE that
> the function returns. In most cases I see the code set errorlevel to
> some value and then the function returns errorlevel.
OK, I *think* the latest changes are pretty close to what you asked for.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94807
Allow the user to press Ctrl-C to abort lengthy DIR (or DIR /p, etc.) operations.
--
v9: programs/cmd: Implement ability to abort lengthy directory operations via Ctrl-C.
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322
On Sun Feb 16 01:21:13 2025 +0000, Brian Wright wrote:
> I updated the bug tracker again.
> In REAPER, the window tree looks a bit like this:
> ```text
> host_window_reparent parent 402, window 200192 (604,402)-(1516,1110)
> host_window_reparent parent 200192, window 200193 (0,0)-(912,708)
> root_coords 1
> host_window_reparent parent 200193, window 1600130 (0,0)-(912,708)
> root_coords 1
> host_window_reparent parent 1600130, window 16012db (0,0)-(644,659)
> root_coords 1
> host_window_reparent parent 16012db, window 2200000 (0,0)-(805,460)
> root_coords 1
> host_window_reparent parent 2200000, window 2000003 (0,0)-(1920,1200)
> root_coords 0
> ```
> I think 402, 200192, and 200193 are from REAPER and the rest from yabridge.
> One detail I noticed is that window 2000003 gets the initial click event
> from yabridge and sends the correct value to
> **cursor:map_event_coords**. I don't know the structure of
> event->window, but there is something in
> [**host_window_configure_child**](https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/winex11.drv/window.c#L229)
> that messes with coordinate offsets.
> Is is possible that **map_event_coords** should check the value of
> `root_coords` (aka `event->send_event`) and provide `(pt->x, pt->y)`
> when this is 0?
> I'll keep picking at things and see if there's more to find.
> -----
> Alternatively, a flag/argument solution would get DAW users back in action.
Bah. There were too many changes to X11DRV_ConfigureNotify for printf debugging, and I wouldn't know what to change that also keeps other programs from breaking.
I can provide detailed logs for 9.21 (or any interim versions), but that's about all I can do
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6569#note_94802
This MR tries to implement the SetIMECompositionRect user driver function and partially the WINE_IME_POST_UPDATE message call. It requires support for zwp_text_input_v3 from the compositor. It supports submitting committed text to the win32 side and notifying the compositor about the area of the text cursor rectangle.
This doesn't add support for embedded pre-edit because I couldn't immediately get it to appear. Either I did something incorrect, or it has something to do with Wayland. On X11, the pre-edit appears completely separated to the application window and doesn't move with it, which doesn't seem to be compatible with Wayland.
If Windows has the surrounding text feature, it does not seem possible to support it here with the provided interface.
The IME is always enabled on the focused surface, like on X11. If Windows has a facility that applications can use to say that it does or does not accept text entry at certain parts of the UI, it does not seem possible to support it here with the provided interface.
--
v10: winewayland: Post IME update for preedit text.
https://gitlab.winehq.org/wine/wine/-/merge_requests/7241
I think it would be very nice to have something like that to reduce the burden of implementing COM interfaces. This shows for instance on the windows.gaming.input module a 30% LoC reduction (from ~6800 to ~4700), even though the module already had a boilerplate helper macros already.
The MR introduces macros to automatically implement each IUnknown method, as well as all of them at once. It also includes a helper to implement IInspectable methods at once, as well as macros to forward both interface methods to a base interface or an outer object. Last, it provides higher-level macros to implement a main interface and any number of sub interfaces and generate IUnknown, forwarding and vtables for all of them at once, with IInspectable support when needed.
It uses widl to generate additional per-interface macros, for things like inheritance and vtable generation. The rest of the macros are otherwise shared in a Wine-specific header.
The implementation is split to show individual macro being used, although they are later replaced by higher-level macros. The individual helpers are still useful in some corner cases where specific behavior needs to be implemented, or for aggregating classes.
--
v5: windows.gaming.input: Use the new COM macros everywhere possible.
windows.gaming.input: Use the new macros for class implementation.
widl: Generate new macros for class implementation.
windows.gaming.input: Use the new macros for IInspectable implementation.
widl: Generate new macros for IInspectable implementation.
windows.gaming.input: Use the new macros for IUnknown implementation.
widl: Generate new macros for IUnknown implementation.
windows.gaming.input: Use the WIDL_impl_IUnknown_QueryInterface boilerplate macros.
widl: Generate some WIDL_impl_IUnknown_QueryInterface boilerplate macros.
windows.gaming.input: Use the WIDL_impl_QueryInterface boilerplate macros.
widl: Generate some WIDL_impl_QueryInterface boilerplate macros.
windows.gaming.input: Use the WIDL_impl_IUnknown_Release boilerplate macros.
widl: Generate some WIDL_impl_IUnknown_Release boilerplate macros.
windows.gaming.input: Use the WIDL_impl_IUnknown_AddRef macros.
widl: Generate some WIDL_impl_IUnknown_AddRef boilerplate macros.
windows.gaming.input: Use the WIDL_impl_IInspectable_forwards boilerplate macros.
widl: Generate some WIDL_impl_IInspectable_forwards boilerplate macros.
windows.gaming.input: Use the WIDL_impl_IUnknown_forwards boilerplate macros.
widl: Generate some WIDL_impl_IUnknown_forwards boilerplate macros.
windows.gaming.input: Use the new WIDL_impl_from_* boilerplate macros.
widl: Generate some WIDL_impl_from_* boilerplate macros.
windows.gaming.input: Use the new WIDL_impl_*Vtbl macros.
widl: Generate WIDL_impl_*Vtbl boilerplate macros.
windows.gaming.input: Use type-prefixed impl_from helpers.
windows.gaming.input: Move struct qualifier inside the macro.
windows.gaming.input: Use a separate interface for IAgileObject.
makedep: Generate some new impl.h headers from the IDLs.
widl: Generate some new impl.h headers from the IDLs.
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/6207