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

Re: [PATCH 2/2] kernel32: conditional variable support

by Nikolay Sivov

On 12/30/2012 00:58, Marcus Meissner wrote: > On Sun, Dec 30, 2012 at 12:02:34AM +0400, Nikolay Sivov wrote: >> On 12/29/2012 23:04, Marcus Meissner wrote: >>> From: Marcus Meissner <marcus(a)jet.(none)> >>> >>> Modeled pretty much after the implementation described >>> in glibc/nptl/DESIGN-condvar.txt with its futex usage >>> sofar replaced by regular Win32 Semaphores. >>> >>> An additional futex implementation is possible. >>> >>> I *think* I have the semantics correct, there is 1 small >>> place where it might have unnecessary CPU usage though. >>> >>> Ciao, Marcus >>> +/********************************************************************** >>> + * InitializeConditionVariable (KERNEL32.@) >>> + */ >>> +VOID WINAPI InitializeConditionVariable(PCONDITION_VARIABLE variable) >>> +{ >>> + struct _condition_var_intern *var; >>> + variable->Ptr = HeapAlloc(GetProcessHeap(),HEAP_ZERO_MEMORY,sizeof(struct _condition_var_intern)); >>> + var = variable->Ptr; >>> + if (!var) return; >>> + InitializeCriticalSection (&var->crit); >>> + var->crit.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": ConditionVariable.cs"); >>> +} >> I don't think it allocates anything at all, what makes me believe >> that is that stuff like CONDITION_VARIABLE_INIT exists. >> After googling a bit for it I see that gallium from mesa has some >> platform dependent stuff in code that uses this API like that: >> >> --- >> +#define pipe_static_condvar(cond) \ >> + /*static*/ pipe_condvar cond = CONDITION_VARIABLE_INIT >> --- >> >> So this structure has nothing more than a single pointer value, >> value 0 of it means it's initialized. I think you could just add >> some tests to see if it's really reset to 0 from initial value. > I am not fully sure. The SDK only has "PVOID Ptr" and that alone > is not enough space to implement conditional variables. Why it's not enough? API calls use pointer to this struct all the time so it's known which cond variable is it sleeping on for example. > > CONDITION_VARIABLE_INIT is only there to init it to NULL. > (if you look at the SDK CONDITION_VARIABLE_INIT seems to be {0} ) > > So I suspect some private data structure is hidden into it. I think it's there to initialize it statically. I believe all of that 'recently' introduced sync APIs (like InitOnce* stuff) must be based on similar mechanism internally that makes them slim and lightweight. > > I however do not know if windows allocates it from heap > or from where exactly. > >> Also regarding tests I don't think randomizing timeouts is a good >> idea cause it adds some unpredictable behaviour and it's not really >> clear if we could rely on such tests results. > I have to think about those some more :/ > > I testing Adobe Lightroom 4 with the patch I encountered more conditions > I should probably also test for. > > Ciao, Marcus >

Dec. 29, 2012

Re: [PATCH 2/2] kernel32: conditional variable support

by Marcus Meissner

On Sun, Dec 30, 2012 at 12:02:34AM +0400, Nikolay Sivov wrote: > On 12/29/2012 23:04, Marcus Meissner wrote: > >From: Marcus Meissner <marcus(a)jet.(none)> > > > >Modeled pretty much after the implementation described > >in glibc/nptl/DESIGN-condvar.txt with its futex usage > >sofar replaced by regular Win32 Semaphores. > > > >An additional futex implementation is possible. > > > >I *think* I have the semantics correct, there is 1 small > >place where it might have unnecessary CPU usage though. > > > >Ciao, Marcus > > >+/********************************************************************** > >+ * InitializeConditionVariable (KERNEL32.@) > >+ */ > >+VOID WINAPI InitializeConditionVariable(PCONDITION_VARIABLE variable) > >+{ > >+ struct _condition_var_intern *var; > >+ variable->Ptr = HeapAlloc(GetProcessHeap(),HEAP_ZERO_MEMORY,sizeof(struct _condition_var_intern)); > >+ var = variable->Ptr; > >+ if (!var) return; > >+ InitializeCriticalSection (&var->crit); > >+ var->crit.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": ConditionVariable.cs"); > >+} > I don't think it allocates anything at all, what makes me believe > that is that stuff like CONDITION_VARIABLE_INIT exists. > After googling a bit for it I see that gallium from mesa has some > platform dependent stuff in code that uses this API like that: > > --- > +#define pipe_static_condvar(cond) \ > + /*static*/ pipe_condvar cond = CONDITION_VARIABLE_INIT > --- > > So this structure has nothing more than a single pointer value, > value 0 of it means it's initialized. I think you could just add > some tests to see if it's really reset to 0 from initial value. I am not fully sure. The SDK only has "PVOID Ptr" and that alone is not enough space to implement conditional variables. CONDITION_VARIABLE_INIT is only there to init it to NULL. (if you look at the SDK CONDITION_VARIABLE_INIT seems to be {0} ) So I suspect some private data structure is hidden into it. I however do not know if windows allocates it from heap or from where exactly. > Also regarding tests I don't think randomizing timeouts is a good > idea cause it adds some unpredictable behaviour and it's not really > clear if we could rely on such tests results. I have to think about those some more :/ I testing Adobe Lightroom 4 with the patch I encountered more conditions I should probably also test for. Ciao, Marcus

