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

Re: [PATCH 1/2] comctl32/taskdialog: Initial support for callback procedure

by Nikolay Sivov

On 30.08.2017 2:06, Fabian Maurer wrote: > Hi, > > Sorry, but there's a bunch of issues I need to point out. Of course we could > address them later, but it'll mean rewriting a bunch of code, and I'd like to > have it correct from the beginning. In general, I think this implementation > limits us too much. No, you're right, if it means large changes later it should be done differently. > > For example, I don't think it's a good idea to have the TASKDIALOGCONFIG > structure being filled in run_test_, we need to be able to control it easily. I have no problem with that, except for maybe initializing size/callback/user data in run_test still. > > Second, we also need to be able to return different values in the callback, > the current approach doesn't account for that. Ok, this is easy to add. > > And the most important part, the separation from messages to send and received > messages to test against. It's better to send the next message only when the > last message arrived, to make sure we process them in the correct order. > I attached a patch to show the problem (apply on top of my in-review patch). > When sending all messages together, the first message is skipped, and we don't > want that to happen. What do you mean by last message arrival? Currently all messages to control are posted, so we don't wait for anything but rely on callback to be called eventually. Do you mean that you want to specify message to be sent to control for every callback individually, like for example in mes_cancel_esc_callback1 - WM_KEYDOWN is sent in response to TDN_CREATED and TDM_CLICK_BUTTON in response to TDN_BUTTON_CLICKED? I'll look into that. > > Please reconsider my approach, I can try to simplify it if you want, but I > think the complexity is justified to solve a complicated problem. Yes, I will, thanks for your comments. > > Regards, > Fabian Maurer > > > >

Aug. 30, 2017

Re: [PATCH 1/2] comctl32/taskdialog: Initial support for callback procedure

by Fabian Maurer

Hi, Sorry, but there's a bunch of issues I need to point out. Of course we could address them later, but it'll mean rewriting a bunch of code, and I'd like to have it correct from the beginning. In general, I think this implementation limits us too much. For example, I don't think it's a good idea to have the TASKDIALOGCONFIG structure being filled in run_test_, we need to be able to control it easily. Second, we also need to be able to return different values in the callback, the current approach doesn't account for that. And the most important part, the separation from messages to send and received messages to test against. It's better to send the next message only when the last message arrived, to make sure we process them in the correct order. I attached a patch to show the problem (apply on top of my in-review patch). When sending all messages together, the first message is skipped, and we don't want that to happen. Please reconsider my approach, I can try to simplify it if you want, but I think the complexity is justified to solve a complicated problem. Regards, Fabian Maurer

Aug. 29, 2017

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

by Matteo Bruni

2017-08-29 17:08 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > On 08/29/2017 05:35 PM, Matteo Bruni wrote: >> >> Right, I think checking that during parsing is a good idea regardless >> of everything else. >> >> I'd argue that either the expression above is too generic (allowing / >> handling register_count > 1) or the other register_count expression >> (the merge one I brought up earlier) is too naive, not expecting the >> same. As it is it just doesn't look right. > > I updated the expression in 'merge' phase by now to do the check, more > exactly, added a check for the case under discussion inside the 'if'. > > To block the situation when register components exceed register size, I > actually should check not just component count but offset also. Perhaps this > should also be done not just for output but for input registers only. This > can't be done cleanly for input registers though for all the cases, as there > might be a relative offset which can still effectively make a multicomponent > operation cross register boundary. Not sure if we should add runtime checks > for that. Native implementation seems to allow all this and does not refuse > to parse preshaders which crossses a register boundary in instruction even > without relative addressing. It does allow some things we already deny now, > so I think I should improve the checks for components during parsing and > block them early when possible, and after that consider preshader > instruction output to affect just one register always in this part. Native may allow that but there is some good chance that it computes incorrect outputs / sets incorrect device states in those cases (i.e. I imagine instructions straddling multiple registers to be unexpected for native too). It's probably not relevant in practice. Anyway, sure, I'm okay with adding those checks in our implementation if you want (mostly to print some WARNs if the unexpected happens) but simply assuming / enforcing a single output register per instruction, along the lines of my last email, seems enough to me.

Aug. 29, 2017

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

by Paul Gofman

