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

Re: steam web browsing on OS X

by Jacek Caban

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

Nov. 2, 2015

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

by Piotr Caban

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. 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? Thanks, Piotr

Nov. 2, 2015

Re: steam web browsing on OS X

by Sebastian Lackner

On 02.11.2015 12:45, Alexandre Julliard wrote: > Sebastian Lackner <sebastian(a)fds-team.de> writes: > >> 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. > > 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 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 > > 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. Best regards, Sebastian

Nov. 2, 2015

Re: [3/7] widl: Ignore assignment of a duplicate uuid. Resend.

by Dmitry Timoshkov

Huw Davies <huw(a)codeweavers.com> wrote: > This still has problems. For example see the idl below. > Also note that this is the cause of the warnings in httpresquest.idl . > > Huw. > > import "objidl.idl"; > [uuid(12345678-0000-0000-0000-000000000000)] > library foo > { > typedef [uuid(12345678-9ab0-0000-0000-000000000000)] struct f {int x;} f; > [uuid(12345678-9abc-0000-0000-000000000000)] > interface IMy : IUnknown > { > HRESULT foo([in]f *ptr); > } > } I'd say that this is yet another bug discovered by this patch, and these bugs have nothing to do with the patch itself, they do exist already. I certainly can have a look at this bug in typelib generation, but I don't think that it's really correct to blame my patch for making existing bugs in typelib generation visible, and because of that reject the patch inclusion. -- Dmitry.

Nov. 2, 2015

Re: steam web browsing on OS X

by Alexandre Julliard

Sebastian Lackner <sebastian(a)fds-team.de> writes: > 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. 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. > 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 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. -- Alexandre Julliard julliard(a)winehq.org

Nov. 2, 2015

Re: [PATCH 1/7] wined3d: Introduce wined3d_device_map_sub_resource().

by Henri Verbeet

On 2 November 2015 at 04:17, Józef Kucia <jkucia(a)codeweavers.com> wrote: > +HRESULT CDECL wined3d_device_map_sub_resource(struct wined3d_device *device, > + struct wined3d_resource *resource, unsigned int sub_resource_idx, > + struct wined3d_map_desc *map_desc, const struct wined3d_box *box, DWORD flags) > +{ > + struct wined3d_resource *sub_resource; > + > + TRACE("device %p, resource %p, sub_resource_idx %u, map_desc %p, box %p, flags %#x.\n", > + device, resource, sub_resource_idx, map_desc, box, flags); > + > + if (!(sub_resource = sub_resource_from_resource(resource, sub_resource_idx))) > + { > + WARN("Invalid sub_resource_idx %u.\n", sub_resource_idx); > + return E_INVALIDARG; > + } > + > + return sub_resource->resource_ops->resource_map(sub_resource, map_desc, box, flags); > +} I suppose in d3d11 this is part of ID3D11DeviceContext because of deferred contexts maps. Perhaps once we have those in wined3d it will end up in the device (although I think it's more likely to become part of some interface on top of wined3d_cs), but right now the "device" parameter isn't even used. I think that for the time being the interface we want is just HRESULT CDECL wined3d_resource_map(struct wined3d_resource *resource, unsigned int sub_resource_idx, struct wined3d_map_desc *map_desc, const struct wined3d_box *box, DWORD flags); > diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h > index fd7bbc8..fb296b1 100644 > --- a/dlls/wined3d/wined3d_private.h > +++ b/dlls/wined3d/wined3d_private.h > @@ -2156,6 +2156,8 @@ struct wined3d_resource_ops > ULONG (*resource_incref)(struct wined3d_resource *resource); > ULONG (*resource_decref)(struct wined3d_resource *resource); > void (*resource_unload)(struct wined3d_resource *resource); > + HRESULT (*resource_map)(struct wined3d_resource *resource, struct wined3d_map_desc *map_desc, > + const struct wined3d_box *box, DWORD flags); > }; > Here we'll probably want something like HRESULT (*resource_sub_resource_map)(struct wined3d_resource *resource, unsigned int sub_resource_idx, struct wined3d_map_desc *map_desc, const struct wined3d_box *box, DWORD flags); That implies having different resource ops for 2D and 3D textures, but that's easy. It also implies returning an error when this is called on a surface or volume. I'm not sure how easy it would be at this point, but we'll probably just want resource ops on sub-resources to print an ERR(). E.g. ideally we'd never call resource_unload() on a surface, but only ever on a texture. But that then means not adding sub-resources to the device's resource list, etc.

