[PATCH 3/6] d3dx9: Match native out of bounds array selector index handling.

[Paul Gofman]

On 05/16/2017 05:53 PM, Matteo Bruni wrote:
> 2017-05-15 23:25 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
>> On 05/15/2017 09:21 PM, Matteo Bruni wrote:
>>> 2017-05-12 14:24 GMT+02:00 Paul Gofman <gofmanp at 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.