January 2010 - Wine-Devel - list.winehq.org

Re: [PATCH] Implement OleCreatePropertyFrame (try 3)

by Geoffrey Hausheer

Is there anything I can do to move this patch forward? I've fixed all the issues that have been raised so far. I have no dea what it is waiting on, and if there is anything I can do to get it committed or if it is just a question of being patient. As I said previously I'd happily write a test for it, but I couldn't find any examples of how to test a modal dialog box in Wine, so I just don't know what the right way to do that is.

Jan. 29, 2010

Re: [2/5] d3d9: Test invalid buffer lock parameters

by Stefan Dösinger

On Friday 29 January 2010 12:21:27 Henri Verbeet wrote: > On 29 January 2010 11:35, Stefan Dösinger <stefan(a)codeweavers.com> wrote: > > +static HMODULE d3d9_handle = 0; > > This is redundant. > > > + hr = IDirect3DVertexBuffer9_Lock(buffer, 0, 0, (void *) &bufstart, > > 0); + ok(hr == D3D_OK, "IDirect3DVertexBuffer9_Lock failed, 0x%08x\n", > > hr); + hr = IDirect3DVertexBuffer9_Unlock(buffer); > > + ok(hr == D3D_OK, "IDirect3DVertexBuffer9_Unlock failed, 0x%08x\n", > > hr); + > > + hr = IDirect3DVertexBuffer9_Lock(buffer, 0, 1024, (void *) &data, > > 0); + ok(hr == D3D_OK, "IDirect3DVertexBuffer9_Lock failed, 0x%08x\n", > > hr); + hr = IDirect3DVertexBuffer9_Unlock(buffer); > > + ok(hr == D3D_OK, "IDirect3DVertexBuffer9_Unlock failed, 0x%08x\n", > > hr); + ok(bufstart == data, "Two locks returned different addresses: > > %p, %p\n", bufstart, data); > > Why would we care if the buffer is always mapped at the same address? > Also note that the test doesn't necessarily prove this is the case. It's more like a sanity check since all the other tests testing the handling of invalid offsets implicitly make the same assumption. I could make this an if(...) skip(); instead of an ERR. As for why we care: We don't really care that it's this specific result, since I don't know any app that depends on this. I mainly want to test the return value, and secondarily show that d3d doesn't in some way 'sanitize' the range(e.g. set offset=0, length=size), but instead of trying to show what is not returned the test just shows that the normal math from the non-error cases is applied. > > + /* This happily wraps around the address space */ > > + hr = IDirect3DVertexBuffer9_Lock(buffer, ~0U, 1024, (void *) &data, > > 0); + ok(hr == D3D_OK, "IDirect3DVertexBuffer9_Lock(start ~0U) failed, > > 0x%08x\n", hr); + hr = IDirect3DVertexBuffer9_Unlock(buffer); > > + ok(hr == D3D_OK, "IDirect3DVertexBuffer9_Unlock failed, 0x%08x\n", > > hr); + ok(bufstart -1 == data, "~0U start lock returned unexpected > > address: expected %p, got %p\n", bufstart - 1, data); > > I'd expect this to break on 64 bit. Also, like the previous test, why > do we care? Do you seriously have an application that does this? True, it'll break, I'll remove the address check(but keep the hr == D3D_OK check).

Jan. 29, 2010

Re: relay log problem

by Alexandre Julliard

Michael Stefaniuc <mstefani(a)redhat.com> writes: > Anyway the usage of write(2) in NTDLL_dbg_vprintf() is *wrong* as it > doesn't check for short writes nor errors. It is good enough for debug > output and better than using the FILE * functions but not good enough to > prevent interleaving. It even introduces an opportunity for parts of a > line to be overwritten in the case of short writes. The usage is correct, checking for short writes or errors in debug output would be much more wrong, and O_APPEND does a fine job at preventing interleaving. More importantly, there's no other sane way to do it. And I doubt you'd be able to trigger short writes in practice when writing to a log file. -- Alexandre Julliard julliard(a)winehq.org

Jan. 29, 2010

Re: [PATCH 3/5] wined3d: Prune invalid states from the state table.

by Stefan Dösinger