Nov. 2, 2015

Re: [PATCH 1/7] wined3d: Introduce wined3d_device_map_sub_resource().

by Stefan Dösinger

> Am 02.11.2015 um 04:17 schrieb Józef Kucia <jkucia(a)codeweavers.com>: > > +HRESULT CDECL wined3d_device_map_sub_resource(struct wined3d_device *device, > + struct wined3d_resource *resource, unsigned int sub_resource_idx, > + struct wined3d_map_desc *map_desc, const struct wined3d_box *box, DWORD flags) > +{ > + struct wined3d_resource *sub_resource; > + > + TRACE("device %p, resource %p, sub_resource_idx %u, map_desc %p, box %p, flags %#x.\n", > + device, resource, sub_resource_idx, map_desc, box, flags); > + > + if (!(sub_resource = sub_resource_from_resource(resource, sub_resource_idx))) > + { > + WARN("Invalid sub_resource_idx %u.\n", sub_resource_idx); > + return E_INVALIDARG; > + } > + > + return sub_resource->resource_ops->resource_map(sub_resource, map_desc, box, flags); I don't like the idea of calling the sub resource here. I think this should forward to wined3d_texture_map for textures for now and pass the sub resource index to the resource_map callback. In the long run we obviously want wined3d_texture_map and wined3d_buffer_map to be merged together, but for now (surface,volume)->(texture) is tricky enough. I have some patches in the works for that.

Nov. 2, 2015

Re: [PATCH 2/6] wined3d: Keep track of FBOs through the GL names (v2).

by Henri Verbeet

On 2 November 2015 at 12:09, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote: > I prefer "GL_COLOR_ATTACHMENT0" over "0x8CE0", but I guess wine_dbg_sprintf would also do the job without requring my own sprintf. > It would, but you don't even need that. You can just return a static string, like in debug_fbostatus(). > I don't trust the padding of the structure array after the 4 byte DWORD on 64 bit architectures. > It probably has some padding on 64-bit, but we don't memcmp() struct fbo_entry.

Nov. 2, 2015

Re: steam web browsing on OS X

by Sebastian Lackner

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 Best Regards, Sebastian

Nov. 2, 2015

Re: [PATCH 2/6] wined3d: Keep track of FBOs through the GL names (v2).

by Stefan Dösinger

> Am 02.11.2015 um 11:48 schrieb Henri Verbeet <hverbeet(a)gmail.com>: > I think I'll hold off on this until the regression from > 1ca9dfc8ee25f4ae188fdacd4d3d56046cef8003 is resolved, but you can > probably split the logging changes into their own patch. Fair enough. > >> + char name[32]; >> + sprintf(name, "Color attachment %u", i); >> + context_dump_fbo_attachment(gl_info, target, GL_COLOR_ATTACHMENT0 + i, name); > sprintf() is one of those things that's usually best avoided. In this > case the string is mostly redundant as well, since you have the > attachment enum. I prefer "GL_COLOR_ATTACHMENT0" over "0x8CE0", but I guess wine_dbg_sprintf would also do the job without requring my own sprintf. >> + entry = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, >> + FIELD_OFFSET(struct fbo_entry, key.objects[object_count])); >> + entry->d3d_render_targets = HeapAlloc(GetProcessHeap(), 0, >> + gl_info->limits.buffers * sizeof(*entry->d3d_render_targets)); >> + context_create_fbo_key(&entry->key, render_targets, depth_stencil, color_location, ds_location, >> + gl_info->limits.buffers); >> + memcpy(entry->d3d_render_targets, render_targets, sizeof(*entry->d3d_render_targets) * gl_info->limits.buffers); >> + entry->d3d_depth_stencil = depth_stencil; > Why HEAP_ZERO_MEMORY? Do we no longer initialize all fields? I don't trust the padding of the structure array after the 4 byte DWORD on 64 bit architectures.

Nov. 2, 2015