Re: [v8 1/6] d3dx9: Implement preshader parsing.

[Matteo Bruni]

2016-03-29 17:22 GMT+02:00 Paul Gofman :
> Signed-off-by: Paul Gofman
> +enum pres_reg_tables
> +{
> - PRES_REGTAB_IMMED, /* immediate double constants from CLIT */

I don't quite recall, I might be contradicting my own earlier comments
here but at this point this comment doesn't seem to add much.

General (and late) note, we usually tend to be more verbose with
variable or type names and avoid too cryptic abbreviations. I don't
think it's worth changing everything now though.
> +struct d3dx_preshader
> +{
> - struct d3dx_regstore regs;
> -
> - unsigned int ins_count;
> - struct d3dx_pres_ins *ins;
> -
> - struct d3dx_const_tab inputs;
>
> +};
> -
> -
>
> +struct d3dx_param_eval

That double empty line seems unnecessary.
> -HRESULT d3dx_create_param_eval(struct d3dx9_base_effect *base_effect, void *byte_code, unsigned int byte_code_size,
> +enum pres_ops
> +{
> - PRESHADER_OP_NOP,
> - PRESHADER_OP_MOV,
> - PRESHADER_OP_MAX_ENUM
>
> +};

Maybe rename _MAX_ENUM to _UNKNOWN? At least for this patch series you
don't really need that enum value though (see below).
> +static HRESULT regstore_alloc_table(struct d3dx_regstore *rs, unsigned int table)
> +{
> - unsigned int size;
> -
> - size = rs->table_sizes[table] * table_info[table].reg_component_count * table_info[table].component_size;
> - if (size)
> - {
> -
>    rs->tables[table] = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, size);
> -
>    rs->table_value_set[table] = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> -
>                sizeof(*rs->table_value_set[table]) *
> -
>                ((rs->table_sizes[table] + PRES_BITMASK_BLOCK_SIZE - 1) / PRES_BITMASK_BLOCK_SIZE));

Indentation.
> +static void regstore_set_values(struct d3dx_regstore *rs, unsigned int table, void *data,
> -
>    unsigned int start_offset, unsigned int count)
>
> +{
> - unsigned int block_idx, start, end, start_block, end_block;
> -
> - if (!count)
> -
>    return;
> -
> - memcpy((BYTE *)rs->tables[table] + start_offset * table_info[table].component_size,
> -
>    data, count * table_info[table].component_size);

Indentation.
> - for (i = 0; i < PRESHADER_OP_MAX_ENUM; ++i)
> -
>    if (ins_code == pres_op_info[i].opcode)
> -
>        break;
> - if (i == PRESHADER_OP_MAX_ENUM)
> - {
> -
>    FIXME("Unknown opcode %#x, raw %#x.\n", ins_code, ins_raw);
> -
>    return NULL;
> - }

I'd use ARRAY_SIZE(pres_op_info) instead of PRESHADER_OP_MAX_ENUM.
> - cdesc = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*cdesc) * desc.Constants);
> - inputs_param = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*inputs_param) * desc.Constants);

AFAICS neither really needs HEAP_ZERO_MEMORY.
You should also check those allocations and bail out if either one fails.
> +static void update_table_sizes_consts(unsigned int *table_sizes, struct d3dx_const_tab *ctab)
> +{
> - unsigned int i, table, max_register;
> -
> - for (i = 0; i < ctab->input_count; ++i)
> - {
> -
>    max_register = ctab->inputs[i].RegisterIndex + ctab->inputs[i].RegisterCount;
> -
>    table = ctab->regset2table[ctab->inputs[i].RegisterSet];
> -
>    if (table < PRES_REGTAB_COUNT)
> -
>        table_sizes[table] = max(table_sizes[table], max_register);
> - }
>
> +}
> -
>
> +static void update_table_size(unsigned int *table_sizes, unsigned int table, unsigned int offset)
> +{
> - table_sizes[table] = max(table_sizes[table], get_reg_offset(table, offset) + 1);
>
> +}

Why not use it also in update_table_sizes_consts()? The "table <
PRES_REGTAB_COUNT" check should then go into the update_table_size()
helper.
> -
>    if (const_count > UINT_MAX / sizeof(double))
> -
>    {
> -
>        WARN("Invalid immediate constants count %u.\n", const_count);
> -
>        return NULL;
> -
>    }
> -
>    if (p - ptr + const_count * (sizeof(double) / sizeof(unsigned int)) >= count)
> -
>    {
> -
>        WARN("Byte code buffer ends unexpectedly.\n");
> -
>        return NULL;
> -
>    }

The first check seems unnecessary, if the idea was to avoid integer
overflows in the second check you can rewrite that one instead (e.g.
by dividing by sizeof(unsigned int) both sides of the inequality).
That second check can probably be improved by verifying that the
constants fit into the CLIT comment instead (see below).
BTW, the function return type is HRESULT, those returns generate
warnings for me.
> - pres->ins = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*pres->ins) * pres->ins_count);

You should probably check this allocation too.
> - for (i = 0; i < pres->ins_count; ++i)
> - {
> -
>    unsigned int *ptr_next;
> -
> -
>    ptr_next = parse_pres_ins(p, count, &pres->ins[i]);
> -
>    if (!ptr_next)
> -
>        return D3DXERR_INVALIDDATA;
> -
>    count -= ptr_next - p;
> -
>    p = ptr_next;
> - }
> -
> - if (!count)
> - {
> -
>    WARN("Byte code buffer ends unexpectedly.\n");
> -
>    return D3DXERR_INVALIDDATA;
> - }

These kinds of size checks probably work but they are somewhat hard to
follow. You could instead check the size of each comment section
(making sure that it doesn't overflow the whole bytecode buffer) and
then check that you don't go outside of each comment while parsing it.
find_bytecode_comment() could do the first check before returning and,
for example, also return the comment size so that the caller can use
it to validate the relevant accesses.
Hopefully what I mean is clear enough, otherwise as usual feel free to ask.
> +err_out:
> - FIXME("Error creating parameter evaluator.\n");
> - d3dx_free_param_eval(peval);
> *peval_out = NULL;
> - return E_NOTIMPL;
> - return;
>
> +}

That return is now unnecessary.