November 2015 - Wine-Devel - list.winehq.org

Re: [PATCH 1/5] winepulse: Add initial stub for pulseaudio support

by Andrew Eikum

On Mon, Nov 02, 2015 at 11:17:40PM +0900, Alexandre Julliard wrote: > Andrew Eikum <aeikum(a)codeweavers.com> writes: > > > From: Maarten Lankhorst <m.b.lankhorst(a)gmail.com> > > > > Includes API compatibility patch by Juergen Tretthahn <orson(a)orson.at>. > > > > Synchronous static data initialization by Andrew Eikum <aeikum(a)codeweavers.com> > > > > Signed-off-by: Andrew Eikum <aeikum(a)codeweavers.com> > > I know that this code is not supposed to be used yet, but if I try to > use it, it's crashing all over the place. Is that really how it's > supposed to be? > I didn't really test it in-progress like that because lots of critical functionality is missing in these early patches. I'm not sure how well our winmm, dsound, and tests will handle missing IAudioRenderClient, for example. It's split up for review purposes, because the full driver is over 3000 lines. I could try reproducing those issues and fixing them if you want, but I'm not sure it's worth the time if the client DLLs and applications won't handle it well anyway. Andrew

Nov. 2, 2015

Re: steam web browsing on OS X

by Jacek Caban

On 11/02/15 13:15, Jacek Caban wrote: > Hi Sebastian, > > On 11/02/15 12:16, Sebastian Lackner wrote: >> On 02.11.2015 11:12, Jacek Caban wrote: >>> Sure. The idea is to pass relative addresses instead to >>> call_syscall_func and let it handle that. Calling call_syscall_func >>> itself may use a constant address if we let it live in user shared data. >>> Did you try that? That's one of those things that one has to try to see >>> if it works in practice. >> Yes, I also looked at this approach. The difficult part to get this correct >> is to initialize user shared data as early as possible, without any call to >> a wrapper function before that. Please note that its not sufficient to wrap >> calls from other dlls, even some ntdll-internal calls have to go through the >> wrappers (for example when loading a library). Those are implemented in >> ntdll usermode on Windows. >> >>> Also I believe that using winebuild for that is the right way. >> I already tried it out in practice and partially disagree. As mentioned above, >> its not sufficient to put autogenerated wrappers between the implementation >> and the ntdll exports. Exactly the same wrappers have to be used from inside >> of the ntdll loader code for example. This means, even if the thunks are >> generated somewhere else, we still have to add them to one of the wine header >> files. If we do not want to generate thunks for all architectures, we might >> even have to add them twice, one time as typedef, and the second time a #define >> as fallback for unsupported architectures. >> >> I'm not saying that its impossible to do it this way, but compared to thiscalls, >> which could easily be generated by winebuild, a macro is similar good here imho. >> If you want to look into the idea to autogenerate thunks, here is one of my >> work-in-progress patches (based on Erichs work): http://ix.io/lLz > See attached patches. On the quick look it seems similar to the one you > mentioned, except it avoids text relocations. The patch is definitely > not finished nor ready for proper review, but it's enough to get things > to build and run. I haven't even tested that with Chromium nor other > apps that need it (well, I haven't tested it at all, really). > > Patch 1 is the interesting one. Patch 2 just marks needed functions as > syscalls. > > Patch 3 is to show how to address your concerns about going through > thunks from inside ntdll. Those use macros from your patch except their > meaning is reverted. I chose two calls that I know will need to be fixed > for Office. I don't know how much more calls will need to be changed. In > general, we can't avoid such problems: we will either need to explicitly > mark calls that need to go through the thunk or those that don't. I > chose the first variant because it seems cleaner and more in line with > how winebuild works. I do believe that implementing things to work the > other way around is also possible in winebuild. > > Cheers, > Jacek It seems that my mail didn't make it to wine-devel, probably due to attachment size. I'm resending without patch 2. Cheers, Jacek

Nov. 2, 2015

Re: [PATCH 04/10] ucrtbase: Handle the swprintf style termination and return values

by Martin Storsjö

