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

Re: [PATCH 6/8] ntdll: Standardize on sysconf to detect the number of processors for all platforms.

by Alexandre Julliard

James Eder <jimportal(a)gmail.com> writes: > @@ -1010,6 +1010,16 @@ static inline void get_cpuinfo(SYSTEM_CPU_INFORMATION* info) > */ > void fill_cpu_info(void) > { > + long num; > + > + num = sysconf(_SC_NPROCESSORS_ONLN); > + if (num < 1) > + { > + num = 1; > + WARN("Failed to detect the number of processors.\n"); > + } > + NtCurrentTeb()->Peb->NumberOfProcessors = num; Have you verified that this works correctly on all the platforms you are changing? -- Alexandre Julliard julliard(a)winehq.org

Oct. 22, 2012

Re: d3dx9_36: Add tests for D3DXSHRotateZ

by Michael Stefaniuc

On 10/22/2012 11:21 AM, Henri Verbeet wrote: > On 22 October 2012 09:06, Rico Schüller <kgbricola(a)web.de> wrote: >> On 19.10.2012 13:15, Nozomi Kodama wrote: >>> >>> This patch adds tests for the patch sent by Rico for D3DXSHRotateZ. >>> >>> Moreover, I changed const in CONST in the declaration of the function in >>> order to uniformize with the file. >> >> >>> + const FLOAT angle[] = ... >> I'd prefer "const" because it is used a little bit more often than CONST, >> but it is only a matter of taste. >> git grep "[ (,]const " | wc -l >> ~61k >> git grep "[ (,]CONST " | wc -l >> ~1.2k >> > IIRC "CONST" is some kind of hack for compilers that don't (fully) > support const. I think it's unlikely you'll be able to compile a > working Wine with such a compiler even if you can find one, these days > probably even things like MSVC, Sun Studio and Clang support const. > And even if you could, something along the lines of AC_C_CONST would > probably be a better solution anyway. It seems tempting to cleanup the > couple of places in Wine that use "CONST", and add a __ONLY_IN_WINELIB > to the #define in include/windef.h. I'm removing CONST in the lines I touch so +1 on a concerted effort to eradicate it. Though at 1.2k lines Alexandre might consider it too spammy. bye michael

Oct. 22, 2012

Re: d3dx9_36: Add tests for D3DXSHRotateZ

by Henri Verbeet

On 22 October 2012 09:06, Rico Schüller <kgbricola(a)web.de> wrote: > On 19.10.2012 13:15, Nozomi Kodama wrote: >> >> This patch adds tests for the patch sent by Rico for D3DXSHRotateZ. >> >> Moreover, I changed const in CONST in the declaration of the function in >> order to uniformize with the file. > > >> + const FLOAT angle[] = ... > I'd prefer "const" because it is used a little bit more often than CONST, > but it is only a matter of taste. > git grep "[ (,]const " | wc -l > ~61k > git grep "[ (,]CONST " | wc -l > ~1.2k > IIRC "CONST" is some kind of hack for compilers that don't (fully) support const. I think it's unlikely you'll be able to compile a working Wine with such a compiler even if you can find one, these days probably even things like MSVC, Sun Studio and Clang support const. And even if you could, something along the lines of AC_C_CONST would probably be a better solution anyway. It seems tempting to cleanup the couple of places in Wine that use "CONST", and add a __ONLY_IN_WINELIB to the #define in include/windef.h.

Oct. 22, 2012

Re: d3dx9_36: Add tests for D3DXSHRotateZ

by Rico Schüller

On 19.10.2012 13:15, Nozomi Kodama wrote: > This patch adds tests for the patch sent by Rico for D3DXSHRotateZ. > > Moreover, I changed const in CONST in the declaration of the function in > order to uniformize with the file. > + const FLOAT angle[] = ... I'd prefer "const" because it is used a little bit more often than CONST, but it is only a matter of taste. git grep "[ (,]const " | wc -l ~61k git grep "[ (,]CONST " | wc -l ~1.2k > + if ( i >= square || ( (order >= D3DXSH_MAXORDER) && ( i >= D3DXSH_MAXORDER * D3DXSH_MAXORDER ) ) ) > + if (l > 0) I'd write it "if (i >= square || ((... for consistency. > + ok(relative_error(expected, out_temp[i]) < admitted_error, "angle %f, order %u index %u, Expected %f, received %f\n", angle[j], order, i, expected, out_temp[i]); Expected -> expected? (2 times) > + expected = table[36 * (l + 3 * j ) + i]; The spacing looks a bit ugly. Cheers Rico

Oct. 22, 2012

Re: [PATCH 5/8] ntdll: Add detection for PF_SSE_DAZ_MODE_AVAILABLE

by David Laight

On Sun, Oct 21, 2012 at 04:16:04PM -0600, James Eder wrote: > Hmmm... the GCC docs say: > > http://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Type-Attributes.html#Type-Attri… > > "Note that the alignment of any given struct or union type is required > by the ISO C standard to be at least a perfect multiple of the lowest > common multiple of the alignments of all of the members of the struct > or union in question. This means that you can effectively adjust the > alignment of a struct or union type by attaching an aligned attribute > to any one of the members of such a type, [...]" > > So, with conforming compilers, the having M128A aligned makes > XMM_SAVE_AREA32 aligned. I tried it out some and I wasn't able to find > a case which GCC (4.7.2) didn't align XMM_SAVE_AREA32 at 16 with M128A > declared the way it is. With the default gcc configuration and parameters used on Linux, 128 bit (16 byte) alignment of on-stack items assumes that the stack is 16 byte aligned. While the code generated by gcc for function calls maintains this alignement, it isn't part of the standard ABI and other code may not do so. If you read up on this you'll find someone tried detecting misaligned stacks and found they happened all the time. Pertinent to wine is that code compiled with MSC will not maintain that alignment, any assembler wrappers are unlikely to as well. There are some options to gcc that affect the generated code, one will force the stack to be realigned if any locals need 16 byte alignement. Changing some of the other options will cause internal compiler faults! The code in gcc is made more complicated by the fact that it is useful to align 8 byte items, but never (AFAICT) necessary to do so, and gcc tries to align on-stack double and long-long items but not realign the stack in order to do so. We've just gone through a series of hoops in NetBSD trying to fix gcc to generate code that doesn't assume 16 byte aligned stack and doesn't fault internally. David -- David Laight: david(a)l8s.co.uk

Oct. 22, 2012

Re: [PATCH 5/8] ntdll: Add detection for PF_SSE_DAZ_MODE_AVAILABLE

by James Eder

Hmmm... the GCC docs say: http://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Type-Attributes.html#Type-Attri… "Note that the alignment of any given struct or union type is required by the ISO C standard to be at least a perfect multiple of the lowest common multiple of the alignments of all of the members of the struct or union in question. This means that you can effectively adjust the alignment of a struct or union type by attaching an aligned attribute to any one of the members of such a type, [...]" So, with conforming compilers, the having M128A aligned makes XMM_SAVE_AREA32 aligned. I tried it out some and I wasn't able to find a case which GCC (4.7.2) didn't align XMM_SAVE_AREA32 at 16 with M128A declared the way it is. The documentation goes on to say: "Note that the effectiveness of aligned attributes may be limited by inherent limitations in your linker. On many systems, the linker is only able to arrange for variables to be aligned up to a certain maximum alignment. (For some linkers, the maximum supported alignment may be very very small.) If your linker is only able to align variables up to a maximum of 8 byte alignment, then specifying aligned(16) in an __attribute__ will still only provide you with 8 byte alignment. See your linker documentation for further information." Seems to work fine for me without the manual alignment, but I that doesn't say much for anyone using some other compiler/linker. Not that I care about those unfortunate souls :P. I was just going for what I figured would get the patch in. Either way will work well enough for me. So I guess I just need to know which way will get the patch in. :) -- Jim

Oct. 21, 2012

Re: [PATCH 5/8] ntdll: Add detection for PF_SSE_DAZ_MODE_AVAILABLE

by David Laight

On Sun, Oct 21, 2012 at 10:56:51AM -0600, James Eder wrote: > static inline void save_fpux( CONTEXT *context ) > { > #ifdef __GNUC__ > /* we have to enforce alignment by hand */ > char buffer[sizeof(XMM_SAVE_AREA32) + 16]; > XMM_SAVE_AREA32 *state = (XMM_SAVE_AREA32 *)(((ULONG_PTR)buffer + > 15) & ~15); > > __asm__ __volatile__( "fxsave %0" : "=m" (*state) ); > context->ContextFlags |= CONTEXT_EXTENDED_REGISTERS; > memcpy( context->ExtendedRegisters, state, sizeof(*state) ); > #endif > } That is an on-stack buffer, all bets are off! Traditionally the SYSV ABI (used by everyone except microsoft) only required the stack to br 4 byte aligned. At some point the gcc bods unilaterally decided to maintain 16 byte alignement (so the SSE2 registers can be saved on stack). The Linux kernel aligns the stack when main (strictly _start) is called, but anything not compiled by gcc (or compiled with other options) can lose that alignment. If you request 32 byte alignment for the save area, (recent enough) gcc will align the stack before allocating the locals. David -- David Laight: david(a)l8s.co.uk

Oct. 21, 2012

Re: [PATCH] services: Fix some weird sizeof usages.

by Christian Costa

Le 21/10/2012 20:27, Dan Kegel a écrit : > Christian Costa wrote: > ... > --- a/programs/services/rpc.c > +++ b/programs/services/rpc.c > ... > @@ -952,7 +952,7 @@ BOOL service_send_command( struct service_entry > *service, HANDLE pipe, > } > r = GetOverlappedResult( pipe, &overlapped, &count, FALSE ); > } > - if (!r || count != sizeof *result) > + if (!r || count != sizeof(*result)) > > I was curious about this, so I looked around a bit. > > There are about 800 instances of sizeof without parens in Wine > vs. 34628 with. > > I would have thought "Heck, this is just like return without > parentheses, it emphasizes that it's not a function call, > maybe it's a good idea." > > But it seems it's only legal to use it without parentheses for values; > you need the parentheses when getting sizeof a type. cf. > c89: http://web.archive.org/web/20040907153347/http://danpop.home.cern.ch/danpop… > http://bytes.com/topic/c/answers/478596-using-sizeof-without-parentheses > huu > Does anybody feel strongly one way or the other? > > > Thanks for the links. I think 800 vs 34628 should answer your question. Anyway while I was debugging I noticed these 2 sizeof in the middle of a lot others with parens and then send a patch to make them follow the file style.

Oct. 21, 2012

re: [PATCH] services: Fix some weird sizeof usages.

by Dan Kegel

Christian Costa wrote: ... --- a/programs/services/rpc.c +++ b/programs/services/rpc.c ... @@ -952,7 +952,7 @@ BOOL service_send_command( struct service_entry *service, HANDLE pipe, } r = GetOverlappedResult( pipe, &overlapped, &count, FALSE ); } - if (!r || count != sizeof *result) + if (!r || count != sizeof(*result)) I was curious about this, so I looked around a bit. There are about 800 instances of sizeof without parens in Wine vs. 34628 with. I would have thought "Heck, this is just like return without parentheses, it emphasizes that it's not a function call, maybe it's a good idea." But it seems it's only legal to use it without parentheses for values; you need the parentheses when getting sizeof a type. cf. c89: http://web.archive.org/web/20040907153347/http://danpop.home.cern.ch/danpop… http://bytes.com/topic/c/answers/478596-using-sizeof-without-parentheses Does anybody feel strongly one way or the other?