On 08/29/2017 05:35 PM, Matteo Bruni wrote: > > Right, I think checking that during parsing is a good idea regardless > of everything else. > > I'd argue that either the expression above is too generic (allowing / > handling register_count > 1) or the other register_count expression > (the merge one I brought up earlier) is too naive, not expecting the > same. As it is it just doesn't look right. I updated the expression in 'merge' phase by now to do the check, more exactly, added a check for the case under discussion inside the 'if'. To block the situation when register components exceed register size, I actually should check not just component count but offset also. Perhaps this should also be done not just for output but for input registers only. This can't be done cleanly for input registers though for all the cases, as there might be a relative offset which can still effectively make a multicomponent operation cross register boundary. Not sure if we should add runtime checks for that. Native implementation seems to allow all this and does not refuse to parse preshaders which crossses a register boundary in instruction even without relative addressing. It does allow some things we already deny now, so I think I should improve the checks for components during parsing and block them early when possible, and after that consider preshader instruction output to affect just one register always in this part. > If you ensure during parsing that, for each preshader instruction, 0 < > component_count <= components_per_register then you can just always > set the initial register_count to 1. Actually you could avoid setting > it at all at that point (more on that below). > >> So maybe I will add an assert for the overlapping registers in this >> 'if' under discussion just in case? > That may also be a good idea. Otherwise, if the "+ out->const_set[i + > 1].register_count" part is replaced with a "+ 1", the overlap case > would be handled trivially. That would make it possible to avoid the > previous register_count initialization entirely, assuming > register_count is set to 1 during merging in the NOT merging case. Yes, sure. > Both options are okay with me. Also, as usual, let me know if I'm > still missing something. > I added a check in this patch by now (as all the other code actually allows for multicomponent operation to cross register boundary by now, and thus register_count can potentially exceed 1 here).

Aug. 29, 2017

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

by Matteo Bruni