On Mon, 2 Nov 2015, Alexandre Julliard wrote: > Piotr Caban <piotr.caban(a)gmail.com> writes: >> On 11/02/15 13:45, Martin Storsjö wrote: >>> After thinking another few minutes about it - sure, this is fine for me. >>> Should I resend the full patchset, or only 4->10 (at least some of them >>> will need some conflict resolution)? If I resend the full patchset, I >>> guess I can fold in your sign-offs in 1-3 (as long as I don't modify >>> those patches)? >> I can sign the patches after you resend them myself (I don't know >> what's the policy in this case). > > I prefer if you sign them again after a resend, if it's folded into the > patch email I won't notice it right away. But if the already signed off > patches are still valid, it's even better to wait for me to commit them, > and then resend only the remaining ones. They are still valid, so I'll resend the rest of the branch later after today's push. // Martin

Nov. 2, 2015

Re: [PATCH 04/10] ucrtbase: Handle the swprintf style termination and return values

by Martin Storsjö

On Mon, 2 Nov 2015, Piotr Caban wrote: > Hi, > > I don't like the code I've proposed in last email. Something like this seems > to be nicer: > static int puts_clbk_str_c99_a(void *ctx, int len, const char *str) > { > struct _str_ctx_a *out = ctx; > > if(!out->buf) > return len; > > if(out->len < len) { > memcpy(out->buf, str, out->len); > out->buf += out->len; > out->len = 0; > return len; > } > > memcpy(out->buf, str, len); > out->buf += len; > out->len -= len; > return len; > } > > /********************************************************************* > * __stdio_common_vsprintf (MSVCRT.@) > */ > int CDECL MSVCRT__stdio_common_vsprintf( unsigned __int64 options, char *str, > MSVCRT_size_t len, const char *format, > MSVCRT__locale_t locale, > __ms_va_list valist ) > { > static const char nullbyte = '\0'; > struct _str_ctx_a ctx = {len, str}; > int ret; > > if (options & ~UCRTBASE_PRINTF_TERMINATION_MASK) > FIXME("options %s not handled\n", wine_dbgstr_longlong(options)); > ret = pf_printf_a(puts_clbk_str_c99_a, &ctx, format, locale, > FALSE, FALSE, arg_clbk_valist, NULL, &valist); > puts_clbk_str_a(&ctx, 1, &nullbyte); > > if(options & UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION) > return ret>len ? -1 : ret; > if(ret>=len) { > if(len) str[len-1] = 0; > return (options & UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR) ? ret > : -2; > } > return ret; > } > if you think that your initial code is better please let me know. No, this feels like the most straightforward and explicit way of implementing it so far - thanks! > Probably in this code UCRTBASE_PRINTF_TERMINATION_MASK define is not > needed. I'd keep it here (as condition for the fixme warning - alternatively add "|| options == 0" to the condition), but remove it once that warning is removed in a later patch. // Martin

Nov. 2, 2015

Re: [PATCH 04/10] ucrtbase: Handle the swprintf style termination and return values

by Alexandre Julliard

Piotr Caban <piotr.caban(a)gmail.com> writes: > On 11/02/15 13:45, Martin Storsjö wrote: >> After thinking another few minutes about it - sure, this is fine for me. >> Should I resend the full patchset, or only 4->10 (at least some of them >> will need some conflict resolution)? If I resend the full patchset, I >> guess I can fold in your sign-offs in 1-3 (as long as I don't modify >> those patches)? > I can sign the patches after you resend them myself (I don't know > what's the policy in this case). I prefer if you sign them again after a resend, if it's folded into the patch email I won't notice it right away. But if the already signed off patches are still valid, it's even better to wait for me to commit them, and then resend only the remaining ones. -- Alexandre Julliard julliard(a)winehq.org

Nov. 2, 2015

Re: [PATCH 04/10] ucrtbase: Handle the swprintf style termination and return values

by Piotr Caban

Hi, I don't like the code I've proposed in last email. Something like this seems to be nicer: static int puts_clbk_str_c99_a(void *ctx, int len, const char *str) { struct _str_ctx_a *out = ctx; if(!out->buf) return len; if(out->len < len) { memcpy(out->buf, str, out->len); out->buf += out->len; out->len = 0; return len; } memcpy(out->buf, str, len); out->buf += len; out->len -= len; return len; } /********************************************************************* * __stdio_common_vsprintf (MSVCRT.@) */ int CDECL MSVCRT__stdio_common_vsprintf( unsigned __int64 options, char *str, MSVCRT_size_t len, const char *format, MSVCRT__locale_t locale, __ms_va_list valist ) { static const char nullbyte = '\0'; struct _str_ctx_a ctx = {len, str}; int ret; if (options & ~UCRTBASE_PRINTF_TERMINATION_MASK) FIXME("options %s not handled\n", wine_dbgstr_longlong(options)); ret = pf_printf_a(puts_clbk_str_c99_a, &ctx, format, locale, FALSE, FALSE, arg_clbk_valist, NULL, &valist); puts_clbk_str_a(&ctx, 1, &nullbyte); if(options & UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION) return ret>len ? -1 : ret; if(ret>=len) { if(len) str[len-1] = 0; return (options & UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR) ? ret : -2; } return ret; } if you think that your initial code is better please let me know. Probably in this code UCRTBASE_PRINTF_TERMINATION_MASK define is not needed. On 11/02/15 13:45, Martin Storsjö wrote: > After thinking another few minutes about it - sure, this is fine for me. > Should I resend the full patchset, or only 4->10 (at least some of them > will need some conflict resolution)? If I resend the full patchset, I > guess I can fold in your sign-offs in 1-3 (as long as I don't modify > those patches)? I can sign the patches after you resend them myself (I don't know what's the policy in this case). Thanks, Piotr

Nov. 2, 2015

Re: [PATCH 1/5] winepulse: Add initial stub for pulseaudio support

by Alexandre Julliard

Andrew Eikum <aeikum(a)codeweavers.com> writes: > From: Maarten Lankhorst <m.b.lankhorst(a)gmail.com> > > Includes API compatibility patch by Juergen Tretthahn <orson(a)orson.at>. > > Synchronous static data initialization by Andrew Eikum <aeikum(a)codeweavers.com> > > Signed-off-by: Andrew Eikum <aeikum(a)codeweavers.com> I know that this code is not supposed to be used yet, but if I try to use it, it's crashing all over the place. Is that really how it's supposed to be? ../../../tools/runtest -q -P wine -T ../../.. -M mmdevapi.dll -p mmdevapi_test.exe.so capture && touch capture.ok capture.c:247: Test succeeded inside todo block: HPC after ReleaseBuffer(0) 102136526 vs. 102136526 capture.c:310: Test succeeded inside todo block: expect DISCONTINUITY 1 capture.c:316: Test succeeded inside todo block: Position 1323 gap 441 wine: Unhandled page fault on write access to 0x00000004 at address 0x7e468d8d (thread 0009), starting debugger... Unhandled exception: page fault on write access to 0x00000004 in 32-bit code (0x7e468d8d). Register dump: CS:0023 SS:002b DS:002b ES:002b FS:0063 GS:006b EIP:7e468d8d ESP:0032fb70 EBP:0032fba8 EFLAGS:00010206( R- -- I - -P- ) EAX:00000000 EBX:7e4741e0 ECX:00000000 EDX:00000000 ESI:00000000 EDI:00125ff0 Stack dump: 0x0032fb70: 7c608058 7e4747f0 0032fba8 7e465a8d 0x0032fb80: 00000001 00000000 7e4747c0 7e4747f0 0x0032fb90: 00125ff0 7e468d0b 0032fbc0 7ed5b16c 0x0032fba0: 00000001 00000000 0032fcb8 7ed40e3e 0x0032fbb0: 00000001 7ed52189 00005b4d 7ed40e3e 0x0032fbc0: 00125ff0 000001b9 00000000 7ed41bc6 Backtrace: =>0 0x7e468d8d AudioClient_Release+0x9d() in winepulse (0x0032fba8) 1 0x7ed40e3e test_audioclient+0xa8d() [/home/julliard/wine/wine/dlls/mmdevapi/tests/../../../include/audioclient.h:365] in mmdevapi_test (0x0032fcb8) 2 0x7ed42e45 func_capture+0x114() [/home/julliard/wine/wine/dlls/mmdevapi/tests/capture.c:1076] in mmdevapi_test (0x0032fd68) 3 0x7ed40014 main+0x393(argc=<is not available>, argv=<is not available>) [/home/julliard/wine/wine/dlls/mmdevapi/tests/../../../include/wine/test.h:584] in mmdevapi_test (0x0032fe18) 4 0x7ed5195b __wine_spec_exe_entry+0x4a(peb=<couldn't compute location>) [/home/julliard/wine/wine/dlls/winecrt0/exe_entry.c:36] in mmdevapi_test (0x0032fe58) 5 0x7b85bc9c call_process_entry+0xb() in kernel32 (0x0032fe78) 6 0x7b85cc1a start_process+0x59(peb=<couldn't compute location>) [/home/julliard/wine/wine/dlls/kernel32/process.c:1104] in kernel32 (0x0032fea8) 7 0x7bc7eaa0 call_thread_func_wrapper+0xb() in ntdll (0x0032fec8) 8 0x7bc8180f call_thread_func+0xde(entry=0x7b85cbc0, arg=0x7ffdf000, frame=0x32ffc8) [/home/julliard/wine/wine/dlls/ntdll/signal_i386.c:2732] in ntdll (0x0032ffa8) 9 0x7bc7ea7e call_thread_entry_point+0x11() in ntdll (0x0032ffc8) 10 0x7bc53a47 start_process+0x16(kernel_start=0x7b85cbc0) [/home/julliard/wine/wine/dlls/ntdll/loader.c:3040] in ntdll (0x0032ffe8) 11 0xf75c239d wine_call_on_stack+0x1c() in libwine.so.1 (0x00000000) 12 0xf75c2500 wine_switch_to_stack+0x1f(func=0x7bc53a30, arg=0x7b85cbc0, stack=0x330000) [/home/julliard/wine/wine/libs/wine/port.c:59] in libwine.so.1 (0xffb19868) 13 0x7bc596c1 LdrInitializeThunk+0x1f0(kernel_start=<couldn't compute location>, unknown2=<couldn't compute location>, unknown3=<couldn't compute location>, unknown4=<couldn't compute location>) [/home/julliard/wine/wine/dlls/ntdll/loader.c:3094] in ntdll (0xffb198a8) 14 0x7b862849 __wine_kernel_init+0x888() [/home/julliard/wine/wine/dlls/kernel32/process.c:1276] in kernel32 (0xffb1a798) 15 0x7bc5a533 __wine_process_init+0x152() [/home/julliard/wine/wine/dlls/ntdll/loader.c:3303] in ntdll (0xffb1a808) 16 0xf75bfe9b wine_init+0x2ca(argc=0x3, argv=0xffb1ad44, error="", error_size=0x400) [/home/julliard/wine/wine/libs/wine/loader.c:959] in libwine.so.1 (0xffb1a858) 17 0x7bf00d6a main+0x79(argc=<is not available>, argv=<is not available>) [/home/julliard/wine/wine/loader/main.c:247] in <wine-loader> (0xffb1ac98) 18 0xf73dfa63 __libc_start_main+0xf2() in libc.so.6 (0x00000000) 0x7e468d8d AudioClient_Release+0x9d in winepulse: movl %ecx,0x4(%eax) -- Alexandre Julliard julliard(a)winehq.org

Nov. 2, 2015

Re: [PATCH 04/10] ucrtbase: Handle the swprintf style termination and return values

by Martin Storsjö

On Mon, 2 Nov 2015, Piotr Caban wrote: > Hi, > > On 11/02/15 11:08, Martin Storsjo wrote: >> @@ -724,12 +724,13 @@ int CDECL MSVCRT__stdio_common_vsprintf( unsigned >> __int64 options, char *str, MS >> struct _str_ctx_a ctx = {len, str}; >> int ret; >> >> - if (options != UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION && >> - options != UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR) >> + if (options & ~UCRTBASE_PRINTF_TERMINATION_MASK) >> FIXME("options %s not handled\n", wine_dbgstr_longlong(options)); >> - ret = pf_printf_a(options & >> UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR ? puts_clbk_str_c99_a : >> puts_clbk_str_a, >> + ret = pf_printf_a(options & >> UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION ? puts_clbk_str_a : >> puts_clbk_str_c99_a, >> &ctx, format, locale, FALSE, FALSE, arg_clbk_valist, NULL, >> &valist); >> puts_clbk_str_a(&ctx, 1, &nullbyte); >> + if ((options & UCRTBASE_PRINTF_TERMINATION_MASK) == 0 && ret >= len) >> + ret = -2; >> return ret; >> } > This code looks like you're trying to make the things other way around. > Shouldn't UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR flag impact the > callback being used? On the other hand > UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION flag should probably > only affect terminating NULL related behavior. Actually, I don't think of them as two separate flags, but as having 3 different modes (with the fourth, with both flags being set, undefined). But I see what you're getting at. > Maybe following code makes more sense: > ret = pf_printf_a(options & UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR ? > puts_clbk_str_c99_a : puts_clbk_str_a, &ctx, format, locale, FALSE, FALSE, > arg_clbk_valist, NULL, &valist); > if(puts_clbk_str_a(&ctx, 1, &nullbyte)==-1 && !(options & > UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION)) { > if(len) str[len-1] = 0; > return -2; > } > return ret; > > What do you think about it? I guess this would work as well, although it took me a little while to figure out why (why the return -2 case won't be invoked when the standard snprintf behaviour is requested - because the nullbyte write will always succeed since the c99 callback left space for it). After thinking another few minutes about it - sure, this is fine for me. Should I resend the full patchset, or only 4->10 (at least some of them will need some conflict resolution)? If I resend the full patchset, I guess I can fold in your sign-offs in 1-3 (as long as I don't modify those patches)? // Martin

Nov. 2, 2015

Re: steam web browsing on OS X

by Sebastian Lackner

On 02.11.2015 13:22, Alexandre Julliard wrote: > > I'm not talking about other platforms, but at a minimum wow64 needs to > be supported. We can't require people to use 32-bit prefixes forever. > As far as I can tell from the sandbox source, Windows 8 and Windows 10 > versions would also have to be handled differently. > If our previous tests were correct, at least for Steam wow64 support doesn't help at all, because a 64-bit steamwebhelper process is started in 64-bit prefixes.

Nov. 2, 2015

Re: steam web browsing on OS X

by Alexandre Julliard

Sebastian Lackner <sebastian(a)fds-team.de> writes: > On 02.11.2015 12:45, Alexandre Julliard wrote: >> It seems that in your patch this is the default behavior, and you have >> to explicitly bypass the syscall. Are there really that many cases where >> it's necessary to go through the syscall? I would have expected that >> internal calls would bypass unless explicitly required. > > Well, the main difficulty is to find out what exactly is required. For loader > functions some applications depend on it, but does that mean we should only use > the thunks there? It seems a bit risky to me, and I would expect this to cause > regressions. If such a solution is preferred, we should definitely test it some > time before shipping it as stable to our users. ;) I don't think it's more risky, our ntdll internals are pretty different from the Windows ones, so it's unlikely that we'd trigger the correct sequence of syscalls either way. >> The problem is that this allows a single version of the thunks, but we >> are most likely going to need multiple variants selected at run-time >> based on the Windows version. That doesn't mean it has to be done in >> winebuild, I agree it could also be done with macros, but it's not as >> simple as the thiscall case. > > Imho it doesn't make much sense to think about other architectures right now. Unless > the linux kernel offers a way to define custom system calls, or we want to trace > every process, we technically can not implement x86_64 support. Also x86 (wow64) support > is only possible when we either build ntdll.dll.so twice, or patch the functions at > runtime. And for other architectures like ARM/ARM64/... I'm not aware of any application > which would need special thunks at all. I'm not talking about other platforms, but at a minimum wow64 needs to be supported. We can't require people to use 32-bit prefixes forever. As far as I can tell from the sandbox source, Windows 8 and Windows 10 versions would also have to be handled differently. -- Alexandre Julliard julliard(a)winehq.org

Nov. 2, 2015