[v3 PATCH] d3d10/effect: Add initial support for indexing expressions.

[Nikolay Sivov]

On 11/9/21 12:29 PM, Matteo Bruni wrote:
>  On Mon, Nov 8, 2021 at 1:30 PM Nikolay Sivov <nsivov at codeweavers.com> wrote:
>> Signed-off-by: Nikolay Sivov <nsivov at codeweavers.com>
>> ---
>>
>> v3: some rework based on received feedback
>>
>> For now for simplicity I removed destination index from property update, with additional
>> test for property type when parsing.
> I like the changes. Unfortunately I keep finding stuff...
> Only one important comment, see below.
>
>>  dlls/d3d10/effect.c       | 577 +++++++++++++++++++++++++++++++++++++-
>>  dlls/d3d10/tests/effect.c | 114 ++++++++
>>  2 files changed, 683 insertions(+), 8 deletions(-)
>>
>> diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c
>> index 78a3d0c386a..1be16771921 100644
>> --- a/dlls/d3d10/effect.c
>> +++ b/dlls/d3d10/effect.c
>> +static float * d3d10_effect_preshader_get_reg_ptr(const struct d3d10_effect_preshader *p,
>> +        enum d3d10_reg_table_type regt, unsigned int offset)
>> +{
>> +    switch (regt)
>> +    {
>> +        case D3D10_REG_TABLE_CONSTANTS:
>> +        case D3D10_REG_TABLE_CB:
>> +        case D3D10_REG_TABLE_RESULT:
>> +        case D3D10_REG_TABLE_TEMP:
>> +            return p->reg_tables[regt].f + offset;
>> +        default:
>> +            return NULL;
>> +    }
>> +}
> I think we want to validate the offset (for the _CONSTANTS table in
> particular, since the other 3 are sized to fit all the accesses). Not
> sure this is the best place for that but, if so, it might look
> something like:
>
>          case D3D10_REG_TABLE_CB:
>          case D3D10_REG_TABLE_RESULT:
>          case D3D10_REG_TABLE_TEMP:
> +            if (offset / sizeof(*p->reg_tables[regt].f) >=
> p->reg_tables[regt].count
> +                    || (offset + sizeof(*p->reg_tables[regt].f)) /
> sizeof(*p->reg_tables[regt].f)
> +                    > p->reg_tables[regt].count)
> +            {
> +                WARN("Invalid table offset %u.\n", offset);
> +                offset = 0;
> +            }
>              return p->reg_tables[regt].f + offset;
>          default:
> +            WARN("Unexpected register type %#x.\n", regt);
>              return NULL;
>      }
>  }
>
> (I expect it to get wrapped badly but hopefully it'll still be readable)

Yes, this won't hurt, and it affects CB as well I think. Another option
is track accesses to all tables, and validate sizes once.
I'll send another iteration with that, let me know if that's acceptable.

>
>> +
>> +static HRESULT d3d10_effect_preshader_eval(struct d3d10_effect_preshader *p)
>> +{
>> +    unsigned int i, j, regt, offset, instr_count, input_count;
>> +    const DWORD *ip = ID3D10Blob_GetBufferPointer(p->code);
>> +    float *dst, *args[4], *retval;
>> +    struct preshader_instr ins;
>> +
>> +    dst = d3d10_effect_preshader_get_reg_ptr(p, D3D10_REG_TABLE_RESULT, 0);
>> +    memset(dst, 0, sizeof(float) * p->reg_tables[D3D10_REG_TABLE_RESULT].count);
>> +
>> +    /* Update constant buffer */
>> +    dst = d3d10_effect_preshader_get_reg_ptr(p, D3D10_REG_TABLE_CB, 0);
>> +    for (i = 0; i < p->vars_count; ++i)
>> +    {
>> +        struct d3d10_ctab_var *v = &p->vars[i];
>> +        memcpy(dst + v->offset, v->v->buffer->u.buffer.local_buffer, v->length * sizeof(*dst));
> This is the one I mentioned at the top. Does this need to take
> v->v->buffer_offset into account for the memcpy() source argument?

Yes, it's definitely wrong. I'll add a test for this.