On Friday 29 January 2010 12:02:39 Henri Verbeet wrote: > You also > seem to have a problem with storing derived limits in > wined3d_gl_limits, but I don't see why. Where the limit is stored is a > separate issue from what it contains though. The name mostly. MaxSimultaneousTextures=8(with arbfp) doesn't come from any GL limit. Plus, as a minor point, the initialization time. The d3d limits on paper depend on the device creation params, the gl limits do not. > > E.g. the app creates the device with > > D3DCREATE_SOFTWARE_VERTEXPROCESSING. This way the vertex pipeline's limits > > don't have any relation to the GL vertex limit > > ... > Which we don't actually implement, and probably never will. In > practice, all the limits are adapter limits, and things like shader > backend and fragment pipe could be selected during adapter > initialization instead of device initialization as well. If that ever > changes it's of course trivial to split gl_info into a device specific > part and an adapter specific part, or even move the entire thing to > the device. > > I think this is a silly distinction, D3D limits are clearly derived > from the capabilities of the GL implementation. In fact, a number of > limits in there are derived limits, because they take things like > quirks and registry settings into account. I'd be willing to rename > "struct wined3d_gl_limits" to "struct wined3d_limits" though. My preference would be this: Put max_ffp_textures, max_ffp_texture_stages, the shader constant limits into a device->limits structure and initialize it at device creation time based on what the pipeline and shader backends report. Alternatively, I am fine with renaming wined3d_gl_limits and selecting the pipeline and shader backend during adapter initialization as you suggest, as long as we separate the nvrc combiner limit from the d3d texture stage limit. If we ever implement a software vertex processing pipeline moving the d3d limit initialization back to the device creation code will be the least concern.

Jan. 29, 2010

Re: relay log problem

by Michael Stefaniuc

Alexandre Julliard wrote: > Michael Stefaniuc <mstefani(a)redhat.com> writes: > >> Incorrect. Wine doesn't use write(2) but vfprintf(3) which operates on >> FILE handles; the output is buffered aka a vfprintf call won't translate >> 1:1 to a write(2) system call. But even if it would directly use >> write(2), that system call isn't atomic. A lot of people consider a >> write smaller/equal the PAGE_SIZE to be atomic but it isn't; one can get >> a short write because of an EINTR. >> >> The ">>" redirection fixes the overwriting corruption but not the >> interleaving one. With multiple threads and enough debug output the >> chances are high to get a few interleaving corruption; i see it >> regularly in the output of winetest.exe runs. > > No, Wine explicitly uses write(2) for debug output, and is careful about > writing only complete lines precisely to avoid the interleaving issue. Hmm ... I have looked at libs/wine/debug.c and not at NTDLL_dbg_vprintf(). Anyway the usage of write(2) in NTDLL_dbg_vprintf() is *wrong* as it doesn't check for short writes nor errors. It is good enough for debug output and better than using the FILE * functions but not good enough to prevent interleaving. It even introduces an opportunity for parts of a line to be overwritten in the case of short writes. > It does require O_APPEND to ensure that the writes don't step over each > other, which is why you need ">>". It definitely makes a difference. Yes it does; it didn't disagree with this fact. bye michael

Jan. 29, 2010

Re: [2/5] d3d9: Test invalid buffer lock parameters

by Henri Verbeet

On 29 January 2010 11:35, Stefan Dösinger <stefan(a)codeweavers.com> wrote: > +static HMODULE d3d9_handle = 0; This is redundant. > + hr = IDirect3DVertexBuffer9_Lock(buffer, 0, 0, (void *) &bufstart, 0); > + ok(hr == D3D_OK, "IDirect3DVertexBuffer9_Lock failed, 0x%08x\n", hr); > + hr = IDirect3DVertexBuffer9_Unlock(buffer); > + ok(hr == D3D_OK, "IDirect3DVertexBuffer9_Unlock failed, 0x%08x\n", hr); > + > + hr = IDirect3DVertexBuffer9_Lock(buffer, 0, 1024, (void *) &data, 0); > + ok(hr == D3D_OK, "IDirect3DVertexBuffer9_Lock failed, 0x%08x\n", hr); > + hr = IDirect3DVertexBuffer9_Unlock(buffer); > + ok(hr == D3D_OK, "IDirect3DVertexBuffer9_Unlock failed, 0x%08x\n", hr); > + ok(bufstart == data, "Two locks returned different addresses: %p, %p\n", bufstart, data); > Why would we care if the buffer is always mapped at the same address? Also note that the test doesn't necessarily prove this is the case. > + /* This happily wraps around the address space */ > + hr = IDirect3DVertexBuffer9_Lock(buffer, ~0U, 1024, (void *) &data, 0); > + ok(hr == D3D_OK, "IDirect3DVertexBuffer9_Lock(start ~0U) failed, 0x%08x\n", hr); > + hr = IDirect3DVertexBuffer9_Unlock(buffer); > + ok(hr == D3D_OK, "IDirect3DVertexBuffer9_Unlock failed, 0x%08x\n", hr); > + ok(bufstart -1 == data, "~0U start lock returned unexpected address: expected %p, got %p\n", bufstart - 1, data); > I'd expect this to break on 64 bit. Also, like the previous test, why do we care? Do you seriously have an application that does this?

