[PATCH 2/3] d3dx9: Support relative addressing in preshader.

[Matteo Bruni]

2017-03-23 16:06 GMT+01:00 Paul Gofman <gofmanp at gmail.com>:
> Signed-off-by: Paul Gofman <gofmanp at gmail.com>
> ---
>  dlls/d3dx9_36/preshader.c    | 103 ++++++++++++++++++++++++++++++++++++-------
>  dlls/d3dx9_36/tests/effect.c |  38 ++++++++--------
>  2 files changed, 106 insertions(+), 35 deletions(-)
>
> diff --git a/dlls/d3dx9_36/preshader.c b/dlls/d3dx9_36/preshader.c
> index 9aedc99..2974ffd 100644
> --- a/dlls/d3dx9_36/preshader.c
> +++ b/dlls/d3dx9_36/preshader.c
> @@ -191,6 +191,8 @@ struct d3dx_pres_operand
>      /* offset is component index, not register index, e. g.
>         offset for component c3.y is 13 (3 * 4 + 1) */
>      unsigned int offset;
> +    enum pres_reg_tables base_reg_table;
> +    unsigned int base_reg_offset;
>  };

It would be nicer to create a struct for storing together table and
offset and to pass it around to the various functions taking both
arguments, like parse_pres_reg().
It should probably be a separate patch before this one.

> +        ptr = parse_pres_reg(ptr + 1, &opr->base_reg_table, &opr->base_reg_offset);
> +    }
> +    else
> +    {
> +        opr->base_reg_table = PRES_REGTAB_COUNT;
> +        ++ptr;
>      }
> -    opr->table = reg_table[*ptr++];
> -    opr->offset = *ptr++;
> +
> +    ptr = parse_pres_reg(ptr, &opr->table, &opr->offset);

This would crash if the previous parse_pres_reg() call returned NULL.

> +    size = rs->table_sizes[table] * table_info[table].reg_component_count;
> +    if (offset >= size)
> +    {
> +        if ((offset & 255) < size)
> +            offset &= 255;
> +        else
> +            offset = offset % size;
> +    }

Is this just to try and avoid the modulo operation in some cases or is
there a deeper reason?
If the former, you could extend that to all the power-of-two sizes by
first checking if that's the case (!(size & size - 1)) and, if so,
and-ing offset with the mask (size - 1). That's assuming the compiler
doesn't do the same optimization already.

Let me know if there is some other reason instead.

>  static double exec_get_arg(struct d3dx_regstore *rs, const struct d3dx_pres_operand *opr, unsigned int comp)
>  {
> -    if (!regstore_is_val_set_reg(rs, opr->table, (opr->offset + comp) / table_info[opr->table].reg_component_count))
> -        WARN("Using uninitialized input, table %u, offset %u.\n", opr->table, opr->offset + comp);
> +    unsigned int base_offset, index;
> +
> +    if (opr->base_reg_table == PRES_REGTAB_COUNT)
> +        index = 0;
> +    else
> +        index = (int)exec_get_reg_value(rs, opr->base_reg_table, opr->base_reg_offset);

I think it's better with lrint() in place of the cast.

> +    if (index >= rs->table_sizes[opr->table] && opr->table == PRES_REGTAB_CONST)
> +    {
> +        unsigned int size_pow2;
> +
> +        for (size_pow2 = 1; size_pow2 < rs->table_sizes[opr->table]; size_pow2 <<= 1)
> +            ;
> +        index &= (size_pow2 - 1);
> +        if (index >= rs->table_sizes[opr->table])
> +            return 0.0;
> +    }

I don't think we necessarily have to follow native's madness on buffer
overflows to the letter. Since you did, that's certainly fine though.