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. 17, 2022

Oct. 17, 2022

> Ah, that is good to know, thanks. Apparently gdb's unwinder works slightly different, than the used we looked at. Also, I am surprised it actually can unwind Windows PE/Coff-based frames. I should probably add that it only does that when it has the debug info for the PE modules, this isn't currently well supported, and needs to be done somehow manually. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1065#note_10927

1 0

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

Oct. 17, 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? > > My point is, that the issue is actually not about CFA not being constant, but that CFA has a semantic that many unwinding tools/libraries rely on, i.e. that CFA is suppose to be the value of the stack pointer at the caller, right before the call. In particular, CFA is suppose to be a value that would be a valid stack pointer. > > In your change, however, CFA gets later set to be a pointer into syscall_frame. Hmm, I need to think about that. When I said it fully works, I should maybe indicate that GDB doesn't usually display local variables for the first frame below the syscall, so maybe it's because of this even though it manages to catch up for lower frames. > > With GDB and with the two other changes I mentioned, it fully works yes. > > Ah, that is good to know, thanks. Apparently gdb's unwinder works slightly different, than the used we looked at. Also, I am surprised it actually can unwind Windows PE/Coff-based frames. FWIW this also works with Valgrind, and you might be interested in https://gitlab.winehq.org/wine/wine/-/merge_requests/1074 too. > > 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. > > Interesting. I am wondering why the `pushfl` instruction corrupts the return address. If this would be the case, wouldn't that not lead to actual runtime issues when the dispatcher returns? Maybe the reason why we don't see an issue with that, is that we are actually setting the CFI rule for the return address based on `syscall_frame` right away? It's not exactly the `pushfl` but more that the dispatcher pops `rip` first, which effectively removes the return address from the stack, then `pushfl` overrides its value. It's not causing problems later because the dispatcher uses the poped `rip` value to return to the caller, but it temporarily confuses GDB stack unwinding. Maybe with your CFI instructions it's not required. > > Yes, that's what I've been told (maybe even *you* did). > > No, that wasnt me :smile:, it's actually my first PR. Also note, that I am not a wine expert, so please forgive me, if I miss something obvious. Well, nothing in this area is obvious to me either. > > Now the other part where unwinding doesn't work well is with the other side of syscalls, with KiUserCallbackDispatcher callbacks. > > Unfortunately, I am not aware of the `KiUserCallbackDispatcher`. So I can't tell if there is more work to be done/how this could be fixed. > > We basically looked at reported unwinding errors in Orbit, and discovered the missing unwind info (CFI) being the root cause of most of them. With this, and some adjustments to our unwinder to cope with certain corner-cases of PE/Coff files, we ended up with less than 1% of unwinding errors. > > Actually, another issue that kept us quite busy is, that the `kernel_stack` is not allocated on the "normal"/"user" stack. That apparently used to be the case before Wine 7. This way, Linux performance collector tools, such as perf_event_open, fail to collect the "user-space" part of the stack. For debugging this is fine, as the program execution is halted and we can just read the data from memory, but for Profiling, we needed to massively work around that. Does someone know about the reason for the two separated stacks? Would it be maybe possible, to actually allocate that "kernel_stack" at the normal stack? The reason I've been told, is that some DRMs are checking that the NT syscalls didn't touch the stack. I don't really know more. Yes, it's already an issue for `perf` captures when you want stack traces that cross user and kernel stacks, and I don't really have any good solution for that. Having an optional working mode which would interleave kernel frames and user frames may be possible but it doesn't seem much tractable. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1065#note_10925

1 0

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

Oct. 17, 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? My point is, that the issue is actually not about CFA not being constant, but that CFA has a semantic that many unwinding tools/libraries rely on, i.e. that CFA is suppose to be the value of the stack pointer at the caller, right before the call. In particular, CFA is suppose to be a value that would be a valid stack pointer. In your change, however, CFA gets later set to be a pointer into syscall_frame. >With GDB and with the two other changes I mentioned, it fully works yes. Ah, that is good to know, thanks. Apparently gdb's unwinder works slightly different, than the used we looked at. Also, I am surprised it actually can unwind Windows PE/Coff-based frames. >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. Interesting. I am wondering why the `pushfl` instruction corrupts the return address. If this would be the case, wouldn't that not lead to actual runtime issues when the dispatcher returns? Maybe the reason why we don't see an issue with that, is that we are actually setting the CFI rule for the return address based on `syscall_frame` right away? >Yes, that's what I've been told (maybe even *you* did). No, that wasnt me :smile:, it's actually my first PR. Also note, that I am not a wine expert, so please forgive me, if I miss something obvious. >Now the other part where unwinding doesn't work well is with the other side of syscalls, with KiUserCallbackDispatcher callbacks. Unfortunately, I am not aware of the `KiUserCallbackDispatcher`. So I can't tell if there is more work to be done/how this could be fixed. We basically looked at reported unwinding errors in Orbit, and discovered the missing unwind info (CFI) being the root cause of most of them. With this, and some adjustments to our unwinder to cope with certain corner-cases of PE/Coff files, we ended up with less than 1% of unwinding errors. Actually, another issue that kept us quite busy is, that the `kernel_stack` is not allocated on the "normal"/"user" stack. That apparently used to be the case before Wine 7. This way, Linux performance collector tools, such as perf_event_open, fail to collect the "user-space" part of the stack. For debugging this is fine, as the program execution is halted and we can just read the data from memory, but for Profiling, we needed to massively work around that. Does someone know about the reason for the two separated stacks? Would it be maybe possible, to actually allocate that "kernel_stack" at the normal stack? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1065#note_10922

1 0

[PATCH 0/4] MR1076: opengl32: Cleanup extension disabling override in preparation for https://gitlab.winehq.org/wine/wine/-/merge_requests/1010.
by Rémi Bernon Oct. 17, 2022

Oct. 17, 2022

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

2 5

[PATCH 0/13] MR1074: Draft: ntdll: Improve Valgrind and GDB integration.
by Rémi Bernon Oct. 17, 2022

Oct. 17, 2022

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` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1074

4 32

[PATCH 0/1] MR1075: include: Add TOOLINFO[AW].
by Alex Henrie (＠alexhenrie) Oct. 17, 2022

Oct. 17, 2022

Needed to compile Tera Term. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1075

3 2

[PATCH 0/2] MR1073: Fix video flickering.
by Akihiro Sagawa (＠sgwaki) Oct. 17, 2022

Oct. 17, 2022

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

4 4

[PATCH 0/1] MR1002: win32u: Fix data race in NtUserGetProcessDpiAwarenessContext.
by Jinoh Kang (＠iamahuman) Oct. 16, 2022

Oct. 16, 2022

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

5 9

[PATCH v3 0/1] MR363: user32/tests: Test for window exposure behaviours.
by Jinoh Kang (＠iamahuman) Oct. 16, 2022

Oct. 16, 2022

Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> -- v3: user32/tests: Test for window exposure behaviours. https://gitlab.winehq.org/wine/wine/-/merge_requests/363

3 3

[PATCH 0/4] MR1052: uiautomationcore: Initial patches for multi-provider HUIANODEs.
by Connor McAdams (＠cmcadams) Oct. 16, 2022

Oct. 16, 2022

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

4 8