October 2012 - Wine-Devel - list.winehq.org

vcomp: single-threaded implementation of _vcomp_fork (try 6)

by Joerg-Cyril.Hoehle＠t-systems.com

Hi, Alexandre Julliard wrote: > +void WINAPIV _vcomp_fork(BOOL ifval, int nargs, void *wrapper, ...) > + __ms_va_list valist; >it's ugly to use varargs only as a hack to get a pointer to the first >argument. Would taking the address of &wrapper (as in asm("...","r=&wrapper") or any C code like "void* stack = &wrapper") suffice to tell the x64 compiler to save register parameters into their allocated stack locations? My belief was that this is specifically the magic performed when the compiler sees a [__ms_]va_list. You seem to imply that va_list is superfluous and that taking the address of any parameter (not necessarily &wrapper but &ifval or &nargs too) would achieve the same effect: all 4 registers rcx, rdx, r8 and r9 get saved to the stack such that the & abstraction works even when the user performs pointer arithmetic. This is the effect that Dan Kegel needs for vcomp. In such a case he would use &wrapper, bypass va_list and correct the stack-relative offsets in the x86 and x64 assembly code by one pointer unit. Regards, Jörg Höhle

Oct. 24, 2012

Re: [PATCH 1/5] d3dx9_36: Implement D3DXFileCreate. (try 3)

by Christian Costa

