On 1/27/22 16:37, Alexandre Julliard wrote:
Jinoh Kang jinoh.kang.kr@gmail.com writes:
loader: Use long instead of int for syscall return type in i386 code.
I was planning to add more syscalls, and found that i386 code used "int" for syscall returns. My assumption was that it was a style issue (presumably 32-bit-area Wine's leftover) that I was expected to address _before_ I could add more inline asm blocks.
[The reason why I found "int" confusing was that I usually read "long" as "machine register-width integer," and "int" as "integer that either only needs to hold small values (e.g. exit code) _or_ has to be 32-bit for some reason." Granted int = long on ILP32, but someone looking at the code might not be able to immediately verify that the type is correct unless they also consider that "int $0x80" is for the i386 ABI (or that the code region is guarded by ifdef __i386__), and i386 is a 32-bit architecture. I supposed that would be extra cognitive load for someone reviewing/auditing the code.]
"long" doesn't mean pointer size in Win32, and since it differs between Win32 and Unix it's confusing, so we try to avoid it as much as possible. Here it's obviously safe, but that still makes it more confusing than "int", not less.
Thanks for the comment. It sounds reasonable. Perhaps if I were going for readability, I should have used "uintptr_t" instead.
I'll switch to int for the next iteration.
loader: Remove GCC <=4.x EBX register spilling workaround for i386.
Similar to above, but debugging related. Again I was adding asm blocks around, so I felt it obliged to fix it before adding more code.
I'm not convinced that this is safe, did you test on a really old gcc?
Yes, I did. See:
https://godbolt.org/z/nq1T1rr93 (gcc -m32 -O2 -fno-PIC)
versus:
https://godbolt.org/z/8ebsK7Mzx (gcc -m32 -O2 -fPIC)
Note that using EBX works flawlessly if PIC is disabled (which is the case for the preloader).
Furthermore I have verified that the patch works on CentOS 7 with GCC 4.8.5:
$ gcc --version gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44) Copyright (C) 2015 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ WINEPRELOADREMAPVDSO=always WINEPRELOADREMAPSTACK=always strace -e trace=mremap ./wine winecfg --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23970, si_uid=1000, si_status=0, si_utime=0, si_stime=0} --- --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23971, si_uid=1000, si_status=0, si_utime=0, si_stime=0} --- --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23972, si_uid=1000, si_status=0, si_utime=0, si_stime=0} --- --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23973, si_uid=1000, si_status=0, si_utime=0, si_stime=0} --- strace: [ Process PID=23969 runs in 32 bit mode. ] mremap(0xf127d000, 12288, 12288, MREMAP_MAYMOVE|MREMAP_FIXED, 0xf1278000) = 0xf1278000 mremap(0xf1280000, 4096, 4096, MREMAP_MAYMOVE|MREMAP_FIXED, 0xf127b000) = 0xf127b000 --- SIGIO {si_signo=SIGIO, si_code=SI_USER, si_pid=23969, si_uid=1000} --- --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23974, si_uid=1000, si_status=0, si_utime=0, si_stime=0} --- --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23976, si_uid=1000, si_status=0, si_utime=0, si_stime=0} --- --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23978, si_uid=1000, si_status=0, si_utime=0, si_stime=0} --- --- SIGUSR1 {si_signo=SIGUSR1, si_code=SI_TKILL, si_pid=23975, si_uid=1000} --- --- SIGUSR1 {si_signo=SIGUSR1, si_code=SI_TKILL, si_pid=23975, si_uid=1000} --- --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23980, si_uid=1000, si_status=0, si_utime=0, si_stime=0} --- 0044:err:ole:start_rpcss Failed to open service manager 003c:fixme:imm:ImeSetActiveContext (0x25ae88, 1): stub 003c:fixme:imm:ImmReleaseContext (00010054, 0025AE88): stub 0044:fixme:imm:ImeSetActiveContext (0x257eb0, 0): stub 0044:fixme:imm:ImmReleaseContext (00010020, 00257EB0): stub
Meanwhile, following are patches that I deem necessary and not gratuitous:
loader: Refactor argv/envp/auxv management.
This was necessary because I had to pass around the pointers to stack objects a lot. Examples include getenv() for letting user control remapping behaviour via env vars, and stack relocation (which requires shifting all the argv/envp/auxv pointers). If the pointer variables were not in one place, the latter would make the code a lot hairy (by passing around all the scattered stack pointers) and unmaintainable.
Well, letting user control it through env vars is exactly what I mean with "unneeded infrastructure".
It was intended to avoid regression by gradually adopting the new behaviour.
My plan was to:
1. Hide the new code path behind a flag. 2. Make it default on Wine-Staging (https://bugs.winehq.org/show_bug.cgi?id=52313). 3. Instruct users experiencing performance problem (intermittent or persistent) to enable the new behaviour and see if it fixes the problem. 4. If enough users have tested the new feature, make it non-configurable.
If you think this process is unnecessary, the environment variables are of course nonsense.
(I've got no replies on the ticket there, and I guessed it might have been better off on the list after all...)
No user is going to want to understand or tweak these things.
Yes. It is intended only for testers and tech/community support, not end-users.
loader: Refactor number parsing to own function.
Number parsing was only for WINEPRELOADRESERVE. Now I need it for parsing /proc/self/maps as well.
You shouldn't need to parse /proc/self/maps at all.
Honestly the part I spent the most time on this patchset was to try to avoid having to parse /proc/self/maps entirely.
The problem is that it's impossible to reliably identity the exact range of Linux vDSO/vvar mapping without reading /proc/self/maps.
1. vDSO hard-codes vvar's offset relative to vDSO. Therefore, remapping vDSO requires vvar to be *also* remapped as well. However, vvar's size and its location relative to vDSO is *not* guaranteed by ABI, and has changed all the time.
- x86: [vvar] orginally resided at a fixed address 0xffffffffff5ff000 (64-bit) [1], but was later changed so that it precedes [vdso] [2]. There, sym_vvar_start is a negative value [3]. text_start is the base address of vDSO, and addr becomes the address of vvar.
- AArch32: [vvar] is a single page and precedes [vdso] [4].
- AArch64: [vvar] is two pages long and precedes [vdso] [5]. Before v5.9, [vvar] was a single page [6].
2. It's very difficult to deduce vDSO and vvar's size and offset relative to each other. Since vvar's symbol does not exist in vDSO's symtab, determining the layout would require parsing vDSO's code.
Also note that CRIU (Checkpoint Restore In Userspace) has maps parsing code just for relocating vDSO [7].
[1] https://lwn.net/Articles/615809/ [2] https://elixir.bootlin.com/linux/v5.16.3/source/arch/x86/entry/vdso/vma.c#L2... [3] https://elixir.bootlin.com/linux/v5.16.3/source/arch/x86/include/asm/vdso.h#... [4] https://elixir.bootlin.com/linux/v5.16.3/source/arch/arm/kernel/vdso.c#L236 [5] https://elixir.bootlin.com/linux/v5.16.3/source/arch/arm64/kernel/vdso.c#L21... [6] https://elixir.bootlin.com/linux/v5.8/source/arch/arm64/kernel/vdso.c#L161 [7] https://github.com/checkpoint-restore/criu/blob/a315774e11b4da1eb36446ae996e...