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
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
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>
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.
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
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);
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
Even after patches by Michael Cronenworth and me, cf.
https://www.winehq.org/pipermail/wine-devel/2020-January/157945.htmlhttps://www.winehq.org/pipermail/wine-devel/2020-January/158454.html
Wine still fails to build with GCC 10 development snapshots.
This is due to a change of defaults in GCC, from -fcommon to -fno-common,
cf. the following from http://gcc.gnu.org/gcc-10/porting_to.html :
A common mistake in C is omitting extern when declaring a global
variable in a header file. If the header is included by several files it
results in multiple definitions of the same variable. In previous GCC
versions this error is ignored. GCC 10 defaults to -fno-common, which
means a linker error will now be reported. To fix this, use extern in
header files when declaring global variables, and ensure each global is
defined in exactly one C file. As a workaround, legacy C code can be
compiled with -fcommon.
The failure left is
gmake[1]: Entering directory '/home/gerald/wine/dlls/schedsvc/tests'
../../../tools/winegcc/winegcc -o schedsvc_test-stripped.exe.so
--wine-objdir ../../.. -fno-PIC -fasynchronous-unwind-tables \
-s -Wb,-F,schedsvc_test.exe -mno-cygwin atsvcapi.o rpcapi.o atsvc_c.o
schrpc_c.o testlist.o \ -lrpcrt4 -lole32 -L/home/gerald/11-i386/lib
schrpc_c.o:(.bss+0x0): multiple definition of `rpc_handle'
atsvc_c.o:(.bss+0x0): first defined here
winebuild: /home/gerald/11-i386/bin/ld failed with status 1
winegcc: ../../../tools/winebuild/winebuild failed
gmake[1]: *** [Makefile:367: schedsvc_test-stripped.exe.so] Error 2
gmake[1]: Leaving directory '/home/gerald/wine/dlls/schedsvc/tests'
gmake: *** [Makefile:8961: dlls/schedsvc/tests] Error 2
atsvc_c.c and schrpc_c.c are auto-generated from atsvc.idl and
schrpc.idl, so I'm not sure how to tackle this.
Anyone who can help and take this?
Thank you,
Gerald
Implement functions used by some games (notably LEGO Island) for
determining which 3D object in a scene was clicked by the mouse cursor.
Fighting Steel also uses this function for mouse over. Previous stubs
would cause LEGO Island to crash upon any click and Fighting Steel
to crash on game start. A patch posted years ago on the bug thread
provided the minimum functionality to prevent crashes, but still
rendered large portions of the game inaccessible without them
implemented correctly.
Picking has been implemented by adding a "pick mode" in
d3d_execute_buffer_execute() which skips any drawing functions
leaving just the vertex processing. Adds click tests for each triangle
when in pick mode for creating an array of D3DPICKRECORDs.
Add a D3DPICKRECORD array and DWORD counter to d3d_device. These are
initiated in d3d_device_init(), allocated/written in
d3d_execute_buffer_execute(), and accessed/read in
d3d_device1_GetPickRecords(). The counter is used to determine the array
size (0 meaning array is not allocated). The array is free'd whenever
the data is no longer necessary by d3d_execute_buffer_execute(),
d3d_device1_GetPickRecords(), and d3d_device_inner_Release().
Add a compliance test to ddraw1 to test whether certain screen points
result in successful picks or not, as well as whether the data returned
from GetPickRecords() is valid and correct.
Stress testing reveals this patch's Pick() implementation may have
slight inaccuracies to the original function; occasionally pixels right
on triangle edges result in successful picks when they don't with the
original function (and vice versa). It may be some sort of floating
point rounding error or other algorithm difference that would be
difficult to determine without seeing the original code. In practice, I
believe this inaccuracy is so negligible that it won't produce any
undesirable results for the user.
v2:
Rebased to latest code (Vijay Kiran Kamuju)
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=10729
Signed-off-by: Matthew Wong <itsmattkc(a)gmail.com>
Signed-off-by: Vijay Kiran Kamuju <infyquest(a)gmail.com>