2012/10/24 Dmitry Timoshkov <dmitry(a)baikal.ru> > Christian Costa <titan.costa(a)gmail.com> wrote: > > > +static HRESULT WINAPI ID3DXFileImpl_QueryInterface(ID3DXFile *iface, > REFIID riid, void **ret_iface) > > +{ > > + TRACE("(%p)->(%s, %p)\n", iface, debugstr_guid(riid), ret_iface); > > + > > + if (IsEqualGUID(riid, &IID_IUnknown) || > > + IsEqualGUID(riid, &IID_ID3DXFile)) > > + { > > + iface->lpVtbl->AddRef(iface); > > Isn't there an appropriate xxx_AddRef() macro? > No. > > > + *ret_iface = iface; > > + return S_OK; > > + } > > + > > + ERR("(%p)->(%s, %p), not found\n", iface, debugstr_guid(riid), > ret_iface); > > FIXME seems more appropriate here. > No. All interfaces are implemented here. > > > + *ret_iface = NULL; > > + return E_NOINTERFACE; > > +} > > ... > > > +HRESULT WINAPI D3DXFileCreate(ID3DXFile **dxfile) > > +{ > > + ID3DXFileImpl *object; > > + > > + TRACE("(%p)\n", dxfile); > > + > > + object = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, > sizeof(*object)); > > What's the reason for HEAP_ZERO_MEMORY here? > It's common to do it this way. Only non zero value are set on creation. Usually several members are set in methods. > > > + if (!object) > > + { > > + ERR("Out of memory\n"); > > + return E_OUTOFMEMORY; > > + } > > The ERR() is useless here, just return E_OUTOFMEMORY in such situations. > > > It's done this way in many places in wine.

Oct. 24, 2012

Re: [PATCH] user32: Fix error handling in MapWindowPoints, ClientToScreen and ScreenToClient and add tests for them. (try 3)

by Nikolay Sivov

On 10/24/2012 11:33, Christian Costa wrote: > > > 2012/10/24 Dmitry Timoshkov <dmitry(a)baikal.ru <mailto:dmitry(a)baikal.ru>> > > Christian Costa <titan.costa(a)gmail.com > <mailto:titan.costa(a)gmail.com>> wrote: > > > BOOL WINAPI ClientToScreen( HWND hwnd, LPPOINT lppnt ) > > { > > + DWORD error = GetLastError(); > > + > > + if (!hwnd) > > + { > > + SetLastError( ERROR_INVALID_WINDOW_HANDLE ); > > + return FALSE; > > + } > > + > > + SetLastError( 0xdeadbeef ); > > MapWindowPoints( hwnd, 0, lppnt, 1 ); > > + > > + if (GetLastError() != 0xdeadbeef) > > + return FALSE; > > + > > + SetLastError(error); > > return TRUE; > > } > > As been said before these games with saving/restoring last error value > are broken. > > > Last time you said wrong so what do you mean by wrong or broken? > The only way to know if MapWindowPoints fails is to set last error > first and check it > after. If I don't restore the previous value, the tests will not pass. > Of course I can > arrange the tests but hey. And I can check windows handle here as > Alexandre said. If error reporting from MapWindowPoints is not sufficient to use it somewhere else you can always move its guts to a separate function and report failure as you want. By the way, it only fails on invalid window handles I guess, is that right? If you need this behaviour you have a failing WIN_GetPtr() in this case. Messing with last error is not a solution obviously, and I don't see where MapWindowPoints even sets it.

Oct. 24, 2012

Re: [PATCH 1/2] user32: Remove redundant sizeof check.

by Ken Thomases

On Oct 24, 2012, at 3:24 AM, Michael Stefaniuc wrote: > @@ -1330,9 +1330,6 @@ static HICON CURSORICON_LoadFromFile( LPCWSTR filename, > } > > dir = (const CURSORICONFILEDIR*) bits; > - if ( filesize < sizeof(*dir) ) > - goto end; > - > if ( filesize < (sizeof(*dir) + sizeof(dir->idEntries[0])*(dir->idCount-1)) ) > goto end; > That doesn't seem redundant to me. It's not safe to access dir->idCount if the file isn't known to be big enough. Regards, Ken

Oct. 24, 2012

Re: [PATCH] user32: Fix error handling in MapWindowPoints, ClientToScreen and ScreenToClient and add tests for them. (try 3)

by Christian Costa

2012/10/24 Dmitry Timoshkov <dmitry(a)baikal.ru> > Christian Costa <titan.costa(a)gmail.com> wrote: > > > BOOL WINAPI ClientToScreen( HWND hwnd, LPPOINT lppnt ) > > { > > + DWORD error = GetLastError(); > > + > > + if (!hwnd) > > + { > > + SetLastError( ERROR_INVALID_WINDOW_HANDLE ); > > + return FALSE; > > + } > > + > > + SetLastError( 0xdeadbeef ); > > MapWindowPoints( hwnd, 0, lppnt, 1 ); > > + > > + if (GetLastError() != 0xdeadbeef) > > + return FALSE; > > + > > + SetLastError(error); > > return TRUE; > > } > > As been said before these games with saving/restoring last error value > are broken. > > > Last time you said wrong so what do you mean by wrong or broken? The only way to know if MapWindowPoints fails is to set last error first and check it after. If I don't restore the previous value, the tests will not pass. Of course I can arrange the tests but hey. And I can check windows handle here as Alexandre said.

Oct. 24, 2012

Re: [PATCH] user32: Fix error handling in MapWindowPoints, ClientToScreen and ScreenToClient and add tests for them. (try 3)

by Dmitry Timoshkov

Christian Costa <titan.costa(a)gmail.com> wrote: > BOOL WINAPI ClientToScreen( HWND hwnd, LPPOINT lppnt ) > { > + DWORD error = GetLastError(); > + > + if (!hwnd) > + { > + SetLastError( ERROR_INVALID_WINDOW_HANDLE ); > + return FALSE; > + } > + > + SetLastError( 0xdeadbeef ); > MapWindowPoints( hwnd, 0, lppnt, 1 ); > + > + if (GetLastError() != 0xdeadbeef) > + return FALSE; > + > + SetLastError(error); > return TRUE; > } As been said before these games with saving/restoring last error value are broken. -- Dmitry.

Oct. 24, 2012

Re: [PATCH 1/5] d3dx9_36: Implement D3DXFileCreate. (try 3)

by Dmitry Timoshkov

Christian Costa <titan.costa(a)gmail.com> wrote: > +static HRESULT WINAPI ID3DXFileImpl_QueryInterface(ID3DXFile *iface, REFIID riid, void **ret_iface) > +{ > + TRACE("(%p)->(%s, %p)\n", iface, debugstr_guid(riid), ret_iface); > + > + if (IsEqualGUID(riid, &IID_IUnknown) || > + IsEqualGUID(riid, &IID_ID3DXFile)) > + { > + iface->lpVtbl->AddRef(iface); Isn't there an appropriate xxx_AddRef() macro? > + *ret_iface = iface; > + return S_OK; > + } > + > + ERR("(%p)->(%s, %p), not found\n", iface, debugstr_guid(riid), ret_iface); FIXME seems more appropriate here. > + *ret_iface = NULL; > + return E_NOINTERFACE; > +} ... > +HRESULT WINAPI D3DXFileCreate(ID3DXFile **dxfile) > +{ > + ID3DXFileImpl *object; > + > + TRACE("(%p)\n", dxfile); > + > + object = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*object)); What's the reason for HEAP_ZERO_MEMORY here? > + if (!object) > + { > + ERR("Out of memory\n"); > + return E_OUTOFMEMORY; > + } The ERR() is useless here, just return E_OUTOFMEMORY in such situations. -- Dmitry.

Oct. 24, 2012

Re: d3dx9_36/tests: Fix sizeof to be more portable (coverity)

by André Hentschel

Am 24.10.2012 02:32, schrieb Henri Verbeet: > On 24 October 2012 02:05, André Hentschel <nerv(a)dawncrow.de> wrote: >> face_remap = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, >> - tc[i].num_faces*sizeof(face_remap)); >> + tc[i].num_faces*sizeof(DWORD*)); >> > I'm don't know what Coverity actually says about this, but this looks > just as wrong to me. It's probably supposed to be > "sizeof(*face_remap)". That's actually what Coverity suggested, but i thought i've seen more sizeofs of this kind. -- Best Regards, André Hentschel

Oct. 24, 2012

Re: d3dx9_36/tests: Fix sizeof to be more portable (coverity)

by Henri Verbeet

On 24 October 2012 02:05, André Hentschel <nerv(a)dawncrow.de> wrote: > face_remap = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, > - tc[i].num_faces*sizeof(face_remap)); > + tc[i].num_faces*sizeof(DWORD*)); > I'm don't know what Coverity actually says about this, but this looks just as wrong to me. It's probably supposed to be "sizeof(*face_remap)".

Oct. 24, 2012

We're in at FOSDEM!

by Jeremy White

I'm stoked - we've been approved for a dev room at FOSDEM on Saturday, February 2nd, in Brussels. It is just the one day, but I figure we can find ways to gather on Friday night and/or Sunday as well. I really think this will be a fun change of pace for us. If it doesn't work out, we'll just go camp out in Marcus's back yard instead <evil grin>. One thing they require from us is a pre set schedule, so I'll be asking people to suggest topics, and then we'll have to weed through them to figure out what makes the most sense. Cheers, Jeremy

Oct. 23, 2012