Jan. 29, 2010

Re: relay log problem

by Alexandre Julliard

Michael Stefaniuc <mstefani(a)redhat.com> writes: > Incorrect. Wine doesn't use write(2) but vfprintf(3) which operates on > FILE handles; the output is buffered aka a vfprintf call won't translate > 1:1 to a write(2) system call. But even if it would directly use > write(2), that system call isn't atomic. A lot of people consider a > write smaller/equal the PAGE_SIZE to be atomic but it isn't; one can get > a short write because of an EINTR. > > The ">>" redirection fixes the overwriting corruption but not the > interleaving one. With multiple threads and enough debug output the > chances are high to get a few interleaving corruption; i see it > regularly in the output of winetest.exe runs. No, Wine explicitly uses write(2) for debug output, and is careful about writing only complete lines precisely to avoid the interleaving issue. It does require O_APPEND to ensure that the writes don't step over each other, which is why you need ">>". It definitely makes a difference. -- Alexandre Julliard julliard(a)winehq.org

Jan. 29, 2010

Re: [PATCH 3/5] wined3d: Prune invalid states from the state table.

by Henri Verbeet

On 29 January 2010 11:01, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote: > On Friday 29 January 2010 00:55:19 Henri Verbeet wrote: >> Almost. The texture_stages limit is supposed to indicate the number of >> texture stages the GL implementation can support, I don't think >> there's a reason to change that. What needs to change is that we >> assign its value based on the fragment pipeline caps instead of based >> on the GL extensions. > Texture stages are a direct3d concept. On the GL side we have > MAX_GENERAL_COMBINERS_NV, 8(atifs extension hardcoded), MAX_TEXTURES(ffp) or > the arbfp shader instruction limit. How those translate into the d3d limit is > an implementation detail of the fragment pipeline and none of the GL extension > loading code's business. > Yes, that's why the value of limits.texture_stages should be based on the fragment pipe caps instead of the detected extensions. You also seem to have a problem with storing derived limits in wined3d_gl_limits, but I don't see why. Where the limit is stored is a separate issue from what it contains though. > Plus, we need the raw GL limit intact as well. Let's take for example gl We could add e.g. the general combiner limit to wined3d_gl_limits if needed, but with that being a pipeline specific detail we might as well keep that internal to the specific implementation. If it means anything, I'm not entirely happy with shader backend specific limits being exposed through wined3d_gl_limits either. > max_textures vs gl max_samplers vs d3d MaxSimultaneousTextures. Assume a > geforce 7 card. max_textures = 4, arbfs pipeline. > > In this situation this code will result in a GL error: > glActiveTexture(GL_TEXTURE5); > glBindTexture(GL_TEXTURE_2D, tex); > glEnable(GL_TEXTURE_2D); > > However, on the d3d side this is perfectly valid and working OK: > SetPixelShader(NULL); > SetTexture(0, tex); > SetTextureStageState(0, D3DTSS_COLOROP, D3DTOP_PREVIOUS); > SetTexture(1, tex); > SetTextureStageState(1, D3DTSS_COLOROP, D3DTOP_PREVIOUS); > SetTexture(2, tex); > SetTextureStageState(2, D3DTSS_COLOROP, D3DTOP_PREVIOUS); > SetTexture(3, tex); > SetTextureStageState(3, D3DTSS_COLOROP, D3DTOP_PREVIOUS); > SetTexture(4, tex); > SetTextureStageState(4, D3DTSS_COLOROP, D3DTOP_PREVIOUS); > SetTexture(5, tex); > SetTextureStageState(5, D3DTSS_COLOROP, D3DTOP_PREVIOUS); > SetTextureStageState(6, D3DTSS_COLOROP, D3DTOP_DISABLE); > > So (gl)max_textures=4, (gl)max_samplers=16, (d3d)MaxSimultaneousTextures=8 > Sure, but I don't see how that's relevant? It's also a bit silly to enable fixed function texturing when you're using arbfs. > Furthermore we can't store MaxSimultaneousTextures in the GL limits, as it can > depend on the device creation parameters. E.g. the app creates the device with > D3DCREATE_SOFTWARE_VERTEXPROCESSING. This way the vertex pipeline's limits > don't have any relation to the GL vertex limits, and we don't know what the > app will do when we initialize the adapter. > Which we don't actually implement, and probably never will. In practice, all the limits are adapter limits, and things like shader backend and fragment pipe could be selected during adapter initialization instead of device initialization as well. If that ever changes it's of course trivial to split gl_info into a device specific part and an adapter specific part, or even move the entire thing to the device. >> > Please use the number of texture blend stages reported from the fragment >> > pipeline rather than gl_limits.texture_stages. texture_stages only works >> > with the nvrc and ffp pipelines, but not arbfp or atifs(it works out ok >> > by chance on most GL cards). >> >> If those aren't the same, the code initializing the texture_stages >> limit is broken, not the code using it. > Your code is manipulating the input into the pipeline code, it is talking in > D3D terms, so the D3D limits apply, not the GL limits. > I think this is a silly distinction, D3D limits are clearly derived from the capabilities of the GL implementation. In fact, a number of limits in there are derived limits, because they take things like quirks and registry settings into account. I'd be willing to rename "struct wined3d_gl_limits" to "struct wined3d_limits" though. > I can't find the original patch in git since it was a long time ago in spring > 08, here are a few following patches though: > 9a6bc683e11bf508a37769640d56f201462b935a > 462ddaa25404cd1a07135f5d7acdd2e298d7dfad > I guess those slipped by.

