October 2022 - Wine-GitLab - list.winehq.org

Re: [PATCH v2 0/1] MR1065: ntdll: Add CFI unwind info to __wine_syscall_dispatcher (x86_64)
by Rémi Bernon Oct. 14, 2022

Oct. 14, 2022

BTW, if the issue is only that the CFA is supposed to stay constant within a CFI procedure, would it work if we were splitting the dispatcher with some jumps / call / fake CFI procedures, at the points where we need to change its register? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1065#note_10742

1 0

Re: [PATCH v2 0/1] MR1065: ntdll: Add CFI unwind info to __wine_syscall_dispatcher (x86_64)
by Rémi Bernon Oct. 14, 2022

Oct. 14, 2022

> I guess the biggest issue with that change is, however, that it actually goes against what the DWARF spec. says about the CFA: >> Typically, the CFA is defined to be the value of the stack pointer at the call site in the previous frame (which may be different from its value on entry to the current frame). > > In particular, your change was unfortunately not working with our version of (libunwindstack)\[https://github.com/google/orbit/tree/main/third_party/libu… and LLDB (unfortunately not yet upstream). Yes, that's what I've been told (maybe even *you* did). > I am actually interested, does your change manage to unwind completely through the dispatcher and through the complete windows stack, as shown here? With GDB and with the two other changes I mentioned, it fully works yes. I'm a bit surprised that you don't need the flags push/pop change, as I believe it corrupts the return address on the stack, but maybe the unwinder has other ways to get it back. Now the other part where unwinding doesn't work well is with the other side of syscalls, with KiUserCallbackDispatcher callbacks. Massaging a bit the frame pointers before entering it I'm able to stitch the stack trace to the frame before entering the syscall but I'm then losing the stack within the syscall. I guess we'd probably need some assembly there too to add adequate CFI info if we want to get that right too. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1065#note_10741

1 0

Re: [PATCH v2 0/1] MR1065: ntdll: Add CFI unwind info to __wine_syscall_dispatcher (x86_64)
by Florian Kübler Oct. 14, 2022

Oct. 14, 2022

>FWIW I'm achieving similar results in GDB with https://gitlab.winehq.org/rbernon/wine/-/commit/bf615abeb0c206e40678cec0583…. >As I've been told it's incorrect I didn't try upstreaming it further, but as far as I'm concerned GDB is fully happy with it, and combined with https://gitlab.winehq.org/rbernon/wine/-/commit/9e900267f6eacb8511e18ff2fc1… and https://gitlab.winehq.org/rbernon/wine/-/commit/e21cf3f7702837557f9cc221b20… it can unwind through the syscall dispatcher just fine. >Imho, this variant with the many macros is likely going to be considered as too complex for the small benefit (it's really only for the convenience of debugging Wine itself with a Unix debugger, which I completely agree, *is* useful, though mostly for developers). ---- Hey, I saw your change and would also have hoped that this actually works, as it is obviously much more elegant not fallback to the .cfi_escape primitives. I guess the biggest issue with that change is, however, that it actually goes against what the DWARF spec. says about the CFA: >Typically, the CFA is defined to be the value of the stack pointer at the call site in the previous frame (which may be different from its value on entry to the current frame). In particular, your change was unfortunately not working with our version of (libunwindstack)[https://github.com/google/orbit/tree/main/third_party/libun… and LLDB (unfortunately not yet upstream). I am actually interested, does your change manage to unwind completely through the dispatcher and through the complete windows stack, as shown here? ![mixed_callstack_unwinding](https://werat.dev/blog/how-wine-works-101/6.png) A note about the macros: I agree, they look horrible at the fist glance, but there are really just for readability. When you ignore those for a second, you actually only need to grasp the `__ASM_CFI_DW_OP_bregN` and `__ASM_CFI_DW_OP_deref` instructions, to get the computation of the registers and the CFA. Regarding the usefulness of that change, this is clearly for devs (not only wine directly, but e.g. also DXVK or similars). Also it is not only for debuggers, but also for profiling, to actually find performance problems in the syscalls. As the whole code here, is already very low level, I think this is actually also pretty helpful for people maintaining the syscall dispatcher, as you can, as said in the commit message, actually step through each instruction of the dispatcher with a debugger. So I really think it is worth the effort. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1065#note_10740

1 0

Re: [PATCH v2 0/1] MR1065: ntdll: Add CFI unwind info to __wine_syscall_dispatcher (x86_64)
by Kevin Puetz (＠puetzk) Oct. 14, 2022

Oct. 14, 2022

I haven't reviewed the actual changes here yet (and I'm not that knowledgeable on CFI either), but I would like to vote that this would be a huge quality-of-life improvement for us. When your app is also winelib (since it has dependencies on both win32 and other system ELF libraries), we almost exclusively end up debugging with a unix debugger, and the fact that backtraces almost always fail at `__wine_syscall_dispatcher` is a big pain. Using `gdb record` so we can rewind to see where the call came from helps, but can't help if all you have is a coredump. So this is would be greatly appreciated. Enough so that I had actually written myself a task to try and teach myself enough about DWARF well enough to try writing it :-) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1065#note_10738

