April 2020 - Wine-devel - list.winehq.org

Re: widl: Set name return value (passed by reference) in the alias case alias case in get_user_type.
by Alexandre Julliard 22 Nov '20

22 Nov '20

Gerald Pfeifer <gerald(a)pfeifer.com> writes: > So, I admit I don't really know this code, but looking at it (triggered > by a warning issued by GCC development versions), I noticed that this > variable passed by reference is not initialized here. It's initialized when we return a type, and it doesn't need to be initialized on NULL return. The code is correct, but you could probably set the variable to NULL in the caller to silence the warning. -- Alexandre Julliard julliard(a)winehq.org

2 1

Error handling in RpcBindingVectorFree (dlls/rpcrt4/rpc_binding.c)
by Gerald Pfeifer 22 Nov '20

22 Nov '20

Looking at RPC_STATUS WINAPI RpcBindingVectorFree( RPC_BINDING_VECTOR** BindingVector ) { RPC_STATUS status; ULONG c; TRACE("(%p)\n", BindingVector); for (c=0; c<(*BindingVector)->Count; c++) { status = RpcBindingFree(&(*BindingVector)->BindingH[c]); } HeapFree(GetProcessHeap(), 0, *BindingVector); *BindingVector = NULL; return RPC_S_OK; } we currently always ignore the outcome of RpcBindingFree and return RPC_S_OK. However, there is one case where RpcBindingFree returns something different (which is if *Binding is null when RPC_S_INVALID_BINDING is returned). What is the proper way of handling this? Just keeping the code as is and removing the unused status variable? Breaking the loop once RpcBindingFree returns something different from RPC_S_OK? Continuing and returning the first / the last status different from RPC_S_OK? Gerald

1 1

[PATCH] xaudio2: Avoid redefining several F3DAudio related constants.
by Gerald Pfeifer 22 Nov '20

22 Nov '20

I noticed a bunch of compiler warnings like the following issued by GCC 10: In file included from ../xaudio2_7/x3daudio.c:29: .../include/F3DAudio.h:69: warning: "SPEAKER_2POINT1" redefined ../../include/x3daudio.h:127: note: this is the location of the previous definition Our own include/x3daudio.h tries to prevent that via #ifndef _SPEAKER_POSITIONS_ #define _SPEAKER_POSITIONS_ However, current it is included *before* the system provided F3DAudio.h, so this safety net falls short. Fixed thusly by changing the order of these two include files so that the system provided one comes first. Gerald Signed-off-by: Gerald Pfeifer <gerald(a)pfeifer.com> --- dlls/xaudio2_7/x3daudio.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/dlls/xaudio2_7/x3daudio.c b/dlls/xaudio2_7/x3daudio.c index 240cd5c101..6f16ba3470 100644 --- a/dlls/xaudio2_7/x3daudio.c +++ b/dlls/xaudio2_7/x3daudio.c @@ -20,15 +20,13 @@ #include "config.h" #include <stdarg.h> - +#include <F3DAudio.h> #include "windef.h" #include "winbase.h" #include "x3daudio.h" #include "wine/debug.h" -#include <F3DAudio.h> - #if XAUDIO2_VER >= 8 || defined X3DAUDIO1_VER WINE_DEFAULT_DEBUG_CHANNEL(xaudio2); #endif -- 2.25.0

2 4

TestBot and graphics tests
by Francois Gouget 24 Aug '20

24 Aug '20

