Re: [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  wrote:
> > Signed-off-by: Nikolay Sivov
> >
> > 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.