1 0

Re: [PATCH v2 0/1] MR1065: ntdll: Add CFI unwind info to __wine_syscall_dispatcher (x86_64)
by Rémi Bernon Oct. 14, 2022

Oct. 14, 2022

FWIW I'm achieving similar results in GDB with https://gitlab.winehq.org/rbernon/wine/-/commit/bf615abeb0c206e40678cec0583…. As I've been told it's incorrect I didn't try upstreaming it further, but as far as I'm concerned GDB is fully happy with it, and combined with https://gitlab.winehq.org/rbernon/wine/-/commit/9e900267f6eacb8511e18ff2fc1… and https://gitlab.winehq.org/rbernon/wine/-/commit/e21cf3f7702837557f9cc221b20… it can unwind through the syscall dispatcher just fine. Imho, this variant with the many macros is likely going to be considered as too complex for the small benefit (it's really only for the convenience of debugging Wine itself with a Unix debugger, which I completely agree, *is* useful, though mostly for developers). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1065#note_10733

1 0

Re: [PATCH v2 0/10] MR969: combase: CoMsgWaitForMultipleHandles message pump.
by Kevin Puetz (＠puetzk) Oct. 14, 2022

Oct. 14, 2022

On Fri Oct 14 13:58:34 2022 +0000, Kevin Puetz wrote: > > I'd suggest moving some of the existing code into helper functions first. > You asked for that the first time too. I've tried, but I just couldn't > find any good seams to carve it along besides consolidating the > PeekMessage calls (e7e4c3d8eee915975e958521b8caffe94ef7a34d). The 3 > major hunks (message pump, the IMessageFilter_MessagePending stuff, and > polling the handles for completions/APCs) all tie into how the loop > exits and the post conditions (re-posting QuitMessage, writing `*index` > (or leaving it unmodified), errors, translating previous Wait* result), > and the order is quite important (because of side-effects like > consuming/preserving QS_ALLPOSTMESSAGE). > If you have any ideas I'd love to hear them. I could certainly pull > those 3 major hunks out into functions, but when they have get passed in > pointers to a bunch of outside-the-loop variables, and their return > value ends up being decoded into ends up with stuff like `if(hr == > S_FALSE) break; else if(FAILED(hr)) goto err;` then the flow control > just turned into spaghetti I hated even more than what it started as. > > ten commits per MR is rather too many (especially when they're non trivial). > Yeah, for lack of coming up with decent helper functions I tried to dice > it *really* finely separate testable commits that changed only one > behavior. I think they are all correct and improvements on the status > quo, but the very first commit fixes the same bug (short-timeout race) > for MTAs that the last finally fixes for STAs, so it felt weird to > propose actually merging it partially. And yet that's really the order > in which the steps make the most sense to read (in terms of minimizing > extraneous code change); getting the MTA out of the loop, so that it's > *only* the mesage loops, then building toward toward the case where > everything is happening in the right order and there's a correct place > to put the timeout. > The part that I dislike about this series is temporarily introducing use > of MWMO_INPUTAVAILABLE; that was a detour born of trying to split the > last 3 commits apart from each other, so there's only one behavior > change at a time. > 1. WAIT_OBJECT_* signaled after dispatching a message > 2. WAIT_IO_COMPLETION after dispatching a message (rather than just once > at the start) > 3. making sure to we're out of relevant messages before escalating > WAIT_TIMOUET to RPC_S_CALLPENDING). > Required some careful work to *preserve* bugs until the next commit. > I guess there's a few things in the middle that basically just > refactor/add tests without changing any observable behavior, so I'll > move those to the front and make a separate MR, just to cut down the > size of this one... And I guess I could also put a lot of the new tests > in as just new `todo_wine` tests, separate from the actual changes that > fix them. That would at least get a lot of lines of code out of this MR, > though not much of the conceptual complexity. Looked at it some more and since I (already) gave up on trying to fix the todo_wine about spurious calls to MessagePending, extracting that much to a helper shouldn't be so bad. It was my (abandoned) theory about how PeekMessage/QueueStatus should interact to suppress those unwanted calls that made things so tangled together - and the didn't make the conformance tests work any better, and it had nothing to do with the actual application I was working on, so it got dropped. The existing behavior (which this MR leaves alone) just needs an early exit to return RPC_E_CALL_CANCELED. So I'll extract that helper too; fixing IMessageFilter todo will probably have to revisit this, but we can just preserve the status quo (and make things more readable) for now. No need to borrow trouble from the future when I'm out of ideas and not trying to tackle that anymore. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/969#note_10685