Here are some things I've learned about PCI-passthrough recently, which would be one way (probably the best) to add "real hardware" to the TestBot. I don't want to give anyone false hopes though: this just went from "this is a mysterious thing I need to learn about" to "I think I know how to do it but have not tried it yet". So graphics card PCI-passthrough is now relatively well documented on the Internet and seems to have seen some use-cases that would indicate it may even be reasonably usable. * There are two machines intended to run real GPU tests for Wine: cw1-hd6800 and cw2-gtx560. For now they are only used to run WineTest daily on Windows 8.1, Windows 10 1507, 1709, 1809 and Linux. That's quite a bunch but it would be much better if they were integrated with the TestBot as that would allow developers to submit their own tests. So I had a look at what it would imply to convert them to VM hosts using QEmu + PCI-passthrough. * First one needs a processor with hardware virtualisation support. For Intel that's VT-d. Both machines have an Intel Core 2600 which supports VT-d. Good. * Second the motherboard too needs to support VT-d. Both machines have an ASRock P67 Extreme4 motherboard. Unfortunately UEFI says "unsupported" next to the "VT-d" setting for the motherboard :-( It looks like there was some confusion as to whether the P67 chipset supported VT-d initially. From what I gathered it's only Q67 that does but this caused some manufacturers, among which ASRock, to initially claim support and later retract it. * Then one needs to add the intel_iommu=on option to the kernel command line (resp. amd_iommu). This is should make all the PCI devices appear in /sys/kernel/iommu_groups. But that folder remains empty which confirms that full VT-d support is missing. * Another important aspect is to have a graphics card which is hot-restartable. In some cases when a VM's graphics card is crashed the only way to reset it is to reboot the host. The TestBot is likely to crash the graphics card, particularly if we do a hard-power off on the VMs like we currently do, and it would relaly be annoying to have to reboot the host everytime the graphics card goes belly up. I don't know if the AMD HD6800 and Nvidia GTX560 are suitable but it's quite possible they are not. All I know for now is that we should avoid AMD's R9 line of graphics cards. I still need to find a couple of suitable reasonably lower power graphics cards: one AMD and one Nvidia. * Then one needs to prevent the host from using the graphics card. Usually that's done by having the host use the processor's IGP and dedicating the discrete GPU to the VMs. Unfortunately the 2600's IGP cannot be active when there's a discrete card so that route is denied to us. Fortunately there's quite a bit of documentation on how to shut down not just X but also the Linux virtual consoles to free the GPU and hand it over to the VMs after boot. Doing so means losing KVM access to the host which is a bit annoying in case something goes wrong. So ideally we'd make sure this does not happen in grub's "safe mode" boot option. * Although I have not done any test yet I'm reasonably certain that PCI-passthrough rules out live snapshots: QEmu would have no way to restore the graphics card's internal state. - For Windows VMs that's not an issue: if we provide a power off snapshot the TestBot already knows how to power on the VM and wait for it to boot (as long as the boot is shorter than the connection timeout but it works out usually). - For Linux VM's that's more of an issue: the TestBot will power on the VM as usual. The problem is when it updates Wine: after recompiling everything it deletes the old snapshot and creates a new one from the current state of the VM, which means a live snapshot. So the TestBot will need to be modified so it knows when and how to power off the VM and take a powered off snapshot. * Since the VM has full control of the graphics card QEmu has no access to the content of the screen. That's not an issue for the normal TestBot operation, just for the initial VM setup. Fortunately the graphics card is connected to a KVM so the screen can be accessed through that means. It does mean assigning the mouse and keyboard to the VM too. Should that prove impractical there are a bunch of other options too: VNC, LookingGlass, Synergy, etc. But the less needs to be installed in the VMs the better. * Also the TestBot uses QEmu to take the screenshots. But QEmu does not have access to the content of the screen. The fix is to use a tool to take the screenshots from within the VM and use TestAgent to retrieve them. On Linux there are standard tools we can use. On Windows there's code floating around we can use. So the next steps would be: * Maybe test on my box using the builtin IGP. But that likely won't be very conclusive beyond confirming the snapshot issues, screen access, etc. * Find a suitable AMD or Nvidia graphics card and test that on my box. That would allow me to fully test integration with the TestBot, check for stability issues, etc. * Then see what can be done with the existing cw1 and cw2 boxes. -- Francois Gouget <fgouget(a)codeweavers.com>

4 10

[PATCH 1/2] fsutil: make usage and unsupported command messages localizable
by Vijay Kiran Kamuju 04 Aug '20

04 Aug '20

split the original patch Signed-off-by: Vijay Kiran Kamuju <infyquest(a)gmail.com>

3 2

[PATCH] winapi_test: Add/remove headers for urlmon test, in tests.dat.
by Serge Gautherie 30 May '20

30 May '20

Signed-off-by: Serge Gautherie <winehq-git_serge_180711(a)gautherie.fr> --- Addendum to b3da4abc5771a7542409412c41b3356255f248b2. --- tools/winapi/tests.dat | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/winapi/tests.dat b/tools/winapi/tests.dat index 3f470d4..57a65e1 100644 --- a/tools/winapi/tests.dat +++ b/tools/winapi/tests.dat @@ -1261,9 +1261,9 @@ Unit tests for data structure packing %header -basetsd.h winnt.h windef.h +guiddef.h urlmon.h %include -- 2.10.0.windows.1

2 1

Request for discussion: Using PE libraries for Wine dependencies
by Vincent Povirk (they/them) 27 May '20

27 May '20

The idea of using PE libraries for some Wine library dependencies came up recently because of a bug in CrossOver's 32-bit support on Mac (which uses 64-bit system libraries). Wine's xaudio libraries don't do any translation of 32-bit structures to 64-bit for the native FAudio, meaning that in this configuration, they don't work at all. If FAudio and xaudio were both built as a PE, 32-bit binary, we wouldn't need this translation. Not all dependencies could be used in PE form, anything that needs to interact with the host system wouldn't work. We'll always need native libraries for X11, OpenGL, Vulkan, and host audio libraries, for example. Only libraries like libpng or FAudio which can function based on win32 API's we provide (and/or other libraries) could work. Advantages: * This reduces the surface area of Wine's interface between the virtual Windows environment and host libraries. That makes the transition to PE simpler, and should make it easier to implement 32-bit with 64-bit host libraries in the official Wine release (because fewer thunks to 64-bit host libraries will be required). * It reduces dependencies on host libraries. If FAudio.dll is shipped with Wine or in some other way, users won't need an FAudio package from the host distribution. * It allows code sharing between Wine itself and the Mono and Gecko addons. It's kinda silly that Wine already ships an FAudio.dll, inside Wine Mono, for its XNA support. Wine's xaudio could theoretically be sharing this code, but it isn't. Requirements: * Any solution should function even if mingw is not available in the build environment. This is a challenge because the headers may not work correctly in the winelib build environment. In particular, the calling convention is different between 64-bit Windows and everywhere else, which requires us to decorate Gecko and Mono exports with CDECL so we can call them correctly. It may be possible to work around this problem by wrapping the headers with pragma GCC target. * We probably shouldn't break anything that works in any currently supported ports. With llvm-mingw, we now have the ability to build PE libraries for arm and aarch64 (I've built SDL2 and FAudio this way myself, but I don't have a test environment for them). I'm not sure of the status of the PowerPC and sparc ports. Are these maintained? Do we care? If we do, we'll need to preserve the ability to use native libraries, or build the libraries in winelib. * Our versions of PE dependency libraries must be independent of an application shipping the same libraries. If we ship an FAudio.dll, and an application ships an incompatible FAudio.dll, Wine and the application should each load their own version. This is not an unlikely scenario, as many libraries make use of C standard library features such as FILE or malloc/free in their API's, which means that the ABI depends on which version of the Microsoft C runtime was used to compile it. I started a proof of concept, but I got stuck on the question of how to distribute the FAudio headers, link-time libraries, and binaries. Part of my proof of concept is a repo on github: https://github.com/madewokherd/wine-pedeps Currently, that project builds a couple of PE libraries and does nothing else. It could be developed further to generate packages of headers, libraries, and/or binaries. I'll be referring to that project as wine-pedeps. Options for distributing dependencies: * Distribute headers and libs in a tarball built from wine-pedeps. The binaries would be part of an addon, also built from wine-pedeps. * Import the headers into the Wine tree, and link using LoadLibrary so we don't need .lib files. The headers could be an output from wine-pedeps that gets dropped into a folder like include/wine/external. For building Gecko and Mono, we'd still get headers and libs from wine-pedeps. No one seems to like this idea. * Depend on headers from the host distribution for the corresponding Linux library, and link using LoadLibrary. I'm not sure if this can work reliably, or even if we can make host headers accessible inside mingw without breaking things. For building Gecko and Mono, we'd still get headers and libs from wine-pedeps. * Import entire projects into the Wine tree, perhaps as a submodule. We would then build them and distribute them with Wine. In case mingw is not available, we'd need the option of building them with winelib, or using a host library instead. I don't think the Wine maintainer likes submodules, and I personally don't like the idea of bloating the Wine source distribution the way Mono's use of this approach has forced bloat on the Wine Mono source tree. * Rely on the host distribution to package mingw headers, libs, and binaries for the dependencies we need. The binaries would then be distributed with Wine. Fedora is the only distribution I'm aware of that has an extensive library of mingw packages. To make this easier on other distros, I could maintain wine-pedeps as an alternative source of headers, libraries, and binaries at build time. The trouble with this approach is that it's not clear how we can keep Wine's libraries independent from the application. Perhaps an automated process could rename the binaries and edit their import tables, or sxs could be added after the fact.

9 33

llvm-mingw support update
by Jacek Caban 14 May '20

14 May '20

Hi all, Last night Martin pushed an update to llvm-mingw bumping version of LLVM to a commit that includes a number of fixes for Wine. See [1] for details. Thank you, Martin! Meantime, Wine got required fixes, so that it all should mostly work together. If you want to try it, just clone [2] git and run: DEFAULT_MSVCRT=msvcrt-os ./build-all.sh /path/to/install If the installation is on PATH, current Wine should be able to use it without any additional tweaks. You should be able to configure it just like configuration on GCC-based mingw works. DEFAULT_MSVCRT=msvcrt-os part is needed because Wine can't deal with mingw-w64 defaulting to crt version other than msvcrt.dll. This is not a problem specific to LLVM, we will hit the same problem on GCC if mingw-w64 is configured to use other crt (usually ucrt, things like msvcrt100 is also possible). It is not yet a popular setup, but it will probably be more popular over time, so it would be great to have it supported. The ultimate solution for Wine is to always use -nodefaultlibs for all its binaries. It's already the case for all Wine builtin DLLs, we just need to do the same for EXEs. I have some unfinished patches for that, but it's not something appropriate for code freeze. I'm experimenting with a smaller fix, because it would be great to have something sooner, but using DEFAULT_MSVCRT=msvcrt-os is required for now. One of nice LLVM features is support for PDB files. If you want to make a build with PDB files, configure Wine like this: configure CROSSCFLAGS="-g -gcodeview -O2" CROSSLDFLAGS="-Wl,-pdb=" #append your usual args and then run make like: make CROSSLDFLAGS="-Wl,-pdb=" The additional make argument is needed because Wine does not yet propagate CROSSLDFLAGS from configure. Patch [3] should fix it. Cheers, Jacek [1] https://github.com/mstorsjo/llvm-mingw/commit/056c1f5cd22b1c5ca76af38f2d1f9… [2] https://github.com/mstorsjo/llvm-mingw [3] https://source.winehq.org/patches/data/176054

2 3

Re: [PATCH] add: SetEnvironmentStringsW
by Zebediah Figura 06 May '20

06 May '20

On 3/17/20 3:13 PM, Frank Uhlig wrote: > On Thu, Mar 12, 2020 at 3:25 AM Zebediah Figura <zfigura(a)codeweavers.com> wrote: >> >> On 3/11/20 4:50 PM, Frank Uhlig wrote: >>> Sorry for the late reply, but have finally gotten back to this patch. >>> >>> On Thu, Feb 20, 2020 at 10:35 PM Zebediah Figura <z.figura12(a)gmail.com> wrote: >>>> >>>> On 2/20/20 3:12 PM, Frank Uhlig wrote: >>>>> Hi Zebediah, thanks for looking into this. Please find my responses >>>>> inline below. I've already incorporated many things, but still have >>>>> some remaining questions and comments. >>>>> >>>>> I would re-submit it soon, and include a "versioning" in the mail tag >>>>> as I've seen on other submissions (i.e., [PATCH v2]). >>>>> >>>>> On Mon, Feb 10, 2020 at 4:38 AM Zebediah Figura <z.figura12(a)gmail.com> wrote: >>>>>> >>>>>> Hello Frank, thanks for the patch. >>>>>> >>>>>> With regard to the title, convention is to put the name of the component >>>>>> first, followed by a colon, then a verb describing what the patch does, >>>>>> so e.g. "kernelbase: Add SetEnvironmentStringsW()." >>>>>> >>>>> >>>>> done. >>>>> >>>>>> I have a few more comments inlined below which might help improve the patch: >>>>>> >>>>>> On 1/22/20 3:33 PM, Frank Uhlig wrote: >>>>>>> From: Frank Uhlig <fuulish(a)users.noreply.github.com> >>>>>>> >>>>>>> Signed-off-by: Frank Uhlig <uhlig.frank(a)gmail.com> >>>>>>> --- >>>>>>> ...ms-win-core-processenvironment-l1-1-0.spec | 2 +- >>>>>>> ...ms-win-core-processenvironment-l1-2-0.spec | 2 +- >>>>>>> dlls/kernel32/kernel32.spec | 2 +- >>>>>>> dlls/kernel32/tests/environ.c | 61 +++++++++++++++++++ >>>>>>> dlls/kernelbase/kernelbase.spec | 2 +- >>>>>>> dlls/kernelbase/process.c | 29 +++++++++ >>>>>>> include/winbase.h | 1 + >>>>>>> 7 files changed, 95 insertions(+), 4 deletions(-) >>>>>>> >>>>>>> diff --git a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec >>>>>>> index e3698d6efd..7a62b74390 100644 >>>>>>> --- a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec >>>>>>> +++ b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec >>>>>>> @@ -15,7 +15,7 @@ >>>>>>> @ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW >>>>>>> @ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA >>>>>>> @ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW >>>>>>> -@ stub SetEnvironmentStringsW >>>>>>> +@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW >>>>>>> @ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA >>>>>>> @ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW >>>>>>> @ stdcall SetStdHandle(long long) kernel32.SetStdHandle >>>>>>> diff --git a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec >>>>>>> index 2c25ee1a07..c93d221c5e 100644 >>>>>>> --- a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec >>>>>>> +++ b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec >>>>>>> @@ -17,7 +17,7 @@ >>>>>>> @ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW >>>>>>> @ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA >>>>>>> @ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW >>>>>>> -@ stub SetEnvironmentStringsW >>>>>>> +@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW >>>>>>> @ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA >>>>>>> @ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW >>>>>>> @ stdcall SetStdHandle(long long) kernel32.SetStdHandle >>>>>>> diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec >>>>>>> index be48ef1694..2b74a4182e 100644 >>>>>>> --- a/dlls/kernel32/kernel32.spec >>>>>>> +++ b/dlls/kernel32/kernel32.spec >>>>>>> @@ -1387,7 +1387,7 @@ >>>>>>> # @ stub SetDynamicTimeZoneInformation >>>>>>> @ stdcall -import SetEndOfFile(long) >>>>>>> # @ stub SetEnvironmentStringsA >>>>>>> -# @ stub SetEnvironmentStringsW >>>>>>> +@ stdcall -import SetEnvironmentStringsW (ptr) >>>>>>> @ stdcall -import SetEnvironmentVariableA(str str) >>>>>>> @ stdcall -import SetEnvironmentVariableW(wstr wstr) >>>>>>> @ stdcall -import SetErrorMode(long) >>>>>>> diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c >>>>>>> index 44a6a0cff0..53b2803e77 100644 >>>>>>> --- a/dlls/kernel32/tests/environ.c >>>>>>> +++ b/dlls/kernel32/tests/environ.c >>>>>>> @@ -579,6 +579,66 @@ static void test_GetEnvironmentStringsW(void) >>>>>>> FreeEnvironmentStringsW(env2); >>>>>>> } >>>>>>> >>>>>>> +static void test_SetEnvironmentStringsW(void) >>>>>>> +{ >>>>>>> + DWORD buf_len; >>>>>>> + BOOL ret; >>>>>>> + DWORD ret_size; >>>>>>> + >>>>>>> + static WCHAR buf[256]; >>>>>>> + >>>>>>> + static WCHAR name[] = {'N','a','m','e',0}; >>>>>>> + static WCHAR value[] = {'V','a','l','u','e',0}; >>>>>>> + static WCHAR env[] = {'N','a','m','e','=','V','a','l','u','e',0}; >>>>>>> + >>>>>>> + static WCHAR eman[] = {'e','m','a','N',0}; >>>>>>> + static WCHAR eulav[] = {'e','u','l','a','V',0}; >>>>>>> + static WCHAR vne[] = {'e','m','a','N','=','e','u','l','a','V',0}; >>>>>>> + >>>>>>> + static WCHAR var[] = {'V','a','r',0}; >>>>>>> + static WCHAR val[] = {'V','a','l',0}; >>>>>>> + static WCHAR rav[] = {'r','a','V',0}; >>>>>>> + static WCHAR lav[] = {'l','a','V',0}; >>>>>>> + static WCHAR mul[] = {'V','a','r','=','V','a','l',' ','r','a','V','=','l','a','V',0}; >>>>>>> + >>>>>>> + static WCHAR empty[] = {'V','a','r','=',0}; >>>>>> >>>>>> You can use wide character string literals in tests (which should also >>>>>> allow you to get rid of some of these declarations). >>>>>> >>>>> >>>>> Didn't change this, b/c I was trying to keep the same style as already >>>>> there and described on the website >>>>> (https://wiki.winehq.org/Developer_Hints#Using_only_C89-compliant_code) >>>> >>>> The wiki is out of date on this particular topic. Someone™ should >>>> probably update it. >>>> >>>>> >>>>>> I suspect that you also want either local variables or (when applicable) >>>>>> "static const". There's no point declaring non-constant variables as static. >>>>>> >>>>> >>>>> Agreed, but kept as is to be consistent; and my test code is now not >>>>> changing the input anymore. >>>> >>>> To be consistent with what? There may be existing code that uses bad >>>> style, but that's not really a good reason to emulate it. >>>> >>> >>> consistent style is less prone to errors. but further, I prefer the >>> array initialization b/c of the visible double-0 termination of the >>> strings. >>> and yes, the static was a remnant of a previous static const, and got removed. >>> >>>>> >>>>>>> + >>>>>>> + buf_len = sizeof(buf) / 2; >>>>>> >>>>>> I personally think using an extra variable is redundant here. I also >>>>>> think "ARRAY_SIZE(buf)" would be more expressive. >>>>>> >>>>> >>>>> Changed to ARRAY_SIZE(buf) >>>>> >>>>>>> + >>>>>>> + ret = SetEnvironmentStringsW(env); >>>>>>> + ok( ret, "Setting environment strings failed\n" ); >>>>>>> + >>>>>>> + ret_size = GetEnvironmentVariableW(name, buf, buf_len); >>>>>>> + ok( ((0 != ret_size) && (0 == wcscmp(buf, value))), >>>>>>> + "Environment String settings resulted in different value\n"); >>>>>> >>>>>> With all of these tests I think it would be helpful to print the actual >>>>>> value of "ret_size" and contents of "buf" in the case the test fails. I >>>>>> would also recommend testing "ret_size" against the exact expected >>>>>> length, and perhaps splitting the two tests into two separate ok() calls. >>>>>> >>>>> >>>>> Made the checks more specific, and included more info in the output >>>>> >>>>>>> + >>>>>>> + ret = SetEnvironmentStringsW(vne); >>>>>>> + ok( ret, "Setting environment strings failed\n" ); >>>>>>> + >>>>>>> + ret_size = GetEnvironmentVariableW(eman, buf, buf_len); >>>>>>> + ok( ((0 != ret_size) && (0 == wcscmp(buf, eulav))), >>>>>>> + "Environment String settings resulted in different value\n"); >>>>>>> + >>>>>>> + ret = SetEnvironmentStringsW(mul); >>>>>>> + ok( ret, "Setting environment strings failed\n" ); >>>>>>> + >>>>>>> + ret_size = GetEnvironmentVariableW(var, buf, buf_len); >>>>>>> + ok( ((0 != ret_size) && (0 == wcscmp(buf, val))), >>>>>>> + "Environment String settings resulted in different value\n"); >>>>>>> + >>>>>>> + ret_size = GetEnvironmentVariableW(rav, buf, buf_len); >>>>>>> + ok( ((0 != ret_size) && (0 == wcscmp(buf, lav))), >>>>>>> + "Environment String settings resulted in different value\n"); >>>>>>> + >>>>>>> + ret = SetEnvironmentStringsW(empty); >>>>>>> + ok( ret, "Setting environment strings failed\n" ); >>>>>>> + >>>>>>> + ret_size = GetEnvironmentVariableW(var, buf, buf_len); >>>>>>> + ok( (0 == ret_size), >>>>>>> + "Environment String settings resulted in different value\n"); >>>>>>> + >>>>>>> +} >>>>>>> + >>>>>>> START_TEST(environ) >>>>>>> { >>>>>>> init_functionpointers(); >>>>>>> @@ -591,4 +651,5 @@ START_TEST(environ) >>>>>>> test_GetComputerNameExA(); >>>>>>> test_GetComputerNameExW(); >>>>>>> test_GetEnvironmentStringsW(); >>>>>>> + test_SetEnvironmentStringsW(); >>>>>>> } >>>>>>> diff --git a/dlls/kernelbase/kernelbase.spec b/dlls/kernelbase/kernelbase.spec >>>>>>> index f36d4d525c..a5e30b938f 100644 >>>>>>> --- a/dlls/kernelbase/kernelbase.spec >>>>>>> +++ b/dlls/kernelbase/kernelbase.spec >>>>>>> @@ -1422,7 +1422,7 @@ >>>>>>> @ stdcall SetDefaultDllDirectories(long) >>>>>>> # @ stub SetDynamicTimeZoneInformation >>>>>>> @ stdcall SetEndOfFile(long) >>>>>>> -@ stub SetEnvironmentStringsW >>>>>>> +@ stdcall SetEnvironmentStringsW(ptr) >>>>>>> @ stdcall SetEnvironmentVariableA(str str) >>>>>>> @ stdcall SetEnvironmentVariableW(wstr wstr) >>>>>>> @ stdcall SetErrorMode(long) >>>>>>> diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c >>>>>>> index a07dddb1fc..97b59b9548 100644 >>>>>>> --- a/dlls/kernelbase/process.c >>>>>>> +++ b/dlls/kernelbase/process.c >>>>>>> @@ -1345,6 +1345,35 @@ BOOL WINAPI DECLSPEC_HOTPATCH SetEnvironmentVariableW( LPCWSTR name, LPCWSTR val >>>>>>> } >>>>>>> >>>>>>> >>>>>>> +/*********************************************************************** >>>>>>> + * SetEnvironmentStringsW (kernelbase.@) >>>>>>> + */ >>>>>>> +BOOL WINAPI DECLSPEC_HOTPATCH SetEnvironmentStringsW( LPWCH NewEnvironment ) >>>>>> >>>>>> "WCHAR *" would probably be preferred. >>>>>> >>>>> >>>>> Any particular reason for this? That's exactly what LPWCH is defined as. >>>> >>>> Broadly, use of pointer typedefs tends to be dispreferred in new code. I >>>> also suspect that "env" would be nicer than "NewEnvironment". >>>> >>> >>> OK, fine by me. >>> >>> >>>>>>> +{ >>>>>>> + >>>>>> >>>>>> Stray newline. >>>>>> >>>>> >>>>> Done. >>>>> >>>>>>> + BOOL rc = FALSE; >>>>>>> + WCHAR *var, *val; >>>>>>> + const WCHAR delim[] = {' ','=','\n',0}; >>>>>>> + >>>>>>> + TRACE( "(%s)\n", debugstr_w(NewEnvironment)); >>>>>>> + >>>>>>> + if (NULL == NewEnvironment) >>>>>>> + return rc; >>>>>> >>>>>> This probably deserves a test. >>>>>> >>>>> >>>>> I tried including a simple test by just calling >>>>> SetEnvironmentStrings(NULL) and that lead to an unhandled exception on >>>>> my windows machine (cross-compiled from 64 bit Linux to 64 bit Windows >>>>> using mingw). No clue how and why, but would welcome any suggestions >>>>> on why this simple thing might fail. >>>> >>>> It's possible that Windows doesn't check the pointer against NULL >>>> either, and in that case there's not much point in doing it in Wine. >>>> >>> >>> You're right, but this is such an elementary check that I would not remove it. >>> >>>>> >>>>>>> + >>>>>>> + var = wcstok(NewEnvironment, delim); >>>>>>> + val = wcstok(NULL, delim); >>>>>> >>>>>> This isn't thread-safe. [It's not stated anywhere that >>>>>> SetEnvironmentStrings() itself is thread-safe, but it costs little to >>>>>> assume it is, and moreover this will race with anything else that calls >>>>>> wcstok().] >>>>> >>>>> True, however I had trouble using the reentrant wcstok_s on my system >>>>> (i.e., 'twas unavailable :). Is there an alternative, specific >>>>> include? (I tride the expected #include <wchar.h> but that didn't >>>>> work). >>>> >>>> It should probably be added to the headers. I think it goes in string.h >>>> and wchar.h, but Jacek might correct me on this. >>>> >>> >>> Switched to wcschr which is available and working. >>> Was trying to include wcstok_s from msvcrt, but without success. What >>> would be the proper way to go about this? >> >> I think that Jacek added it in the interim ;-) > > Interesting, wcstok_s is available in the headers, however linking for > the kernelbase.dll.so with undefined reference. Not sure though which > additional library should be included!? Er, yes, I forgot this was kernelbase. Indeed you can't use wcstok_s in that case, since ntdll doesn't export it. > >> >> That said, you probably shouldn't use it, if tests show that >> SetEnvironmentStrings() doesn't modify the argument. (If it does, I >> guess that gives you free license.) >> >>> >>>>> >>>>>> >>>>>>> + >>>>>>> + while (var != NULL) { >>>>>>> + if (FALSE == (rc = SetEnvironmentVariableW(var, val))) >>>>>>> + break; >>>>>>> + var = wcstok(NULL, delim); >>>>>>> + val = wcstok(NULL, delim); >>>>>>> + } >>>>>>> + >>>>>>> + return rc; >>>>>>> +} >>>>>> >>>>>> Broadly, this seems surprising; is this really how the function is >>>>>> supposed to work? >>>>>> >>>>>> I think your patch could use more interesting tests. For example: >>>>>> >>>>>> * Immediately I'd expect GetEnvironmentStrings() to return the exact >>>>>> same pointer that SetEnvironmentStrings() set. Is this in fact the case? >>>>>> If so, how do A and W variants (of both Get/Set) interact? >>>>>> >>>>> >>>>> Confusing. GetEnvironmentStringsW doesn't return a pointer. Haven't >>>>> seen any info on the GetEnvironmentStringsA variant. >>>> >>>> Er, what? GetEnvironmentStringsA() and GetEnvironmentStringsW() return >>>> pointers to char and WCHAR respectively. >>>> >>> >>> Sorry, misread. However, GetEnvironmentStringsW returns a pointer to a >>> new copy of the environment strings with each call. Probably from the >>> same source that gets modified by SetEnvironmentStringsW. >>> Not really checkable from the outside, I gather. >> >> Okay, so part of that comment was based on the mistaken impression that >> GetEnvironmentStrings() returned a pointer to a constant string instead >> of allocating a new set every time. Apologies for the confusion that may >> have caused. >> >>> >>>>> >>>>>> * Are values not present in the call to SetEnvironmentStrings() deleted >>>>>> from the environment? >>>>> >>>>> doc doesn't specify anything about this. I'd expect no. >>>> >>>> The documentation is often less specific than desired, and also often >>>> incorrect; that's why we write tests, and code our implementations >>>> against them. It's not obvious whether SetEnvironmentStrings() means >>>> "add these several strings to the environment" or "replace the entire >>>> environment with these strings", hence the question. >>>> >>> >>> Yes, on a Windows machine a fresh environment is set up and all the >>> previous variables are discarded. In the wine patch now as well. >> >> One thing you'll need to take into consideration, then, is that there >> are several environment variables Wine needs to function correctly. In >> practice I think this means pretty much anything prefixed with WINE, >> though you may want to maintain a manual list. >> >> Are there any others I'm not thinking of? >> >> (Host libraries would presumably need them—and if I'm using e.g. >> GST_DEBUG I'd prefer not to have to jump through hoops to make that >> work, although if SetEnvironmentStrings() is uncommon I can't complain >> too much, and really all I'd need to do is hack >> RtlSetCurrentEnvironment() to set that variable afterward. On the one >> hand, host libraries would use getenv() anyway, but on the other hand, >> if we move to PE files for some of them, this becomes a potential >> problem again. All in all, I'm not sure this is a salient enough concern >> to warrant doing anything about it...) >> > > If you're using getenv, you're getting (as you say) variables from the > host. Those vars are not touched by this function and ought to not get > reset. Or is wine copying the host env into its own internal > Windows-like representation (WinRep)? > Are those WINE-prefixed environment variables within the host or the WinRep? The host environment gets copied into the Win32 environment (with some modifications; see is_special_env_var() in dlls/ntdll/env.c). Wine also adds some variables to the Win32 environment not present in the host environment; see set_additional_environment() and set_wow64_environment(). In particular I'm concerned about environment variables such as WINEDLLDIR, WINECONFIGDIR, i.e. those set by set_wine_path_variable(); certain components (e.g. mscoree, setupapi) need those to be set to function. It's also maybe worth a test to see if even non-Wine-specific variables set by ntdll (e.g. ProgramFilesDir) get stripped from the environment. > >>> >>>>> >>>>>> >>>>>> * The PSDK header suggests that SetEnvironmentStrings() should receive a >>>>>> doubly null-terminated argument. I'd advise testing with an environment >>>>>> containing a null separator (e.g. L"one=two\0three=four\0".) >>>>>> >>>>> >>>>> Done. >>>>> >>>>>> * You treat '\n' as a separator, but have no tests for it. Also, if both >>>>>> space and newline are a separator, what about other whitespace >>>>>> characters (tab, carriage return)? >>>>> >>>>> I have reduced the number of possible separators to just include '=' >>>>> and conform with the doubly-null terminated string expectation. This >>>>> also conforms with the few examples I've found. >>>>> >>>>>> >>>>>> * What about strings that violate the assumptions this implementation >>>>>> makes? For example, "one=", "one", "=two", "one=two=three", "one=two >>>>>> three=four"... >>>>>> >>>>> >>>>> Included a few more tests. >>> >>> Just to note, "one=two=three" is valid and will assign 'two=three' to >>> the variable 'one' (on windows). >>> one= is also not illegal, AFAICT. >>> >>>>> >>>>>> * Is this function really supposed to modify its argument? (Yes, it's >>>>>> not declared constant, but that doesn't always mean that it will.) >>>>>> >>>>> >>>>> Done, included a buffer that's modified instead. >>>>> >>>>> >>>>>>> + >>>>>>> + >>>>>>> /*********************************************************************** >>>>>>> * Process/thread attribute lists >>>>>>> ***********************************************************************/ >>>>>>> diff --git a/include/winbase.h b/include/winbase.h >>>>>>> index 655eb48f0f..0c1aa19b42 100644 >>>>>>> --- a/include/winbase.h >>>>>>> +++ b/include/winbase.h >>>>>>> @@ -2621,6 +2621,7 @@ WINBASEAPI BOOL WINAPI SetEndOfFile(HANDLE); >>>>>>> WINBASEAPI BOOL WINAPI SetEnvironmentVariableA(LPCSTR,LPCSTR); >>>>>>> WINBASEAPI BOOL WINAPI SetEnvironmentVariableW(LPCWSTR,LPCWSTR); >>>>>>> #define SetEnvironmentVariable WINELIB_NAME_AW(SetEnvironmentVariable) >>>>>>> +WINBASEAPI BOOL WINAPI SetEnvironmentStringsW(LPWCH); >>>>>>> WINBASEAPI UINT WINAPI SetErrorMode(UINT); >>>>>>> WINBASEAPI BOOL WINAPI SetEvent(HANDLE); >>>>>>> WINBASEAPI VOID WINAPI SetFileApisToANSI(void); >>>>>>> >>>>>> >>>>> >>>> >>>> >>> >>

2 1

[PATCH v4 1/2] ntdll/tests: Test user_shared_data timestamp updates.
by Rémi Bernon 05 May '20

05 May '20

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- v4: * New way to test timestamps, still some little variations after the updates are implemented, as we may miss a server update. * Add checks for InterruptTime, including some weird offset on testbot w864. * Use a single page for the whole system, initialized when wineboot.exe completes its creation, using its USD initial values. * Use atomic writes if possible, or assume that reordering guarantees on volatile accesses will be enough. PROT_WRITE doesn't always implies PROT_READ and atomic exchange would probably need readable page. We could also be mmap RW on the server side. * Address a race condition when receiving the usd fd by including it in the server call uninterruptible section. * Use the same 0x10000 size in ntdll initial mapping, later remapping and for the server side page creation. dlls/ntdll/tests/time.c | 78 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/dlls/ntdll/tests/time.c b/dlls/ntdll/tests/time.c index 5382a952d8d8..fe71d4458c0f 100644 --- a/dlls/ntdll/tests/time.c +++ b/dlls/ntdll/tests/time.c @@ -18,7 +18,9 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */ +#define NONAMELESSUNION #include "ntdll_test.h" +#include "ddk/wdm.h" #define TICKSPERSEC 10000000 #define TICKSPERMSEC 10000 @@ -29,6 +31,7 @@ static VOID (WINAPI *pRtlTimeFieldsToTime)( PTIME_FIELDS TimeFields, PLARGE_IN static NTSTATUS (WINAPI *pNtQueryPerformanceCounter)( LARGE_INTEGER *counter, LARGE_INTEGER *frequency ); static NTSTATUS (WINAPI *pRtlQueryTimeZoneInformation)( RTL_TIME_ZONE_INFORMATION *); static NTSTATUS (WINAPI *pRtlQueryDynamicTimeZoneInformation)( RTL_DYNAMIC_TIME_ZONE_INFORMATION *); +static BOOL (WINAPI *pRtlQueryUnbiasedInterruptTime)( ULONGLONG *time ); static const int MonthLengths[2][12] = { @@ -153,6 +156,79 @@ static void test_RtlQueryTimeZoneInformation(void) wine_dbgstr_w(tzinfo.DaylightName)); } +static ULONGLONG read_ksystem_time(volatile KSYSTEM_TIME *time) +{ + ULONGLONG high, low; + do + { + high = time->High1Time; + low = time->LowPart; + } + while (high != time->High2Time); + return high << 32 | low; +} + +static void test_user_shared_data_time(void) +{ + KSHARED_USER_DATA *user_shared_data = (void *)0x7ffe0000; + ULONGLONG t1, t2, t3; + int i = 0; + + i = 0; + do + { + t1 = GetTickCount(); + if (user_shared_data->NtMajorVersion <= 5 && user_shared_data->NtMinorVersion <= 1) + t2 = ((ULONG64)user_shared_data->TickCountLowDeprecated * user_shared_data->TickCountMultiplier) >> 24; + else + t2 = (read_ksystem_time(&user_shared_data->u.TickCount) * user_shared_data->TickCountMultiplier) >> 24; + t3 = GetTickCount(); + } while(t3 < t1 && i++ < 1); /* allow for wrap, but only once */ + + ok(t1 <= t2, "USD TickCount / GetTickCount are out of order: %s %s\n", + wine_dbgstr_longlong(t1), wine_dbgstr_longlong(t2)); + todo_wine + ok(t2 <= t3, "USD TickCount / GetTickCount are out of order: %s %s\n", + wine_dbgstr_longlong(t2), wine_dbgstr_longlong(t3)); + + i = 0; + do + { + LARGE_INTEGER system_time; + NtQuerySystemTime(&system_time); + t1 = system_time.QuadPart; + t2 = read_ksystem_time(&user_shared_data->SystemTime); + NtQuerySystemTime(&system_time); + t3 = system_time.QuadPart; + } while(t3 < t1 && i++ < 1); /* allow for wrap, but only once */ + + todo_wine + ok(t1 <= t2, "USD SystemTime / NtQuerySystemTime are out of order %s %s\n", + wine_dbgstr_longlong(t1), wine_dbgstr_longlong(t2)); + ok(t2 <= t3, "USD SystemTime / NtQuerySystemTime are out of order %s %s\n", + wine_dbgstr_longlong(t2), wine_dbgstr_longlong(t3)); + + if (!pRtlQueryUnbiasedInterruptTime) + win_skip("skipping RtlQueryUnbiasedInterruptTime tests\n"); + else + { + i = 0; + do + { + pRtlQueryUnbiasedInterruptTime(&t1); + t2 = read_ksystem_time(&user_shared_data->InterruptTime); + pRtlQueryUnbiasedInterruptTime(&t3); + } while(t3 < t1 && i++ < 1); /* allow for wrap, but only once */ + + todo_wine + ok(t1 <= t2, "USD InterruptTime / RtlQueryUnbiasedInterruptTime are out of order %s %s\n", + wine_dbgstr_longlong(t1), wine_dbgstr_longlong(t2)); + ok(t2 <= t3 || broken(t2 == t3 + 82410089070) /* w864 has some weird offset on testbot */, + "USD InterruptTime / RtlQueryUnbiasedInterruptTime are out of order %s %s\n", + wine_dbgstr_longlong(t2), wine_dbgstr_longlong(t3)); + } +} + START_TEST(time) { HMODULE mod = GetModuleHandleA("ntdll.dll"); @@ -163,6 +239,7 @@ START_TEST(time) (void *)GetProcAddress(mod, "RtlQueryTimeZoneInformation"); pRtlQueryDynamicTimeZoneInformation = (void *)GetProcAddress(mod, "RtlQueryDynamicTimeZoneInformation"); + pRtlQueryUnbiasedInterruptTime = (void *)GetProcAddress(mod, "RtlQueryUnbiasedInterruptTime"); if (pRtlTimeToTimeFields && pRtlTimeFieldsToTime) test_pRtlTimeToTimeFields(); @@ -170,4 +247,5 @@ START_TEST(time) win_skip("Required time conversion functions are not available\n"); test_NtQueryPerformanceCounter(); test_RtlQueryTimeZoneInformation(); + test_user_shared_data_time(); } -- 2.26.1

4 9