On Fri Oct 14 13:58:34 2022 +0000, Kevin Puetz wrote:
> 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.
Ok, opened !1068 to start with, with just some of the simpler changes that didn't involve the timeout/pumping behavior. This PR is now rebased on top of that, but marked draft and will can sit until we that first part landed so it gets smaller.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/969#note_10760
Salvage tests and fix a leak found in another review: see https://gitlab.winehq.org/wine/wine/-/merge_requests/897#note_9175 points 1 and 2. 3. is why !897 was reverted.
This does not contain the (incorrect) changes to VariantCopyInd, but instead fixes the leak identified in VariantClear (VT_RECORD needs to free pvRecord), and the wrong allocator being used in VariantCopy (it should have been IMalloc, rather than `HeapAlloc`, as shown by the fact IMallocSpy observes these allocations).
This also cleans up some dubious behavior by the test IRecordInfoImpl, which was modifying a VARIANT that it did not own or receive as an argument in the middle of VariantClear. This was likely undefined behavior, and in any case concealed the heap leak.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/1035
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
> 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
>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
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