August 2017 - Wine-Devel - list.winehq.org

Re: [PATCH 2/2] d3dx9: Get rid of constants modification bitmasks.

by Matteo Bruni

2017-08-10 12:02 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> The patch is fine overall, I only have a few questions / nitpicks. I'll skip the part about the delay with the review because it's old news by now... :/ > --- > dlls/d3dx9_36/d3dx9_private.h | 3 +- > dlls/d3dx9_36/preshader.c | 312 ++++++++++++++++++------------------------ > 2 files changed, 135 insertions(+), 180 deletions(-) > > diff --git a/dlls/d3dx9_36/d3dx9_private.h b/dlls/d3dx9_36/d3dx9_private.h > index 2727463a18..5ac5e63aeb 100644 > --- a/dlls/d3dx9_36/d3dx9_private.h > +++ b/dlls/d3dx9_36/d3dx9_private.h > @@ -230,7 +230,7 @@ enum pres_reg_tables > struct d3dx_const_param_eval_output > { > struct d3dx_parameter *param; > - unsigned int table; > + enum pres_reg_tables table; The change is good but (unrelated to this patch) the enum type should really use the singular form. > static void dump_bytecode(void *data, unsigned int size) > @@ -674,6 +614,40 @@ static HRESULT append_const_set(struct d3dx_const_tab *const_tab, struct d3dx_co > return D3D_OK; > } > > +static void append_pres_const_set(struct d3dx_const_tab *const_tab, struct d3dx_preshader *pres) Nitpick: this function appends all the struct d3dx_const_param_eval_output from the preshader so calling it "append_pres_const_sets" (or even something more explicit to that effect, e.g. clarifying that you're appending entries about the preshader outputs, which are shader inputs) would be preferable. > +{ > + unsigned int i; > + struct d3dx_const_param_eval_output const_set; > + > + memset(&const_set, 0, sizeof(const_set)); You could initialize const_set in the definition instead, like: struct d3dx_const_param_eval_output const_set = {NULL}; which looks slightly better to me. It doesn't matter much of course. > @@ -971,10 +946,30 @@ static HRESULT get_constants_desc(unsigned int *byte_code, struct d3dx_const_tab > if (FAILED(hr = init_set_constants_param(out, ctab, hc, inputs_param[index]))) > goto cleanup; > } > + if (pres) > + append_pres_const_set(out, pres); > if (out->const_set_count) > { > struct d3dx_const_param_eval_output *new_alloc; > > + qsort(out->const_set, out->const_set_count, sizeof(*out->const_set), compare_const_set); > + for (i = 0; i < out->const_set_count - 1; ++i) > + { > + if (out->const_set[i].constant_class == D3DXPC_FORCE_DWORD > + && out->const_set[i + 1].constant_class == D3DXPC_FORCE_DWORD > + && out->const_set[i].table == out->const_set[i + 1].table > + && out->const_set[i].register_index + out->const_set[i].register_count > + >= out->const_set[i + 1].register_index) > + { > + out->const_set[i].register_count = out->const_set[i + 1].register_index > + + out->const_set[i + 1].register_count - out->const_set[i].register_index; I think this might lose account of some registers. E.g. consider the case where const_set[i] has register_index == 0 and register_count == 4 while const_set[i + 1] has register_index == 1 and register_count == 1. This would seem to lose track of the last two components. I know that preshaders found in the wild seem not to overwrite already written output registers but I wouldn't feel safe with (silently) depending on it. > + memmove(&out->const_set[i + 1], &out->const_set[i + 2], sizeof(out->const_set[i]) > + * (out->const_set_count - i - 2)); > + --out->const_set_count; > + --i; Mostly matter of personal preference but rather than decrementing the for counter in the if when coalescing I'd write this as a while and increment "i" only in the (currently absent) else case. > @@ -1529,22 +1534,56 @@ static void set_constants(struct d3dx_regstore *rs, struct d3dx_const_tab *const > data += param->rows * param->columns; > } > start_offset = get_offset_reg(table, const_set->register_index); > - if (table_info[table].type == param_type) > - regstore_set_modified(rs, table, start_offset, > - get_offset_reg(table, const_set->register_count) * const_set->element_count); > - else > + if (table_info[table].type != param_type) > regstore_set_data(rs, table, start_offset, (unsigned int *)rs->tables[table] + start_offset, > get_offset_reg(table, const_set->register_count) * const_set->element_count, param_type); > } > + > + > const_tab->update_version = new_update_version; One blank line too many. > -} > + if (!update_device) > + return D3D_OK; > > + for (const_idx = 0; const_idx < const_tab->const_set_count; ++const_idx) > + { > + struct d3dx_const_param_eval_output *const_set = &const_tab->const_set[const_idx]; > + > + if (device_update_all || (const_set->param > + ? is_param_dirty(const_set->param, update_version) : pres_dirty)) > + { > + enum pres_reg_tables table = const_set->table; > + > + if (table == device_table && device_start + device_count == const_set->register_index) Again nitpicking: are there better names for those variables? Maybe use the "current_" prefix instead? It's pretty clear what's going on either way, so up to you.

Aug. 28, 2017

Re: [PATCH] ws2_32: Also check the FQDN when injecting the local IP.

by Alexandre Julliard

Stefan Dösinger <stefan(a)codeweavers.com> writes: > @@ -6576,6 +6592,8 @@ int WINAPI WS_getaddrinfo(LPCSTR nodename, LPCSTR servname, const struct WS_addr > > hostname = get_hostname(); > if (!hostname) return WSA_NOT_ENOUGH_MEMORY; > + fqdn = get_fqdn(); > + if (!fqdn) return WSA_NOT_ENOUGH_MEMORY; It seems to me that you shouldn't need to retrieve both separately, the FQDN contains the hostname already. -- Alexandre Julliard julliard(a)winehq.org

Aug. 28, 2017

Re: Issue with suspended process

by Alexandre Julliard

Jonathan Doron <jond(a)wizery.com> writes: > Sure what type of tests would you like to see? Basically you'd create a suspended process and then poke around and test what has or hasn't been initialized yet. You can use the various things that the Wine loader does during init as a guide to what to test. Then you can also try changing things, resume the process and check that it has the expected effect. -- Alexandre Julliard julliard(a)winehq.org

Aug. 25, 2017

Re: [PATCH resend] msxml3: Remove CRs in domtext_put_data and add them in domtext_get_xml.

by Alex Henrie

2017-08-17 13:34 GMT-06:00 Nikolay Sivov <bunglehead(a)gmail.com>: > On 03.08.2017 18:17, Alex Henrie wrote: >> Fixes https://bugs.winehq.org/show_bug.cgi?id=42468 >> >> To be clear, I am not trying to fix all whitespace processing issues. I >> am only trying to prevent "" from being erroneously inserted into >> the output. >> >> Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com> >> --- >> dlls/msxml3/tests/domdoc.c | 106 +++++++++++++++++++++++++++++++++++++-------- >> dlls/msxml3/text.c | 42 +++++++++++++++--- >> 2 files changed, 122 insertions(+), 26 deletions(-) >> >> diff --git a/dlls/msxml3/tests/domdoc.c b/dlls/msxml3/tests/domdoc.c > > I think it needs more tests, for xml:space and put_preserveWhiteSpace(). Okay, I've added tests for xml:space="preserve" and preserveWhiteSpace. >> diff --git a/dlls/msxml3/text.c b/dlls/msxml3/text.c >> index 3a12d4bf62..6814c1f817 100644 >> --- a/dlls/msxml3/text.c >> +++ b/dlls/msxml3/text.c >> @@ -519,10 +519,15 @@ static HRESULT WINAPI domtext_get_xml( >> BSTR* p) >> { >> domtext *This = impl_from_IXMLDOMText( iface ); >> + HRESULT hr; >> >> TRACE("(%p)->(%p)\n", This, p); >> >> - return node_get_xml(&This->node, FALSE, p); >> + hr = node_get_xml(&This->node, FALSE, p); >> + if (hr == S_OK) >> + *p = EnsureCorrectEOL(*p); >> + >> + return hr; > > Will node_get_xml(..., TRUE, ...) work? Yes, thanks for the suggestion! >> } >> >> static HRESULT WINAPI domtext_transformNode( >> @@ -616,15 +621,38 @@ static HRESULT WINAPI domtext_put_data( >> BSTR data) >> { >> domtext *This = impl_from_IXMLDOMText( iface ); >> - static const WCHAR rnW[] = {'\r','\n',0}; >> + BSTR normalized_data = NULL; >> + HRESULT hr; >> + int i, j; >> >> TRACE("(%p)->(%s)\n", This, debugstr_w(data)); >> >> - if (data && !strcmpW(rnW, data)) >> - This->node.node->name = xmlStringTextNoenc; >> - else >> - domtext_reset_noenc(This); >> - return node_set_content(&This->node, data); >> + if (data) >> + { >> + /* normalize line endings */ >> + normalized_data = SysAllocStringLen(NULL, SysStringLen(data)); >> + if (!normalized_data) return E_OUTOFMEMORY; >> + for (i = 0, j = 0; data[i]; i++) >> + { >> + if (data[i] == '\r') >> + { >> + if (data[i + 1] == '\n') >> + continue; /* change \r\n to just \n */ >> + else >> + normalized_data[j] = '\n'; /* change \r by itself to \n */ >> + } >> + else >> + normalized_data[j] = data[i]; >> + j++; >> + } >> + normalized_data[j] = 0; > > The only purpose for that is to inhibit libxml2 escaping, right? Could > this be done the way it is now, but with something more elaborate than > comparing to "\r\n"? No, because we only want to inhibit the escaping of "\r" to "". We don't want to inhibit the escaping of other characters such as "&" and "<". -Alex

Aug. 25, 2017

Re: Issue with suspended process

by Jonathan Doron

Sure what type of tests would you like to see? One thing to note is the application I'm referring to Tracker which is part of MS toolchain is a Microsoft application, so no hacks or anything that copy protection software did that sometimes was not fully compatible with all the loaders changes MS did through out the years. On Fri, Aug 25, 2017 at 4:41 PM, Alexandre Julliard <julliard(a)winehq.org> wrote: > Jon Doron <arilou(a)gmail.com> writes: > > > The interesting thing is that in the past Wine behavior was correct > > and the change that basically broke it is commit: > > > > commit 0f5fc117a2320bb7e21b9ae4d07d717bed413171 > > Author: Alexandre Julliard <julliard(a)winehq.org> > > Date: Mon Nov 19 14:27:07 2007 +0100 > > > > ntdll: Unblock signals in process init only after the dlls have > > been imported. > > > > > > Unfortunately though there I could not find any reason why the change > > was made or if there is any bug that it fixed back then (2007) . > > There were apps (usually copy protection) that expected to find some > dlls already loaded in order to patch them. Still, it's very possible > that the initialization order isn't quite correct yet. The first step > would be to write some test cases. > > -- > Alexandre Julliard > julliard(a)winehq.org > > >

Aug. 25, 2017

Re: Issue with suspended process

by Alexandre Julliard

Jon Doron <arilou(a)gmail.com> writes: > The interesting thing is that in the past Wine behavior was correct > and the change that basically broke it is commit: > > commit 0f5fc117a2320bb7e21b9ae4d07d717bed413171 > Author: Alexandre Julliard <julliard(a)winehq.org> > Date: Mon Nov 19 14:27:07 2007 +0100 > > ntdll: Unblock signals in process init only after the dlls have > been imported. > > > Unfortunately though there I could not find any reason why the change > was made or if there is any bug that it fixed back then (2007) . There were apps (usually copy protection) that expected to find some dlls already loaded in order to patch them. Still, it's very possible that the initialization order isn't quite correct yet. The first step would be to write some test cases. -- Alexandre Julliard julliard(a)winehq.org

Aug. 25, 2017

Re: [v2] d3d9/tests: Add test for SW shader in MVP mode.

by Paul Gofman

This is a pre-existing failure. On 08/25/2017 12:58 PM, Marvin wrote: > 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 > https://testbot.winehq.org/JobDetails.pl?Key=32837 > > Your paranoid android. > > > === w8 (32 bit visual) === > visual.c:7952: Test failed: Input test: Quad 3(2crd-wrongidx) returned color 0x00ff00ff, expected 0x00ff0080 >

Aug. 25, 2017

Re: [v2] d3d9/tests: Add test for SW shader in MVP mode.

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 https://testbot.winehq.org/JobDetails.pl?Key=32837 Your paranoid android. === w8 (32 bit visual) === visual.c:7952: Test failed: Input test: Quad 3(2crd-wrongidx) returned color 0x00ff00ff, expected 0x00ff0080

Aug. 25, 2017

Re: Issue with suspended process

by Stefan Dösinger

Hi, On 24/08/17 16:41, Jonathan Doron wrote: > The next thing that happens is LdrInitializeThunk continues and fixing > up the EXE module imports, this is not the same behavior as Windows in > which the Ldr only starts running after the thread has > resumed. Depending on what you want to do in the end you may be able to work around the issue by adding a LoadLibraryA("Whatever_injected.dll") somewhere in the init code. That might allow you to use your tool. It helped when I was trying to use pixwin from the DX SDK a while ago. > commit 0f5fc117a2320bb7e21b9ae4d07d717bed413171 > Author: Alexandre Julliard <julliard(a)winehq.org > <mailto:julliard(a)winehq.org>> > Date: Mon Nov 19 14:27:07 2007 +0100 > > ntdll: Unblock signals in process init only after the dlls have > been imported. Alexandre is on vacation at the moment as far as I know. He might reply later if he notices the mail in his return inbox flood :-) . bugs.winehq.org tracks which commit fixed a bug, but unfortunately that system was not in place in 2007. Stefan

Aug. 25, 2017

Re: [PATCH 3/3] d3d9/tests: Add test for SW shader in MVP mode.

by Henri Verbeet

On 24 August 2017 at 17:27, Paul Gofman <gofmanp(a)gmail.com> wrote: > On 08/24/2017 04:39 PM, Henri Verbeet wrote: >> On 23 August 2017 at 16:17, Paul Gofman <gofmanp(a)gmail.com> wrote: >>> + /* wined3d does not guarantee 0 for out of bounds constant access by >>> default. */ >> >> IIRC neither does Windows, in general. Matteo may have a better >> recollection of the details, but IIRC while it mostly works out in >> practice on current NVIDIA hardware, it doesn't necessarily everywhere >> else, and is essentially hardware/driver specific behaviour. >> > So should I maybe change the test not to check for exact value but rather > for not matching constant c256? > It might still match by chance. Perhaps it's best to just not test the result of the draw and instead document why.

Aug. 24, 2017