May 2023 - Wine-gitlab - list.winehq.org

Re: [PATCH v2 0/2] MR2700: msxml3: Rename msxml3 to winexml and point msxml* to it.
by Tim Clem (＠tclem) 02 May '23

02 May '23

On Tue May 2 20:07:50 2023 +0000, Alexandre Julliard wrote: > I'm not convinced that this justifies a Wine-specific module. > The usual solution for problems with native dlls is "don't do that", > i.e. fix the builtin so that native is not required. > It's hard to tell without more details, but in this case an app-specific > dll override may also be enough. That's fair. How would you feel if the guts were moved from msxml3 to msxml? Native overrides of msxml1 are presumably very uncommon (there's nothing in winetricks, e.g.), so it would basically accomplish the same independence of the msxml*.dlls. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2700#note_31794

1 0

[PATCH v8 0/10] MR159: vkd3d-shader/hlsl: Broaden resources support v2, part 2.
by Francisco Casas (＠fcasas) 02 May '23

02 May '23

Mainly comprises support for allocating arrays of resources, and loading from them, for both SM1 and SM4. -- v8: vkd3d-shader/hlsl: Support resource arrays when writting SM4. vkd3d-shader/hlsl: Write resource loads in SM1. vkd3d-shader/hlsl: Write sampler declarations in SM1. vkd3d-shader/hlsl: Track objects sampling dimension. vkd3d-shader/hlsl: Track object components usage and allocate registers accordingly. tests: Test objects as parameters. vkd3d-shader/hlsl: Skip object components when creating input/output copies. vkd3d-shader/hlsl: Add fixme for uniform copies for objects within structs. vkd3d-shader/hlsl: Support multiple-register variables in object regsets. tests: Add additional texture array register reservation tests. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/159

2 11

Re: [PATCH v3 0/1] MR499: server: Allow cancelling alerted asyncs.
by Zebediah Figura (＠zfigura) 02 May '23

02 May '23

