Re: [PATCH 3/6] d3dx9: Match native out of bounds array selector index handling.
2017-05-12 14:24 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c index 9c07f9c..21aa5d7 100644 --- a/dlls/d3dx9_36/effect.c +++ b/dlls/d3dx9_36/effect.c @@ -1051,7 +1051,9 @@ static HRESULT d3dx9_get_param_value_ptr(struct d3dx_pass *pass, struct d3dx_sta FIXME("Preshader structure is null.\n"); return D3DERR_INVALIDCALL; } - if (update_all || is_param_eval_input_dirty(param->param_eval)) + /* According to the tests, native d3dx handles the case of array index evaluated to -1 + * in a specific way. */ + if (state->index == 0xffffffffu || is_param_eval_input_dirty(param->param_eval, pass->update_version))
Hmm, so that's the index value set at initialization, thus getting a -1 in the first evaluation ends up breaking the "selector changed" check in native? You might want to spell that out in some form in the comment. AFAIU there is no other way a -1 is stored in state->index, correct?
On 05/15/2017 09:21 PM, Matteo Bruni wrote:
2017-05-12 14:24 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c index 9c07f9c..21aa5d7 100644 --- a/dlls/d3dx9_36/effect.c +++ b/dlls/d3dx9_36/effect.c @@ -1051,7 +1051,9 @@ static HRESULT d3dx9_get_param_value_ptr(struct d3dx_pass *pass, struct d3dx_sta FIXME("Preshader structure is null.\n"); return D3DERR_INVALIDCALL; } - if (update_all || is_param_eval_input_dirty(param->param_eval)) + /* According to the tests, native d3dx handles the case of array index evaluated to -1 + * in a specific way. */ + if (state->index == 0xffffffffu || is_param_eval_input_dirty(param->param_eval, pass->update_version)) Hmm, so that's the index value set at initialization, thus getting a -1 in the first evaluation ends up breaking the "selector changed" check in native? You might want to spell that out in some form in the comment. Well, it now looks like a leftover, it looks like now with the current version of the patch when parameter evaluation is fully transactional and actual recompute does not depend on update_all flag, this check 'state->index == 0xffffffffu' is redundant and I can just remove it, it does not break any test. I think it was needed on some of my previous versions of patches.
Now this part can be removed, it should be just if (is_param_eval_input_dirty(param->param_eval)) ... The check for array_idx == 0xffffffffu below is required though to match a special behaviour on -1 index value handling.
AFAIU there is no other way a -1 is stored in state->index, correct?
There is the other way to get -1 to index selector, it can be computed by preshader with corresponding parameters values set, and it is in the test. The overall logic I could "interpolate" from the tests is: 1. Whenever BeginPass or GetPassDesc return a success while the actual index value should be out of bound, they return the value for previously computed "good" index; 2. Unlike BeginPass and GetPassDesc, CommitChanges never returns failure on out of bound index, though still does not set a state in the case when BeginPass would return an error. BeginPass and GetPassDesc can either return failulre or not with the same actual index value to be computed, depending on what was happening before. 3. Whenever index recompute is required (based on updated parameters), the out of bound check is performed and error is returned from BeginPass or GetPassDesc. When the recomputed index is out of bound stored index value is never updated. 4. When the index recompute is not required (there was BeginPass or CommitChanges since last index parameter update, even if it computed out of bound index), the array element corresponding to the previous ever successfully selected is used. 5. If the index value is '-1', first array element is selected, and no errors are returned in this case. 6. GetPassDesc() does not 'reset' the modified parameters, multiple successive calls to it consistently return failure. If the 'modified state' of index was reset by BeginPass() or CommitChanges(), GetPassDesc() will also return success with previous good selection, just like BeginPass(). All this looks a bit like dragon poker to me, but now with transactional update logic just a special treatment of '-1' value and allowing a sort of bug not modifying previous index on out of bound compute seems to match all this.
2017-05-15 23:25 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 05/15/2017 09:21 PM, Matteo Bruni wrote:
2017-05-12 14:24 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c index 9c07f9c..21aa5d7 100644 --- a/dlls/d3dx9_36/effect.c +++ b/dlls/d3dx9_36/effect.c @@ -1051,7 +1051,9 @@ static HRESULT d3dx9_get_param_value_ptr(struct d3dx_pass *pass, struct d3dx_sta FIXME("Preshader structure is null.\n"); return D3DERR_INVALIDCALL; } - if (update_all || is_param_eval_input_dirty(param->param_eval)) + /* According to the tests, native d3dx handles the case of array index evaluated to -1 + * in a specific way. */ + if (state->index == 0xffffffffu || is_param_eval_input_dirty(param->param_eval, pass->update_version))
Hmm, so that's the index value set at initialization, thus getting a -1 in the first evaluation ends up breaking the "selector changed" check in native? You might want to spell that out in some form in the comment.
Well, it now looks like a leftover, it looks like now with the current version of the patch when parameter evaluation is fully transactional and actual recompute does not depend on update_all flag, this check 'state->index == 0xffffffffu' is redundant and I can just remove it, it does not break any test. I think it was needed on some of my previous versions of patches.
Now this part can be removed, it should be just if (is_param_eval_input_dirty(param->param_eval)) ...
The check for array_idx == 0xffffffffu below is required though to match a special behaviour on -1 index value handling.
AFAIU there is no other way a -1 is stored in state->index, correct?
There is the other way to get -1 to index selector, it can be computed by preshader with corresponding parameters values set, and it is in the test.
Isn't array_idx set to 0 in that case before being stored in state->index though?
The overall logic I could "interpolate" from the tests is:
1. Whenever BeginPass or GetPassDesc return a success while the actual index value should be out of bound, they return the value for previously computed "good" index;
2. Unlike BeginPass and GetPassDesc, CommitChanges never returns failure on out of bound index, though still does not set a state in the case when BeginPass would return an error. BeginPass and GetPassDesc can either return failulre or not with the same actual index value to be computed, depending on what was happening before.
3. Whenever index recompute is required (based on updated parameters), the out of bound check is performed and error is returned from BeginPass or GetPassDesc. When the recomputed index is out of bound stored index value is never updated.
4. When the index recompute is not required (there was BeginPass or CommitChanges since last index parameter update, even if it computed out of bound index), the array element corresponding to the previous ever successfully selected is used.
5. If the index value is '-1', first array element is selected, and no errors are returned in this case.
6. GetPassDesc() does not 'reset' the modified parameters, multiple successive calls to it consistently return failure. If the 'modified state' of index was reset by BeginPass() or CommitChanges(), GetPassDesc() will also return success with previous good selection, just like BeginPass().
All this looks a bit like dragon poker to me, but now with transactional update logic just a special treatment of '-1' value and allowing a sort of bug not modifying previous index on out of bound compute seems to match all this.
Okay, I went back and rechecked the tests. I see that I missed a few bits and that -1 is actually weird. Not that it really matters, I'm just trying to explain this as accidental behavior rather than some weird kind of intentional decision on MS's part. It's probably still the case but I fail to see how it would happen. Oh well.
On 05/16/2017 05:53 PM, Matteo Bruni wrote:
2017-05-15 23:25 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 05/15/2017 09:21 PM, Matteo Bruni wrote:
2017-05-12 14:24 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c index 9c07f9c..21aa5d7 100644 --- a/dlls/d3dx9_36/effect.c +++ b/dlls/d3dx9_36/effect.c @@ -1051,7 +1051,9 @@ static HRESULT d3dx9_get_param_value_ptr(struct d3dx_pass *pass, struct d3dx_sta FIXME("Preshader structure is null.\n"); return D3DERR_INVALIDCALL; } - if (update_all || is_param_eval_input_dirty(param->param_eval)) + /* According to the tests, native d3dx handles the case of array index evaluated to -1 + * in a specific way. */ + if (state->index == 0xffffffffu || is_param_eval_input_dirty(param->param_eval, pass->update_version)) Hmm, so that's the index value set at initialization, thus getting a -1 in the first evaluation ends up breaking the "selector changed" check in native? You might want to spell that out in some form in the comment. Well, it now looks like a leftover, it looks like now with the current version of the patch when parameter evaluation is fully transactional and actual recompute does not depend on update_all flag, this check 'state->index == 0xffffffffu' is redundant and I can just remove it, it does not break any test. I think it was needed on some of my previous versions of patches.
Now this part can be removed, it should be just if (is_param_eval_input_dirty(param->param_eval)) ...
The check for array_idx == 0xffffffffu below is required though to match a special behaviour on -1 index value handling.
AFAIU there is no other way a -1 is stored in state->index, correct?
There is the other way to get -1 to index selector, it can be computed by preshader with corresponding parameters values set, and it is in the test. Isn't array_idx set to 0 in that case before being stored in state->index though? Yes, exactly. Setting state->index to -1 below in effect creation is not exactly meaningless though, as it triggers -1 -> 0 wrap on "unmodified" index recompute if the "good" index was never set previously. But actually as I just realized it can be changed to 'state->index = 0' with the same effect, and removed after that as structures are zero initialized anyway :) So this array_idx -1 check should be the only "weird" special case to stay I suppose. The overall logic I could "interpolate" from the tests is:
1. Whenever BeginPass or GetPassDesc return a success while the actual index value should be out of bound, they return the value for previously computed "good" index;
2. Unlike BeginPass and GetPassDesc, CommitChanges never returns failure on out of bound index, though still does not set a state in the case when BeginPass would return an error. BeginPass and GetPassDesc can either return failulre or not with the same actual index value to be computed, depending on what was happening before.
3. Whenever index recompute is required (based on updated parameters), the out of bound check is performed and error is returned from BeginPass or GetPassDesc. When the recomputed index is out of bound stored index value is never updated.
4. When the index recompute is not required (there was BeginPass or CommitChanges since last index parameter update, even if it computed out of bound index), the array element corresponding to the previous ever successfully selected is used.
5. If the index value is '-1', first array element is selected, and no errors are returned in this case.
6. GetPassDesc() does not 'reset' the modified parameters, multiple successive calls to it consistently return failure. If the 'modified state' of index was reset by BeginPass() or CommitChanges(), GetPassDesc() will also return success with previous good selection, just like BeginPass().
All this looks a bit like dragon poker to me, but now with transactional update logic just a special treatment of '-1' value and allowing a sort of bug not modifying previous index on out of bound compute seems to match all this. Okay, I went back and rechecked the tests. I see that I missed a few bits and that -1 is actually weird.
Not that it really matters, I'm just trying to explain this as accidental behavior rather than some weird kind of intentional decision on MS's part. It's probably still the case but I fail to see how it would happen. Oh well.
I can't imagine how it can all be an intentional behaviour, like returning error on first BeginPass() and then success on the second using previous array index. It looks clearly like a small bug in stored index update logic and error handling to me, which we easily reproduce without having some special cases in getting index. Special behaviour of -1 make me a bit curious too. I attributed it to some special handling of index update for "unitialized" index value coded with -1, but I am not sure I have any clear guess on what exactly is done with this -1 index there, though seemingly reproduce the exhibited behaviour.
participants (2)
-
Matteo Bruni -
Paul Gofman