Dec. 29, 2012

Re: [PATCH 2/2] kernel32: conditional variable support

by Nikolay Sivov

On 12/29/2012 23:04, Marcus Meissner wrote: > From: Marcus Meissner <marcus(a)jet.(none)> > > Modeled pretty much after the implementation described > in glibc/nptl/DESIGN-condvar.txt with its futex usage > sofar replaced by regular Win32 Semaphores. > > An additional futex implementation is possible. > > I *think* I have the semantics correct, there is 1 small > place where it might have unnecessary CPU usage though. > > Ciao, Marcus > +/********************************************************************** > + * InitializeConditionVariable (KERNEL32.@) > + */ > +VOID WINAPI InitializeConditionVariable(PCONDITION_VARIABLE variable) > +{ > + struct _condition_var_intern *var; > + variable->Ptr = HeapAlloc(GetProcessHeap(),HEAP_ZERO_MEMORY,sizeof(struct _condition_var_intern)); > + var = variable->Ptr; > + if (!var) return; > + InitializeCriticalSection (&var->crit); > + var->crit.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": ConditionVariable.cs"); > +} I don't think it allocates anything at all, what makes me believe that is that stuff like CONDITION_VARIABLE_INIT exists. After googling a bit for it I see that gallium from mesa has some platform dependent stuff in code that uses this API like that: --- +#define pipe_static_condvar(cond) \ + /*static*/ pipe_condvar cond = CONDITION_VARIABLE_INIT --- So this structure has nothing more than a single pointer value, value 0 of it means it's initialized. I think you could just add some tests to see if it's really reset to 0 from initial value. Also regarding tests I don't think randomizing timeouts is a good idea cause it adds some unpredictable behaviour and it's not really clear if we could rely on such tests results.

Dec. 29, 2012

Re: [PATCH 1/2] kernel32: refine conditional variable test

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 http://testbot.winehq.org/JobDetails.pl?Key=23659 Your paranoid android. === WXPPROSP3 (32 bit sync) === sync.c:948: Test failed: ChangeTimerQueueTimer

Dec. 29, 2012

Re: [2/2] gdi32/tests: Added tests for GetFontResourceInfoW (try 2)

by Dmitry Timoshkov

Stefan Leichter <Stefan.Leichter(a)camline.com> wrote: > First of all: The issues your are pointing out have been in the previous > version of the patch too. Why didn't you complain in your first review. To my > understanding this is a wast of my time complaining about two issues and > keeping the rest secret for the next version of the patch. It's often happens with subsequent reviews, and there is nothing magic or secret about it: first review was about the whole approach while next one is more about the actual details. > Now to the technical stuff: > - i agree to change the data type to WCHAR > - what you mean with "protect from insufficient buffer size" it totaly unclear to > me I intentionally left 'BYTE buffer[MAX_PATH];' in the quote, think about it. > - the tests are written for a font in %windor%\fonts. This directory is empty > after a clean .wine directory was created. Changing the font does not help at > all. But it will once Wine starts to copy its own fonts there at some point. -- Dmitry.

Dec. 29, 2012

Re: [PATCH] gdi32: Ensure a fixed-pitch full-width character has double width of a half-width character.

by Alexandre Julliard