2017-08-29 1:16 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > On 08/29/2017 01:51 AM, Matteo Bruni wrote: >> >> 2017-08-28 23:32 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: >>> >>> On 08/28/2017 09:00 PM, Matteo Bruni wrote: >>>> >>>> >>>>> @@ -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. >>> >>> This 'if' selects just preshader output constants. The entries for >>> const_set >>> handled (merged) in this 'if' are those provided preshader instructions >>> which output anything to shader constants. They can potentially overlap >>> (that's why '>=' in the last 'if' check). I am not sure what can be >>> wrong >>> with that? We track preshader output as a single "commit", the same does >>> native implementation according to our tests. >> >> I don't have any issues with the if condition. It's the >> const_set[i].register_count assignment which looks troublesome since >> it doesn't consider the old count. In the (artificial, even forced if >> you want) example I gave const_set[i].register_count would go from 4 >> to 2 which doesn't seem right. >> > Here is how this register_count is currently evaluated: > > ---- snip ---- > > const_set.register_count = max(get_reg_offset(reg->table, > pres_op_info[ins->op].func_all_comps ? 1 : > ins->component_count), 1); > > ---- snip ---- > > This expression is written for some generic case of component_count and > components per register, but unless I am missing something this expression > should not evaluate to anything but 1 for the real cases. We do not check > that component count in instruction does not exceed component count per > register during preshader parse though (just checking if it is between 1 and > 4). Right, I think checking that during parsing is a good idea regardless of everything else. I'd argue that either the expression above is too generic (allowing / handling register_count > 1) or the other register_count expression (the merge one I brought up earlier) is too naive, not expecting the same. As it is it just doesn't look right. If you ensure during parsing that, for each preshader instruction, 0 < component_count <= components_per_register then you can just always set the initial register_count to 1. Actually you could avoid setting it at all at that point (more on that below). > So maybe I will add an assert for the overlapping registers in this > 'if' under discussion just in case? That may also be a good idea. Otherwise, if the "+ out->const_set[i + 1].register_count" part is replaced with a "+ 1", the overlap case would be handled trivially. That would make it possible to avoid the previous register_count initialization entirely, assuming register_count is set to 1 during merging in the NOT merging case. Both options are okay with me. Also, as usual, let me know if I'm still missing something.

Aug. 29, 2017

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

by Paul Gofman

On 08/29/2017 01:51 AM, Matteo Bruni wrote: > 2017-08-28 23:32 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: >> On 08/28/2017 09:00 PM, Matteo Bruni wrote: >>> >>>> @@ -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. >> This 'if' selects just preshader output constants. The entries for const_set >> handled (merged) in this 'if' are those provided preshader instructions >> which output anything to shader constants. They can potentially overlap >> (that's why '>=' in the last 'if' check). I am not sure what can be wrong >> with that? We track preshader output as a single "commit", the same does >> native implementation according to our tests. > I don't have any issues with the if condition. It's the > const_set[i].register_count assignment which looks troublesome since > it doesn't consider the old count. In the (artificial, even forced if > you want) example I gave const_set[i].register_count would go from 4 > to 2 which doesn't seem right. > Here is how this register_count is currently evaluated: ---- snip ---- const_set.register_count = max(get_reg_offset(reg->table, pres_op_info[ins->op].func_all_comps ? 1 : ins->component_count), 1); ---- snip ---- This expression is written for some generic case of component_count and components per register, but unless I am missing something this expression should not evaluate to anything but 1 for the real cases. We do not check that component count in instruction does not exceed component count per register during preshader parse though (just checking if it is between 1 and 4). So maybe I will add an assert for the overlapping registers in this 'if' under discussion just in case?

Aug. 28, 2017

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

by Matteo Bruni

2017-08-28 23:32 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > On 08/28/2017 09:00 PM, Matteo Bruni wrote: >> >> >>> @@ -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. > > This 'if' selects just preshader output constants. The entries for const_set > handled (merged) in this 'if' are those provided preshader instructions > which output anything to shader constants. They can potentially overlap > (that's why '>=' in the last 'if' check). I am not sure what can be wrong > with that? We track preshader output as a single "commit", the same does > native implementation according to our tests. I don't have any issues with the if condition. It's the const_set[i].register_count assignment which looks troublesome since it doesn't consider the old count. In the (artificial, even forced if you want) example I gave const_set[i].register_count would go from 4 to 2 which doesn't seem right.

Aug. 28, 2017

Re: [PATCH] comctl32/taskdialog: Add callback and tests

by Fabian Maurer

Hello, sorry I thought I already sent this, but it apparently didn't work. > This looks suspicious, documentation is not strict on this, but it seems > TDN_CREATED corresponds for to WM_SHOWWINDOW stage. I'm not sure, I'll look into it. > > + /* Custom messages*/ > > What do you mean by "custom"? I meant taskdialog specific messages. Guess I'll change it to "Taskdialog messages" > > - if (verification_flag_checked) *verification_flag_checked = TRUE; > > + if (verification_flag_checked) *verification_flag_checked = FALSE; > > Why do you need this flag value change? Because that's what the tests told me windows behaves like. > > > return S_OK; > > > > } > > > > diff --git a/dlls/comctl32/tests/taskdialog.c > > b/dlls/comctl32/tests/taskdialog.c index c937127897..867361617c 100644 > > --- a/dlls/comctl32/tests/taskdialog.c > > +++ b/dlls/comctl32/tests/taskdialog.c > > @@ -24,13 +24,250 @@ > > > > #include "winuser.h" > > #include "commctrl.h" > > > > +#include "wine/list.h" > > > > #include "wine/test.h" > > #include "v6util.h" > > > > +#include "msg.h" > > + > > +#define WM_TD_CALLBACK (WM_APP) /* Custom dummy message to wrap callback > > notifications */ + > > +#define NUM_MSG_SEQUENCES 1 > > +#define TASKDIALOG_SEQ_INDEX 0 > > + > > +#define TEST_NUM_BUTTONS 20 /* Number of custom buttons to test with */ > > + > > +#define ID_START 20 /* Lower IDs might be used by the system */ > > +#define ID_START_BUTTON (ID_START + 0) > > > > static HRESULT (WINAPI *pTaskDialogIndirect)(const TASKDIALOGCONFIG *, > > int *, int *, BOOL *); static HRESULT (WINAPI *pTaskDialog)(HWND, > > HINSTANCE, const WCHAR *, const WCHAR *, const WCHAR *,> > > TASKDIALOG_COMMON_BUTTON_FLAGS, const WCHAR *, int *); > > > > +static struct msg_sequence *sequences[NUM_MSG_SEQUENCES]; > > + > > +/* Message lists to test against */ > > + > > +struct message_send_info > > +{ > > + UINT send_message; > > + WPARAM send_wparam; > > + LPARAM send_lparam; > > + > > + BOOL post; /* post instead of send */ > > + const CHAR *title_target; /* control text, 0 means it's send to the > > dialog form instead */ +}; > > + > > +struct message_info > > +{ > > + UINT recv_message; /* Message the callback receives */ > > + WPARAM recv_wparam; > > + LPARAM recv_lparam; > > + > > + HRESULT ret; /* Value the callback should return */ > > + > > + struct message_send_info send[9]; /* Message to send to trigger the > > next callback message */ +}; > > + > > +static const struct message_info *current_message_info; > > + > > +static const struct message_info mes_return_press_ok[] = { > > + { TDN_CREATED, 0, 0, S_OK, { > > + { WM_KEYDOWN, VK_RETURN, 0, TRUE }, > > + { 0 }}}, > > + { TDN_BUTTON_CLICKED, IDOK, 0, S_OK, {{ 0 }}}, > > + { 0 } > > +}; > > + > > +static const struct message_info mes_cancel_button_press[] = { > > + { TDN_CREATED, 0, 0, S_OK, { > > + { TDM_CLICK_BUTTON, IDOK, 0, TRUE }, > > + { TDM_CLICK_BUTTON, IDOK, 0, TRUE }, > > + { TDM_CLICK_BUTTON, IDOK, 0, TRUE }, > > + { 0 }}}, > > + { TDN_BUTTON_CLICKED, IDOK, 0, S_FALSE, {{ 0 }}}, > > + { TDN_BUTTON_CLICKED, IDOK, 0, 0xFF, {{ 0 }}}, /* Random return value > > tested here */ + { TDN_BUTTON_CLICKED, IDOK, 0, S_OK, {{ 0 }}}, > > + { 0 } > > +}; > > + > > +static const struct message_info mes_send_button_directly[] = { > > + { TDN_CREATED, 0, 0, S_OK, { > > + { WM_LBUTTONDOWN, MK_LBUTTON, 0, TRUE, "02" }, > > + { WM_LBUTTONUP, 0, 0, TRUE, "02" }, > > + { 0 }}}, > > + { TDN_BUTTON_CLICKED, ID_START + 2, 0, S_OK, {{ 0 }}}, > > + { 0 } > > +}; > > This looks too complicated, I'd just check notifications callback > received, using 'struct message' data, same way WM_NOTIFY tests work. I have more tests currently available, like cancelling tests. They're not part of this patch, but I figured I'll do it right from the beginning. > > +static HWND get_child_from_title(HWND hwnd_parent, const CHAR *title) > > +{ > > + taskdialog_child = NULL; > > + EnumChildWindows(hwnd_parent, enum_taskdialog_children_proc, > > (LPARAM)title); + return taskdialog_child; > > +} > > I don't think we need to get that deep. Are you sure? I figured it would make sense to actually test for simulated mouse clicks instead of just sending the taskdialog specific click messages. > I think for first iteration test should simply check > creation/destruction notifications and callback user data value. I'll take care of the issues and resend the patch as simplified version. Regards Fabian Maurer

Aug. 28, 2017

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

by Paul Gofman

On 08/28/2017 09:00 PM, Matteo Bruni wrote: > >> @@ -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. This 'if' selects just preshader output constants. The entries for const_set handled (merged) in this 'if' are those provided preshader instructions which output anything to shader constants. They can potentially overlap (that's why '>=' in the last 'if' check). I am not sure what can be wrong with that? We track preshader output as a single "commit", the same does native implementation according to our tests.

Aug. 28, 2017

Wine fails to compile in kernel32

by Fabian Maurer

Hello wine-devel, I'm running ArchLinux with gcc 7.2.0. Since commit e99811aee2dff88c41d6e2875401810a85325839 wine fails to build with > ../../../wine-git/dlls/kernel32/module.c: In function ‘load_library’: > ../../../wine-git/dlls/kernel32/module.c:987:44: error: initializer element is not constant > static const DWORD unsupported_flags = load_library_search_flags | Minimal test-program to show the error, just use something like "gcc main.c" to compile. ---------- static const int load_library_search_flags = 0; int main() { static const int unsupported_flags = load_library_search_flags; return 0; } ---------- Looks like an compiler error, but I'm not sure. Mind looking into that? Regards, Fabian Maurer

Aug. 28, 2017