1 0

[PATCH v2 0/1] MR1024: xmllite/writer: Correctly partially duplicate strings
by David Kahurani (＠kahurani) Oct. 14, 2022

Oct. 14, 2022

Currently, the code copies one extra character than requested and does not terminate the string. Signed-off-by: David Kahurani <k.kahurani(a)gmail.com> -- v2: xmllite/writer: Correctly partially duplicate strings https://gitlab.winehq.org/wine/wine/-/merge_requests/1024

4 11

[PATCH v5 0/5] MR874: mfmediaengine: Avoid vertically flipping video.
by Rémi Bernon Oct. 14, 2022

Oct. 14, 2022

This shows that media session samples are correctly oriented, but that mfmediaengine renders them upside down. -- v5: mfmediaengine: Remove vertical flipping of video frames. mfmediaengine/tests: Check IMFMediaEngine_TransferVideoFrames output orientation. mfmediaengine/tests: Pass a device manager and output format to create_media_engine. mf/tests: Check sample grabber RGB / NV12 orientation. mf/tests: Factor test grabber callback implementations. https://gitlab.winehq.org/wine/wine/-/merge_requests/874

3 9

[PATCH 0/1] MR1062: d2d1: Use a gcc pragma instead of a compiler flag for -fexcess-precision.
by Alexandre Julliard (＠julliard) Oct. 14, 2022

Oct. 14, 2022

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1062

4 3

Re: [PATCH v5 0/5] MR944: win32u: Make desktop resize logic asynchronous.
by Rémi Bernon Oct. 14, 2022

Oct. 14, 2022

On Thu Oct 13 19:11:33 2022 +0000, Rémi Bernon wrote: > Sorry, I'll have a look. I'm having a hard time reproducing it, is this with some specific tests? The first commit indeed changes some behavior, where it was only repositioning the current process windows before and is now repositioning all windows from the desktop thread. I suspect that it could be about the message being sent to the desktop, which is now waiting for all windows to reposition by broadcasting a message itself. If they aren't actively processing messages, it could probably lock up. I created https://gitlab.winehq.org/wine/wine/-/merge_requests/1056 to address that issue, but I cannot confirm if it fixes anything. Other than that I experienced some random and seldom crashes running d3d9:device for instance, but I'm not sure to see how it's related. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/944#note_10671

1 0