Re: [v7 2/7] d3dx9: Implement preshader parsing.

[Matteo Bruni]

2016-03-28 9:20 GMT+02:00 Paul Gofman :
> Signed-off-by: Paul Gofman
>
> Changes
> - Changed opcode function to take double arguments and return double

Good catch.
> +static struct op_info pres_op_info[] =
> +{
> - {0x000, "nop", 0, 0, NULL    }, /* PRESHADER_OP_NOP */
> - {0x100, "mov", 1, 0, pres_mov}, /* PRESHADER_OP_MOV */
>
> +};

I think this can be const.
> +struct d3dx_pres_ins
> +{
> - enum pres_ops op;
> - /* first input argument is scalar,
> -
>   scalar component is propagated */
> - BOOL scalar_op;
> - unsigned int component_count;
> - struct d3dx_pres_operand inputs[3];

The maximum number of input operands could use a #define. Not a huge
deal though.
> +static void regstore_free_tables(struct d3dx_regstore *rs)
> +{
> - unsigned int i;
> -
> - for (i = PRES_REGTAB_FIRST; i <= PRES_REGTAB_LAST; ++i)
> - {
> -
>    if (rs->tables[i])
> -
>        HeapFree(GetProcessHeap(), 0, rs->tables[i]);
> -
>    if (rs->table_value_set[i])
> -
>        HeapFree(GetProcessHeap(), 0, rs->table_value_set[i]);
> - }
>
> +}

You don't need to check for non-NULL, HeapFree() handles NULL
arguments just fine.
> +static unsigned int *find_bytecode_comment(unsigned int *ptr, unsigned int fourcc)
> +{
> - while ((*ptr & 0xffff) == 0xfffe)
> - {
> -
>    if (*(ptr + 1) == fourcc)
> -
>        return ptr + 2;
> -
>    ptr += 1 + (*ptr >> 16);
> - }
> - return NULL;
>
> +}

This is probably paranoid but in theory a broken or malicious effect
could send you rummaging outside of the bytecode buffer so all the
offsets should be checked. I don't think it's critical in practice
i.e. I'll probably sleep just fine if you decide to ignore this
issue...
> +static unsigned int *parse_pres_arg(unsigned int *ptr, struct d3dx_pres_operand *opr)
> +{
> - static const enum pres_reg_tables reg_table[8] =
> - {
> -
>    PRES_REGTAB_COUNT, PRES_REGTAB_IMMED, PRES_REGTAB_CONST, PRES_REGTAB_COUNT,
> -
>    PRES_REGTAB_OCONST, PRES_REGTAB_OBCONST, PRES_REGTAB_OICONST, PRES_REGTAB_REG

Matter of taste, so feel free to ignore this, but PRES_REGTAB_REG
sounds a bit vague to me. I'd go with something like PRES_REGTAB_TEMP.
> - if (input_count != pres_op_info[i].input_count)
> - {
> -
>    FIXME("Actual input args %u, expected %u, instruction %s.\n", input_count,
> -
>            pres_op_info[i].input_count, pres_op_info[i].mnem);
> -
>    return NULL;
> - }
> - for (i = 0; i < input_count; ++i)
> - {
> -
>    ptr = parse_pres_arg(ptr, &ins->inputs[i]);
> -
>    if (!ptr)
> -
>        return NULL;
> - }

Another pretty paranoid potential check, here you could verify that
input_count is < ARRAY_SIZE(ins->inputs). Of course that could only
fail if we encounter some instruction with more than 3 components AND
we update pres_op_info BUT we forget to update struct d3dx_pres_ins,
but I don't think it would hurt to be safe...
> - for (i = 0; i < pres->ins_count; ++i)
> - {
> -
>    unsigned int table;
> -
> -
>    for (j = 0; j < pres_op_info[pres->ins[i].op].input_count; ++j)
> -
>    {
> -
>        table = pres->ins[i].inputs[j].table;
> -
>        max_offset = get_reg_offset(table, pres->ins[i].inputs[j].offset + pres->ins[i].component_count - 1);
> -
>        pres->regs.table_sizes[table] = max(pres->regs.table_sizes[table], max_offset + 1);
> -
>    }
> -
>    table = pres->ins[i].output.table;
> -
>    max_offset = get_reg_offset(table, pres->ins[i].output.offset + pres->ins[i].component_count - 1);
> -
>    pres->regs.table_sizes[table] = max(pres->regs.table_sizes[table], max_offset + 1);
> - }
> - update_table_sizes_consts(pres->regs.table_sizes, &pres->inputs);

It probably makes sense to add a small update_table_size() helper (or
similar), the "same" 3 lines are repeated in the loop for the input
operands, for the output operand and in update_table_sizes_consts().
> - for (i = PRES_REGTAB_IMMED + 1; i <= PRES_REGTAB_LAST; ++i)

It depends on what is coming up next but maybe it would make sense to
add an enum for "PRES_REGTAB_IMMED + 1" (incidentally,
PRES_REGTAB_FIRST is never used in this patch series).
> - {
> -
>    TRACE("table_sizes[%u] %u.\n", i, peval->pres.regs.table_sizes[i]);

That trace looks a bit like a debugging leftover.
> -
>    if (FAILED(regstore_alloc_table(&peval->pres.regs, i)))
> -
>    {
> -
>        hr = E_OUTOFMEMORY;
> -
>        goto err_out;
> -
>    }
> - }
> -
> - if (TRACE_ON(d3dx))
> -
>    dump_bytecode(byte_code, byte_code_size);
> -
> - *peval_out = peval;
> - TRACE("retval %u, *peval_out %p.\n", D3D_OK, *peval_out);

Same with this. This one in particular doesn't seem worth keeping.
> - return D3D_OK;
> -
>
> +err_out:
> - FIXME("Error creating parameter evaluator.\n");
> - d3dx_free_param_eval(peval);
> *peval_out = NULL;
> - return E_NOTIMPL;
> - TRACE("retval %u, *peval_out %p.\n", hr, *peval_out);
> - return hr;
>
> +}

You could print the HRESULT in the FIXME (e.g. FIXME("Error creating
parameter evaluator, returning %#x.\n", hr)). Tracing the value of
*peval_out doesn't seem particularly worthwhile.
> +static void d3dx_free_const_tab(struct d3dx_const_tab *ctab)
> +{
> - if (ctab->inputs)
> -
>    HeapFree(GetProcessHeap(), 0, ctab->inputs);
> - if (ctab->inputs_param)
> -
>    HeapFree(GetProcessHeap(), 0, ctab->inputs_param);
> - if (ctab->ctab)
> -
>    ID3DXConstantTable_Release(ctab->ctab);
>
> +}
> -
>
> +static void d3dx_free_preshader(struct d3dx_preshader *pres)
> +{
> - if (pres->ins)
> -
>    HeapFree(GetProcessHeap(), 0, pres->ins);
> -
> - regstore_free_tables(&pres->regs);
> - d3dx_free_const_tab(&pres->inputs);
>
> }

You can drop all those ifs (I think there are a few more of them in
the middle of the patch I haven't specifically mentioned).