Oct. 21, 2012

Re: [PATCH 5/8] ntdll: Add detection for PF_SSE_DAZ_MODE_AVAILABLE

by James Eder

Gah... hit the wrong reply button so here we go again for some. On Sun, Oct 21, 2012 at 10:06 AM, Joris Huizer <joris_huizer(a)yahoo.com> wrote: > On 10/21/2012 05:49 PM, James Eder wrote: >> >> On Sat, Oct 20, 2012 at 7:06 PM, Chris Robinson <chris.kcat(a)gmail.com> >> wrote: >>> >>> On 10/20/2012 05:40 PM, James Eder wrote: >>>> >>>> + /* Intel says we need a zeroed 16-byte aligned buffer */ >>>> + char buffer[512 + 16]; >>>> + XMM_SAVE_AREA32 *state = (XMM_SAVE_AREA32 *)(((ULONG_PTR)buffer + >>>> 15) >>>> & ~15); >>>> + memset(buffer, 0, sizeof(buffer)); >>>> + >>>> + __asm__ __volatile__( "fxsave %0" : "=m" (*state) : "m" (*state) ); >>> >>> >>> Wouldn't this be simpler? >>> >>> DECLSPEC_ALIGN(16) XMM_SAVE_AREA32 state; >>> memset(state, 0, sizeof(state)); >>> __asm__ __volatile__("fxsave %0" : "=m" (*&state) : "m" (*&state)); >>> >>> May also want to make sure the two structs are packed. >>> >>> >> I used that alignment method because I saw it done that way other >> places in Wine. I figured there must have been a good reason for >> doing it that way (issue with some build environments?) but perhaps >> I'm being paranoid. >> >> > I would think the construct is necessary when allocating memory (the > allocation functions don't allow to require a certain alignment as far as I > know) > That might be where you saw this being done? > > I was going off of save_fpux in dlls/ntdll/signal_i386.c: static inline void save_fpux( CONTEXT *context ) { #ifdef __GNUC__ /* we have to enforce alignment by hand */ char buffer[sizeof(XMM_SAVE_AREA32) + 16]; XMM_SAVE_AREA32 *state = (XMM_SAVE_AREA32 *)(((ULONG_PTR)buffer + 15) & ~15); __asm__ __volatile__( "fxsave %0" : "=m" (*state) ); context->ContextFlags |= CONTEXT_EXTENDED_REGISTERS; memcpy( context->ExtendedRegisters, state, sizeof(*state) ); #endif } -- Jim

Oct. 21, 2012