> > > I'd propose that the constituent conditions of each flag be refactored into separate state variables first. Paradoxically, it may even help combine two or more binary flags into an enum as the semantics become more simpler. > > > > Probably, but it's not clear to me how exactly to improve them? > > I'm yet to come with a concrete proposal, but the basic idea is that they'll be decomposed until some subset of components can be condensed into a phase enum (or the like). > > We can already do this with `signaled` and `unknown_status`, for (a not really good) example. Having a phase enumeration would be great if we can make it work, but I'm not sure that we can really decompose anything that currently exists. Decomposing either of these two would require introducing new flags. Granted, those might have simpler meaning, but... > > > * `direct_result`: from Wine codebase, `a flag if we're passing result directly from request instead of APC`. > > > > The idea behind direct_result is that there are two paths for asyncs: > > > > * Requests that can be filled immediately. The idea here is then that we can do this in two server requests: one to send and/or retrieve the data, then the client can set iosb and/or output buffers, and then the second server request is done to trigger completion after the iosb and output buffers have been filled. This is the "direct_result" case. The prototypical example is a named pipe ioctl. The first request varies, but as you've observed, it's basically anything that calls create_request_async(), and then the request handler needs to not pend the async. > > Nit: "pend the async" may need some clarification. There are two possible interpretations to "pend an async": > > 1. Queueing an async for deferred asynchronous completion. > 1. Simply calling `set_async_pending(async)`. > > For case 2, `direct_result` can still be true if we terminate the async immediately afterwards. This is akin to calling `IoMarkIrpPending(Irp);` and immediately following it with a call to `IoCompleteRequest(Irp, IO_NO_INCREMENT);`. > > I'll assume that you meant case 1 here. Yes, that's what I meant. > > > * Requests that can't be filled immediately. In this case we use an APC to notify the client when the request is filled. This is the "!direct_result" case. The APC is a callback in a sense, but you can also look at it as two separate requests. As with the direct_result case, we need the server to return the iosb and output buffer (the APC arguments), then the client fills them, then it sends another request to the server (returning from the APC) that lets the server queue completion. > > > > > * The async was created via create_request_async(), and > > > > The other path is register_async, and I think that should just go away. The idea of register_async is that the client tries to do i/o, gets EWOULDBLOCK, and then sends an async to the server. It was once used for sockets, and now I believe only applies to some "special" unix file types: serial, char device, mailslot, probably also FIFO. Besides being kind of redundant, it has a fundamental problem, as we've discovered with sockets: inauspiciously timed I/O on a "ready" file can jump ahead of queued I/O. I think in general we want to replace it with something that looks like the socket path. > > I think that depends on whether the file object serializes I/O requests. If the file object can complete I/O requests in a nondeterministic order, then I don't think the approach merits extra round-tripping overhead. Well, it's serial devices and mailslots, which can probably be tested on Windows, and char/FIFO devices, which can't. But honestly, I'm not sure testing is worth the trouble. It'd be impossible to prove for sure that Windows queues I/O, but I think it's also the only sensible thing to do. Perhaps more importantly, I think the lesser number of paths we have for asyncs, the better. If we can eliminate one potential outlier by making them work like sockets, it'll be that much easier to work with the async logic. > Side note: what if `server_read_file` is blocked in an alertable wait for an async handle with `async->unknown_status == 1`, and the blocked thread is sent a user APC? I suppose the thread should not be alerted *until after* the driver returns from the dispatcher. Yeah, that part isn't really correct as-is. Perhaps more importantly, if the async returns success I don't think the thread should be alerted at all. It's probably not a huge deal, though, since FILE_SYNCHRONOUS_IO_ALERT is not actually exposed through kernel32. > > > That said, this logic seems questionable. It looks kind of like it exists to avoid calling async_set_result() twice, but that shouldn't be possible anyway. It should only be possible to reach async_satisfied() once. I guess a sufficiently misbehaving client could clone the async handle and then wait on it twice, though... and probably async_set_result() doesn't protect against being called multiple times... > > Yeah, `async_set_result` may need its own flag; perhaps `fully_completed` from your suggestion, `settled`, `finalized`, `completion_sent`, or `completion_notified`. That's possibly better. In this case I think the important thing is also to document what we're trying to protect against. > > > > * Any of: > > > * async_handoff() has not been called, or > > > > I'm not sure this is a meaningful state. Until we get to async_handoff() the async isn't *really* initialized. Honestly, it wouldn't be the craziest thing in the world to defer async creation until that point, possibly by building up some sort of parameters structure that we pass to read/write/ioctl fd callbacks instead of a whole async object. But I don't know, that may be more trouble than its worth. > > `queue_async(q, async);` may precede `async_handoff(async, ...);`. Since `queue_async` needs the async to be materialized, we would need to reorder these two calls. Indeed, queuing makes it nontrivial. Probably more trouble than its worth, then. > > > I suppose we could introduce some sort of bool helper instead of having a direct_result field. It seems relatively clear to me, or at least, not worse than its constitutent logic, but if someone less used to the code has a suggestion, that does deserve wait. It's worth pointing out though that given that part of the condition for whether something is a direct result is "was it ever unknown_status", we'd need another bool flag anyway. I guess then the logic would be as simple as "!was_unknown_status && initial_status != STATUS_PENDING". > > Rather, I want `direct_result` (or a similar flag) to mirror the client state: to be precise, whether the client side has went through the synchronous IOSB filling code section at the time of last state synchronization. Ideally, it should be strictly independent of the server or I/O operation state, so that the async code can be decomposed into an I/O state handling component and a client notification handling component. I don't think I understand what you're trying to do here, which is a bit concerning. > > > "signaled" is a kind of similar case, where you could in theory break it down into a collection of simpler bits, but not all of those simpler bits currently exist. Fundamentally, "signaled" is "blocking ? fully_completed : !unknown_status". > > Er, to be exact, it's `blocking ? fully_completed : (!unknown_status && initial_status_sent_to_client)`, where `initial_status_sent_to_client = irp_completion_deferred || fully_completed`. I guess that's true, though I wouldn't really describe/design it that way. If we reported the initial status directly from the first server request, then the answer to "is the async signaled?" is "it doesn't matter", but for consistency/simplicity I think it should be "yes" rather than "no". Yes, it's signaled, but nobody's waiting on it so it doesn't matter. > > > > * `async_wake_obj(async)` has been called when `async->unknown_status = 0`, or > > > > async_wake_obj() is horrid, because it's basically a leaky abstraction. There should be a better way to organize this, but I'm not really sure how. It kind of looks like we should wake up the async in async_set_initial_status(), but the problem is that ntoskrnl can either return the initial status of an IRP by itself but leave the IRP pending, or it can return the initial *and* final status of a complete IRP, and for the sake of efficiency I don't think we want to wake up the async in async_set_initial_status() in the latter case. That would basically result in a SIGUSR1 when we could just return the APC by interrupting the wait. It would *work* but it would be slower. > > It does not work, since the IOSB has not been filled by `APC_ASYNC_IO` from `set_irp_result` at the time of wake-up. > > What I suppose is that `async_wake_obj` should just be renamed to `async_commit_pending_status` or something similar. I don't think this is much of an improvement. It's still leaking the abstraction. > > > * (Regardless of `async->blocking`) `async_set_result(async, status, ...)` has been called with `async->terminated && (!async->alerted || status != STATUS_PENDING)`. > > > > Which is to say: the client finished processing an APC and didn't restart the async. > > An interesting bit in the condition above is that `STATUS_PENDING` is interpreted *differently* depending on the value of `async->alerted`. I think it's a little inconsistent; we should just have a "restart" boolean flag and fail entirely if `restart && !async->alerted`. Possibly, yes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/499#note_31791

1 0

[PATCH v2 0/1] MR2725: d3dx9: Fix dst pitch for compressed format in D3DXLoadSurfaceFromMemory().
by Paul Gofman (＠gofman) 02 May '23

02 May '23

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54901 The pitch is treated as a byte offset between the rows of blocks in tx_compress_dxtn(), so is the Pitch from _LockRect() so it doesn't need a fixup. That pitch has any effect only when dst texture stride is larger than width being compressed, that's why it was mostly not causing problems before. -- v2: d3dx9: Fix dst pitch for compressed format in D3DXLoadSurfaceFromMemory(). https://gitlab.winehq.org/wine/wine/-/merge_requests/2725

2 2

[PATCH 0/1] MR2725: d3dx9: Fix dst pitch for compressed format in D3DXLoadSurfaceFromMemory().
by Paul Gofman (＠gofman) 02 May '23

02 May '23

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54901 The pitch is treated as a byte offset between the rows of blocks in tx_compress_dxtn(), so is the Pitch from _LockRect() so it doesn't need a fixup. That pitch has any effect only when dst texture stride is larger than width being compressed, that's why it was mostly not causing problems before. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2725

3 5

[PATCH v7 0/10] MR159: vkd3d-shader/hlsl: Broaden resources support v2, part 2.
by Francisco Casas (＠fcasas) 02 May '23

02 May '23

Mainly comprises support for allocating arrays of resources, and loading from them, for both SM1 and SM4. -- v7: vkd3d-shader/hlsl: Support resource arrays when writting SM4. vkd3d-shader/hlsl: Write resource loads in SM1. vkd3d-shader/hlsl: Write sampler declarations in SM1. vkd3d-shader/hlsl: Track objects sampling dimension. vkd3d-shader/hlsl: Track object components usage and allocate registers accordingly. tests: Test objects as parameters. vkd3d-shader/hlsl: Skip object components when creating input/output copies. vkd3d-shader/hlsl: Add fixme for uniform copies for objects within structs. vkd3d-shader/hlsl: Support multiple-register variables in object regsets. tests: Add additional texture array register reservation tests. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/159

2 10

[PATCH v6 0/10] MR159: vkd3d-shader/hlsl: Broaden resources support v2, part 2.
by Francisco Casas (＠fcasas) 02 May '23

02 May '23

Mainly comprises support for allocating arrays of resources, and loading from them, for both SM1 and SM4. -- v6: vkd3d-shader/hlsl: Support resource arrays when writting SM4. vkd3d-shader/hlsl: Write resource loads in SM1. vkd3d-shader/hlsl: Write sampler declarations in SM1. vkd3d-shader/hlsl: Track objects sampling dimension. vkd3d-shader/hlsl: Track object components usage and allocate registers accordingly. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/159

4 13

[PATCH 0/4] MR193: vkd3d-shader/hlsl: Add support for writing RWStructuredBuffer declarations.
by Nikolay Sivov (＠nsivov) 02 May '23

02 May '23

Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/193

3 9

[PATCH v7 0/11] MR1074: Draft: ntdll: Improve Valgrind and GDB integration.
by Rémi Bernon 02 May '23

02 May '23

Valgrind support requires a fork, which I've published to https://gitlab.winehq.org/rbernon/valgrind. The fork implements loading DWARF debug info from PE files, instead of the old and broken upstream PDB support. I've tried to upstream these changes a long time ago but didn't receive any feedback. I think we could maybe consider keeping a fork, which I'm happy to maintain, as the changes aren't too large. We may want to investigate adding 32-on-64 support, which may require a bit more changes (to VEX specifically, because its amd64 guest doesn't support segment register manipulation). The changes here are not all related to Valgrind, and I'll create separate MR for those which may make sense independently from Valgrind / GDB. Also included is a suppression file to silent some annoying false positives, many of which are coming from the cross-stack accesses during syscalls, which are confusing Valgrind's stack heuristics. One can try this out with something like: `WINELOADERNOEXEC=1 valgrind --suppressions=tools/valgrind.supp wine64/loader/wine64 wine64/programs/winecfg/winecfg.exe` -- v7: ntdll: Avoid writing to invalid memory in i386 unix dispatcher. ntdll: Set %rsp before args in x86_64 call_user_mode_callback. ntdll: Fix incorrect i386 call_user_mode_callback CFI. ntdll: Avoid marking freed block header as undefined for valgrind. ntdll: Force HEAP_TAIL_CHECKING_ENABLED flag with valgrind. ntdll: Fix valgrind notifications from ntdll.so. ntdll: Import valgrind headers for PE side ntdll. ntdll: Maintain a PE module link map and expose it to GDB. ntdll: Pass a UNICODE_STRING to load_builtin and virtual_map_image. loader: Expose a shadow copy of ld.so link map to GDB. ntdll: Add .cfi_signal_frame to __wine_syscall_dispatcher. https://gitlab.winehq.org/wine/wine/-/merge_requests/1074

1 11

[PATCH 0/1] MR2723: evr: Fix incorrect integral computation.
by eric pouech (＠epo) 02 May '23

02 May '23

Note: typeof (int * unsigned) is unsigned. So: - on 64bit CPUs, where sizeof(int) = 4 < sizeof(void*) = 8, - when the result of the multiplication is supposed to be negative - there's no progation of the negative sign from 32bit to 64 bit integers Fixes a crash in Age of Empire II. Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2723

4 4