Akihiro Sagawa <sagawa.aki(a)gmail.com> writes: > --- > dlls/gdi32/freetype.c | 20 +++++++++-- > dlls/gdi32/tests/font.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 102 insertions(+), 3 deletions(-) It doesn't work here: ../../../tools/runtest -q -P wine -M gdi32.dll -T ../../.. -p gdi32_test.exe.so font.c && touch font.ok font.c:3755: Test failed: expected 19, got 0 (charset 128) -- Alexandre Julliard julliard(a)winehq.org

Dec. 28, 2012

Re: [3/9] server: Use the same object type for both ends of a named pipe. (try 2)

by Alexandre Julliard

Adam Martinson <adam.r.martinson(a)gmail.com> writes: > From 8eb230b505ddd986b689caf96510fbe016d721a7 Mon Sep 17 00:00:00 2001 > From: Adam Martinson <adam.r.martinson(a)gmail.com> > Date: Thu, 20 Dec 2012 19:55:13 -0600 > Subject: [PATCH 3/9] server: Use the same object type for both ends of a named pipe. > > Makes either end of a named pipe is readable after the other has been closed. > (If the pipe has not been disconnected) This is still changing way too many things at once. Also, using an array for pipe ends doesn't make much sense, just use two variables. -- Alexandre Julliard julliard(a)winehq.org

Dec. 28, 2012

Re: wine-1.5.20 regression compared to 1.5.19 and previous; MinGW/gcc 4.7.0 segfaults under wineconsole

by Alan W. Irwin

On 2012-12-28 13:45+0100 Frédéric Delanoy wrote: > Other users have issues with wine but they don't generally complain as > loudly as you did. It is possible I was not cautious enough with my tone, but let's leave it like this. Wine-1.5.20 has introduced an obvious regression for an important Windows app (the MinGW gcc compiler for Windows) that has been working for years for prior Wine versions. Alan __________________________ Alan W. Irwin Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net) the Time Ephemerides project (timeephem.sf.net) PLplot scientific plotting software package (plplot.sf.net) the libLASi project (unifont.org/lasi) the Loads of Linux Links project (loll.sf.net) and the Linux Brochure Project (lbproject.sf.net) __________________________ Linux-powered Science __________________________

Dec. 28, 2012

Re: [2/2] gdi32/tests: Added tests for GetFontResourceInfoW (try 2)

by Stefan Leichter

Monday 17 December 2012 Dmitry Timoshkov <dmitry(a)baikal.ru> > Stefan Leichter <Stefan.Leichter(a)camline.com> wrote: > > + BYTE buffer[MAX_PATH]; > > This should be WCHAR if you decided to test unicode version of the API > (although testing ANSI variant would be much easier), that will help > to avoid all those casts, and protect from insufficient buffer size. > Also please use a font that exists in Wine, such as sserife.fon, but > be prepared that it could be not found since font file name is locale > dependent. First of all: The issues your are pointing out have been in the previous version of the patch too. Why didn't you complain in your first review. To my understanding this is a wast of my time complaining about two issues and keeping the rest secret for the next version of the patch. Now to the technical stuff: - i agree to change the data type to WCHAR - what you mean with "protect from insufficient buffer size" it totaly unclear to me - the tests are written for a font in %windor%\fonts. This directory is empty after a clean .wine directory was created. Changing the font does not help at all. -- Stefan

Dec. 28, 2012

Re: [PATCH] advapi32: do not use invalid pSD value (Coverity)

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 http://testbot.winehq.org/JobDetails.pl?Key=23641 Your paranoid android. === WNT4WSSP6 (32 bit service) === service.c:1859: Test failed: Expected ERROR_INSUFFICIENT_BUFFER, got 87 === W2KPROSP4 (32 bit service) === service.c:1859: Test failed: Expected ERROR_INSUFFICIENT_BUFFER, got 487 service.c:1862: Test failed: Expected QueryServiceObjectSecurity to succeed: result 0 === WXPPROSP3 (32 bit service) === service.c:1859: Test failed: Expected ERROR_INSUFFICIENT_BUFFER, got 487 service.c:1862: Test failed: Expected QueryServiceObjectSecurity to succeed: result 0 === W2K3R2SESP2 (32 bit service) === service.c:1859: Test failed: Expected ERROR_INSUFFICIENT_BUFFER, got 487 service.c:1862: Test failed: Expected QueryServiceObjectSecurity to succeed: result 0

Dec. 28, 2012