Jan. 29, 2010

Re: [2/5] d3d9: Test invalid buffer lock parameters

by Stefan Dösinger

On Friday 29 January 2010 10:35:11 Henri Verbeet wrote: > On 28 January 2010 22:50, Stefan Dösinger <stefan(a)codeweavers.com> wrote: > > + * Copyright (C) 2010 Stefan Dösinger(For CodeWeavers) > > Please use UTF-8 like the other source files. (I'd also suggest a > space and a lowercase 'F'). Oh, crappy windows editor. I'll fix this.

Jan. 29, 2010

Re: relay log problem

by Michael Stefaniuc

Ken Thomases wrote: > On Jan 28, 2010, at 5:21 PM, Michael Stefaniuc wrote: > >> On 01/28/2010 10:59 PM, Henri Verbeet wrote: >>> On 28 January 2010 22:39, Stefan Leichter<Stefan.Leichter(a)camline.com> wrote: >>>> Is this a know problem? Is a work around available? >>>> >>> Yeah, that's a known problem. As a workaround you can use ">>" >>> (append) instead of ">" to redirect to the file. You'll still need the >>> "2>&1" to redirect stderr to stdout, of course. >> That won't workaround the problem. The problem is that there are >> multiple threads/programs that write at the same time to >> stdout/stderr. I have seen it with fixme/err/warn messages too; even >> with crash dumps. > > Using >> in the shell causes the stdout file descriptor to be opened > with O_APPEND. Using 2>&1 makes stderr dup the file descriptor, so it > gets the same flag. > > With O_APPEND, the kernel ensures that all writes append to the file. > Multiple threads or programs using an O_APPEND descriptor can't > overwrite each other. If the code uses multiple writes for a single > line, then those multiple writes can be interleaved, but can't > overwrite. Right, Henri and I talked about that on irc. The "overwrite" corruption is fixed by using ">>". So yes, when writting to a log file redirecting with ">>" is better. > And, for the most part, Wine's debug logging uses single > writes so it won't even interleave. Incorrect. Wine doesn't use write(2) but vfprintf(3) which operates on FILE handles; the output is buffered aka a vfprintf call won't translate 1:1 to a write(2) system call. But even if it would directly use write(2), that system call isn't atomic. A lot of people consider a write smaller/equal the PAGE_SIZE to be atomic but it isn't; one can get a short write because of an EINTR. The ">>" redirection fixes the overwriting corruption but not the interleaving one. With multiple threads and enough debug output the chances are high to get a few interleaving corruption; i see it regularly in the output of winetest.exe runs. bye michael

Jan. 29, 2010