July 2016 - Wine-Devel - list.winehq.org

Re: [v1 1/1] dinput: Sort virtual joysticks by name on the Mac

by Ken Thomases

On Jul 16, 2016, at 3:15 AM, David Lawrie <david.dljunk(a)gmail.com> wrote: > > Tested on OS X 10.10.5. > > Signed-off-by: David Lawrie <david.dljunk(a)gmail.com> > --- > dlls/dinput/joystick_osx.c | 35 ++++++++++++++++++++++++++++++++--- > 1 file changed, 32 insertions(+), 3 deletions(-) > > diff --git a/dlls/dinput/joystick_osx.c b/dlls/dinput/joystick_osx.c > index 637692b..22bdc4f 100644 > --- a/dlls/dinput/joystick_osx.c > +++ b/dlls/dinput/joystick_osx.c > @@ -193,6 +193,26 @@ static long get_device_property_long(IOHIDDeviceRef device, CFStringRef key) > return result; > } > > +static CFStringRef get_device_name(IOHIDDeviceRef device) > +{ > + CFTypeRef ref; > + CFStringRef name = CFSTR("Default Name"); You can probably do better for a fallback. For example, you can format the vendor ID and product ID into a string. For an extreme take on this, see the Copy_DeviceName() function in Apple's HID Calibrator sample code: https://developer.apple.com/library/mac/samplecode/HID_Calibrator/Listings/… > + > + if(device) > + { > + assert(IOHIDDeviceGetTypeID() == CFGetTypeID(device)); > + > + ref = IOHIDDeviceGetProperty(device, CFSTR(kIOHIDProductKey)); > + > + if (ref && CFStringGetTypeID() == CFGetTypeID(ref)) > + name = ref; > + else if (ref) > + name = CFCopyDescription(ref); This introduces a memory-management problem, a.k.a. a leak. This copy is never released. If you follow my suggestion above for formatting a string as a fallback if the product name isn't available, you'd also have to release that. So, you would change this function's name to copy_device_name() and make sure all code paths return an owned reference to the caller, which the caller has to release. Basically, in the case where IOHIDDeviceGetProperty() gives you a string, you'd have to copy it to match the other code paths. Of course, you'd have to make sure the callers do, in fact, release the references. > + } > + > + return name; > +} > + > static long get_device_location_ID(IOHIDDeviceRef device) > { > return get_device_property_long(device, CFSTR(kIOHIDLocationIDKey)); > @@ -203,7 +223,15 @@ static void copy_set_to_array(const void *value, void *context) > CFArrayAppendValue(context, value); > } > > -static CFComparisonResult device_location_comparator(const void *val1, const void *val2, void *context) > +static CFComparisonResult device_name_comparator(IOHIDDeviceRef device1, IOHIDDeviceRef device2) > +{ > + CFStringRef name1 = get_device_name(device1), name2 = get_device_name(device2); > + CFStringCompareFlags compareOptions = kCFCompareForcedOrdering | kCFCompareNumerically; Any reason you've added this separate variable instead of just inlining the options in the call? > + > + return CFStringCompare(name1, name2, compareOptions); > +} -Ken

July 17, 2016

Re: WineHQ upgrade breaking dinput8:dinput?

by Bernhard Übelacker

Hello, just tried to find out when this started exactly. First by using old packages from snapshot.debian.org. That revealed it started between packages: gcc-mingw-w64-i686_4.6.4-4+9_i386.deb gcc-mingw-w64-i686_4.8.2-5+10_i386.deb A git bisect of gcc history points to this commit: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=ff2a5adaa72fef87cac689a40… ff2a5adaa72fef87cac689a40c23258a30b304c8 is the first bad commit commit ff2a5adaa72fef87cac689a40c23258a30b304c8 Author: hubicka <hubicka(a)138bc75d-0d04-0410-961f-82ee72b054a4> Date: Sun Apr 22 21:28:07 2012 +0000 * lto-symtab.c (lto_varpool_replace_node): Do not merge needed flags. (...much longer commit message...) git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk(a)186687 138bc75d-0d04-0410-961f-82ee72b054a4 :040000 040000 d706bdeafbddb9f5f02474e39a245dca08f3334a 508082a31919fe9e907f70b3b3278d4f8d953fac M gcc Testbot runs: Good: (gcc 62c34bf, last one before ff2a5ad) https://testbot.winehq.org/JobDetails.pl?Key=24283 Bad: (gcc ff2a5ad) https://testbot.winehq.org/JobDetails.pl?Key=24280 Hope this could be of any help to identify the issue. Kind regards, Bernhard Notes: - Did not install gcc build system wide. Instead set the PATH that i686-w64-mingw32-gcc was taken from there. - Initial wine configure step was done using Jessie default gcc-mingw-w64-i686 4.9.1-19+14.3. Then the iterations were just done by removing objects from dlls/dinput8/tests and make dinput8_crosstest.exe. - GCC history before 1e37e371 did segfault while building, therefore started from there. Am 24.04.2016 um 20:30 schrieb Francois Gouget: > > I have rechecked this to make sure the test does not have a subtle bug > that causes it to depend on the stack layout. > > > In the process I have reduced the test to the minimum set necessary to > reproduce the bug (see attachment). But in the end I did not find > anything wrong so it does look like a compiler bug. > > I don't know how to proceed from there. > > * I think it would be better if someone more knowlegeable than me > reported this bug to the MinGW guys. Maybe someone who could undertand > what happens in the assembly code? > > * On the Wine side it would really be nice to get this test (and > the three others that are impacted) to stop failing. But removing all > the static directives really does not feel right. Would it be better > to arrange for these tests to be compiled with -O0? Any other cleaner > workaround. > > So I'm really hoping someone can step up and move this forward. > > > Here are the relevant bugs: > * dinput8:dinput regression caused by new compiler > https://bugs.winehq.org/show_bug.cgi?id=40384 > > * usp10:usp10 regression caused by new cross-compiler > https://bugs.winehq.org/show_bug.cgi?id=40385 > > * wininet:url regression caused by new cross-compiler > https://bugs.winehq.org/show_bug.cgi?id=40386 > > * Unexplained new random comctl32:header failure (cross-compiler issue?) > https://bugs.winehq.org/show_bug.cgi?id=40442 > > > >

July 17, 2016

Re: [PATCH] winegstreamer: Fixes for video flipping

by Jan Schmidt

On 16/07/16 09:12, Jan Schmidt wrote: > On 16/07/16 05:25, Andrew Eikum wrote: > >>> Fix state setting of new elements - always just >>> set them to PAUSED. gst_element_sync_state_with_parent() >>> can be problematic, and we only need these in paused >>> or playing anyway. >>> >> >> I'm not sure about this part. It makes sense to me for all members of >> a container to be in the same state. Why is it better for these to be >> PAUSED while others are PLAYING? If it's needed, this should be its >> own patch, too. > > This is actually the part that fixes bug 40764 (although the refcounting > ones are needed to avoid crashes too). You can set them to PLAYING or > PAUSED - it doesn't actually matter. The only difference between playing > and paused states in GStreamer is in elements that do operations on the > clock, such as audio or video sinks. For videoflip and videoconvert the > important thing is that they aren't still in READY state by the time > they receive data, because then their pads will not have been activated. > That's the core of the race condition that's causing 40764. Actually, I just realised this isn't true. The actual cause of 40764 is replacing the bus on decodebin which is now inside the container bin. It was OK when decodebin was the top-level element, but not when it's a child element. Setting the bus on the top-level bin is sufficient to fix 40764 reliably, which I'd fixed separately while cleaning up the ref-counting. Continuing to use gst_element_sync_state_to_parent here seems OK. New patches coming soon. J. -- Jan Schmidt, Centricular Ltd - http://centricular.com/

July 15, 2016

Re: [PATCH] winegstreamer: Fixes for video flipping

by Jan Schmidt

On 16/07/16 05:25, Andrew Eikum wrote: > Thanks very much for working on this, I'm glad to have more eyes > looking at it. Comments in-line below. > > On Sat, Jul 16, 2016 at 02:51:10AM +1000, Jan Schmidt wrote: >> The videoflip element doesn't handle all formats, >> so some formats like Intel Indeo 3 will fail. >> Add videoconvert to do format conversion when it's >> needed. >> > > Shouldn't the decodebin handle this during format negotiation when the > videoflip is connected to it? No, decodebin only handles decoding - it stops and outputs pads once it successfully decodes raw video or audio. >> Fix some refcount issues too - add a new element >> to a bin takes ownership of the floating reference, >> so there's no need to keep pointers around to unref >> later. >> > > Good catch. Could you separate this out into a different, stand-alone > patch from the other changes? OK >> Fix state setting of new elements - always just >> set them to PAUSED. gst_element_sync_state_with_parent() >> can be problematic, and we only need these in paused >> or playing anyway. >> > > I'm not sure about this part. It makes sense to me for all members of > a container to be in the same state. Why is it better for these to be > PAUSED while others are PLAYING? If it's needed, this should be its > own patch, too. This is actually the part that fixes bug 40764 (although the refcounting ones are needed to avoid crashes too). You can set them to PLAYING or PAUSED - it doesn't actually matter. The only difference between playing and paused states in GStreamer is in elements that do operations on the clock, such as audio or video sinks. For videoflip and videoconvert the important thing is that they aren't still in READY state by the time they receive data, because then their pads will not have been activated. That's the core of the race condition that's causing 40764. >> @@ -846,13 +855,16 @@ static void init_new_decoded_pad(GstElement *bin, GstPad *pad, GSTImpl *This) >> >> gst_util_set_object_arg(G_OBJECT(pin->flipfilter), "method", "vertical-flip"); >> >> - gst_bin_add(GST_BIN(This->container), pin->flipfilter); >> - gst_element_sync_state_with_parent(pin->flipfilter); >> + gst_element_set_state (vconv, GST_STATE_PAUSED); >> + gst_bin_add(GST_BIN(This->container), vconv); /* bin takes ownership */ >> + gst_element_set_state (pin->flipfilter, GST_STATE_PAUSED); >> + gst_bin_add(GST_BIN(This->container), pin->flipfilter); /* bin takes ownership */ >> >> - pin->flip_sink = gst_element_get_static_pad(pin->flipfilter, "sink"); >> + gst_element_link (vconv, pin->flipfilter); >> + >> + pin->flip_sink = gst_element_get_static_pad(vconv, "sink"); >> if(!pin->flip_sink){ >> WARN("Couldn't find sink on flip filter\n"); >> - gst_object_unref(pin->flipfilter); > > I believe this leaves a dangling element in the container. Should > these lines be replaced with calls to gst_bin_remove() instead? Those lines replace the insertion of videoflip with the insertion of 'videoconvert -> videoflip' as a linked pair and then treats them as a single unit - getting the sink pad of the first element, and the source pad of the videoflip. The elements are owned by the bin afterward, so there's no need to keep a ref or clean them up explictly. They'll be disposed of automatically when the container is unreffed on cleanup. Cheers, Jan. > Andrew > -- Jan Schmidt, Centricular Ltd - http://centricular.com/

July 15, 2016

Re: [PATCH] winegstreamer: Fixes for video flipping

by Andrew Eikum

Thanks very much for working on this, I'm glad to have more eyes looking at it. Comments in-line below. On Sat, Jul 16, 2016 at 02:51:10AM +1000, Jan Schmidt wrote: > The videoflip element doesn't handle all formats, > so some formats like Intel Indeo 3 will fail. > Add videoconvert to do format conversion when it's > needed. > Shouldn't the decodebin handle this during format negotiation when the videoflip is connected to it? > Fix some refcount issues too - add a new element > to a bin takes ownership of the floating reference, > so there's no need to keep pointers around to unref > later. > Good catch. Could you separate this out into a different, stand-alone patch from the other changes? > Fix state setting of new elements - always just > set them to PAUSED. gst_element_sync_state_with_parent() > can be problematic, and we only need these in paused > or playing anyway. > I'm not sure about this part. It makes sense to me for all members of a container to be in the same state. Why is it better for these to be PAUSED while others are PLAYING? If it's needed, this should be its own patch, too. > @@ -846,13 +855,16 @@ static void init_new_decoded_pad(GstElement *bin, GstPad *pad, GSTImpl *This) > > gst_util_set_object_arg(G_OBJECT(pin->flipfilter), "method", "vertical-flip"); > > - gst_bin_add(GST_BIN(This->container), pin->flipfilter); > - gst_element_sync_state_with_parent(pin->flipfilter); > + gst_element_set_state (vconv, GST_STATE_PAUSED); > + gst_bin_add(GST_BIN(This->container), vconv); /* bin takes ownership */ > + gst_element_set_state (pin->flipfilter, GST_STATE_PAUSED); > + gst_bin_add(GST_BIN(This->container), pin->flipfilter); /* bin takes ownership */ > > - pin->flip_sink = gst_element_get_static_pad(pin->flipfilter, "sink"); > + gst_element_link (vconv, pin->flipfilter); > + > + pin->flip_sink = gst_element_get_static_pad(vconv, "sink"); > if(!pin->flip_sink){ > WARN("Couldn't find sink on flip filter\n"); > - gst_object_unref(pin->flipfilter); I believe this leaves a dangling element in the container. Should these lines be replaced with calls to gst_bin_remove() instead? Andrew

July 15, 2016

Re: [PATCH 2/9] d3drm: Implement IDirect3DRMDevice*::InitFromD3D. (v2)

by Aaryaman Vasishta

On Fri, Jul 15, 2016 at 5:34 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > I've just fixed some these up in the past, and let some others slide, > but please watch your line length. > > Right, sorry about that. I've started using a page guide in XCode so this shouldn't happen again. I'll resend the fixed patch.

July 15, 2016

Fwd: Re: [PATCH] wined3d: Implement SM5 bfrev opcode in glsl.

by Guillaume Charifi

Right, I was sure to forget to git add something... Do I need to resend everything, or is it ok if I send a patch that only enables the required extension, and will need to be committed before the others? Le 15/07/2016 13:59, Józef Kucia a écrit : > On Fri, Jul 15, 2016 at 1:25 PM, Guillaume Charifi > <guillaume.charifi(a)sfr.fr> wrote: >> Signed-off-by: Guillaume Charifi <guillaume.charifi(a)sfr.fr> >> --- >> dlls/wined3d/glsl_shader.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c >> index 5e23920..376d110 100644 >> --- a/dlls/wined3d/glsl_shader.c >> +++ b/dlls/wined3d/glsl_shader.c >> @@ -3538,6 +3538,7 @@ static void shader_glsl_map2gl(const struct wined3d_shader_instruction *ins) >> case WINED3DSIH_ROUND_Z: instruction = "trunc"; break; >> case WINED3DSIH_SQRT: instruction = "sqrt"; break; >> case WINED3DSIH_UMAX: instruction = "max"; break; >> + case WINED3DSIH_BFREV: instruction = "bitfieldReverse"; break; > I think this requires you to enable appriopriate extension in GLSL > (GL_ARB_gpu_shader5, in this case). I haven't looked closely at the > other patches but they seem to have the same issue.

July 15, 2016

Re: [PATCH] wined3d: Implement SM5 bfrev opcode in glsl.

by Henri Verbeet

On 15 July 2016 at 13:59, Józef Kucia <joseph.kucia(a)gmail.com> wrote: > On Fri, Jul 15, 2016 at 1:25 PM, Guillaume Charifi > <guillaume.charifi(a)sfr.fr> wrote: >> Signed-off-by: Guillaume Charifi <guillaume.charifi(a)sfr.fr> >> --- >> dlls/wined3d/glsl_shader.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c >> index 5e23920..376d110 100644 >> --- a/dlls/wined3d/glsl_shader.c >> +++ b/dlls/wined3d/glsl_shader.c >> @@ -3538,6 +3538,7 @@ static void shader_glsl_map2gl(const struct wined3d_shader_instruction *ins) >> case WINED3DSIH_ROUND_Z: instruction = "trunc"; break; >> case WINED3DSIH_SQRT: instruction = "sqrt"; break; >> case WINED3DSIH_UMAX: instruction = "max"; break; >> + case WINED3DSIH_BFREV: instruction = "bitfieldReverse"; break; > > I think this requires you to enable appriopriate extension in GLSL > (GL_ARB_gpu_shader5, in this case). I haven't looked closely at the > other patches but they seem to have the same issue. > Yeah, although the plan was to require GLSL 4.30 for shader model 5, which would have the various bitfield operations. I think this raises questions about how these patches were validated.

July 15, 2016

Re: [PATCH 4/9] d3drm: Implement IDirect3DRMViewport*::Init. (v5)

by Henri Verbeet

On 15 July 2016 at 12:32, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote: > diff --git a/dlls/d3drm/viewport.c b/dlls/d3drm/viewport.c > index 9fd627d..743fb75 100644 > --- a/dlls/d3drm/viewport.c > +++ b/dlls/d3drm/viewport.c > @@ -270,19 +279,126 @@ static HRESULT WINAPI d3drm_viewport1_GetClassName(IDirect3DRMViewport *iface, D > static HRESULT WINAPI d3drm_viewport2_Init(IDirect3DRMViewport2 *iface, IDirect3DRMDevice3 *device, > IDirect3DRMFrame3 *camera, DWORD x, DWORD y, DWORD width, DWORD height) > { > - FIXME("iface %p, device %p, camera %p, x %u, y %u, width %u, height %u stub!\n", > + struct d3drm_viewport *viewport = impl_from_IDirect3DRMViewport2(iface); > + D3DVIEWPORT vp; > + D3DVALUE scale; > + IDirect3D *d3d1 = NULL; > + D3DCOLOR color; > + IDirect3DDevice *d3d_device = NULL; > + IDirect3DMaterial *material = NULL; > + D3DMATERIAL mat; > + D3DMATERIALHANDLE hmat; > + HRESULT hr = D3DRM_OK; > + ... > + color = IDirect3DRMFrame3_GetSceneBackground(camera); > + /* Create material (ambient/diffuse/emissive?), set material */ > + if (FAILED(hr = IDirect3D_CreateMaterial(d3d1, &material, NULL))) > + goto cleanup; > + > + memset(&mat, 0, sizeof(mat)); > + memcpy(&mat.diffuse, &color, sizeof(color)); That doesn't look right.

July 15, 2016

Re: [PATCH 2/9] d3drm: Implement IDirect3DRMDevice*::InitFromD3D. (v2)

by Henri Verbeet

On 15 July 2016 at 12:32, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote: > @@ -3984,6 +3985,8 @@ static void test_create_device_from_d3d1(void) > ok(ref2 > ref1, "expected ref2 > ref1, got ref1 = %u , ref2 = %u.\n", ref1, ref2); > device_ref2 = get_refcount((IUnknown *)d3ddevice1); > ok(device_ref2 > device_ref1, "Expected device_ref2 > device_ref1, got device_ref1 = %u, device_ref2 = %u.\n", device_ref1, device_ref2); > + d3d_ref2 = get_refcount((IUnknown *)d3d1); > + ok(d3d_ref2 > d3d_ref1, "Expected d3d_ref2 > d3d_ref1, got d3d_ref1 = %u, d3d_ref2 = %u.\n", d3d_ref1, d3d_ref2); > ret_val = IDirect3DRMDevice_GetWidth(device1); > ok(ret_val == rc.right, "Expected device width = 300, got %u.\n", ret_val); > ret_val = IDirect3DRMDevice_GetHeight(device1); > @@ -4053,6 +4056,133 @@ static void test_create_device_from_d3d1(void) > device_ref2 = get_refcount((IUnknown *)d3ddevice1); > ok(device_ref2 == device_ref1, "Expected device_ref2 == device_ref1, got device_ref1 = %u, device_ref2 = %u.\n", device_ref1, device_ref2); > > + /* InitFromD3D tests */ > + hr = IDirect3DRM_CreateObject(d3drm1, &CLSID_CDirect3DRMDevice, NULL, &IID_IDirect3DRMDevice, (void **)&device1); > + ok(SUCCEEDED(hr), "Cannot get IDirect3DRMDevice interface (hr = %#x).\n", hr); > + > + hr = IDirect3DRMDevice_InitFromD3D(device1, NULL, d3ddevice1); > + ok(hr == D3DRMERR_BADVALUE, "Expected hr == D3DRMERR_BADVALUE, got %#x.\n", hr); > + hr = IDirect3DRMDevice_InitFromD3D(device1, d3d1, NULL); > + ok(hr == D3DRMERR_BADVALUE, "Expected hr == D3DRMERR_BADVALUE, got %#x.\n", hr); > + > + hr = IDirect3DRMDevice_InitFromD3D(device1, d3d1, d3ddevice1); > + ok(SUCCEEDED(hr), "Failed to initialise IDirect3DRMDevice interface (hr = %#x)\n", hr); > + ref2 = get_refcount((IUnknown *)d3drm1); > + ok(ref2 > ref1, "expected ref2 > ref1, got ref1 = %u , ref2 = %u.\n", ref1, ref2); > + device_ref2 = get_refcount((IUnknown *)d3ddevice1); > + ok(device_ref2 > device_ref1, "Expected device_ref2 > device_ref1, got device_ref1 = %u, device_ref2 = %u.\n", device_ref1, device_ref2); I've just fixed some these up in the past, and let some others slide, but please watch your line length.

July 15, 2016

Re: [PATCH] wined3d: Implement SM5 bfrev opcode in glsl.

by Józef Kucia

On Fri, Jul 15, 2016 at 1:25 PM, Guillaume Charifi <guillaume.charifi(a)sfr.fr> wrote: > Signed-off-by: Guillaume Charifi <guillaume.charifi(a)sfr.fr> > --- > dlls/wined3d/glsl_shader.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c > index 5e23920..376d110 100644 > --- a/dlls/wined3d/glsl_shader.c > +++ b/dlls/wined3d/glsl_shader.c > @@ -3538,6 +3538,7 @@ static void shader_glsl_map2gl(const struct wined3d_shader_instruction *ins) > case WINED3DSIH_ROUND_Z: instruction = "trunc"; break; > case WINED3DSIH_SQRT: instruction = "sqrt"; break; > case WINED3DSIH_UMAX: instruction = "max"; break; > + case WINED3DSIH_BFREV: instruction = "bitfieldReverse"; break; I think this requires you to enable appriopriate extension in GLSL (GL_ARB_gpu_shader5, in this case). I haven't looked closely at the other patches but they seem to have the same issue.

July 15, 2016

Re: [PATCH] msscript: Initial support for hosting script engines

by Jacek Caban

Hi Nikolay, On 14.07.2016 16:35, Nikolay Sivov wrote: > +static HRESULT init_script_host(const CLSID *clsid, ScriptHost **ret) > +{ > + IObjectSafety *objsafety; > + ScriptHost *host; > + HRESULT hr; > + > + *ret = NULL; > + > + host = heap_alloc(sizeof(*host)); > + if (!host) > + return E_OUTOFMEMORY; > + > + host->IActiveScriptSite_iface.lpVtbl = &ActiveScriptSiteVtbl; > + host->ref = 1; > + host->script = NULL; > + host->parse = NULL; > + host->clsid = *clsid; > + > + hr = CoCreateInstance(&host->clsid, NULL, CLSCTX_INPROC_SERVER|CLSCTX_INPROC_HANDLER, > + &IID_IActiveScript, (void**)&host->script); > + if (FAILED(hr)) { > + WARN("Failed to create an instance for %s, %#x\n", debugstr_guid(clsid), hr); > + goto failed; > + } > + > + hr = IActiveScript_QueryInterface(host->script, &IID_IObjectSafety, (void**)&objsafety); > + if (FAILED(hr)) { > + FIXME("Could not get IObjectSafety, %#x\n", hr); > + goto failed; > + } > + > + hr = IObjectSafety_SetInterfaceSafetyOptions(objsafety, &IID_IActiveScriptParse, INTERFACESAFE_FOR_UNTRUSTED_DATA, 0); > + if (FAILED(hr)) > + FIXME("SetInterfaceSafetyOptions failed, %#x\n", hr); This failure is important, please handle it well. > + IObjectSafety_Release(objsafety); > + > + hr = IActiveScript_SetScriptSite(host->script, &host->IActiveScriptSite_iface); > + if (FAILED(hr)) { > + WARN("SetScriptSite failed, %#x\n", hr); > + goto failed; > + } > + > + hr = IActiveScript_QueryInterface(host->script, &IID_IActiveScriptParse, (void**)&host->parse); > + if (FAILED(hr)) { > + WARN("Failed to get IActiveScriptParse, %#x\n", hr); > + goto failed; > + } This and... > + hr = IActiveScriptParse_InitNew(host->parse); > + if (FAILED(hr)) { > + WARN("InitNew failed, %#x\n", hr); > + goto failed; > + } ...this failure will cause ref leak. You have already set script site, so engine holds a reference to ScriptHost, but you only release one reference on failure. Using release_script_engine on failure should fix that. > + > + *ret = host; > + return S_OK; > + > +failed: > + IActiveScriptSite_Release(&host->IActiveScriptSite_iface); > + return hr; > +} > + > static HRESULT WINAPI ScriptControl_QueryInterface(IScriptControl *iface, REFIID riid, void **ppv) > { > ScriptControl *This = impl_from_IScriptControl(iface); > @@ -257,6 +498,10 @@ static ULONG WINAPI ScriptControl_Release(IScriptControl *iface) > if(!ref) { > if (This->site) > IOleClientSite_Release(This->site); > + if (This->host) { > + release_script_engine(This->host); > + IActiveScriptSite_Release(&This->host->IActiveScriptSite_iface); > + } > heap_free(This); > } > > @@ -321,15 +566,47 @@ static HRESULT WINAPI ScriptControl_Invoke(IScriptControl *iface, DISPID dispIdM > static HRESULT WINAPI ScriptControl_get_Language(IScriptControl *iface, BSTR *p) > { > ScriptControl *This = impl_from_IScriptControl(iface); > - FIXME("(%p)->(%p)\n", This, p); > - return E_NOTIMPL; > + LPOLESTR progidW; > + HRESULT hr; > + > + TRACE("(%p)->(%p)\n", This, p); > + > + if (!p) > + return E_POINTER; > + > + *p = NULL; > + > + if (!This->host) > + return S_OK; > + > + hr = ProgIDFromCLSID(&This->host->clsid, &progidW); > + if (FAILED(hr)) > + return hr; > + > + *p = SysAllocStringLen(progidW, lstrlenW(progidW)); How about SysAllocString instead? > + CoTaskMemFree(progidW); > + return *p ? S_OK : E_OUTOFMEMORY; > } > > static HRESULT WINAPI ScriptControl_put_Language(IScriptControl *iface, BSTR language) > { > ScriptControl *This = impl_from_IScriptControl(iface); > - FIXME("(%p)->(%s)\n", This, debugstr_w(language)); > - return E_NOTIMPL; > + CLSID clsid; > + > + TRACE("(%p)->(%s)\n", This, debugstr_w(language)); > + > + if (language && FAILED(CLSIDFromProgID(language, &clsid))) > + return CTL_E_INVALIDPROPERTYVALUE; > + > + if (This->host) { > + IActiveScriptSite_Release(&This->host->IActiveScriptSite_iface); > + This->host = NULL; > + } This will leave zombie script host and engine. You should probably use release_script_engine here as well. > + > + if (!language) > + return S_OK; > + > + return init_script_host(&clsid, &This->host); > } > > static HRESULT WINAPI ScriptControl_get_State(IScriptControl *iface, ScriptControlStates *p) > @@ -1379,6 +1656,7 @@ static HRESULT WINAPI ScriptControl_CreateInstance(IClassFactory *iface, IUnknow > script_control->ref = 1; > script_control->site = NULL; > script_control->cp_list = NULL; > + script_control->host = NULL; > > ConnectionPoint_Init(&script_control->cp_scsource, script_control, &DIID_DScriptControlSource); > ConnectionPoint_Init(&script_control->cp_propnotif, script_control, &IID_IPropertyNotifySink); > Thanks, Jacek

July 15, 2016

Re: [PATCH 4/9] d3drm: Implement IDirect3DRMViewport*::Init. (v5)

by Aaryaman Vasishta

Hi, On Fri, Jul 15, 2016 at 4:27 PM, Michael Stefaniuc <mstefani(a)redhat.com> wrote: > On 07/15/2016 12:45 PM, Aaryaman Vasishta wrote: > > > > > > On Fri, Jul 15, 2016 at 4:02 PM, Aaryaman Vasishta > > <jem456.vasishta(a)gmail.com <mailto:jem456.vasishta(a)gmail.com>> wrote: > > > > struct d3drm_viewport > > { > > struct d3drm_object obj; > > + IDirect3DRMFrame *camera; > > IDirect3DRMViewport IDirect3DRMViewport_iface; > > IDirect3DRMViewport2 IDirect3DRMViewport2_iface; > > + IDirect3DViewport *d3d_viewport; > > + IDirect3DMaterial *material; > > IDirect3DRM *d3drm; > > D3DVALUE back; > > D3DVALUE front; > > > > I've decided to keep the interface instead of using the struct directly, > > since it'll be easier to use the interface functions for transformations > > and other paramaters. Referencing the interface via the object for every > You can do that as long as you can use only methods of those interfaces. > As soon as you need to access fields of the object struct directly you > need to save the object pointer. > > bye > michael > > Right, I'll keep that in mind once an implementation requires it :) Cheers, Aaryaman

July 15, 2016

Re: [PATCH 4/9] d3drm: Implement IDirect3DRMViewport*::Init. (v5)

by Michael Stefaniuc

On 07/15/2016 12:45 PM, Aaryaman Vasishta wrote: > > > On Fri, Jul 15, 2016 at 4:02 PM, Aaryaman Vasishta > <jem456.vasishta(a)gmail.com <mailto:jem456.vasishta(a)gmail.com>> wrote: > > struct d3drm_viewport > { > struct d3drm_object obj; > + IDirect3DRMFrame *camera; > IDirect3DRMViewport IDirect3DRMViewport_iface; > IDirect3DRMViewport2 IDirect3DRMViewport2_iface; > + IDirect3DViewport *d3d_viewport; > + IDirect3DMaterial *material; > IDirect3DRM *d3drm; > D3DVALUE back; > D3DVALUE front; > > I've decided to keep the interface instead of using the struct directly, > since it'll be easier to use the interface functions for transformations > and other paramaters. Referencing the interface via the object for every You can do that as long as you can use only methods of those interfaces. As soon as you need to access fields of the object struct directly you need to save the object pointer. bye michael

July 15, 2016

Re: [PATCH 4/9] d3drm: Implement IDirect3DRMViewport*::Init. (v5)

by Aaryaman Vasishta

On Fri, Jul 15, 2016 at 4:02 PM, Aaryaman Vasishta < jem456.vasishta(a)gmail.com> wrote: > struct d3drm_viewport > { > struct d3drm_object obj; > + IDirect3DRMFrame *camera; > IDirect3DRMViewport IDirect3DRMViewport_iface; > IDirect3DRMViewport2 IDirect3DRMViewport2_iface; > + IDirect3DViewport *d3d_viewport; > + IDirect3DMaterial *material; > IDirect3DRM *d3drm; > D3DVALUE back; > D3DVALUE front; > I've decided to keep the interface instead of using the struct directly, since it'll be easier to use the interface functions for transformations and other paramaters. Referencing the interface via the object for every function call seems a bit redundant. Also I think refcounts should be handled seamlessly this way. A reference to the background material created internally is also kept here. As Stefan had pointed out, the material is not AddRef'd since its handle is used instead, so it's probably safer to keep the reference to ourselves and release it when the object is destroyed. Cheers, Aaryaman

July 15, 2016

Re: [PATCH 4/4] d3drm: Implement IDirect3DRMDevice*::Add/DeleteDestroyCallback. (v2)

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Am 2016-07-11 um 08:46 schrieb Aaryaman Vasishta: > Umm, It's still not clear whether the interface is QI'd to > IDirect3DRMObject or referenced directly. Our implementation > doesn't do the AddRef, but I don't think refcounting matters much > in this case, since the callbacks happen right before the object > gets destroyed. For the mailing list archives: We talked about this on IRC, and it turns out that native always passes the version 1 interface to the callback, regardless of which version was used to call AddDestroyCallback. (or rather, it passes the IDirect3DRMObject interface, which happens to share the vtable with the version 1 interface in all objects as far as we know. Our code already implements this behavior. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXh973AAoJEN0/YqbEcdMwoXEP/24KdStf2VVxBKk62V26TVnw z8lzY1T6gLR/6ryzOuYAOKjeXRv+ZZgdPyiGhU1jgzXISKy1yAsZIckW5vi+r+2d niTt++P+/ldoEAxix2Eh4xf0boNjr3VtKVIU14DXpkpOb+VHE6Ldm25Bv0yrhCUm Ktziod3pqJxVRjDIzFgskyxUyzuVDy7bRKO5JDqOseLh84yVfKFD1Xw1sNW5po/S 30uyUaj6xC04etUd/VAnE9PRiWYWBcGi3hZkVRzJZ8y0dAZQu68ymACP6/5tcT95 FNPhBcOEKB0zDS1kIi5Hv4AaVX9nLSlWlfSZxJJykN8pX564Z62ZZIU3lCq/gJYl sdp/dC5lmbJxlkk5TtPxS1KsLUk5akm4Ra/t0JbfHBnfqeys0tt8BiJWpcD+Xw1Z oJ/eqwIbuQw2RsszfcmqWlLJ9LsY8smh5j4PwqS6d0z+SkjQSOyKWlgKyLtb95X+ 02HlKWiWn9eGKBVHcox91l0aRvYY6Ho0n4It6tzlG/1HzH0uyyKM/vdWXmkFwCWe WV0R9jjJB1lKkniVi5FMLQjMGAoDtteNHTMUk+rDFA5tRxqCZiOPvgDarLe70ZaJ KkfgR9OGlZmjeXdmvAyyn3ckwSWLj2JhGu9bmwuOsRzNs+Ljhfickd7izDubPlqw 4VqTcZqSHLRgTjEF18Bt =gTtW -----END PGP SIGNATURE-----

July 14, 2016

Re: [PATCH 05/12] d3drm: Implement IDirect3DRMDevice*::InitFromD3D.

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Am 2016-07-14 um 00:33 schrieb Aaryaman Vasishta: > It still returns E_NOINTERFACE. This means that CreateObject does > keep track of which interface was requested within the object after > all. We could probably mimic this by using a version field within > the struct, which can be looked up and InitFromD3D would be handled > accordingly, similar to the version field in struct d3d_device. > This could potentially be used in other functions with similar > behavior on different versions as well. Don't worry too much about the InitFromD3D? corner case. Keep it in mind for debugging, but I doubt there's an application that tries to do this and depends on this thing to fail. But yeah, you may need some sort of version tagging for future things at some point. Ddraw has something similar. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXh96HAAoJEN0/YqbEcdMwAicP/0sYEw43mK4ghm5H7uea/QHw 9Uv1PY2WGRPWoPPe4YXFK12ZD2E3gzSuTLPGhJruezqhOWB8Bxp0WPFX5q8IYTaf X30PjDOeRBvPh5ptHdRddXPDxGo89GhAFd+dz2lt3t5z5FdVWKyY7opjQhWS6nJF nNMbRC1di9zCuCyo/UGG/ILXxMPD9VAnH7Fahy4xWcRZFLAxJH8thPaiGbin7Nwi InWVqiUjG44BYff8roYXylu+rUIVBYdp8FHw5gowdNWAyDkMbGhHUtctlTqgPCM6 Irc+UOqmsxel2ccGlKHk1zDFjGbsNYkrLORCLeAtjaAhL+IaLgbrq+eu84DX7OQA XTO/PS0RVUwL6o+rsB6AG6Wsqq+dAHEDq0adSru4IqDrWlWBEAy6F9mYt1Qms8np Qa9J+1jo6fc7Eq35vcs0uW/Ymd+EH7qs4KHljJRsxMM/OzTOOmYIrJeBOmEo+oNq eGOGxNDa++gihpcNV7JzsfGj9Ar+vmfwyIRg8REGTisHzylIhlw22nRhmzr5k2rK mqI2M6KXlpG1lAQOSqpPKhdLLdPUwcWiRu7d+RDNadV3ypBlGgqjS6YY1mCUdynR yjK9Fabvo3RQV7bRC42rygIdYDvO8Bpj4cctejfD0Y0iuCw5VlJFbRXerjKNS3c6 Skm0afo5Ovb6y0rWLH+M =tnTR -----END PGP SIGNATURE-----

July 14, 2016

Re: Testbot 3D tests broken

by Matteo Bruni

2016-07-14 8:50 GMT+02:00 Patrick Rudolph <siro(a)das-labor.org>: > On 2016-07-13 09:53 PM, Matteo Bruni wrote: >> 2016-07-12 17:51 GMT+02:00 Patrick Rudolph <siro(a)das-labor.org>: >>> Hi, >>> can you have a look at the testbot, it doesn't run d3d8 and d3d9 tests. >>> Other 3D tests might be affected, too. >>> As you can see here it reports no test failures, but it skipped all >>> tests: >>> https://testbot.winehq.org/JobDetails.pl?Key=24151 >>> https://testbot.winehq.org/JobDetails.pl?Key=24150 >> >> Not sure if you already got an answer on IRC or elsewhere. Anyway, the >> tests are actually executed on OS versions with the WARP device >> available, i.e. Win8 and newer. WARP device doesn't entirely match >> "usual" hardware devices WRT test results and since we really only >> care to match hardware behavior (but we still want the tests to pass >> on the testbot) there are a number of hacks in the tests. >> > Should I don't care about pre Win8 test results ? Certainly you should care, but you need to test yourself on real hardware, or ask someone to run the test for you. > Both, d3d8 and d3d9 games, are likely to be used with WinXp or Win7. > Does it make sense to test them on Win8 or newer ? > As likely every wine user is running wined3d on real hardware, shouldn't > there be test systems that have real GPUs ? Yep, but we can't have that right now with the testbot. I can't find a specific bug number but I'm pretty sure Francois knows about it either way.

July 14, 2016

Re: Testbot 3D tests broken

by Patrick Rudolph

On 2016-07-13 09:53 PM, Matteo Bruni wrote: > 2016-07-12 17:51 GMT+02:00 Patrick Rudolph <siro(a)das-labor.org>: >> Hi, >> can you have a look at the testbot, it doesn't run d3d8 and d3d9 tests. >> Other 3D tests might be affected, too. >> As you can see here it reports no test failures, but it skipped all >> tests: >> https://testbot.winehq.org/JobDetails.pl?Key=24151 >> https://testbot.winehq.org/JobDetails.pl?Key=24150 > > Not sure if you already got an answer on IRC or elsewhere. Anyway, the > tests are actually executed on OS versions with the WARP device > available, i.e. Win8 and newer. WARP device doesn't entirely match > "usual" hardware devices WRT test results and since we really only > care to match hardware behavior (but we still want the tests to pass > on the testbot) there are a number of hacks in the tests. > Should I don't care about pre Win8 test results ? Both, d3d8 and d3d9 games, are likely to be used with WinXp or Win7. Does it make sense to test them on Win8 or newer ? As likely every wine user is running wined3d on real hardware, shouldn't there be test systems that have real GPUs ? >> Regards, >> Patrick

July 14, 2016

Re: kernel32: Overhaul the handling of argv in set_process_name().

by Ken Thomases

On Jul 13, 2016, at 8:36 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > > On 14.07.2016 00:46, Ken Thomases wrote: >>> >>> Please correct me if I'm wrong, but to my knowledge, the "ugly" shift >>> string method which depends on the exact memory layout only makes sense >>> on Linux. On all other operating systems the cmdline is obtained in a >>> different way and it does not matter what is stored in the process. >> >> That's what I thought at first, too, but no. Shifting the strings is what affects the output of ps on macOS, too. The call to setprogname() affects some other process metadata query APIs (e.g. libproc), but I'm not aware of built-in commands which demonstrate their output. >> >> The setproctitle() docs I've found online suggest that modifying argv (at least, argv[0]) works on FreeBSD, too. It's just that setproctitle() is preferred. >> <https://www.freebsd.org/cgi/man.cgi?query=setproctitle&sektion=3> >> >> -Ken >> > > Okay, if its unclear what else we might break then its probably better to keep > it in the version you already have. Thanks for the explanation. ;) You're welcome. Thank you for reviewing. :) -Ken

July 13, 2016

Re: kernel32: Overhaul the handling of argv in set_process_name().

by Sebastian Lackner

On 14.07.2016 00:46, Ken Thomases wrote: >> >> Please correct me if I'm wrong, but to my knowledge, the "ugly" shift >> string method which depends on the exact memory layout only makes sense >> on Linux. On all other operating systems the cmdline is obtained in a >> different way and it does not matter what is stored in the process. > > That's what I thought at first, too, but no. Shifting the strings is what affects the output of ps on macOS, too. The call to setprogname() affects some other process metadata query APIs (e.g. libproc), but I'm not aware of built-in commands which demonstrate their output. > > The setproctitle() docs I've found online suggest that modifying argv (at least, argv[0]) works on FreeBSD, too. It's just that setproctitle() is preferred. > <https://www.freebsd.org/cgi/man.cgi?query=setproctitle&sektion=3> > > -Ken > Okay, if its unclear what else we might break then its probably better to keep it in the version you already have. Thanks for the explanation. ;) Regards, Sebastian

July 13, 2016

Re: [2/2] kernel32/tests: Add tests for GetThreadSelectorEntry.

by Sebastian Lackner

On 14.07.2016 00:26, Marvin wrote: > Hi, > > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? > Full results can be found at > https://testbot.winehq.org/JobDetails.pl?Key=24204 > > Your paranoid android. > > > === w1064 (32 bit thread) === > thread.c:1086: Test failed: got 0 with 5 (expected FALSE with ERROR_GEN_FAILURE) > This is an existing failure.

July 13, 2016

Re: [PATCH 05/12] d3drm: Implement IDirect3DRMDevice*::InitFromD3D.

by Aaryaman Vasishta

On Sun, Jul 10, 2016 at 9:10 PM, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote: > > > + hr = IDirect3DRMDevice2_InitFromD3D(device2, d3d1, d3ddevice1); > > + ok(hr == E_NOINTERFACE, "Expected hr == E_NOINTERFACE, got > %#x.\n", hr); > What happens if you QI IDirect3DRMDevice1 from IDirect3DRMDevice2 and call > InitFromD3D on it? I would expect it to succeed because common decency > suggests that CreateObject doesn't leave a history of which interface was > requested and there's no CLSID_CDirect3DRMDevice2 or > CLSID_CDirect3DRMDevice3. Still worth a check though I think. > It still returns E_NOINTERFACE. This means that CreateObject does keep track of which interface was requested within the object after all. We could probably mimic this by using a version field within the struct, which can be looked up and InitFromD3D would be handled accordingly, similar to the version field in struct d3d_device. This could potentially be used in other functions with similar behavior on different versions as well. Cheers, Aaryaman

July 13, 2016

Re: kernel32: Overhaul the handling of argv in set_process_name().

by Ken Thomases

On Jul 13, 2016, at 3:13 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > > On 13.07.2016 21:52, Ken Thomases wrote: >> On Jul 13, 2016, at 2:25 PM, Sebastian Lackner >> <sebastian(a)fds-team.de> wrote: >>> >>> On 13.07.2016 20:20, Ken Thomases wrote: >>> >>>> Moving the code to the bottom could be done, although it would >>>> mean two #ifdef HAVE_SETPROCTITLE blocks, one to set >>>> shift_strings and the other to actually make the call. The >>>> reason is that the shifting has to happen before the call to >>>> setprogname() because it's documented to keep the pointer that's >>>> passed in. We can't pass it (a substring of) argv[1] and then >>>> shift the strings. >>> >>> I'm not sure if two #ifdefs is actually that bad, it probably >>> should depend on the operating system anyway. >> >> By "it" you mean the decision to shift the strings? Why should that >> depend on the OS? What reliable way do we have to detect if the OS >> works that way other than the strings actually being contiguous? > > Please correct me if I'm wrong, but to my knowledge, the "ugly" shift > string method which depends on the exact memory layout only makes sense > on Linux. On all other operating systems the cmdline is obtained in a > different way and it does not matter what is stored in the process. That's what I thought at first, too, but no. Shifting the strings is what affects the output of ps on macOS, too. The call to setprogname() affects some other process metadata query APIs (e.g. libproc), but I'm not aware of built-in commands which demonstrate their output. The setproctitle() docs I've found online suggest that modifying argv (at least, argv[0]) works on FreeBSD, too. It's just that setproctitle() is preferred. <https://www.freebsd.org/cgi/man.cgi?query=setproctitle&sektion=3> -Ken

July 13, 2016

Re: [2/2] kernel32/tests: Add tests for GetThreadSelectorEntry.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24204 Your paranoid android. === w1064 (32 bit thread) === thread.c:1086: Test failed: got 0 with 5 (expected FALSE with ERROR_GEN_FAILURE)

July 13, 2016

Re: kernel32: Overhaul the handling of argv in set_process_name().

by Sebastian Lackner

On 13.07.2016 21:52, Ken Thomases wrote: > On Jul 13, 2016, at 2:25 PM, Sebastian Lackner > <sebastian(a)fds-team.de> wrote: >> >> On 13.07.2016 20:20, Ken Thomases wrote: >> >>> Moving the code to the bottom could be done, although it would >>> mean two #ifdef HAVE_SETPROCTITLE blocks, one to set >>> shift_strings and the other to actually make the call. The >>> reason is that the shifting has to happen before the call to >>> setprogname() because it's documented to keep the pointer that's >>> passed in. We can't pass it (a substring of) argv[1] and then >>> shift the strings. >> >> I'm not sure if two #ifdefs is actually that bad, it probably >> should depend on the operating system anyway. > > By "it" you mean the decision to shift the strings? Why should that > depend on the OS? What reliable way do we have to detect if the OS > works that way other than the strings actually being contiguous? Please correct me if I'm wrong, but to my knowledge, the "ugly" shift string method which depends on the exact memory layout only makes sense on Linux. On all other operating systems the cmdline is obtained in a different way and it does not matter what is stored in the process. > >> On systems where it doesn't make a difference (like FreeBSD, ...) >> this step can always be safely skipped. > > By "systems where it doesn't make a difference", you mean those that > have setproctitle()? By "this step" you mean shifting the strings? > That's what my patch already does. > What I mean is that checking the operating system might be better than checking if specific functions are available. To explain it with some pseudo code, I was thinking about something like this: --- snip --- #ifdef __linux__ // shift_strings check if (shift_strings) { // ... } else #endif { // fallback method } #ifdef HAVE_SETPROCTITLE // ... #endif #ifdef HAVE_SETPROGNAME // ... #endif #ifdef HAVE_PRCTL // ... #endif --- snip --- Regards, Sebastian

July 13, 2016

Re: kernel32: Overhaul the handling of argv in set_process_name().

by Ken Thomases

On Jul 13, 2016, at 2:37 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > > On 13.07.2016 21:25, Sebastian Lackner wrote: >> On 13.07.2016 20:20, Ken Thomases wrote: >>>>> However, setproctitle() is specifically documented as a preferred >>>>> alternative to the technique of overwriting the arg strings, so >>>>> don't shift the strings if that's available. >>>> >>>> Wouldn't it make sense to remove the directory component also in >>>> this case, to be consistent? By moving the code to the bottom all >>>> functions would be called in the same way. >>> >>> Well, the only reason we remove the directory component for the other >>> calls is because they only allow limited storage. I'm not aware of a >>> similar limit for setproctitle(). >>> >>> Also, setproctitle() is theoretically more similar to the shifting of >>> the argument strings than to setprogname() or prctl(PR_SET_NAME). >>> And we don't make any attempt to remove the directory component for >>> the string shifting. (I.e. "ps" will show the full command line, >>> potentially including Windows-style .exe path.) >> >> Well, its still very different compared to shifting because all the >> following arguments are stripped away. I did some quick tests and it >> seems like there are also methods which do preserve the additional >> arguments in the "ps" output: >> >> * Either a direct sysctl invocation as done in the setproctitle() >> implementation itself. >> >> * Or, first shift the arguments (both methods work), and then call: >> setproctitle("some string"); >> setproctitle(NULL); >> The second call restores the process title as seen by the app. > > Actually it looks like we can get the same result just by concatenating > all arguments on the Wine side. The separation of the arguments is lost > in the "ps" output anyway. Depending on how you want to proceed I'll > probably send a patch after you are done with the cleanup. ;) I'm happy to have my current patch committed and you can submit a patch to pass the concatenation of the args to setproctitle() if you like. :) -Ken

July 13, 2016

Re: Testbot 3D tests broken

by Matteo Bruni

2016-07-12 17:51 GMT+02:00 Patrick Rudolph <siro(a)das-labor.org>: > Hi, > can you have a look at the testbot, it doesn't run d3d8 and d3d9 tests. > Other 3D tests might be affected, too. > As you can see here it reports no test failures, but it skipped all > tests: > https://testbot.winehq.org/JobDetails.pl?Key=24151 > https://testbot.winehq.org/JobDetails.pl?Key=24150 Not sure if you already got an answer on IRC or elsewhere. Anyway, the tests are actually executed on OS versions with the WARP device available, i.e. Win8 and newer. WARP device doesn't entirely match "usual" hardware devices WRT test results and since we really only care to match hardware behavior (but we still want the tests to pass on the testbot) there are a number of hacks in the tests. > Regards, > Patrick

July 13, 2016

Re: kernel32: Overhaul the handling of argv in set_process_name().

by Ken Thomases

On Jul 13, 2016, at 2:25 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > > On 13.07.2016 20:20, Ken Thomases wrote: > >> Moving the code to the bottom could be done, although it would mean >> two #ifdef HAVE_SETPROCTITLE blocks, one to set shift_strings and the >> other to actually make the call. The reason is that the shifting has >> to happen before the call to setprogname() because it's documented to >> keep the pointer that's passed in. We can't pass it (a substring of) >> argv[1] and then shift the strings. > > I'm not sure if two #ifdefs is actually that bad, it probably should > depend on the operating system anyway. By "it" you mean the decision to shift the strings? Why should that depend on the OS? What reliable way do we have to detect if the OS works that way other than the strings actually being contiguous? > On systems where it doesn't make > a difference (like FreeBSD, ...) this step can always be safely skipped. By "systems where it doesn't make a difference", you mean those that have setproctitle()? By "this step" you mean shifting the strings? That's what my patch already does. In other words, I don't understand what you're getting at with this paragraph. -Ken

July 13, 2016

Re: [PATCH v2 2/3] winex11.drv: avoid passing NULL to memcpy (clang)

by Matteo Bruni

2016-07-09 8:42 GMT+02:00 Alexandre Julliard <julliard(a)winehq.org>: > Kirill Smirnov <kirill.k.smirnov(a)gmail.com> writes: > >> According to the standard, memcpy() and its friends >> should never accept NULL as argument, otherwise >> the behavior is undefined and optimizer takes this >> into account. >> >> Below in the same file (line 206) caller passes >> NULL as lpComp argument triggering this case. >> >> This patch is of the same nature as the following: >> >> commit 5a3ad0ecf08c00fc0b7b2f24e55be27b7c3f5da0 >> Author: Kirill Smirnov <kirill.k.smirnov(a)gmail.com> >> Date: Mon May 9 18:56:14 2016 +0300 >> >> winhlp32: Do not pass NULL to strchr() (spotted by clang). > > strchr has to access the pointer so obviously it can't take NULL, but > memcpy with a zero length shouldn't, so it's not very useful compiler > behavior IMO. Sadly it seems the standard does allow it... Yeah and gcc has also been exploiting that for more aggressive optimizations for a while (see https://gcc.gnu.org/gcc-4.9/porting_to.html) deb274226783ab886bdb44876944e156757efe2b was probably the one significant precedent for Wine.

July 13, 2016

Re: [PATCH 1/2] include: Fix include arguments.

by Matteo Bruni

This probably makes sense (although, WRT the patch title, this is probably more of a consistency improvement rather than a "fix"). Maybe split the d3dtypes.h change to a separate patch?

July 13, 2016

Re: include: Add d3dx10math.inl.

by Matteo Bruni

2016-07-04 19:26 GMT+02:00 Andrey Gusev <andrey.goosev(a)gmail.com>: +#ifndef __D3DX10MATH_INL__ +#define __D3DX10MATH_INL__ My copy of the file (from the latest DX SDK) uses __D3DXMATH_INL__ as header guard. I think we should do the same for compatibility. +/* constructors & operators */ Nitpicking but this comment doesn't seem particularly useful. +inline D3DXVECTOR2::D3DXVECTOR2(const FLOAT *pf) Just use plain float. + if(!pf) return; Space before '(', return on a separate line. +inline D3DXVECTOR2::operator FLOAT* () +{ + return (FLOAT*)&x; Please put a space between "float" and '*' in the cast. Probably do the same in the operator definition, although I don't really care in that regard. +inline D3DXMATRIX D3DXMATRIX::operator - () const +{ + return D3DXMATRIX(-_11, -_12, -_13, -_14, + -_21, -_22, -_23, -_24, + -_31, -_32, -_33, -_34, + -_41, -_42, -_43, -_44); +} This is almost identical to the MS's header. Please use 8 space indentation for line continuations. Probably drop the space between the "operator" keyword and the operator and between the operator and the opening parenthesis, in the operator declaration. +inline D3DXCOLOR::operator UINT () const +{ + UINT _r = r >= 1.0f ? 0xff : r <= 0.0f ? 0x00 : (UINT)(r * 255.0f + 0.5f); + UINT _g = g >= 1.0f ? 0xff : g <= 0.0f ? 0x00 : (UINT)(g * 255.0f + 0.5f); + UINT _b = b >= 1.0f ? 0xff : b <= 0.0f ? 0x00 : (UINT)(b * 255.0f + 0.5f); + UINT _a = a >= 1.0f ? 0xff : a <= 0.0f ? 0x00 : (UINT)(a * 255.0f + 0.5f); + + return (_a << 24) | (_r << 16) | (_g << 8) | (_b << 0); +} This one is also virtually identical. There seems to be room for writing a slightly different but equivalent implementation. FWIW identifiers starting with an underscore are supposed to be reserved, to some degree. It's generally better to avoid them. +inline D3DXFLOAT16::D3DXFLOAT16() +{ +} These default constructors are in (native) d3dx10math.h. I know that with d3dx9* headers we did put those in the .inl file but I think that was a bad idea. +inline BOOL D3DXFLOAT16::operator == (const D3DXFLOAT16 &f) const +{ This one is also almost identical, implementation-wise. Now maybe there isn't that much room for variations here but still, at the very least replace the "== 0" with a '!'. If you can find other reasonable ways to differentiate, that's even better. +inline D3DXCOLOR::D3DXCOLOR(UINT col) +{ + const FLOAT f = 1.0f / 255.0f; + r = f * (FLOAT)(unsigned char)(col >> 16); + g = f * (FLOAT)(unsigned char)(col >> 8); + b = f * (FLOAT)(unsigned char)(col >> 0); + a = f * (FLOAT)(unsigned char)(col >> 24); +} Another one essentially identical. I would write it as something like: inline D3DXCOLOR::D3DXCOLOR(UINT col) { float inv = 1.0f / 255.0f; a = ((col >> 24) & 0xff) * inv; r = ((col >> 16) & 0xff) * inv; g = ((col >> 8) & 0xff) * inv; b = (col & 0xff) * inv; } +static inline D3DXCOLOR* D3DXColorLerp(D3DXCOLOR *pout, const D3DXCOLOR *pc1, const D3DXCOLOR *pc2, FLOAT s) Don't use those prefixed argument names: out, c1 and c2 work just fine. Also you want a space between D3DXCOLOR and '*' and no space between '*' and the function name. +{ + if ( !pout || !pc1 || !pc2 ) return NULL; Please no spaces after the '(' and before the ')'. Return on a separate line. + pout->r = (1-s) * (pc1->r) + s *(pc2->r); You want spaces around the '-' operator and a space after the multiplication operator. No need for the parentheses around pc1->r and pc2->r. Regardless, MS's header uses the "optimized" linear interpolation formula, with just one multiplication. Your current implementation will generally give slightly different results (because of the limited precision of floats) so you should stick to the same formula instead. This isn't an exhaustive list of issues. Many of the above comments apply to multiple places over the patch.

July 13, 2016

Re: kernel32: Overhaul the handling of argv in set_process_name().

by Sebastian Lackner

On 13.07.2016 21:25, Sebastian Lackner wrote: > On 13.07.2016 20:20, Ken Thomases wrote: >>>> However, setproctitle() is specifically documented as a preferred >>>> alternative to the technique of overwriting the arg strings, so >>>> don't shift the strings if that's available. >>> >>> Wouldn't it make sense to remove the directory component also in >>> this case, to be consistent? By moving the code to the bottom all >>> functions would be called in the same way. >> >> Well, the only reason we remove the directory component for the other >> calls is because they only allow limited storage. I'm not aware of a >> similar limit for setproctitle(). >> >> Also, setproctitle() is theoretically more similar to the shifting of >> the argument strings than to setprogname() or prctl(PR_SET_NAME). >> And we don't make any attempt to remove the directory component for >> the string shifting. (I.e. "ps" will show the full command line, >> potentially including Windows-style .exe path.) > > Well, its still very different compared to shifting because all the > following arguments are stripped away. I did some quick tests and it > seems like there are also methods which do preserve the additional > arguments in the "ps" output: > > * Either a direct sysctl invocation as done in the setproctitle() > implementation itself. > > * Or, first shift the arguments (both methods work), and then call: > setproctitle("some string"); > setproctitle(NULL); > The second call restores the process title as seen by the app. Actually it looks like we can get the same result just by concatenating all arguments on the Wine side. The separation of the arguments is lost in the "ps" output anyway. Depending on how you want to proceed I'll probably send a patch after you are done with the cleanup. ;)

July 13, 2016

Re: kernel32: Overhaul the handling of argv in set_process_name().

by Sebastian Lackner

On 13.07.2016 20:20, Ken Thomases wrote: >>> However, setproctitle() is specifically documented as a preferred >>> alternative to the technique of overwriting the arg strings, so >>> don't shift the strings if that's available. >> >> Wouldn't it make sense to remove the directory component also in >> this case, to be consistent? By moving the code to the bottom all >> functions would be called in the same way. > > Well, the only reason we remove the directory component for the other > calls is because they only allow limited storage. I'm not aware of a > similar limit for setproctitle(). > > Also, setproctitle() is theoretically more similar to the shifting of > the argument strings than to setprogname() or prctl(PR_SET_NAME). > And we don't make any attempt to remove the directory component for > the string shifting. (I.e. "ps" will show the full command line, > potentially including Windows-style .exe path.) Well, its still very different compared to shifting because all the following arguments are stripped away. I did some quick tests and it seems like there are also methods which do preserve the additional arguments in the "ps" output: * Either a direct sysctl invocation as done in the setproctitle() implementation itself. * Or, first shift the arguments (both methods work), and then call: setproctitle("some string"); setproctitle(NULL); The second call restores the process title as seen by the app. > > Moving the code to the bottom could be done, although it would mean > two #ifdef HAVE_SETPROCTITLE blocks, one to set shift_strings and the > other to actually make the call. The reason is that the shifting has > to happen before the call to setprogname() because it's documented to > keep the pointer that's passed in. We can't pass it (a substring of) > argv[1] and then shift the strings. I'm not sure if two #ifdefs is actually that bad, it probably should depend on the operating system anyway. On systems where it doesn't make a difference (like FreeBSD, ...) this step can always be safely skipped. > > -Ken >

July 13, 2016

Re: [PATCH 4/5] d3d11/tests: Add test for writing special floating-point values to render target.

by Henri Verbeet

On 13 July 2016 at 10:52, Józef Kucia <jkucia(a)codeweavers.com> wrote: > +#ifndef NAN > +static inline float __port_nan(void) > +{ > + static const unsigned __nan_bytes = 0x7fc00000; > + return *(const float *)&__nan_bytes; > +} > +#define NAN __port_nan() > +#endif This looks unused. > + const struct vec4 *value = get_readback_vec4(&rb, x, y); > + ok(isnan(value->x) && value->y == INFINITY && value->z == -INFINITY && value->w == 1.0f, > + "Got unexpected value {%.8e, %.8e, %.8e, %.8e} at (%u, %u).\n", > + value->x, value->y, value->z, value->w, x, y); It would slightly change the meaning of the test, but you could also just read these back as uvec4's and compare the bit patterns. > @@ -9312,4 +9400,5 @@ START_TEST(d3d11) > test_check_feature_support(); > test_create_unordered_access_view(); > test_immediate_constant_buffer(); > + test_special_float_values(); For what it's worth, those are usually called "floating-point specials". (And hence fp_special_test() in the d3d9 tests.)

July 13, 2016

Re: kernel32: Overhaul the handling of argv in set_process_name().

by Ken Thomases

On Jul 12, 2016, at 2:45 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > > On 12.07.2016 06:44, Ken Thomases wrote: >> This fixes several problems with the code: >> >> * The code had been assuming that the argument strings pointed to by the argv >> array are contiguous iff certain process-name-setting functions are available. >> This doesn't seem reliable. Instead, test if it's true and shift the strings >> if so. > > Thanks for working on this, looks indeed like a nice improvement. You're welcome and thanks. :) >> However, setproctitle() is specifically documented as a preferred alternative >> to the technique of overwriting the arg strings, so don't shift the strings >> if that's available. > > Wouldn't it make sense to remove the directory component also in this case, to > be consistent? By moving the code to the bottom all functions would be called > in the same way. Well, the only reason we remove the directory component for the other calls is because they only allow limited storage. I'm not aware of a similar limit for setproctitle(). Also, setproctitle() is theoretically more similar to the shifting of the argument strings than to setprogname() or prctl(PR_SET_NAME). And we don't make any attempt to remove the directory component for the string shifting. (I.e. "ps" will show the full command line, potentially including Windows-style .exe path.) Moving the code to the bottom could be done, although it would mean two #ifdef HAVE_SETPROCTITLE blocks, one to set shift_strings and the other to actually make the call. The reason is that the shifting has to happen before the call to setprogname() because it's documented to keep the pointer that's passed in. We can't pass it (a substring of) argv[1] and then shift the strings. -Ken

July 13, 2016

Re: ntdll/tests: Use inline implementation of NtCurrentTeb for exception tests.

by Huw Davies

On Wed, Jul 13, 2016 at 11:30:07AM +0200, Sebastian Lackner wrote: > On 13.07.2016 09:50, Marvin wrote: > > === wvistau64 (32 bit exception) === > > exception.c:489: Test failed: 42: Unexpected eip 0x3d0000/0x3d0001 > > exception: unhandled exception 80000004 at 003D0000 > > exception.c:489: Test failed: 42: Unexpected eip 0x77baed97/0x3d0001 > > > > === w7u (32 bit exception) === > > exception.c:489: Test failed: 42: Unexpected eip 0x280000/0x280001 > > exception: unhandled exception 80000004 at 00280000 > > exception.c:489: Test failed: 42: Unexpected eip 0x77cde34f/0x280001 > > These failures were already existing before and are unrelated to this patch. Indeed. <cough> vm1 kernel upgrade <cough> ;-) Huw.

July 13, 2016

Re: ntdll/tests: Use inline implementation of NtCurrentTeb for exception tests.

by Sebastian Lackner

On 13.07.2016 09:50, Marvin wrote: > Hi, > > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? > Full results can be found at > https://testbot.winehq.org/JobDetails.pl?Key=24168 > > Your paranoid android. > > > === wvistau64 (32 bit exception) === > exception.c:489: Test failed: 42: Unexpected eip 0x3d0000/0x3d0001 > exception: unhandled exception 80000004 at 003D0000 > exception.c:489: Test failed: 42: Unexpected eip 0x77baed97/0x3d0001 > > === w7u (32 bit exception) === > exception.c:489: Test failed: 42: Unexpected eip 0x280000/0x280001 > exception: unhandled exception 80000004 at 00280000 > exception.c:489: Test failed: 42: Unexpected eip 0x77cde34f/0x280001 > > === w1064 (32 bit exception) === > An error occurred while waiting for the test to complete: network read timed out (wait2/connect:AgentVersion.h:0/9) > The test VM has crashed, rebooted or lost connectivity (or the TestAgent server died) > > === w1064 (64 bit exception) === > An error occurred while waiting for the test to complete: network read timed out (wait2/connect:AgentVersion.h:0/9) > The test VM has crashed, rebooted or lost connectivity (or the TestAgent server died) > These failures were already existing before and are unrelated to this patch.

July 13, 2016

Re: ntdll/tests: Use inline implementation of NtCurrentTeb for exception tests.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24168 Your paranoid android. === wvistau64 (32 bit exception) === exception.c:489: Test failed: 42: Unexpected eip 0x3d0000/0x3d0001 exception: unhandled exception 80000004 at 003D0000 exception.c:489: Test failed: 42: Unexpected eip 0x77baed97/0x3d0001 === w7u (32 bit exception) === exception.c:489: Test failed: 42: Unexpected eip 0x280000/0x280001 exception: unhandled exception 80000004 at 00280000 exception.c:489: Test failed: 42: Unexpected eip 0x77cde34f/0x280001 === w1064 (32 bit exception) === An error occurred while waiting for the test to complete: network read timed out (wait2/connect:AgentVersion.h:0/9) The test VM has crashed, rebooted or lost connectivity (or the TestAgent server died) === w1064 (64 bit exception) === An error occurred while waiting for the test to complete: network read timed out (wait2/connect:AgentVersion.h:0/9) The test VM has crashed, rebooted or lost connectivity (or the TestAgent server died)

July 13, 2016

Re: [PATCH] wined3d: Fix the size parameter of glVertexAttrib*Pointer().

by Józef Kucia

On Wed, Jul 13, 2016 at 9:01 AM, Guillaume Charifi <guillaume.charifi(a)sfr.fr> wrote: > Yes, you're right, don't worry, only stupid local changes with unexpected > side-effects. > But anyway, shouldn't it be more explicit to use component_count in this > case ? I guess that "gl_vtx_format" might be a little misleading name.

July 13, 2016

Re: [PATCH] wined3d: Fix the size parameter of glVertexAttrib*Pointer().

by Guillaume Charifi

Yes, you're right, don't worry, only stupid local changes with unexpected side-effects. But anyway, shouldn't it be more explicit to use component_count in this case ? Le 13/07/2016 08:28, Józef Kucia a écrit : > On Wed, Jul 13, 2016 at 2:48 AM, Guillaume Charifi > <guillaume.charifi(a)sfr.fr> wrote: >> The mistake is obvious when looking at the GL 3.0 spec: >> p29: >> void VertexAttribPointer(uint index, int size, enum type, boolean normalized, sizei stride, const void *pointer); >> void VertexAttribIPointer(uint index, int size, enum type, sizei stride, const void *pointer); >> >> p30: >> Command Sizes >> VertexAttribPointer 1, 2, 3, 4 >> VertexAttribIPointer 1, 2, 3, 4 >> >> Signed-off-by: Guillaume Charifi <guillaume.charifi(a)sfr.fr> >> --- >> dlls/wined3d/state.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) > This looks wrong to me. The "gl_vtx_format" field is equal to the > "component_count" field. There's one exception though, the > "gl_vtx_format" is set to GL_BGRA for WINED3DFMT_B8G8R8A8_UNORM. > GL_BGRA was added as an accepted value by GL_ARB_vertex_array_bgra > (core since OpenGL 3.2). > > What are you trying to fix?

July 13, 2016

Re: [PATCH] wined3d: Fix the size parameter of glVertexAttrib*Pointer().

by Józef Kucia

On Wed, Jul 13, 2016 at 2:48 AM, Guillaume Charifi <guillaume.charifi(a)sfr.fr> wrote: > The mistake is obvious when looking at the GL 3.0 spec: > p29: > void VertexAttribPointer(uint index, int size, enum type, boolean normalized, sizei stride, const void *pointer); > void VertexAttribIPointer(uint index, int size, enum type, sizei stride, const void *pointer); > > p30: > Command Sizes > VertexAttribPointer 1, 2, 3, 4 > VertexAttribIPointer 1, 2, 3, 4 > > Signed-off-by: Guillaume Charifi <guillaume.charifi(a)sfr.fr> > --- > dlls/wined3d/state.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) This looks wrong to me. The "gl_vtx_format" field is equal to the "component_count" field. There's one exception though, the "gl_vtx_format" is set to GL_BGRA for WINED3DFMT_B8G8R8A8_UNORM. GL_BGRA was added as an accepted value by GL_ARB_vertex_array_bgra (core since OpenGL 3.2). What are you trying to fix?

July 13, 2016

Re: ncrypt: add some ncrypt stubs

by Sebastian Lackner

On 12.07.2016 17:47, Alex Henrie wrote: > + return NTE_INVALID_PARAMETER; > > I think NTE_NOT_SUPPORTED makes more sense for stub functions in this > DLL. Other than that, the patch looks good. > > -Alex > > I also agree that a different error code might be better, however even more important it does not compile here because there are semicolons missing in the header file. It probably would also make sense to send the header file changes in separate patches.

July 12, 2016

Re: kernel32: Overhaul the handling of argv in set_process_name().

by Sebastian Lackner

On 12.07.2016 06:44, Ken Thomases wrote: > This fixes several problems with the code: > > * The code had been assuming that the argument strings pointed to by the argv > array are contiguous iff certain process-name-setting functions are available. > This doesn't seem reliable. Instead, test if it's true and shift the strings > if so. Thanks for working on this, looks indeed like a nice improvement. > > However, setproctitle() is specifically documented as a preferred alternative > to the technique of overwriting the arg strings, so don't shift the strings > if that's available. Wouldn't it make sense to remove the directory component also in this case, to be consistent? By moving the code to the bottom all functions would be called in the same way. > > * Use the last path component, recognizing backslash as a path separator, for > setprogname() in addition to prctl(). First, setprogname() is documented as > searching for the last component itself, but it doesn't understand Windows- > style paths, so we need to help it. Second, on some platforms (e.g. macOS), > setprogname(), like prctl(), has a fairly small internal length limit (e.g. > 32 characters). So, concentrate on the most meaningful part of the path. > > * Remove argv[0] from argv whether or not there are any process-name-setting > functions available. This is necessary for the proper functioning of Wine, > so it must be done on all platforms. This part of the logic was lost with > commit 5a4576ee0. > > * Call all available process-name-setting functions instead of treating them > as mutually exclusive alternatives. This is also logic that was lost with > commit 5a4576ee0. > > Signed-off-by: Ken Thomases <ken(a)codeweavers.com> > --- > dlls/kernel32/process.c | 63 +++++++++++++++++++++++++++++-------------------- > 1 file changed, 37 insertions(+), 26 deletions(-) >

July 12, 2016

Re: Ubuntu considering dropping 32-bit libraries

by Josh DuBois

On 7/6/16 10:47 AM, Josh DuBois wrote: > I will probably post to the Ubuntu thread to at least mention Wine, > unless someone here is involved in that community and thinks they can > do a better job. Fwiw I did post to the Ubuntu list asking them to take Wine into consideration with respect to building 32-bit libraries. I got an encouraging response: https://lists.ubuntu.com/archives/ubuntu-devel/2016-July/039452.html They emphasized that they're not even considering changes to the availability of 32-bit libraries until 18.10, which is a couple of years out. Also it sounds like they had Wine in mind to some degree already, even though it wasn't mentioned in the thread. They did mention asking users to run Wine in a Snap container, and also floated a vague idea that some kind of changes to Wine might make the issue easier for them. I don't know how either of those things would go (in terms of changes to Wine, there was no specific proposal, so neither do they). But, at least for now, things seem positive in that they're not taking any immediate steps which would break things and they haven't forgotten about us :-)

July 12, 2016

Testbot 3D tests broken

by Patrick Rudolph

Hi, can you have a look at the testbot, it doesn't run d3d8 and d3d9 tests. Other 3D tests might be affected, too. As you can see here it reports no test failures, but it skipped all tests: https://testbot.winehq.org/JobDetails.pl?Key=24151 https://testbot.winehq.org/JobDetails.pl?Key=24150 Regards, Patrick

July 12, 2016

Re: ncrypt: add some ncrypt stubs

by Alex Henrie

+ return NTE_INVALID_PARAMETER; I think NTE_NOT_SUPPORTED makes more sense for stub functions in this DLL. Other than that, the patch looks good. -Alex

July 12, 2016

Re: [PATCH 3/3] wined3d/resource: Performance improvements, try 2

by Henri Verbeet

This could do with a more descriptive subject line. On 11 July 2016 at 18:13, Patrick Rudolph <siro(a)das-labor.org> wrote: > Tests showed that D3DLOCK_NOOVERWRITE is prefered over D3DLOCK_DISCARD > and that D3DLOCK_NOOVERWRITE doesn't depend on dynamic buffers. "preferred", but you'll probably want to say "takes precedence over" instead. > Unigine Heavon maps a lot of buffers with both flags, but run on about > 85% GPU-load, as WINE removes both hints and the game has to wait for > the buffer to be mapped. "Unigine Heaven", "Wine" > OpenGL core profile version string: 3.3 (Core Profile) Mesa 12.1.0-devel (git-60c68c5) It doesn't make much of a difference here, but note that unless you set the "MaxVersionGL" registry key, wined3d doesn't use core profiles. > --- > dlls/d3d9/tests/visual.c | 10 +++++----- > dlls/wined3d/resource.c | 33 ++++++++++++++++----------------- > 2 files changed, 21 insertions(+), 22 deletions(-) What about the d3d8 tests? > @@ -359,29 +359,28 @@ DWORD wined3d_resource_sanitize_map_flags(const struct wined3d_resource *resourc > { > /* Not all flags make sense together, but Windows never returns an error. > * Catch the cases that could cause issues. */ > - if (flags & WINED3D_MAP_READONLY) > + if ((flags & (WINED3D_MAP_DISCARD | WINED3D_MAP_NOOVERWRITE)) == > + (WINED3D_MAP_DISCARD | WINED3D_MAP_NOOVERWRITE)) > { > - if (flags & WINED3D_MAP_DISCARD) > - { > - WARN("WINED3D_MAP_READONLY combined with WINED3D_MAP_DISCARD, ignoring flags.\n"); > - return 0; > - } > - if (flags & WINED3D_MAP_NOOVERWRITE) > - { > - WARN("WINED3D_MAP_READONLY combined with WINED3D_MAP_NOOVERWRITE, ignoring flags.\n"); > - return 0; > - } > + WARN("WINED3D_MAP_DISCARD and WINED3D_MAP_NOOVERWRITE used together, using WINED3D_MAP_NOOVERWRITE.\n"); > + flags &= ~WINED3D_MAP_DISCARD; > + } > + if ((flags & (WINED3D_MAP_READONLY | WINED3D_MAP_NOOVERWRITE)) == > + (WINED3D_MAP_READONLY | WINED3D_MAP_NOOVERWRITE)) > + { > + WARN("WINED3D_MAP_READONLY and WINED3D_MAP_NOOVERWRITE used together, clearing WINED3D_MAP_READONLY.\n"); > + flags &= ~WINED3D_MAP_READONLY; > } > - else if ((flags & (WINED3D_MAP_DISCARD | WINED3D_MAP_NOOVERWRITE)) > - == (WINED3D_MAP_DISCARD | WINED3D_MAP_NOOVERWRITE)) > + > + if ((flags & (WINED3D_MAP_READONLY | WINED3D_MAP_DISCARD)) == > + (WINED3D_MAP_READONLY | WINED3D_MAP_DISCARD)) > { > - WARN("WINED3D_MAP_DISCARD and WINED3D_MAP_NOOVERWRITE used together, ignoring.\n"); > + WARN("WINED3D_MAP_READONLY combined with WINED3D_MAP_DISCARD, ignoring flags.\n"); > return 0; > } > - else if (flags & (WINED3D_MAP_DISCARD | WINED3D_MAP_NOOVERWRITE) > - && !(resource->usage & WINED3DUSAGE_DYNAMIC)) > + else if ((flags & WINED3D_MAP_DISCARD) && !(resource->usage & WINED3DUSAGE_DYNAMIC)) > { > - WARN("DISCARD or NOOVERWRITE map on non-dynamic buffer, ignoring.\n"); > + WARN("DISCARD on non dynamic buffer, ignoring.\n"); > return 0; > } This changes several things at once. One (logical) change per commit, please.

July 12, 2016

Re: [PATCH 1/3] d3d9/tests/visual: Add Vertexbuffer locking tests, try 2

by Henri Verbeet

On 11 July 2016 at 18:13, Patrick Rudolph <siro(a)das-labor.org> wrote: > +static void test_vb_lock_flags(void) > +{ > + IDirect3DVertexBuffer9 *buffer; > + IDirect3DDevice9 *device; > + IDirect3D9 *d3d9; > + DWORD i, j, tri, quads; > + HWND window; > + HRESULT hr; > + ULONG size; > + unsigned refcount; "ULONG refcount;", but that's minor. > + { > + "0", > + 0, > + 0, > + FALSE, > + FALSE > + }, This may have been inspired by other tests, but as long as it fits I'd prefer the more compact option of keeping this all on a single line. You may have to shorten the "flags_s" values a bit to do that. > + static struct > + { > + struct > + { > + struct vec3 position; > + DWORD diffuse; > + } strip[4]; > + } > + *quad_strip_array, quad_strip1 = > + { > + { > + {{-1.0f, -1.0f, 0.0f}, 0xffff0000}, > + {{-1.0f, 1.0f, 0.0f}, 0xff00ff00}, > + {{ 1.0f, -1.0f, 0.0f}, 0xff0000ff}, > + {{ 1.0f, 1.0f, 0.0f}, 0xffffffff}, > + } > + }, > + quad_strip2 = > + { > + { > + {{-1.0f, -1.0f, 0.0f}, 0xffffff00}, > + {{-1.0f, 1.0f, 0.0f}, 0xffffff00}, > + {{ 1.0f, -1.0f, 0.0f}, 0xffffff00}, > + {{ 1.0f, 1.0f, 0.0f}, 0xffffff00}, > + } > + }; These are triangle strips or quads, but I don't think "quad strips" makes sense here. "quad_strip1" and "quad_strip2" can be const, and "quad_strip_array" doesn't need to be static, right? > + hr = IDirect3DDevice9_CreateVertexBuffer(device, size, tests[i].usage, 0, D3DPOOL_DEFAULT, &buffer, NULL); > + ok(SUCCEEDED(hr), "Failed to create vertex buffer, hr %#x.\n", hr); > + if (!SUCCEEDED(hr)) > + continue; The continue is pointless, the test would have already failed at that point. > + if (tests[i].unsyncronized == unsyncronized) > + break; This means the test will only try the smallest buffer size for cases that you expect to be synchronised, right? That seems problematic, since the reason you try the larger buffer sizes is that you can't necessarily reliably detect the unsynchronised cases with the smaller buffer sizes. > + if (tests[i].todo) > + { > + todo_wine ok(tests[i].unsyncronized == unsyncronized, "Expected buffer mapped %s. Flags = %s. Usage = %d.\n", > + tests[i].unsyncronized ? "unsyncronized" : "syncronized", tests[i].flags_s, tests[i].usage); > + } > + else > + { > + ok(tests[i].unsyncronized == unsyncronized, "Expected buffer mapped %s. Flags = %s. Usage = %d.\n", > + tests[i].unsyncronized ? "unsyncronized" : "syncronized", tests[i].flags_s, tests[i].usage); > + } I think "todo_wine_if" would be appropriate here. Please use %#x to print the usage. Note that ddraw has the mostly equivalent flags DDLOCK_NOOVERWRITE and DDLOCK_DISCARDCONTENTS as well. Direct3D 10 has D3D10_MAP_WRITE_DISCARD and D3D10_MAP_WRITE_NO_OVERWRITE, but you can't use them at the same time because they're not flags. Direct3D 11 is similar to Direct3D 10. You'll need to account for the testbot failures in some way. They may be specific to WARP, but it's suspicious that the "D3DLOCK_NOOVERWRITE" tests do seem to pass. Also, please follow the usual conventions for the subject line. (I.e., "d3d9/tests:", "d3d8/tests:", "wined3d:".)

July 12, 2016

Re: [PATCH] dxgi: Implement IDXGISwapChain GetFullscreenState

by Henri Verbeet

On 11 July 2016 at 12:08, Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> wrote: > +static HRESULT STDMETHODCALLTYPE dxgi_swapchain_GetFullscreenState(IDXGISwapChain *iface, > + BOOL *fullscreen, IDXGIOutput **target) > +{ > + HRESULT hr = S_OK; > + > + TRACE("iface %p, fullscreen %p, target %p semi-stub.\n", iface, fullscreen, target); > + > + if (fullscreen) > + *fullscreen = TRUE; > + > + if (target) > + hr = dxgi_swapchain_GetContainingOutput(iface, target); > + > + return hr; > +} That's not a real implementation, of course. > @@ -496,6 +498,22 @@ static void test_create_swapchain(void) > refcount = IUnknown_Release(parent); > todo_wine ok(refcount == 4, "Got unexpected refcount %u.\n", refcount); > > + hr = IDXGISwapChain_GetFullscreenState(swapchain, NULL, NULL); > + ok(hr == S_OK, "GetFullscreenState failed, hr %#x.\n", hr); > + > + hr = IDXGISwapChain_GetFullscreenState(swapchain, &fullscreen, NULL); > + ok(hr == S_OK, "GetFullscreenState failed, hr %#x.\n", hr); > + > + hr = IDXGISwapChain_GetFullscreenState(swapchain, NULL, &target); > + ok(hr == S_OK, "GetFullscreenState failed, hr %#x.\n", hr); > + if(target) > + IDXGIOutput_Release(target); > + > + hr = IDXGISwapChain_GetFullscreenState(swapchain, &fullscreen, &target); > + ok(hr == S_OK, "GetFullscreenState failed, hr %#x.\n", hr); > + if(target) > + IDXGIOutput_Release(target); > + And this doesn't test much beyond the function returning S_OK.

July 12, 2016

Re: [PATCH v2] kernel32: Use setprogname(3) to set the program name if available.

by Ken Thomases

On Jul 7, 2016, at 6:02 PM, Ken Thomases <ken(a)codeweavers.com> wrote: > > Also, why should set_process_name() shift the strings up within the (supposedly contiguous) string buffer? For the HAVE_SETPROCTITLE, it doesn't do that. Prior to Chip's commit, it didn't do that on macOS. So, Theodore's original question, "Why wouldn’t setprogname(argv[1]) be adequate?", is pretty close. It should have been, "Why wouldn’t setprogname(argv[1]) followed by shifting the elements of argv up one be adequate?" > > I'm not even sure why the string buffer is shifted for Linux. It looks like it's leftover logic that's no longer relevant (having to do with vdm?). OK, I have figured out what I was missing. Much to my surprise, modifying the argv strings buffer is actually the means by which the command shown by, for example, ps is modified. The prctl() call on Linux affects a different value (the "comm" field rather than "command"). I have submitted a patch to address the other shortcomings of the current code. -Ken

July 11, 2016

Re: ntoskrnl.exe: add IoAttachDevice stub

by Sebastian Lackner

On 12.07.2016 05:07, Austin English wrote: > + > +/*********************************************************************** > + * IoAttachDevice (NTOSKRNL.EXE.@) > + */ > +NTSTATUS IoAttachDevice(DEVICE_OBJECT *source, UNICODE_STRING *target, DEVICE_OBJECT *attached) You forgot to specify the calling convention (WINAPI) - this is probably also the reason why your stub is not sufficient yet. Otherwise the patch looks fine to me. > +{ > + FIXME("(%p, %s, %p): stub\n", source, debugstr_us(target), attached); > + return STATUS_NOT_IMPLEMENTED; > +}

July 11, 2016

Re: [PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

by Sebastian Lackner

On 11.07.2016 22:07, John Sheu wrote: > Updating the patch now. Note in particular with the SetLastError() > calls, that the error messages immediately after the API calls do > print the result of GetLastError(), so it would be good to return the > last error to a known state for that. > > Thanks! > -John Sheu > Usually SetLastError() is only used when the error code is tested explicitly, not when the error is printed in an ok() message for debugging purposes. If there are test failures, a developer will have to investigate what exactly is going wrong anyway.

July 11, 2016

Re: [PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

by John Sheu

Updating the patch now. Note in particular with the SetLastError() calls, that the error messages immediately after the API calls do print the result of GetLastError(), so it would be good to return the last error to a known state for that. Thanks! -John Sheu

July 11, 2016

Re: [PATCH 3/3] wined3d/resource: Performance improvements, try 2

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24152 Your paranoid android. === w8 (32 bit visual) === visual.c:8177: Test failed: Input test: Quad 3(2crd-wrongidx) returned color 0x00ff00ff, expected 0x00ff0080 visual.c:8345: Test failed: Got unexpected color 0x00ff0000 for no color attribute test. visual.c:4529: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 0. visual.c:4529: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 512. === w864 (32 bit visual) === visual.c:4529: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 0. visual.c:4529: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 512. === w1064 (32 bit visual) === visual.c:4529: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 0. visual.c:4529: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 512. === w864 (64 bit visual) === visual.c:4529: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 0. visual.c:4529: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 512. === w1064 (64 bit visual) === visual.c:4529: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 0. visual.c:4529: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 512.

July 11, 2016

Re: [PATCH 1/3] d3d9/tests/visual: Add Vertexbuffer locking tests, try 2

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24150 Your paranoid android. === w8 (32 bit visual) === visual.c:4524: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 0. visual.c:4524: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 512. === w864 (32 bit visual) === visual.c:4524: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 0. visual.c:4524: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 512. === w1064 (32 bit visual) === visual.c:4524: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 0. visual.c:4524: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 512. === w864 (64 bit visual) === visual.c:4524: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 0. visual.c:4524: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 512. === w1064 (64 bit visual) === visual.c:4524: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 0. visual.c:4524: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Usage = 512.

July 11, 2016

Re: [v3 1/2] winejoystick.drv: Print location ID in debugstr_device on the Mac

by Ken Thomases

OK. To answer your earlier question, anyway: yes, you would need to resubmit the whole series, even individual patches which didn't change. For those, you should mark them "resend" instead of bumping the version number. I'm not entirely sure it was right of me to sign off on the one which was fine, since it wouldn't work without the earlier patch. I wanted to indicate that it was fine. That uncertainty is why I didn't do it for the other one. *shrug* -Ken > On Jul 11, 2016, at 3:35 AM, DavidL <david.dljunk(a)gmail.com> wrote: > > Actually it looks like I am going to make a slight change to device_location_comparator to get rid of another set of compilation warnings - I took a look at button_usage_comparator and modeled the new version after that which eliminated the warning messages. > > Cheers, > David > > On Sun, Jul 10, 2016 at 10:25 PM, DavidL <david.dljunk(a)gmail.com <mailto:david.dljunk(a)gmail.com>> wrote: > Hi Ken, > > Thanks, I'll make the changes. For the dinput & winmm patch 2/3 & 2/2, should I resubmit them as part of my version 4 of the overall patches even if they have been signed-off on? (although I think only the dinput one was signed off on) I'm not anticipating making any changes to signed-off sorting code patch based on the comments. > > Cheers, > David > > On Sun, Jul 10, 2016 at 3:36 PM, Ken Thomases <ken(a)codeweavers.com <mailto:ken(a)codeweavers.com>> wrote: > On Jun 30, 2016, at 7:02 PM, David Lawrie <david.dljunk(a)gmail.com <mailto:david.dljunk(a)gmail.com>> wrote: > > > > Tested on OS X 10.10.5. > > > > Signed-off-by: David Lawrie <david.dljunk(a)gmail.com <mailto:david.dljunk(a)gmail.com>> > > --- > > dlls/winejoystick.drv/joystick_osx.c | 30 ++++++++++++++++++++++++++++-- > > 1 file changed, 28 insertions(+), 2 deletions(-) > > This has similar issues as the dinput version, the main one being the pointless "result" variable in get_device_property_long(). > > -Ken > > >

July 11, 2016

Re: [v3 2/3] dinput: Sort devices by location ID on the Mac

by DavidL

This patch will be superseded in a later version. Signed-off-by: David Lawrie <david.dljunk(a)gmail.com> On Sun, Jul 10, 2016 at 3:22 PM, Ken Thomases <ken(a)codeweavers.com> wrote: > Signed-off-by: Ken Thomases <ken(a)codeweavers.com> > > > >

July 11, 2016

Re: [v3 3/3] dinput: Alter function name and code style

by DavidL

I am compiling with -Wdeclaration-after-statement but oddly enough the compiler did not generate that warning for dinput. However, I don't know why not, because I see what you're saying and will make the necessary change anyway. David On Sun, Jul 10, 2016 at 3:33 PM, Ken Thomases <ken(a)codeweavers.com> wrote: > On Jun 30, 2016, at 7:02 PM, David Lawrie <david.dljunk(a)gmail.com> wrote: > > @@ -490,10 +490,10 @@ static int find_osx_devices(void) > > if (devset) > > { > > CFIndex num_devices, num_main_elements, idx; > > - CFMutableArrayRef devices = > CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks); > > - CFSetApplyFunction(devset, CFSetApplierFunctionCopyToCFArray, > devices); > > - CFRelease( devset); > > - num_devices = CFArrayGetCount(devices); > > + num_devices = CFSetGetCount(devset); > > + CFMutableArrayRef devices = > CFArrayCreateMutable(kCFAllocatorDefault, num_devices, > &kCFTypeArrayCallBacks); > > You've now interleaved statements and declarations. Wine's code standard > requires that declarations all come before statements. So, you could > change the assignment of num_devices to an initialization or split the > declaration and initialization of devices to separate declaration and > assignment. > > Wine should be using -Wdeclaration-after-statement for its compile > commands. Therefore, this change would have added a new warning. I know > that some of the Mac files have a lot of warnings, which makes it hard to > see new ones, but you should strive to avoid adding more. > > -Ken > >

July 11, 2016

Re: usp10/tests: Use SetRect() instead of open coding it.

by Michael Stefaniuc

On 07/11/2016 10:06 AM, Marvin wrote: > Hi, > > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? > Full results can be found at > https://testbot.winehq.org/JobDetails.pl?Key=24145 > > Your paranoid android. > > > === w1064 (32 bit usp10) === > usp10.c:1467: Test failed: 0: uJustification incorrect (0) > usp10.c:1467: Test failed: 1: uJustification incorrect (0) > usp10.c:1467: Test failed: 2: uJustification incorrect (0) > usp10.c:1467: Test failed: 3: uJustification incorrect (0) > usp10.c:1467: Test failed: 4: uJustification incorrect (0) > usp10.c:1467: Test failed: 5: uJustification incorrect (0) > usp10.c:1467: Test failed: 6: uJustification incorrect (0) > usp10.c:1467: Test failed: 7: uJustification incorrect (0) > usp10.c:1467: Test failed: 8: uJustification incorrect (0) > usp10.c:1467: Test failed: 9: uJustification incorrect (0) > usp10.c:1483: Test failed: 3: uJustification incorrect (0) > usp10.c:1483: Test failed: 5: fDiacritic incorrect (1) > usp10.c:1483: Test failed: 5: fZeroWidth incorrect (1) > usp10.c:1483: Test failed: 15: fDiacritic incorrect (1) > usp10.c:1483: Test failed: 15: fZeroWidth incorrect (1) > > === w1064 (64 bit usp10) === > usp10.c:1467: Test failed: 0: uJustification incorrect (0) > usp10.c:1467: Test failed: 1: uJustification incorrect (0) > usp10.c:1467: Test failed: 2: uJustification incorrect (0) > usp10.c:1467: Test failed: 3: uJustification incorrect (0) > usp10.c:1467: Test failed: 4: uJustification incorrect (0) > usp10.c:1467: Test failed: 5: uJustification incorrect (0) > usp10.c:1467: Test failed: 6: uJustification incorrect (0) > usp10.c:1467: Test failed: 7: uJustification incorrect (0) > usp10.c:1467: Test failed: 8: uJustification incorrect (0) > usp10.c:1467: Test failed: 9: uJustification incorrect (0) > usp10.c:1483: Test failed: 3: uJustification incorrect (0) > usp10.c:1483: Test failed: 5: fDiacritic incorrect (1) > usp10.c:1483: Test failed: 5: fZeroWidth incorrect (1) > usp10.c:1483: Test failed: 15: fDiacritic incorrect (1) > usp10.c:1483: Test failed: 15: fZeroWidth incorrect (1) This are per-existing errors. bye michael

July 11, 2016

Re: [v3 1/2] winejoystick.drv: Print location ID in debugstr_device on the Mac

by DavidL

Actually it looks like I am going to make a slight change to device_location_comparator to get rid of another set of compilation warnings - I took a look at button_usage_comparator and modeled the new version after that which eliminated the warning messages. Cheers, David On Sun, Jul 10, 2016 at 10:25 PM, DavidL <david.dljunk(a)gmail.com> wrote: > Hi Ken, > > Thanks, I'll make the changes. For the dinput & winmm patch 2/3 & 2/2, > should I resubmit them as part of my version 4 of the overall patches even > if they have been signed-off on? (although I think only the dinput one was > signed off on) I'm not anticipating making any changes to signed-off > sorting code patch based on the comments. > > Cheers, > David > > On Sun, Jul 10, 2016 at 3:36 PM, Ken Thomases <ken(a)codeweavers.com> wrote: > >> On Jun 30, 2016, at 7:02 PM, David Lawrie <david.dljunk(a)gmail.com> wrote: >> > >> > Tested on OS X 10.10.5. >> > >> > Signed-off-by: David Lawrie <david.dljunk(a)gmail.com> >> > --- >> > dlls/winejoystick.drv/joystick_osx.c | 30 ++++++++++++++++++++++++++++-- >> > 1 file changed, 28 insertions(+), 2 deletions(-) >> >> This has similar issues as the dinput version, the main one being the >> pointless "result" variable in get_device_property_long(). >> >> -Ken >> >> >

July 11, 2016

Re: usp10/tests: Use SetRect() instead of open coding it.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24145 Your paranoid android. === w1064 (32 bit usp10) === usp10.c:1467: Test failed: 0: uJustification incorrect (0) usp10.c:1467: Test failed: 1: uJustification incorrect (0) usp10.c:1467: Test failed: 2: uJustification incorrect (0) usp10.c:1467: Test failed: 3: uJustification incorrect (0) usp10.c:1467: Test failed: 4: uJustification incorrect (0) usp10.c:1467: Test failed: 5: uJustification incorrect (0) usp10.c:1467: Test failed: 6: uJustification incorrect (0) usp10.c:1467: Test failed: 7: uJustification incorrect (0) usp10.c:1467: Test failed: 8: uJustification incorrect (0) usp10.c:1467: Test failed: 9: uJustification incorrect (0) usp10.c:1483: Test failed: 3: uJustification incorrect (0) usp10.c:1483: Test failed: 5: fDiacritic incorrect (1) usp10.c:1483: Test failed: 5: fZeroWidth incorrect (1) usp10.c:1483: Test failed: 15: fDiacritic incorrect (1) usp10.c:1483: Test failed: 15: fZeroWidth incorrect (1) === w1064 (64 bit usp10) === usp10.c:1467: Test failed: 0: uJustification incorrect (0) usp10.c:1467: Test failed: 1: uJustification incorrect (0) usp10.c:1467: Test failed: 2: uJustification incorrect (0) usp10.c:1467: Test failed: 3: uJustification incorrect (0) usp10.c:1467: Test failed: 4: uJustification incorrect (0) usp10.c:1467: Test failed: 5: uJustification incorrect (0) usp10.c:1467: Test failed: 6: uJustification incorrect (0) usp10.c:1467: Test failed: 7: uJustification incorrect (0) usp10.c:1467: Test failed: 8: uJustification incorrect (0) usp10.c:1467: Test failed: 9: uJustification incorrect (0) usp10.c:1483: Test failed: 3: uJustification incorrect (0) usp10.c:1483: Test failed: 5: fDiacritic incorrect (1) usp10.c:1483: Test failed: 5: fZeroWidth incorrect (1) usp10.c:1483: Test failed: 15: fDiacritic incorrect (1) usp10.c:1483: Test failed: 15: fZeroWidth incorrect (1)

July 11, 2016

Re: [PATCH 4/4] d3drm: Implement IDirect3DRMDevice*::Add/DeleteDestroyCallback. (v2)

by Aaryaman Vasishta

On Mon, Jul 11, 2016 at 1:11 PM, Aaryaman Vasishta < jem456.vasishta(a)gmail.com> wrote: > CC'ing to wine-devel again. > > On Mon, Jul 11, 2016 at 1:10 PM, Aaryaman Vasishta < > jem456.vasishta(a)gmail.com> wrote: > >> >> >> On Mon, Jul 11, 2016 at 1:06 PM, Aaryaman Vasishta < >> jem456.vasishta(a)gmail.com> wrote: >> >>> >>> >>> On Mon, Jul 11, 2016 at 12:05 PM, Stefan Dösinger < >>> stefandoesinger(a)gmail.com> wrote: >>> >>>> -----BEGIN PGP SIGNED MESSAGE----- >>>> Hash: SHA256 >>>> >>>> Am 2016-07-10 um 23:14 schrieb Aaryaman Vasishta: >>>> > Anyways, the vtable qi tests tests written so far show that >>>> > querying IDirect3DRMObject from a particular interface will return >>>> > version 1 of that interface. So technically it all comes down to >>>> > calling >>>> > >>>> IDirect3DRM<Interface_name>::AddDestroyCallback(&object->version1_vtable, >>>> > ...);. In our implementation of this method (see d3drm_main.c), we >>>> > have passed version 1 of the relevant interface while calling the >>>> > destroy callbacks within d3drm_object_destroy. The tests seem to >>>> > agree with that implementation so far. >>>> >>> Also, I may have written the example incorrectly. You're right about the >> fact that calling device3->AddDestroyCallback will pass device3 into this >> function. But internally this interface will be QI'd to IDirect3DRMObject, >> and then passed as the first parameter to the destroy callback itself. >> > Umm, It's still not clear whether the interface is QI'd to IDirect3DRMObject or referenced directly. Our implementation doesn't do the AddRef, but I don't think refcounting matters much in this case, since the callbacks happen right before the object gets destroyed.

July 11, 2016

Re: [PATCH 4/4] d3drm: Implement IDirect3DRMDevice*::Add/DeleteDestroyCallback. (v2)

by Aaryaman Vasishta

CC'ing to wine-devel again. On Mon, Jul 11, 2016 at 1:10 PM, Aaryaman Vasishta < jem456.vasishta(a)gmail.com> wrote: > > > On Mon, Jul 11, 2016 at 1:06 PM, Aaryaman Vasishta < > jem456.vasishta(a)gmail.com> wrote: > >> >> >> On Mon, Jul 11, 2016 at 12:05 PM, Stefan Dösinger < >> stefandoesinger(a)gmail.com> wrote: >> >>> -----BEGIN PGP SIGNED MESSAGE----- >>> Hash: SHA256 >>> >>> Am 2016-07-10 um 23:14 schrieb Aaryaman Vasishta: >>> > Anyways, the vtable qi tests tests written so far show that >>> > querying IDirect3DRMObject from a particular interface will return >>> > version 1 of that interface. So technically it all comes down to >>> > calling >>> > >>> IDirect3DRM<Interface_name>::AddDestroyCallback(&object->version1_vtable, >>> > ...);. In our implementation of this method (see d3drm_main.c), we >>> > have passed version 1 of the relevant interface while calling the >>> > destroy callbacks within d3drm_object_destroy. The tests seem to >>> > agree with that implementation so far. >>> >> Also, I may have written the example incorrectly. You're right about the > fact that calling device3->AddDestroyCallback will pass device3 into this > function. But internally this interface will be QI'd to IDirect3DRMObject, > and then passed as the first parameter to the destroy callback itself. > >

July 11, 2016

Re: [PATCH 4/4] d3drm: Implement IDirect3DRMDevice*::Add/DeleteDestroyCallback. (v2)

by Aaryaman Vasishta

On Mon, Jul 11, 2016 at 12:05 PM, Stefan Dösinger <stefandoesinger(a)gmail.com > wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA256 > > Am 2016-07-10 um 23:14 schrieb Aaryaman Vasishta: > > Anyways, the vtable qi tests tests written so far show that > > querying IDirect3DRMObject from a particular interface will return > > version 1 of that interface. So technically it all comes down to > > calling > > IDirect3DRM<Interface_name>::AddDestroyCallback(&object->version1_vtable, > > ...);. In our implementation of this method (see d3drm_main.c), we > > have passed version 1 of the relevant interface while calling the > > destroy callbacks within d3drm_object_destroy. The tests seem to > > agree with that implementation so far. > Well, but you can call AddDestroyCallback on the version 3 interface too: > > IDirect3DRMDevice *dev1; > IDirect3DRMDevice3 *dev3; > > dev1 = create_device(); > dev3 = dev1->qi(IID_IDirect3DRMDevice3); > > dev1->AddDestroyCallback(...); > dev3->AddDestroyCallback(...); > > The current code will always pass dev1 to the callback. It may be > correct, it may be wrong. > > This is where the obj == ctxt->obj test comes in. obj is the interface passed to the callback, and ctxt->obj is the one we pass into the context, which is our interface QI'd to IDirect3DRMObject. Now if we consider this test + the vtable QI tests, we can (in)directly prove that the interface passed to the destroy callback is always the version 1 of that interface. The tests would have failed on wine if e.g. I passed version 2 or 3 interface into the destroy callback. I agree that there are no direct tests confirming this though, but hopefully the two tests combined should be enough to confirm this. Cheers, Aaryaman

July 11, 2016

Re: [PATCH 4/4] d3drm: Implement IDirect3DRMDevice*::Add/DeleteDestroyCallback. (v2)

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Am 2016-07-10 um 23:14 schrieb Aaryaman Vasishta: > Anyways, the vtable qi tests tests written so far show that > querying IDirect3DRMObject from a particular interface will return > version 1 of that interface. So technically it all comes down to > calling > IDirect3DRM<Interface_name>::AddDestroyCallback(&object->version1_vtable, > ...);. In our implementation of this method (see d3drm_main.c), we > have passed version 1 of the relevant interface while calling the > destroy callbacks within d3drm_object_destroy. The tests seem to > agree with that implementation so far. Well, but you can call AddDestroyCallback on the version 3 interface too: IDirect3DRMDevice *dev1; IDirect3DRMDevice3 *dev3; dev1 = create_device(); dev3 = dev1->qi(IID_IDirect3DRMDevice3); dev1->AddDestroyCallback(...); dev3->AddDestroyCallback(...); The current code will always pass dev1 to the callback. It may be correct, it may be wrong. But yeah, we won't accidentally pass an IDirect3DRMFrame interface to the callback if AddDestroyCallback was called on a device. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXgz5NAAoJEN0/YqbEcdMw7RAP/1am78HbnuobIjjjBJYz+pCb lOPqKNZP2+dRIbJzTpd9Jgwz8Vonishwn9B+ZF6X71neWOHFG7xGFsGFq8OvuNBO c+jSOdrd/jkHLf61pllRBOk7QykoLCFIoaIbj843xeTeseabND7bwZwXVBKt34Gb hce0wquaITIntu5UPcLVnr3ak0meaADaXETRjiFamVnAPPmmyr3WyAYenVOT9P8c 1J8UneOEzBPXUtdsc8V4S0qQfpOEo9W8t1uh0M9FndAUNZyF9k7f6TzlEkA4e6C2 JzG5TfsIKg6LWgnX+WmGZ4ByZVfzdA/x0kWu7t0Gp9Wbb28rPMWs4yF2IU5uAw1q lHWIutHmmDCC7sxdyOOYp28ej1pxO3g23INynyUB3GTwAGyX2v/F+5a7Ge19IP1d 8A93T1id4ZZLzhF13oWHqFPhLLa1uoeAg0bpiXrC9SodK3x6YKrdBK/n/fyrHY3j 8fSrYBQwCt0ojV4Y7mqEZpG7M3+2asis26BdpaHGemcAlqEOThQOiIJiuvw/loWZ wkBGrzz5nkL6wTZHioPGCBkLm1vgD5H1N+wyYWDXQ2Adm+fgp7xP+VMMZ+66obCk CQgork1bnSkBsMWvafaGHim3+pbJdMXK1dbE3RjjdwC5bz2UsT2AxqH4ldJXEjxe gf+UMdoIzcyXS68E+Bpb =vqhG -----END PGP SIGNATURE-----

July 11, 2016

Re: [v3 1/2] winejoystick.drv: Print location ID in debugstr_device on the Mac

by DavidL

Hi Ken, Thanks, I'll make the changes. For the dinput & winmm patch 2/3 & 2/2, should I resubmit them as part of my version 4 of the overall patches even if they have been signed-off on? (although I think only the dinput one was signed off on) I'm not anticipating making any changes to signed-off sorting code patch based on the comments. Cheers, David On Sun, Jul 10, 2016 at 3:36 PM, Ken Thomases <ken(a)codeweavers.com> wrote: > On Jun 30, 2016, at 7:02 PM, David Lawrie <david.dljunk(a)gmail.com> wrote: > > > > Tested on OS X 10.10.5. > > > > Signed-off-by: David Lawrie <david.dljunk(a)gmail.com> > > --- > > dlls/winejoystick.drv/joystick_osx.c | 30 ++++++++++++++++++++++++++++-- > > 1 file changed, 28 insertions(+), 2 deletions(-) > > This has similar issues as the dinput version, the main one being the > pointless "result" variable in get_device_property_long(). > > -Ken > >

July 11, 2016

Re: [PATCH] ddraw/tests: Skip A4R4G4B4 ckey tests on Nvidia (v3)

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24144 Your paranoid android. === w1064 (32 bit ddraw4) === ddraw4.c:2540: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (64 bit ddraw4) === ddraw4.c:2540: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (32 bit ddraw7) === ddraw7.c:2270: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (64 bit ddraw7) === ddraw7.c:2270: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (32 bit ddraw1) === ddraw1.c:2204: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (64 bit ddraw1) === ddraw1.c:2204: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (32 bit ddraw2) === ddraw2.c:2355: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (64 bit ddraw2) === ddraw2.c:2355: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768).

July 10, 2016

Re: [v3 1/2] winejoystick.drv: Print location ID in debugstr_device on the Mac

by Ken Thomases

On Jun 30, 2016, at 7:02 PM, David Lawrie <david.dljunk(a)gmail.com> wrote: > > Tested on OS X 10.10.5. > > Signed-off-by: David Lawrie <david.dljunk(a)gmail.com> > --- > dlls/winejoystick.drv/joystick_osx.c | 30 ++++++++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) This has similar issues as the dinput version, the main one being the pointless "result" variable in get_device_property_long(). -Ken

July 10, 2016

Re: [v3 3/3] dinput: Alter function name and code style

by Ken Thomases

On Jun 30, 2016, at 7:02 PM, David Lawrie <david.dljunk(a)gmail.com> wrote: > @@ -490,10 +490,10 @@ static int find_osx_devices(void) > if (devset) > { > CFIndex num_devices, num_main_elements, idx; > - CFMutableArrayRef devices = CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks); > - CFSetApplyFunction(devset, CFSetApplierFunctionCopyToCFArray, devices); > - CFRelease( devset); > - num_devices = CFArrayGetCount(devices); > + num_devices = CFSetGetCount(devset); > + CFMutableArrayRef devices = CFArrayCreateMutable(kCFAllocatorDefault, num_devices, &kCFTypeArrayCallBacks); You've now interleaved statements and declarations. Wine's code standard requires that declarations all come before statements. So, you could change the assignment of num_devices to an initialization or split the declaration and initialization of devices to separate declaration and assignment. Wine should be using -Wdeclaration-after-statement for its compile commands. Therefore, this change would have added a new warning. I know that some of the Mac files have a lot of warnings, which makes it hard to see new ones, but you should strive to avoid adding more. -Ken

July 10, 2016

Re: [PATCH 4/4] d3drm: Implement IDirect3DRMDevice*::Add/DeleteDestroyCallback. (v2)

by Aaryaman Vasishta

On Mon, Jul 11, 2016 at 3:18 AM, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA256 > > Am 2016-07-10 um 21:49 schrieb Aaryaman Vasishta: > > I believe the "obj == ctxt->obj" tests in destroy_callback and > > destroy_callback1 should cover this. See test_destroy_callback. > If I understand it correctly not quite, because you're only ever > calling AddDestroyCallback via IDirect3DRMObject, never via > IDirect3DRMSomethingElse. > > (Re-CCed wine-devel) > My bad, I may have clicked on "Reply" instead of "Reply to All" on gmail again.. Anyways, the vtable qi tests tests written so far show that querying IDirect3DRMObject from a particular interface will return version 1 of that interface. So technically it all comes down to calling IDirect3DRM<Interface_name>::AddDestroyCallback(&object->version1_vtable, ...);. In our implementation of this method (see d3drm_main.c), we have passed version 1 of the relevant interface while calling the destroy callbacks within d3drm_object_destroy. The tests seem to agree with that implementation so far. Do correct me if I have misunderstood something here, though. Cheers, Aaryaman

July 10, 2016

Re: [v3 1/3] dinput: Print location ID in debugstr_device on the Mac

by Ken Thomases

Hi, Sorry for the late review. See in-line comments: On Jun 30, 2016, at 7:02 PM, David Lawrie <david.dljunk(a)gmail.com> wrote: > > +static void get_device_property_long(IOHIDDeviceRef in_device, CFStringRef in_key, long * out_value) As Dmitry suggested, you could make this simply return the property value rather than provide it through the indirect output parameter. > +{ > + Boolean result = FALSE; This variable isn't really used. It's assigned, but never read. > + CFTypeRef type; This variable isn't holding a type, it's holding a value or, if you prefer, an object reference. (The fact that its type is "CFTypeRef" doesn't imply it holds a type. Frankly, "CFTypeRef" is also a lousy name, but that's on Apple.) > + > + if (in_device) > + { > + assert(IOHIDDeviceGetTypeID() == CFGetTypeID(in_device)); > + > + type = IOHIDDeviceGetProperty(in_device, in_key); > + > + if (type) > + { > + if (CFNumberGetTypeID() == CFGetTypeID(type)) You could combine these two "if" statements using &&. -Ken

July 10, 2016

Re: [PATCH 4/4] d3drm: Implement IDirect3DRMDevice*::Add/DeleteDestroyCallback. (v2)

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Am 2016-07-10 um 21:49 schrieb Aaryaman Vasishta: > I believe the "obj == ctxt->obj" tests in destroy_callback and > destroy_callback1 should cover this. See test_destroy_callback. If I understand it correctly not quite, because you're only ever calling AddDestroyCallback via IDirect3DRMObject, never via IDirect3DRMSomethingElse. (Re-CCed wine-devel) -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXgsLBAAoJEN0/YqbEcdMwlsUP/2AIUfNl//T23Ik/gnL5FHVd 4axt1f/F5l1Hb5C3049Eh/se5SfF8jM6YYJte62al60f8nf0ERCW8S9JCzRueV2+ Q4ETvXbxhffKh9Ceb8W83ijxkwGf3GplypnXK7OQnqFUg88AR4YTGIL4I5pTPZVq Bw/MKgOfnDAHn+QsRDnzaQTtZcJPDNx+mrf26S/x15Wdi+PQjdVWb3uPbtgfkHN4 TWVwAgTQwPcnJQDaQ2wmWlEG+uxvxqpCF5BFCf1JwRV6oMdYYCQeHH+nvBrnPjEN /7Upv092RGbazucxMr6OWc9GJaCy4K9pfer8U2Ahdb47eAsF9UTO4AoW718GnIfU CJPvh8kMxukMw81U3U5Dr61v9cxd2xywz8ZMWMfKScrN4b6W7/hiE19B3+0w/XHi JHDf0NlPG8RDS+LR3N67X7dkaVqtEkRNCyVfAKdOozlz2EIlzs6lHmYNSxd17GO1 foIPVad9Ovo7nNQhB6FK+0zMZCKAz7be6ai9orlCk4ThhBhVwlaQXX16LBxgrYBi cU/3emOonGzto6sqzDrXyTNEqQTzvFMXnqitH/ITgvKuey0GBLHVSeOAHdECbrN3 bdGYdYPuXnxdksTey/zGORo4u0plUxS3gXiAGM0Ch0oCb6E9uzWMqKJpH6rIoR22 yAuKU7HOboWb3rlcAs0i =s5bk -----END PGP SIGNATURE-----

July 10, 2016

Re: [v2 01/10] webservices: Add support for writing double values.

by Sebastian Lackner

On 10.07.2016 20:30, Hans Leidekker wrote: > On Sun, 2016-07-10 at 03:28 +0200, Sebastian Lackner wrote: >> Unfortunately your patch does not work as expected here (gcc 5.3): >> >> ../../../tools/runtest -q -P wine -T ../../.. -M webservices.dll -p webservices_test.exe.so writer && touch writer.ok >> writer.c:358: Test failed: 1941: got 11 expected 10 >> writer.c:360: Test failed: 1941: got <t>:E-3</t>>000000004</t> expected <t>1E-2</t> >> writer.c:358: Test failed: 1941: got 12 expected 11 >> writer.c:360: Test failed: 1941: got <t>-:E-3</t>000000004</t> expected <t>-1E-2</t> >> Makefile:346: die Regel für Ziel „writer.ok“ scheiterte >> >> If you need any details to investigate this issue please let me know. > > Does this patch help? Note that it needs autoconf && autoheader. > Thanks. Yes, your proposed patch fixes the test failures. At first powl() wasn't detected, but then I remembered that this is a well-known bug when compiling with -Werror: conftest.c:370:6: warning: conflicting types for built-in function 'powl' Nevertheless, this also affects some other functions, so it probably should be fixed independently (if we care about it). Best regards, Sebastian

July 10, 2016

Re: [PATCH 4/4] d3drm: Implement IDirect3DRMDevice*::Add/DeleteDestroyCallback. (v2)

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Am 2016-07-10 um 17:41 schrieb Aaryaman Vasishta: > static HRESULT WINAPI d3drm_device1_AddDestroyCallback(IDirect3DRMDevice *iface, > D3DRMOBJECTCALLBACK cb, void *ctx) > { > - FIXME("iface %p, cb %p, ctx %p stub!\n", iface, cb, ctx); > + struct d3drm_device *device = impl_from_IDirect3DRMDevice(iface); > > - return E_NOTIMPL; > + TRACE("iface %p, cb %p, ctx %p.\n", iface, cb, ctx); > + > + return d3drm_device3_AddDestroyCallback(&device->IDirect3DRMDevice3_iface, cb, ctx); > } It doesn't fit in this patch, but at some point we should test which interface is passed to the destroy callback: Is it the interface on which AddDestroyCallback was called, or is it the interface returned by iface->QueryInterface(&IID_IDirect3DRMObject)? Please correct me if we already have tests for that and I missed them :- ) . -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXgpstAAoJEN0/YqbEcdMwWkcP/j25Gk+jAQlDrb0l0YV5Tz0k V0D/PZW7lHMcGCeAW4Y8GyzzKAFVD+d8YL9RRHcxyWbUnl5f6nR/yjAe3da6FFpD AF+vDvKGlV1POG1QhC8d/34SxVYXsngKNq2tvxYVQOhldvKh9d9Ti255dLFicqG3 aBBzPF1OvY9ZaxQJQAyJdU9944YtaH3CUd6cxjOH95jR1hAiz55akKWDRQHm7Uc+ S77/XRaaUmGTX5gpma/EeY7Rb/eG3cEDF8UBoCI6rfu+kjIEa+KTJVZKzeSA5+RU xPETE28jUX94XZnRz8lqLJbSRboBXSa2MWDdb2KvBUqSTLPS1cTm55cLYRngWDAD czF8Wfo+UOfGeG9oGhorUE2GkOleDd/WtXQtAfJeAvxfBxd4aA5v66Z7IwhcVeEV sSEhsqxFfc31VBB/f4Ii3F/5nFyMN+LfcDUIHI4tmOMjwj+VMlIES//71BvYQpqm iJNo9DPSVUyqApIgPSNsYL7vORUl2AKsklJKk4fxka1j1010TLJdpFIyv4leLfL8 mBUY13S+wzaEObhztQJiMJ5usWu9BoClm38x4Oj5JzMZ2Pg2If0WAcVRLOLkb9Br GIyfN5K89SUmiJp6GoMtKG99QXypMsg1fanYe6imBTL8Cj5tGuVAoJlPDMpc8i07 JxITcNBrsu5/WwP8tGeq =Zf/v -----END PGP SIGNATURE-----

July 10, 2016

Re: WineHQ upgrade breaking dinput8:dinput?

by André Hentschel

Am 26.06.2016 um 17:31 schrieb André Hentschel: > Am 24.04.2016 um 21:12 schrieb Francois Gouget: >> >> I have added binaries compiled with my MinGW with -O2 and -O0, and >> compiled with the TestBot compiler. I also added assembly files and >> compiler version information. All of this is available at: >> >> http://fgouget.free.fr/tmp/dinput8-dinput.tar.xz >> >> >> For immediate reference: >> My compiler is the Debian Testing compiler (buggy): >> $ i686-w64-mingw32-gcc --version >> i686-w64-mingw32-gcc (GCC) 5.3.1 20160205 >> 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. >> >> The TestBot compiler is the Debian 7.4 compiler (not buggy): >> $ i686-w64-mingw32-gcc --version >> i686-w64-mingw32-gcc (GCC) 4.6.3 >> Copyright (C) 2011 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. >> >> >> And the WineTest.exe binary is compiled with the Debian 8 compiler >> which is presumably close to the current Debian Testing one. > > What happens if we compile it using Clang? (totally easy to do so by using wclang [1] to generate a wrapper around clang and mingw-w64) > Or for me to try it, what's the best way to verify? does it depend on the wine version somewhat? > > [1] https://github.com/tpoechtrager/wclang > I noticed that my mingw-w64 has the same issue, so it was easy to try it out and it works! Could someone install wclang on that machine and make wine using it please? It's as easy as running configure like: ./configure CROSSCC=i686-w64-mingw32-clang ... or ./configure --host=i686-w64-mingw32 CC=i686-w64-mingw32-clang host_alias=i686-w64-mingw32 ... I compiled with (just exchanged i686-w64-mingw32-gcc with i686-w64-mingw32-clang): i686-w64-mingw32-clang -c -o device.cross.o ../../../../dlls/dinput8/tests/device.c -I. -I../../../../dlls/dinput8/tests \ -I../../../include -I../../../../include -D__WINESRC__ -DWINE_CROSSTEST -g -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0 i686-w64-mingw32-clang -c -o dinput.cross.o ../../../../dlls/dinput8/tests/dinput.c -I. -I../../../../dlls/dinput8/tests \ -I../../../include -I../../../../include -D__WINESRC__ -DWINE_CROSSTEST -g -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0 i686-w64-mingw32-clang -c -o testlist.cross.o testlist.c -I. -I../../../../dlls/dinput8/tests -I../../../include \ -I../../../../include -D__WINESRC__ -DWINE_CROSSTEST -g -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0 ../../../tools/winegcc/winegcc -o dinput8_crosstest.exe -b i686-w64-mingw32 -B../../../tools/winebuild \ --lib-suffix=.cross.a device.cross.o dinput.cross.o testlist.cross.o \ ../../../dlls/dinput8/libdinput8.cross.a ../../../dlls/ole32/libole32.cross.a \ ../../../dlls/user32/libuser32.cross.a ../../../dlls/advapi32/libadvapi32.cross.a

July 10, 2016

Re: [v2 01/10] webservices: Add support for writing double values.

by Hans Leidekker

On Sun, 2016-07-10 at 03:28 +0200, Sebastian Lackner wrote: > Unfortunately your patch does not work as expected here (gcc 5.3): > > ../../../tools/runtest -q -P wine -T ../../.. -M webservices.dll -p webservices_test.exe.so writer && touch writer.ok > writer.c:358: Test failed: 1941: got 11 expected 10 > writer.c:360: Test failed: 1941: got <t>:E-3</t>>000000004</t> expected <t>1E-2</t> > writer.c:358: Test failed: 1941: got 12 expected 11 > writer.c:360: Test failed: 1941: got <t>-:E-3</t>000000004</t> expected <t>-1E-2</t> > Makefile:346: die Regel für Ziel „writer.ok“ scheiterte > > If you need any details to investigate this issue please let me know. Does this patch help? Note that it needs autoconf && autoheader.

July 10, 2016

Re: [PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

by Sebastian Lackner

On 08.07.2016 00:16, John Sheu wrote: > Signed-off-by: John Sheu <sheu(a)google.com> > --- > dlls/kernel32/tests/fiber.c | 291 +++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 258 insertions(+), 33 deletions(-) > > diff --git a/dlls/kernel32/tests/fiber.c b/dlls/kernel32/tests/fiber.c > index 1e4ca66..4f61f80 100644 > --- a/dlls/kernel32/tests/fiber.c > +++ b/dlls/kernel32/tests/fiber.c > @@ -33,9 +33,13 @@ static BOOL (WINAPI *pFlsFree)(DWORD); > static PVOID (WINAPI *pFlsGetValue)(DWORD); > static BOOL (WINAPI *pFlsSetValue)(DWORD,PVOID); > > -static LPVOID fibers[2]; > +static LPVOID fibers[3]; > static BYTE testparam = 185; > -static WORD cbCount; > +static DWORD fls_index_to_set = FLS_OUT_OF_INDEXES; > +static PVOID fls_value_to_set = NULL; > + > +static int fiberCount = 0; > +static int cbCount = 0; > > static VOID init_funcs(void) > { > @@ -59,15 +63,44 @@ static VOID init_funcs(void) > > static VOID WINAPI FiberLocalStorageProc(PVOID lpFlsData) > { > + ok(lpFlsData == fls_value_to_set, > + "FlsData expected not to be changed, value is %p, expected %p\n", > + lpFlsData, fls_value_to_set); > cbCount++; > - ok(lpFlsData == (PVOID) 1587, "FlsData expected not to be changed\n"); > } > > static VOID WINAPI FiberMainProc(LPVOID lpFiberParameter) > { > BYTE *tparam = (BYTE *)lpFiberParameter; > - cbCount++; > + fiberCount++; > ok(*tparam == 185, "Parameterdata expected not to be changed\n"); > + if (fls_index_to_set != FLS_OUT_OF_INDEXES) > + { > + if (!pFlsSetValue || !pFlsGetValue) > + { > + win_skip( "Fiber Local Storage not supported\n" ); You can get rid of this check, fls_index_to_set should not be set when Fiber Local Storage is not supported. > + } > + else > + { > + PVOID pret; Usually "void *" types and variable names without "p" prefix are preferred. > + BOOL bret; > + > + pret = pFlsGetValue(fls_index_to_set); > + ok(pret == NULL, "FlsGetValue returned %p, expected NULL\n", pret); > + > + /* Set the FLS value */ > + SetLastError( 0xdeadbeef ); You can remove this line when you are not testing the error value. > + bret = pFlsSetValue(fls_index_to_set, fls_value_to_set); > + ok(bret, "FlsSetValue failed with error %u\n", GetLastError()); > + > + /* Verify that FlsGetValue retrieves the value set by FlsSetValue */ > + SetLastError( 0xdeadbeef ); > + pret = pFlsGetValue(fls_index_to_set); > + ok(pret == fls_value_to_set, > + "FlsGetValue returned %p, expected %p\n", pret, fls_value_to_set); > + ok(GetLastError() == ERROR_SUCCESS, "FlsGetValue error %u\n", GetLastError()); > + } > + } > pSwitchToFiber(fibers[0]); > } > > @@ -110,9 +143,23 @@ static void test_ConvertFiberToThread(void) > } > } > > +static void test_IsThreadAFiber(BOOL is_fiber) > +{ > + if (pIsThreadAFiber) > + { > + BOOL ret = pIsThreadAFiber(); > + ok(ret == is_fiber, "IsThreadAFiber reported %d, expected %d\n", ret, is_fiber); > + } > + else > + { > + win_skip( "IsThreadAFiber not present\n" ); > + return; return at the end of a function is unnecessary. Also, I'm not sure if it makes much sense to have this as a separate function. You could probably just inline it in this case. > + } > +} > + > static void test_FiberHandling(void) > { > - cbCount = 0; > + fiberCount = 0; > fibers[0] = pCreateFiber(0,FiberMainProc,&testparam); > ok(fibers[0] != 0, "CreateFiber failed with error %d\n", GetLastError()); > pDeleteFiber(fibers[0]); > @@ -124,12 +171,11 @@ static void test_FiberHandling(void) > else > test_ConvertThreadToFiber(); > > - > fibers[1] = pCreateFiber(0,FiberMainProc,&testparam); > ok(fibers[1] != 0, "CreateFiber failed with error %d\n", GetLastError()); > > pSwitchToFiber(fibers[1]); > - ok(cbCount == 1, "Wrong callback count: %d\n", cbCount); > + ok(fiberCount == 1, "Wrong fiber count: %d\n", fiberCount); > pDeleteFiber(fibers[1]); > > if (!pCreateFiberEx) > @@ -143,46 +189,72 @@ static void test_FiberHandling(void) > ok(fibers[1] != 0, "CreateFiberEx failed with error %d\n", GetLastError()); > > pSwitchToFiber(fibers[1]); > - ok(cbCount == 2, "Wrong callback count: %d\n", cbCount); > + ok(fiberCount == 2, "Wrong fiber count: %d\n", fiberCount); > pDeleteFiber(fibers[1]); > > - if (!pIsThreadAFiber) > - { > - win_skip( "IsThreadAFiber not present\n" ); > - return; > - } > - > - ok(pIsThreadAFiber(), "IsThreadAFiber reported FALSE\n"); > + test_IsThreadAFiber(TRUE); > test_ConvertFiberToThread(); > - ok(!pIsThreadAFiber(), "IsThreadAFiber reported TRUE\n"); > + test_IsThreadAFiber(FALSE); > } > > -static void test_FiberLocalStorage(PFLS_CALLBACK_FUNCTION cbfunc) > +static void test_FiberLocalStorage(void) > { > - DWORD fls; > + DWORD fls, fls_2; > BOOL ret; > - PVOID val = (PVOID) 1587; > + PVOID val; "void *" would be preferred here. > > - if (!pFlsAlloc) > + if (!pFlsAlloc || !pFlsSetValue || !pFlsGetValue || !pFlsFree) > { > win_skip( "Fiber Local Storage not supported\n" ); > return; > } > - cbCount = 0; > > - fls = pFlsAlloc(cbfunc); > - ok(fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %d\n", GetLastError()); > + /* Test an unallocated index > + * FlsFree should fail > + * FlsGetValue and FlsSetValue should succeed > + */ > + SetLastError( 0xdeadbeef ); > + ret = pFlsFree( 127 ); > + ok( !ret, "freeing fls index 127 (unallocated) succeeded\n" ); > + ok( GetLastError() == ERROR_INVALID_PARAMETER, > + "freeing fls index 127 (unallocated) wrong error %u\n", GetLastError() ); > > - ret = pFlsSetValue(fls, val); > - ok(ret, "FlsSetValue failed\n"); > - ok(val == pFlsGetValue(fls), "FlsGetValue failed\n"); > + SetLastError( 0xdeadbeef ); This is unnecessary if you don't test the error. > + val = pFlsGetValue( 127 ); > + ok( val == NULL, > + "getting fls index 127 (unallocated) failed with error %u\n", GetLastError() ); > + > + SetLastError( 0xdeadbeef ); Same here. > + ret = pFlsSetValue( 127, (PVOID) 0x217 ); "(void *)0x217" would be preferred here (and at a couple of places below). > + ok( ret, "setting fls index 127 (unallocated) failed with error %u\n", GetLastError() ); > + > + SetLastError( 0xdeadbeef ); > + val = pFlsGetValue( 127 ); > + ok( val == (PVOID) 0x217, "fls index 127 (unallocated) wrong value %p\n", val ); > + ok( GetLastError() == ERROR_SUCCESS, > + "getting fls index 127 (unallocated) failed with error %u\n", GetLastError() ); > + > + /* FlsFree, FlsGetValue, and FlsSetValue out of bounds should return > + * ERROR_INVALID_PARAMETER > + */ > + SetLastError( 0xdeadbeef ); > + ret = pFlsFree( 128 ); > + ok( !ret, "freeing fls index 128 (out of bounds) succeeded\n" ); > + ok( GetLastError() == ERROR_INVALID_PARAMETER, > + "freeing fls index 128 (out of bounds) wrong error %u\n", GetLastError() ); > > - ret = pFlsFree(fls); > - ok(ret, "FlsFree failed\n"); > - if (cbfunc) > - todo_wine ok(cbCount == 1, "Wrong callback count: %d\n", cbCount); > + SetLastError( 0xdeadbeef ); > + ret = pFlsSetValue( 128, (PVOID) 0x217 ); > + ok( !ret, "setting fls index 128 (out of bounds) succeeded\n" ); > + ok( GetLastError() == ERROR_INVALID_PARAMETER, > + "setting fls index 128 (out of bounds) wrong error %u\n", GetLastError() ); > + > + SetLastError( 0xdeadbeef ); > + val = pFlsGetValue( 128 ); It would make sense to test "val" aswell. > + ok( GetLastError() == ERROR_INVALID_PARAMETER, > + "setting fls index 128 (out of bounds) wrong error %u\n", GetLastError() ); The function above is no set function. > > - /* test index 0 */ > + /* Test index 0 */ > SetLastError( 0xdeadbeef ); > val = pFlsGetValue( 0 ); > ok( !val, "fls index 0 set to %p\n", val ); > @@ -196,6 +268,7 @@ static void test_FiberLocalStorage(PFLS_CALLBACK_FUNCTION cbfunc) > ok( !val, "fls index 0 wrong value %p\n", val ); > ok( GetLastError() == ERROR_INVALID_PARAMETER, "setting fls index wrong error %u\n", GetLastError() ); > > + /* Test creating an FLS index */ > fls = pFlsAlloc( NULL ); > ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed\n" ); > ok( fls != 0, "fls index 0 allocated\n" ); > @@ -205,11 +278,162 @@ static void test_FiberLocalStorage(PFLS_CALLBACK_FUNCTION cbfunc) > ok( ret, "setting fls index %u failed\n", fls ); > val = pFlsGetValue( fls ); > ok( val == (void *)0xdeadbeef, "fls index %u wrong value %p\n", fls, val ); > + ok( GetLastError() == ERROR_SUCCESS, > + "getting fls index %u failed with error %u\n", fls, GetLastError() ); > pFlsFree( fls ); > + > + /* Undefined behavior: verify the value is NULL after it the slot is freed */ > + val = pFlsGetValue( fls ); > + ok( val == NULL, "fls index %u wrong value %p\n", fls, val ); > + ok( GetLastError() == ERROR_SUCCESS, > + "getting fls index %u failed with error %u\n", fls, GetLastError() ); > + > + /* Undefined behavior: verify the value is settable after the slot is freed */ > ret = pFlsSetValue( fls, (void *)0xdeadbabe ); > ok( ret, "setting fls index %u failed\n", fls ); > val = pFlsGetValue( fls ); > ok( val == (void *)0xdeadbabe, "fls index %u wrong value %p\n", fls, val ); > + > + /* Try to create the same FLS index again, and verify that is initialized to NULL */ > + fls_2 = pFlsAlloc( NULL ); > + ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed\n" ); > + /* If this fails it is not an API error, but the test will be inconclusive */ > + ok( fls_2 == fls, "different FLS index allocated, was %u, now %u\n", fls, fls_2 ); > + val = pFlsGetValue( fls_2 ); > + ok( val == NULL, "fls index %u wrong value %p\n", fls, val ); > + ok( GetLastError() == ERROR_SUCCESS, > + "getting fls index %u failed with error %u\n", fls_2, GetLastError() ); A SetLastError() call is missing here, otherwise this test is not really convincing. > + pFlsFree( fls_2 ); > +} > + > +static void test_FiberLocalStorageCallback(PFLS_CALLBACK_FUNCTION cbfunc) { All the other functions have the "{" on the next line. > + DWORD fls; > + BOOL ret; > + PVOID val, val2; > + > + if (!pFlsAlloc || !pFlsSetValue || !pFlsGetValue || !pFlsFree) > + { > + win_skip( "Fiber Local Storage not supported\n" ); > + return; > + } > + > + /* Test that the callback is executed */ > + SetLastError( 0xdeadbeef ); This is unnecessary. > + cbCount = 0; > + fls = pFlsAlloc( cbfunc ); > + ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() ); > + > + SetLastError( 0xdeadbeef ); This also. > + val = (PVOID) 0x1587; > + fls_value_to_set = val; > + ret = pFlsSetValue( fls, val ); > + ok(ret, "FlsSetValue failed with error %u\n", GetLastError() ); > + > + val2 = pFlsGetValue( fls ); > + ok(val == val2, "FlsGetValue returned %p, expected %p\n", val2, val); > + > + SetLastError( 0xdeadbeef ); Same here. > + ret = pFlsFree( fls ); > + ok(ret, "FlsFree failed with error %u\n", GetLastError() ); > + todo_wine > + { > + ok( cbCount == 1, "Wrong callback count: %d\n", cbCount ); > + } > + > + /* Test that callback is not executed if value is NULL */ > + SetLastError( 0xdeadbeef ); Same here. > + cbCount = 0; > + fls = pFlsAlloc( cbfunc ); > + ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() ); > + > + SetLastError( 0xdeadbeef ); Same here. > + ret = pFlsSetValue( fls, NULL ); > + ok( ret, "FlsSetValue failed with error %u\n", GetLastError() ); > + > + SetLastError( 0xdeadbeef ); Same here. > + pFlsFree( fls ); > + ok( ret, "FlsFree failed with error %u\n", GetLastError() ); > + ok( cbCount == 0, "Wrong callback count: %d\n", cbCount ); > +} > + > +static void test_FiberLocalStorageWithFibers(PFLS_CALLBACK_FUNCTION cbfunc) { > + PVOID val1 = (PVOID) 0x314; > + PVOID val2 = (PVOID) 0x152; > + BOOL ret; > + > + if (!pConvertThreadToFiber || !pSwitchToFiber || !pDeleteFiber || !pConvertFiberToThread) > + { > + win_skip( "Fibers not supported\n" ); > + return; > + } > + if (!pFlsAlloc || !pFlsFree || !pFlsSetValue || !pFlsGetValue) > + { > + win_skip( "Fiber Local Storage not supported\n" ); > + return; > + } > + > + > + fls_index_to_set = pFlsAlloc(cbfunc); > + ok(fls_index_to_set != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %d\n", GetLastError()); Not really critical, but in the code above you used %u for GetLastError values. > + > + test_ConvertThreadToFiber(); > + > + fiberCount = 0; > + cbCount = 0; > + fibers[1] = pCreateFiber(0,FiberMainProc,&testparam); > + fibers[2] = pCreateFiber(0,FiberMainProc,&testparam); > + ok(fibers[1] != 0, "CreateFiber failed with error %d\n", GetLastError()); > + ok(fibers[2] != 0, "CreateFiber failed with error %d\n", GetLastError()); CreateFiber returns a pointer, so it would be better to compare against NULL. > + ok(fiberCount == 0, "Wrong fiber count: %d\n", fiberCount); > + ok(cbCount == 0, "Wrong callback count: %d\n", cbCount); > + > + fiberCount = 0; > + cbCount = 0; > + fls_value_to_set = val1; > + pSwitchToFiber(fibers[1]); > + ok(fiberCount == 1, "Wrong fiber count: %d\n", fiberCount); > + ok(cbCount == 0, "Wrong callback count: %d\n", cbCount); > + > + fiberCount = 0; > + cbCount = 0; > + fls_value_to_set = val2; > + pSwitchToFiber(fibers[2]); > + ok(fiberCount == 1, "Wrong fiber count: %d\n", fiberCount); > + ok(cbCount == 0, "Wrong callback count: %d\n", cbCount); > + > + fls_value_to_set = val2; > + ret = pFlsSetValue(fls_index_to_set, fls_value_to_set); > + ok(ret, "FlsSetValue failed\n"); > + ok(val2 == pFlsGetValue(fls_index_to_set), "FlsGetValue failed\n"); > + > + fiberCount = 0; > + cbCount = 0; > + fls_value_to_set = val1; > + pDeleteFiber(fibers[1]); > + ok(fiberCount == 0, "Wrong fiber count: %d\n", fiberCount); > + todo_wine > + { > + ok(cbCount == 1, "Wrong callback count: %d\n", cbCount); > + } > + > + fiberCount = 0; > + cbCount = 0; > + fls_value_to_set = val2; > + pFlsFree(fls_index_to_set); > + ok(fiberCount == 0, "Wrong fiber count: %d\n", fiberCount); > + todo_wine > + { > + ok(cbCount == 2, "Wrong callback count: %d\n", cbCount); > + } > + > + fiberCount = 0; > + cbCount = 0; > + fls_value_to_set = val1; > + pDeleteFiber(fibers[2]); > + ok(fiberCount == 0, "Wrong fiber count: %d\n", fiberCount); > + ok(cbCount == 0, "Wrong callback count: %d\n", cbCount); > + > + test_ConvertFiberToThread(); > } > > START_TEST(fiber) > @@ -223,6 +447,7 @@ START_TEST(fiber) > } > > test_FiberHandling(); > - test_FiberLocalStorage(NULL); > - test_FiberLocalStorage(FiberLocalStorageProc); > + test_FiberLocalStorage(); > + test_FiberLocalStorageCallback(FiberLocalStorageProc); > + test_FiberLocalStorageWithFibers(FiberLocalStorageProc); > } >

July 10, 2016

Re: [PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

by Sebastian Lackner

On 10.07.2016 19:51, André Hentschel wrote: > Am 10.07.2016 um 19:38 schrieb Nikolay Sivov: >> On 10.07.2016 20:31, André Hentschel wrote: >>> Hi, >>> >>> comments inline >>> >>> Am 08.07.2016 um 00:16 schrieb John Sheu: >>>> +static void test_FiberLocalStorageCallback(PFLS_CALLBACK_FUNCTION cbfunc) { >>>> + DWORD fls; >>>> + BOOL ret; >>>> + PVOID val, val2; >>>> + >>>> + if (!pFlsAlloc || !pFlsSetValue || !pFlsGetValue || !pFlsFree) >>>> + { >>>> + win_skip( "Fiber Local Storage not supported\n" ); >>>> + return; >>>> + } >>>> + >>>> + /* Test that the callback is executed */ >>>> + SetLastError( 0xdeadbeef ); >>>> + cbCount = 0; >>>> + fls = pFlsAlloc( cbfunc ); >>>> + ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() ); >>>> + >>>> + SetLastError( 0xdeadbeef ); >>>> + val = (PVOID) 0x1587; >>>> + fls_value_to_set = val; >>>> + ret = pFlsSetValue( fls, val ); >>>> + ok(ret, "FlsSetValue failed with error %u\n", GetLastError() ); >>>> + >>>> + val2 = pFlsGetValue( fls ); >>>> + ok(val == val2, "FlsGetValue returned %p, expected %p\n", val2, val); >>>> + >>>> + SetLastError( 0xdeadbeef ); >>>> + ret = pFlsFree( fls ); >>>> + ok(ret, "FlsFree failed with error %u\n", GetLastError() ); >>>> + todo_wine >>>> + { >>>> + ok( cbCount == 1, "Wrong callback count: %d\n", cbCount ); >>>> + } >>> >>> >>> Please make those one-liners, like >>> todo_wine ok(...); >>> and the patch should be fine then >> >> How is this a stopper? > > It's not directly a stopper. > But I personally really don't like it that way and it somehow prevents me from a sign-off. And I think I'll add myself as a Maintainer for that file, so the style should fit somewhat. Its up to you of course, but please note that you can also send a fixed version yourself. Just make sure to add a From: header such that the original author does not get lost.

July 10, 2016

Re: [PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

by André Hentschel

Am 10.07.2016 um 19:38 schrieb Nikolay Sivov: > On 10.07.2016 20:31, André Hentschel wrote: >> Hi, >> >> comments inline >> >> Am 08.07.2016 um 00:16 schrieb John Sheu: >>> +static void test_FiberLocalStorageCallback(PFLS_CALLBACK_FUNCTION cbfunc) { >>> + DWORD fls; >>> + BOOL ret; >>> + PVOID val, val2; >>> + >>> + if (!pFlsAlloc || !pFlsSetValue || !pFlsGetValue || !pFlsFree) >>> + { >>> + win_skip( "Fiber Local Storage not supported\n" ); >>> + return; >>> + } >>> + >>> + /* Test that the callback is executed */ >>> + SetLastError( 0xdeadbeef ); >>> + cbCount = 0; >>> + fls = pFlsAlloc( cbfunc ); >>> + ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() ); >>> + >>> + SetLastError( 0xdeadbeef ); >>> + val = (PVOID) 0x1587; >>> + fls_value_to_set = val; >>> + ret = pFlsSetValue( fls, val ); >>> + ok(ret, "FlsSetValue failed with error %u\n", GetLastError() ); >>> + >>> + val2 = pFlsGetValue( fls ); >>> + ok(val == val2, "FlsGetValue returned %p, expected %p\n", val2, val); >>> + >>> + SetLastError( 0xdeadbeef ); >>> + ret = pFlsFree( fls ); >>> + ok(ret, "FlsFree failed with error %u\n", GetLastError() ); >>> + todo_wine >>> + { >>> + ok( cbCount == 1, "Wrong callback count: %d\n", cbCount ); >>> + } >> >> >> Please make those one-liners, like >> todo_wine ok(...); >> and the patch should be fine then > > How is this a stopper? It's not directly a stopper. But I personally really don't like it that way and it somehow prevents me from a sign-off. And I think I'll add myself as a Maintainer for that file, so the style should fit somewhat.

July 10, 2016

Re: [PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

by Nikolay Sivov

On 10.07.2016 20:31, André Hentschel wrote: > Hi, > > comments inline > > Am 08.07.2016 um 00:16 schrieb John Sheu: >> +static void test_FiberLocalStorageCallback(PFLS_CALLBACK_FUNCTION cbfunc) { >> + DWORD fls; >> + BOOL ret; >> + PVOID val, val2; >> + >> + if (!pFlsAlloc || !pFlsSetValue || !pFlsGetValue || !pFlsFree) >> + { >> + win_skip( "Fiber Local Storage not supported\n" ); >> + return; >> + } >> + >> + /* Test that the callback is executed */ >> + SetLastError( 0xdeadbeef ); >> + cbCount = 0; >> + fls = pFlsAlloc( cbfunc ); >> + ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() ); >> + >> + SetLastError( 0xdeadbeef ); >> + val = (PVOID) 0x1587; >> + fls_value_to_set = val; >> + ret = pFlsSetValue( fls, val ); >> + ok(ret, "FlsSetValue failed with error %u\n", GetLastError() ); >> + >> + val2 = pFlsGetValue( fls ); >> + ok(val == val2, "FlsGetValue returned %p, expected %p\n", val2, val); >> + >> + SetLastError( 0xdeadbeef ); >> + ret = pFlsFree( fls ); >> + ok(ret, "FlsFree failed with error %u\n", GetLastError() ); >> + todo_wine >> + { >> + ok( cbCount == 1, "Wrong callback count: %d\n", cbCount ); >> + } > > > Please make those one-liners, like > todo_wine ok(...); > and the patch should be fine then How is this a stopper? > > >> + >> + /* Test that callback is not executed if value is NULL */ >> + SetLastError( 0xdeadbeef ); >> + cbCount = 0; >> + fls = pFlsAlloc( cbfunc ); >> + ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() ); >> + >> + SetLastError( 0xdeadbeef ); >> + ret = pFlsSetValue( fls, NULL ); >> + ok( ret, "FlsSetValue failed with error %u\n", GetLastError() ); >> + >> + SetLastError( 0xdeadbeef ); >> + pFlsFree( fls ); >> + ok( ret, "FlsFree failed with error %u\n", GetLastError() ); >> + ok( cbCount == 0, "Wrong callback count: %d\n", cbCount ); >> +} > > >

July 10, 2016

Re: [PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

by André Hentschel

Hi, comments inline Am 08.07.2016 um 00:16 schrieb John Sheu: > +static void test_FiberLocalStorageCallback(PFLS_CALLBACK_FUNCTION cbfunc) { > + DWORD fls; > + BOOL ret; > + PVOID val, val2; > + > + if (!pFlsAlloc || !pFlsSetValue || !pFlsGetValue || !pFlsFree) > + { > + win_skip( "Fiber Local Storage not supported\n" ); > + return; > + } > + > + /* Test that the callback is executed */ > + SetLastError( 0xdeadbeef ); > + cbCount = 0; > + fls = pFlsAlloc( cbfunc ); > + ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() ); > + > + SetLastError( 0xdeadbeef ); > + val = (PVOID) 0x1587; > + fls_value_to_set = val; > + ret = pFlsSetValue( fls, val ); > + ok(ret, "FlsSetValue failed with error %u\n", GetLastError() ); > + > + val2 = pFlsGetValue( fls ); > + ok(val == val2, "FlsGetValue returned %p, expected %p\n", val2, val); > + > + SetLastError( 0xdeadbeef ); > + ret = pFlsFree( fls ); > + ok(ret, "FlsFree failed with error %u\n", GetLastError() ); > + todo_wine > + { > + ok( cbCount == 1, "Wrong callback count: %d\n", cbCount ); > + } Please make those one-liners, like todo_wine ok(...); and the patch should be fine then > + > + /* Test that callback is not executed if value is NULL */ > + SetLastError( 0xdeadbeef ); > + cbCount = 0; > + fls = pFlsAlloc( cbfunc ); > + ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() ); > + > + SetLastError( 0xdeadbeef ); > + ret = pFlsSetValue( fls, NULL ); > + ok( ret, "FlsSetValue failed with error %u\n", GetLastError() ); > + > + SetLastError( 0xdeadbeef ); > + pFlsFree( fls ); > + ok( ret, "FlsFree failed with error %u\n", GetLastError() ); > + ok( cbCount == 0, "Wrong callback count: %d\n", cbCount ); > +}

July 10, 2016

Re: [PATCH 07/12] d3drm: Implement IDirect3DRMViewport*::Init. (v4)

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Am 2016-07-10 um 17:34 schrieb Aaryaman Vasishta: > You mean accessing the struct directly instead of via the > interface functions? I'm fine with that too. The interface > functions do provide a way to get most of what's needed (I could be > wrong though, maybe some internal data that's not revealed outside > can still be utilized within calls), but I guess using the struct > directly would be cleaner, and safer in the long run. If you expect that the external API is enough stick to the interface. It's no big issue to change it later if needed. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXgoRjAAoJEN0/YqbEcdMwcBcQAI/7D9EYR3M+/ctNPdf1q2C7 YTZnYO6zbSUwflEP5zt7T2rDzZ+eG7PeLowu3LE+pNdUAkbLGGbJQ7tIaBTsYLG2 lOFCaqNehH0OPzW1CpOeu42US2qMRWnsmDJRnopUDiOnHyuNa5PqRlzncKJl1x3/ ci2JRQ2SiVdjvQTp0OGPkAXMLAri7R3kdrIIgZ+gnytWJ91fCeO016Jg1OxeV3Rr 5fOUx6Fk3+uS0e36+D8SRgF5mRBdP2o6g2Hzio+2+zScwonvIehVo/WeeTXRIZob 0AWhvs0Jf6JkQaVLYkR0p3LuRR2PhBsfVccr3pgt10kZG2hYKr4YCAXGsuhlp77r CasFGOTMv/f4AnzTpQSCvaR34omrZYbvVgHlP1X1VRHnUxUQfgqmDjuaHtAq8Kql miQ6cNkLW1AybZx43+rRJnKWsuvviXTgSTek8ccMOcPeB07RkS4bpRuepBNYW5mW Vz9uqMotRXqSpZaaggHZB2fIJ5uah/LPcSbwfdiqVNYcaVexbSeYdZm+tzHPO3Ie EydiWAYRDJ8LxG4t8hyjNGkUnk+aGa5o2XzGdKQLhY02wXWQKLFAKMez2280NK8T S8A1AoR04h2elZ85MyGh+bRs4zZwQGm0zKSHkkNa+uoaJLZfmPcNy3X1pMXyHK+b dDGyJaf2q/kwKA6w/UA9 =2S4r -----END PGP SIGNATURE-----

July 10, 2016

Re: [PATCH 07/12] d3drm: Implement IDirect3DRMViewport*::Init. (v4)

by Aaryaman Vasishta

On Sun, Jul 10, 2016 at 9:39 PM, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA256 > > Am 2016-07-07 um 14:52 schrieb Aaryaman Vasishta: > > - {&CLSID_CDirect3DRMTexture, d3drm_create_texture_object}, > > - {&CLSID_CDirect3DRMDevice, d3drm_create_device_object}, > > + { &CLSID_CDirect3DRMTexture, d3drm_create_texture_object }, > > + { &CLSID_CDirect3DRMDevice, d3drm_create_device_object }, > > + { &CLSID_CDirect3DRMViewport, d3drm_create_viewport_object }, > I guess Visual Studio or xcode re-added the spaces between { and &. Henri > removed them in the slightly modified version of "d3drm: Use a table in > d3drm3_CreateObject() to create objects in a generic manner." > True. I actually thought this was fine since the QI test tables were formatted in a similar way (see test_d3drm_qi, test_texture_qi etc). But I guess the current format is considered better, so I'll go with that for the implementations. > > > + if (FAILED(hr = IDirect3DRMFrame3_QueryInterface(camera, > &IID_IDirect3DRMFrame, (void **)&viewport->camera))) > > + goto cleanup; > Aren't you more likely to need the implementation structure of the camera > in the future? > You mean accessing the struct directly instead of via the interface functions? I'm fine with that too. The interface functions do provide a way to get most of what's needed (I could be wrong though, maybe some internal data that's not revealed outside can still be utilized within calls), but I guess using the struct directly would be cleaner, and safer in the long run. > > > + > > + if (material) > > + IDirect3DMaterial_Release(material); > In the success case this will destroy the material you just created. Our > IDirect3DViewport::SetBackground implementation does not AddRef the > material. I don't think we have any tests for this, but because > SetBackground accepts a handle instead of a COM object I don't expect it to > AddRef. > > Overall it seems questionable to call camera->GetSceneBackground and apply > the color here. I'd expect that the camera background can be changed after > creating the viewport. > Ah, I was under the impression that IDirect3DViewport keeps a reference to the material (should have studied its implementation here). So I guess a reference to the material should be stored in the struct as well, and released on destroy. Cheers, Aaryaman

July 10, 2016

Re: [PATCH 05/12] d3drm: Implement IDirect3DRMDevice*::InitFromD3D.

by Aaryaman Vasishta

On Sun, Jul 10, 2016 at 9:10 PM, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA256 > > Hi, > > Patches 1-4 look good to me, once the fixes you mentioned and we talked > about on IRC are implemented. > > Am 2016-07-07 um 14:52 schrieb Aaryaman Vasishta: > > struct d3drm_frame > > { > > + struct d3drm_object obj; > This seems unrelated. > Could be leftover from a rebase. Will remove it. > > > + if (device->ddraw) > > + hr = D3DRMERR_BADOBJECT; > I guess you're not returning here right away because you want to addref > d3drm later. Wouldn't it be better to move those AddRefs in front in this > case? > > The QueryInterface / GetRenderTarget code below is unlikely to fail, if it > does something is severely wrong (A d3d device without a render target???) > and getting the reference leak right is the last of your worries. > This also brings me to another problem. What if init is called using a different d3d/device? I don't think it should re-set the new d3d/device on an existing object. I guess the double initialization path should be handled separately. I should probably add tests for this and change implementations accordingly. > > > device->width = desc.dwWidth; > > device->height = desc.dwHeight; > If I read the diff right this code will set width and height to 0 on an > already initialized device. > Right, this should probably be only set in the case where the device is not already initialized. > > > static HRESULT WINAPI > d3drm_device3_GetDirect3DDevice2(IDirect3DRMDevice3 *iface, > IDirect3DDevice2 **d3d_device) > > { > > struct d3drm_device *device = impl_from_IDirect3DRMDevice3(iface); > > + HRESULT hr; > > > > TRACE("iface %p, d3d_device %p.\n", iface, d3d_device); > > > > - IDirect3DDevice_QueryInterface(device->device, > &IID_IDirect3DDevice2, (void**)d3d_device); > > + if (FAILED(hr = IDirect3DDevice_QueryInterface(device->device, > &IID_IDirect3DDevice2, (void**)d3d_device))) > > + hr = D3DRMERR_BADOBJECT; > > > > - return D3DRM_OK; > > + return hr; > > } > I think this change (like the device2_GetDirect3DDevice2 one) can be moved > to a separate patch. > Right, this was noticed by me and I've split these changes into a separate patch (as discussed on IRC). > > > + hr = IDirect3DRMDevice_InitFromD3D(device1, d3d1, d3ddevice1); > > + ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, > got %#x.\n", hr); > > + ref3 = get_refcount((IUnknown *)d3ddevice1); > > + ok(ref3 > device_ref2, "Expected ref3 > device_ref2, got ref3 = %u, > device_ref2 = %u.\n", ref3, device_ref2); > > + ref3 = get_refcount((IUnknown *)d3d1); > > + ok(ref3 > d3d_ref2, "Expected ref3 > d3d_ref2, got ref3 = %u, > d3d_ref2 = %u.\n", ref3, d3d_ref2); > You forgot to test the reference of d3drm1. > > > + hr = IDirect3DRMDevice_QueryInterface(device1, > &IID_IDirect3DRMDevice2, (void **)&device2); > > + ok(SUCCEEDED(hr), "Cannot get IDirect3DRMDevice2 Interface (hr = > %x).\n", hr); > > + hr = IDirect3DRMDevice2_GetDirect3DDevice2(device2, &d3ddevice2); > > + ok(SUCCEEDED(hr), "Expected hr == D3DRM_OK, got %#x.\n", hr); > > + ok(d3ddevice2 == NULL, "Expected d3ddevice2 == NULL, got %p.\n", > d3ddevice2); > > + IDirect3DRMDevice2_Release(device2); > You forgot d3ddevice2 = 0xdeadbeef here. > > > + hr = IDirect3DRM2_CreateObject(d3drm2, &CLSID_CDirect3DRMDevice, > NULL, &IID_IDirect3DRMDevice2, (void **)&device2); > > + ok(SUCCEEDED(hr), "Cannot get IDirect3DRMDevice2 interface (hr = > %#x).\n", hr); > > + > > + hr = IDirectDraw_QueryInterface(ddraw1, &IID_IDirect3D, (void > **)&d3d1); > > + ok(SUCCEEDED(hr), "Cannot get IDirect3D interface (hr = %x).\n", > hr); > > + if (SUCCEEDED(hr = IDirect3DDevice2_QueryInterface(d3ddevice2, > &IID_IDirect3DDevice, (void **)&d3ddevice1))) > > + { > > + hr = IDirect3DRMDevice2_InitFromD3D(device2, d3d1, d3ddevice1); > > + ok(hr == E_NOINTERFACE, "Expected hr == E_NOINTERFACE, got > %#x.\n", hr); > What happens if you QI IDirect3DRMDevice1 from IDirect3DRMDevice2 and call > InitFromD3D on it? I would expect it to succeed because common decency > suggests that CreateObject doesn't leave a history of which interface was > requested and there's no CLSID_CDirect3DRMDevice2 or > CLSID_CDirect3DRMDevice3. Still worth a check though I think. > True, it makes sense. These tests should be added in here as well. Will do the same for patch 6 too. I will do the rest of the changes and resend the patch, along with the fixes to similar problems to patch 6 as well. Thanks for the review! :) Cheers, Aaryaman

July 10, 2016

Re: [PATCH 07/12] d3drm: Implement IDirect3DRMViewport*::Init. (v4)

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Am 2016-07-07 um 14:52 schrieb Aaryaman Vasishta: > - {&CLSID_CDirect3DRMTexture, d3drm_create_texture_object}, > - {&CLSID_CDirect3DRMDevice, d3drm_create_device_object}, > + { &CLSID_CDirect3DRMTexture, d3drm_create_texture_object }, > + { &CLSID_CDirect3DRMDevice, d3drm_create_device_object }, > + { &CLSID_CDirect3DRMViewport, d3drm_create_viewport_object }, I guess Visual Studio or xcode re-added the spaces between { and &. Henri removed them in the slightly modified version of "d3drm: Use a table in d3drm3_CreateObject() to create objects in a generic manner." > + if (FAILED(hr = IDirect3DRMFrame3_QueryInterface(camera, &IID_IDirect3DRMFrame, (void **)&viewport->camera))) > + goto cleanup; Aren't you more likely to need the implementation structure of the camera in the future? > + > + color = IDirect3DRMFrame3_GetSceneBackground(camera); > + /* Create material (ambient/diffuse/emissive?), set material */ > + if (FAILED(hr = IDirect3D_CreateMaterial(d3d1, &material, NULL))) > + goto cleanup; > + > + memset(&mat, 0, sizeof(mat)); > + memcpy(&mat.diffuse, &color, sizeof(color)); > + > + if (FAILED(hr = IDirect3DMaterial_SetMaterial(material, &mat))) > + goto cleanup; > + > + if (FAILED(hr = IDirect3DMaterial_GetHandle(material, d3d_device, &hmat))) > + goto cleanup; > + > + hr = IDirect3DViewport_SetBackground(viewport->d3d_viewport, hmat); > + > +cleanup: > + > + if (material) > + IDirect3DMaterial_Release(material); In the success case this will destroy the material you just created. Our IDirect3DViewport::SetBackground implementation does not AddRef the material. I don't think we have any tests for this, but because SetBackground accepts a handle instead of a COM object I don't expect it to AddRef. Overall it seems questionable to call camera->GetSceneBackground and apply the color here. I'd expect that the camera background can be changed after creating the viewport. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXgnMdAAoJEN0/YqbEcdMwk+IP/2DZH4tPgtCN/PHoB/ZdsVwJ oodz6Sax5LZUaumbXZme4CPgPZKSKr44ue3qho/5cQicRISuJ3G6I6vFmkiiHol6 nFmY9cTJiNeXzd6NcJq5KK6IUmgrzO5YjGgXIbzgzEiTycbws/lo5fVwF74LSYn7 RMPg+U4qMaHVT/w/rb7dWWqPugSbm8cIGfp8Zjz2VrO+JX+S1Qs3ymCZViyMfCRz A1VtyJh/u72GlfCoRoUiT6DDsMjw1BCMogR4asDkQmoTdkc7XFRU3MYQwUN7cFY2 bxos78d6xozUwvDRoezrtRLh4gQis9gudaNYv4Dt4fZjNOSvCtA8fMbl35gXuJog ooROiaXSoW5oBrzv++Fvp38cixS0j9MMECNDgs1yCCKt0a/B4/IFEQMvyGt5LzRG zeJ2XzDEZI3ntsbeVdKPRuH6m7ROlQLfp+Q0ac+MrhJSQtnAd88mXVfU5U8330b4 DNyeSmyPhBpaRp62WZxMCeOct7psy1p4LYkbbE5QZ3eFS2QhHlnPC6PzNtcUbDqI RG4J+ZhquDnT57ViyjX1vKt6tXPlDOkwJf0AApwQ84OGs7ATNci/7Z1WuyULx9sR xPrpNYUPbFIxcadmvaa7jO+aaOXoNWFQQz7rJdCVBxUY1T4Bz8+FyzQ0O8Ld/xNj b7LndQJz3AbU//fQ1gdO =8gsI -----END PGP SIGNATURE-----

July 10, 2016

Re: [PATCH 05/12] d3drm: Implement IDirect3DRMDevice*::InitFromD3D.

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Hi, Patches 1-4 look good to me, once the fixes you mentioned and we talked about on IRC are implemented. Am 2016-07-07 um 14:52 schrieb Aaryaman Vasishta: > struct d3drm_frame > { > + struct d3drm_object obj; This seems unrelated. > + if (device->ddraw) > + hr = D3DRMERR_BADOBJECT; I guess you're not returning here right away because you want to addref d3drm later. Wouldn't it be better to move those AddRefs in front in this case? The QueryInterface / GetRenderTarget code below is unlikely to fail, if it does something is severely wrong (A d3d device without a render target???) and getting the reference leak right is the last of your worries. > device->width = desc.dwWidth; > device->height = desc.dwHeight; If I read the diff right this code will set width and height to 0 on an already initialized device. > static HRESULT WINAPI d3drm_device3_GetDirect3DDevice2(IDirect3DRMDevice3 *iface, IDirect3DDevice2 **d3d_device) > { > struct d3drm_device *device = impl_from_IDirect3DRMDevice3(iface); > + HRESULT hr; > > TRACE("iface %p, d3d_device %p.\n", iface, d3d_device); > > - IDirect3DDevice_QueryInterface(device->device, &IID_IDirect3DDevice2, (void**)d3d_device); > + if (FAILED(hr = IDirect3DDevice_QueryInterface(device->device, &IID_IDirect3DDevice2, (void**)d3d_device))) > + hr = D3DRMERR_BADOBJECT; > > - return D3DRM_OK; > + return hr; > } I think this change (like the device2_GetDirect3DDevice2 one) can be moved to a separate patch. > + hr = IDirect3DRMDevice_InitFromD3D(device1, d3d1, d3ddevice1); > + ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); > + ref3 = get_refcount((IUnknown *)d3ddevice1); > + ok(ref3 > device_ref2, "Expected ref3 > device_ref2, got ref3 = %u, device_ref2 = %u.\n", ref3, device_ref2); > + ref3 = get_refcount((IUnknown *)d3d1); > + ok(ref3 > d3d_ref2, "Expected ref3 > d3d_ref2, got ref3 = %u, d3d_ref2 = %u.\n", ref3, d3d_ref2); You forgot to test the reference of d3drm1. > + hr = IDirect3DRMDevice_QueryInterface(device1, &IID_IDirect3DRMDevice2, (void **)&device2); > + ok(SUCCEEDED(hr), "Cannot get IDirect3DRMDevice2 Interface (hr = %x).\n", hr); > + hr = IDirect3DRMDevice2_GetDirect3DDevice2(device2, &d3ddevice2); > + ok(SUCCEEDED(hr), "Expected hr == D3DRM_OK, got %#x.\n", hr); > + ok(d3ddevice2 == NULL, "Expected d3ddevice2 == NULL, got %p.\n", d3ddevice2); > + IDirect3DRMDevice2_Release(device2); You forgot d3ddevice2 = 0xdeadbeef here. > + hr = IDirect3DRM2_CreateObject(d3drm2, &CLSID_CDirect3DRMDevice, NULL, &IID_IDirect3DRMDevice2, (void **)&device2); > + ok(SUCCEEDED(hr), "Cannot get IDirect3DRMDevice2 interface (hr = %#x).\n", hr); > + > + hr = IDirectDraw_QueryInterface(ddraw1, &IID_IDirect3D, (void **)&d3d1); > + ok(SUCCEEDED(hr), "Cannot get IDirect3D interface (hr = %x).\n", hr); > + if (SUCCEEDED(hr = IDirect3DDevice2_QueryInterface(d3ddevice2, &IID_IDirect3DDevice, (void **)&d3ddevice1))) > + { > + hr = IDirect3DRMDevice2_InitFromD3D(device2, d3d1, d3ddevice1); > + ok(hr == E_NOINTERFACE, "Expected hr == E_NOINTERFACE, got %#x.\n", hr); What happens if you QI IDirect3DRMDevice1 from IDirect3DRMDevice2 and call InitFromD3D on it? I would expect it to succeed because common decency suggests that CreateObject doesn't leave a history of which interface was requested and there's no CLSID_CDirect3DRMDevice2 or CLSID_CDirect3DRMDevice3. Still worth a check though I think. Cheers, Stefan -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXgmxrAAoJEN0/YqbEcdMw9dQP/RbovFFjwIdo5ho2BcxWi1Sm 68U7n+sX+HAPmHPc2W/fOK1scbPaQz84e3cHj+dsFClCUKNLUpZ1eGdJDu1u9Fgh ZJVC9hgw8rzC+IDGr5vdmuzzOj65mI7KslJX9CfD9tnXigLtlvPf3me8jnhjL51b Eq8N1rStZTh805gDBEnmr/oqgQFkRoHKgs5lLIWq2inAfF3dq0BiThBGpC+eq19H YcihqOJWgSXrbXQYtTPjFtjp3jqqFa32+/ZQufCEAELcYUjM2gG23+O/rT5sa4d2 QuQOIhdUnTR9G6WnoZXgxKwxtJTvTu02GKlk5gaIkpWFJ2O+mPJuiBmylLoucQ6V BkYkhORuT2WYKainbTx3qNJaxBPSa5/y4HWTpQhzYH0LryFBLkL8MiVjO+pGk7QM 4wHTeL4ZXmjtyYOs6qfuxNIEJyfDs0hU5850lWdVD6MLaj3JwZYRLedCJ9TwV7Jb Qz3JQIK6YPOgTSqehIGTwzxgcbvjjiLP2zalEV4HOIqKeEkodcBxVS0QiXSDbuAj gLhbqwlRMshT5Dtt1Umi2KUoGljFjh1rbe42cyVxXcoa72D+g9LQUb4d0hUP/Ppt 6lNhfq+Vf2W+FmdB2Hv/WWof7vBV59r/TwjjExrN00yRGCLNLF3BaK8lzjeJprGO H6J9OAsscEyzMNpB+l0W =exYP -----END PGP SIGNATURE-----

July 10, 2016

Re: [PATCH 1/2] d3d9/tests/visual: Add Vertexbuffer locking tests

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Hi Patrick, I ran your tests on one of my Nvidia machines, see below. Thanks for looking into this. I had problems with a few games (3DMark2001, Call of Duty Modern Warfare) doing stupid buffer / texture mapping calls that forced my command stream code to wait. Your findings may have some answers to why they work better on Windows. Please port this test to ddraw, d3d8, d3d10 and d3d11 as well. I know it's a lot of work, but the APIs might have different behavior for compatibility reasons, and I know at least one ddraw game that requires that we ignore redundant D3DLOCK_DISCARD locks (see commit d3520ef4, bug 32485). Am 2016-07-08 um 17:15 schrieb Patrick Rudolph: > + { > + "D3DLOCK_NOOVERWRITE", > + D3DLOCK_NOOVERWRITE, > + FALSE, > + TRUE, > + TRUE > + }, > + { It seems that this is not true on my Nvidia card: > visual.c:21966: Driver string: "nvd3dum.dll" > visual.c:21967: Description string: "NVIDIA GeForce GT 650M" > visual.c:21970: Device name string: "\\.\DISPLAY1" > visual.c:21972: Driver version 10.18.13.5900 > ... > visual.c:5503: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE. Dynamic = 0. > visual.c:5503: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 0. If AMD GPUs accept D3DLOCK_NOOVERWRITE on non-dynamic buffers we can pick any behavior in Wine because games can't rely on it on Windows. I'll run the test on AMD GPUs later, currently all my machines with AMD GPUs are in a rather bad shape :-\ . > + hr = IDirect3DDevice9_SetRenderState(device, D3DRS_FOGENABLE, FALSE); > + ok(SUCCEEDED(hr), "Failed to disable fog, hr %#x.\n", hr); This is redundant. > + /* sanity check */ > + hr = IDirect3DDevice9_CreateVertexBuffer(device, sizeof(quad_strip1), 0, 0, D3DPOOL_DEFAULT, &buffer, NULL); > + ok(SUCCEEDED(hr), "Failed to create vertex buffer, hr %#x.\n", hr); Is there any place where this would fail, but the other tests succeed? My sense is that if something as basic as a vertex buffer draw is broken we'd have mass test failures anyway. > + /* Increase triangle draw count for faster GPUs. > + Limit size to 0x40000 as bigger values causes the Nvidia driver to crash. */ As mentioned I'll test this on other machines as well, it might make sense to use a pixel shader that calculates square roots in a loop to slow down the GPU if the current setup is unreliable. > + if (tests[i].dynamic) > + hr = IDirect3DDevice9_CreateVertexBuffer(device, size, D3DUSAGE_DYNAMIC, 0, D3DPOOL_DEFAULT, &buffer, NULL); > + else > + hr = IDirect3DDevice9_CreateVertexBuffer(device, size, 0, 0, D3DPOOL_DEFAULT, &buffer, NULL); You could store a DWORD of flags in the tests structure and set it either to 0 or D3DUSAGE_DYNAMIC. That way you can avoid the branch and it makes the tests array easier to read IMO. > + hr = IDirect3DVertexBuffer9_Lock(buffer, 0, size, (void **)&quad_strip_array, 0); > + ok(SUCCEEDED(hr), "Failed to lock vertex buffer, hr %#x.\n", hr); > + if (!SUCCEEDED(hr) || !quad_strip_array) > + { > + IDirect3DVertexBuffer9_Release(buffer); > + continue; > + } You don't really need the if branch. If the call fails the ok() call will already mean that the test fails, and we don't really care about error handling after that. I.e., if you have a test failure it's no problem if you segfault afterwards. (It would matter if you ok is marked todo_wine or has a broken() clause because it fails in some cases on Windows.) > + ok(hr == D3D_OK, "Failed to set stream source, hr %#x.\n", hr); Please consistently use SUCCEEDED(hr). Yeah, I know the existing code isn't perfect regarding this :-( . > + hr = IDirect3DVertexBuffer9_Lock(buffer, (quads - 1) * sizeof(quad_strip1), sizeof(quad_strip1), (void **)&quad_strip_array, D3DLOCK_READONLY); What is the purpose of the readonly lock here? I think it would be interesting to test how D3DLOCK_READONLY behaves regarding synchronization too. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXgk0zAAoJEN0/YqbEcdMwDDMP/3lJ/4Uu4hHSy9KWeLz5wDxH mq9cmVCon/34BlPOTAzIvYJ1tEoFnNGrXur5iuYgOuCq/4m8TZA7W10pCYEJ7yYE xS90d2udHsxYBEBoPDXzUff5GFdkRv53AX6IdYc2BmuPN+jA9+j51TxN9qWqYMHK XNV76BDZC8EZ+mQyTov82CsSsn0K3TvhBa/1njDlbUvuNuvuaDm7qzBMqooZycgv AejEiI/dwHPyu0dmAJxHjpEJl5FozdOELJwoRtXFCp2LANnWqCAugKkjD9O1kQsb u0/iKowANpHkP+VS1PkG5dZeIEJjM2lHlopUigwjeBY/SQ/Pdlh2wfJT3HWGKnEo mnACOI4xHGp5XyNX6iit1W5mjRtv0ZiMNaTawjvptTfMM54XopsuMw+hAK+6itLo 7fgccAGUhZo+Uun2p5ceZSr+2+J7q3TUtWlh633m8yW1jz/82jer2soyDoiFw+ap uo3+w8iQ2RUTrmD2kJ7dh0ZSaHeqSh522yi3fExbyyyFQiGd5S1CBaUgrHdnt56Z ciQWEb/hhJyAhLhlbjqJuaYaaozBwXrWCkpX2uRluOoMZFN3H7mi76jtNLys8mOO eQRQrw1j9vuN/vxEz1yg5NQMvV9PImrc1p56HYn2NXMqVptABjhcnyRpNwnB2ar+ PH/xwUDQRY9CTGRkt4dR =d9bG -----END PGP SIGNATURE-----

July 10, 2016

Re: [v2 01/10] webservices: Add support for writing double values.

by Sebastian Lackner

On 08.07.2016 10:20, Hans Leidekker wrote: > v2: Fix build failure. > > Signed-off-by: Hans Leidekker <hans(a)codeweavers.com> > --- > dlls/webservices/reader.c | 8 +-- > dlls/webservices/tests/writer.c | 113 +++++++++++++++++++++++++++++++++ > dlls/webservices/webservices_private.h | 2 + > dlls/webservices/writer.c | 100 +++++++++++++++++++++++++++++ > include/webservices.h | 5 ++ > 5 files changed, 224 insertions(+), 4 deletions(-) > Unfortunately your patch does not work as expected here (gcc 5.3): ../../../tools/runtest -q -P wine -T ../../.. -M webservices.dll -p webservices_test.exe.so writer && touch writer.ok writer.c:358: Test failed: 1941: got 11 expected 10 writer.c:360: Test failed: 1941: got <t>:E-3</t>>000000004</t> expected <t>1E-2</t> writer.c:358: Test failed: 1941: got 12 expected 11 writer.c:360: Test failed: 1941: got <t>-:E-3</t>000000004</t> expected <t>-1E-2</t> Makefile:346: die Regel für Ziel „writer.ok“ scheiterte If you need any details to investigate this issue please let me know. Regards, Sebastian

July 9, 2016

Re: explorerframe/tests: Use SetRect() instead of open coding it.

by Michael Stefaniuc

On 07/09/2016 02:09 PM, Marvin wrote: > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? > Full results can be found at > https://testbot.winehq.org/JobDetails.pl?Key=24129 > > Your paranoid android. > > > === w7pro64 (64 bit nstc) === > nstc.c:1953: Test failed: Got event 17, count 1 This is a spurious error on Win7 according to the daily test results. For whatever reason it seems to fail more reliable when run standalone as all my 3 patches produced it. bye michael

July 9, 2016

Re: explorerframe/tests: Use EqualRect() instead of open coding it.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24131 Your paranoid android. === w7pro64 (32 bit nstc) === nstc.c:1941: Test failed: Got event 17, count 1 === w7pro64 (64 bit nstc) === nstc.c:1951: Test failed: Got event 17, count 1

July 9, 2016

Re: explorerframe/tests: The width is between the RECT left and right fields

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24130 Your paranoid android. === w7pro64 (64 bit nstc) === nstc.c:1946: Test failed: Got event 17, count 1

July 9, 2016

Re: explorerframe/tests: Use SetRect() instead of open coding it.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24129 Your paranoid android. === w7pro64 (64 bit nstc) === nstc.c:1953: Test failed: Got event 17, count 1

July 9, 2016

Re: [PATCH v2 2/3] winex11.drv: avoid passing NULL to memcpy (clang)

by Alexandre Julliard

Kirill Smirnov <kirill.k.smirnov(a)gmail.com> writes: > According to the standard, memcpy() and its friends > should never accept NULL as argument, otherwise > the behavior is undefined and optimizer takes this > into account. > > Below in the same file (line 206) caller passes > NULL as lpComp argument triggering this case. > > This patch is of the same nature as the following: > > commit 5a3ad0ecf08c00fc0b7b2f24e55be27b7c3f5da0 > Author: Kirill Smirnov <kirill.k.smirnov(a)gmail.com> > Date: Mon May 9 18:56:14 2016 +0300 > > winhlp32: Do not pass NULL to strchr() (spotted by clang). strchr has to access the pointer so obviously it can't take NULL, but memcpy with a zero length shouldn't, so it's not very useful compiler behavior IMO. Sadly it seems the standard does allow it... -- Alexandre Julliard julliard(a)winehq.org

July 9, 2016

Re: [PATCH 2/2] wined3d/resource: Performance improvements

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24126 Your paranoid android. === w8 (32 bit visual) === visual.c:4578: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 0. visual.c:4578: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 1. === w864 (32 bit visual) === visual.c:4578: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 0. visual.c:4578: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 1. === w1064 (32 bit visual) === visual.c:4578: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 0. visual.c:4578: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 1. === w864 (64 bit visual) === visual.c:4578: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 0. visual.c:4578: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 1. === w1064 (64 bit visual) === visual.c:4578: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 0. visual.c:4578: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 1.

July 8, 2016

Re: [PATCH 1/2] d3d9/tests/visual: Add Vertexbuffer locking tests

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24125 Your paranoid android. === w8 (32 bit visual) === visual.c:4573: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 0. visual.c:4573: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 1. === w864 (32 bit visual) === visual.c:4573: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 0. visual.c:4573: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 1. === w1064 (32 bit visual) === visual.c:4573: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 0. visual.c:4573: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 1. === w864 (64 bit visual) === visual.c:4573: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 0. visual.c:4573: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 1. === w1064 (64 bit visual) === visual.c:4573: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 0. visual.c:4573: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 1.

July 8, 2016

Re: [PATCH] xmllite/writer: Support trivial case of WriteAttributeString()

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24119 Your paranoid android. === build (build) === Patch failed to apply

July 8, 2016

Re: [PATCH 2/2] wscript: Added WScript.Quit tests.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24118 Your paranoid android. === build (build) === Patch failed to apply

July 8, 2016

Re: [PATCH 5/5] d3d11/tests: Add test for immediate constant buffer.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24114 Your paranoid android. === build (build) === Patch failed to apply

July 8, 2016

Re: [PATCH 3/5] d3d10core/tests: Add test for vertex formats.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=24113 Your paranoid android. === build (build) === Patch failed to apply

July 8, 2016