2016-03-28 9:20 GMT+02:00 Paul Gofman gofmanp@gmail.com:
Signed-off-by: Paul Gofman gofmanp@gmail.com
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).
On 03/29/2016 02:16 AM, Matteo Bruni wrote:
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.
I intentionally added this TRACE with output pointer as otherwise I cannot find init trace related to particular preshader. E. g., if I have some weirdness or error message when param_eval is used much later (among a hundred of other param evals), I want to find the log records where this preshader was created which has disassembly and binary blob. Without this TRACE I have no sure way to match the used preshader with its parsing trace. So unless you are strictly opposed to this TRACE I would keep it to make future bug hunting easier.
The TRACE on error return a few lines below is a leftover.
2016-03-29 1:39 GMT+02:00 Paul Gofman gofmanp@gmail.com:
I intentionally added this TRACE with output pointer as otherwise Icannot find init trace related to particular preshader. E. g., if I have some weirdness or error message when param_eval is used much later (among a hundred of other param evals), I want to find the log records where this preshader was created which has disassembly and binary blob. Without this TRACE I have no sure way to match the used preshader with its parsing trace. So unless you are strictly opposed to this TRACE I would keep it to make future bug hunting easier.
I'm okay with printing the pointer or similar info but that kind of trace is pretty ugly. You could trace the pointer when you allocate the struct, or together with the struct state pointer / the state parameter in the caller. I guess you can also leave the trace there, if it says something like:
TRACE("Created parameter evaluator %p.\n", *peval_out);
BTW, notice that you are never checking the return value from d3dx_create_param_eval(). You should either do that or remove the return value from the function altogether.
On 03/29/2016 04:25 PM, Matteo Bruni wrote:
I'm okay with printing the pointer or similar info but that kind of trace is pretty ugly. You could trace the pointer when you allocate the struct, or together with the struct state pointer / the state parameter in the caller. I guess you can also leave the trace there, if it says something like:
TRACE("Created parameter evaluator %p.\n", *peval_out);
This is almost exactly what I am suggesting in the next e-mail I've just sent.
BTW, notice that you are never checking the return value from d3dx_create_param_eval(). You should either do that or remove the return value from the function altogether.
I removed all the return value checks from creating param evaluator to let the program continue and not to fail effect creation, exactly as it did without preshaders before. I have nothing to do with the return value at the creation right now. At the same time, having such a function with parsing and a lot of possible errors without return value would seem very strange. Maybe we could keep the return value in case we will need this in future? E. g. we may want to decide to fail shaders creation on preshader parsing failure.
2016-03-29 15:33 GMT+02:00 Paul Gofman gofmanp@gmail.com:
I removed all the return value checks from creating param evaluator to let the program continue and not to fail effect creation, exactly as it did without preshaders before. I have nothing to do with the return value at the creation right now. At the same time, having such a function with parsing and a lot of possible errors without return value would seem very strange. Maybe we could keep the return value in case we will need this in future? E. g. we may want to decide to fail shaders creation on preshader parsing failure.
Maybe we'll want to change that in the future but for now returning a value which isn't checked from an internal function doesn't seem very useful. I'd just drop the return value from d3dx_create_param_eval(), reintroducing that if and when necessary should be trivial.
I'm fine with the other things you mentioned. Hopefully the various additional length checks won't make the code significantly more convoluted (but if you judge they do and resend without them it's okay too).
On 03/29/2016 02:16 AM, Matteo Bruni wrote:
+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...
I will pass through bytecode size parameter and add a size check. There is D3DXGetShaderConstantTable function used in another function taking the same bytecode reference and doing comment search in a similar way, and it does not have size parameter, so it will crash anyway on insane bytecode. The same is for d3d shader create functions. But those functions do not modify the memory at bytecode, and segfault on read access cannot be used alone to inject anything. At the same time parse_preshader writes to that pointer a 0xfffe0000 constant (restoring a memory a bit later) to make it suitable D3DXGetShaderConstantTable, so actually adding these checks does not seem much paranoid.
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).
PRES_REGTAB_FIRST was used in tables free function. I now removed _FIRST/_LAST, used 0/_COUNT instead, and introduced PRES_REGTAB_FIRST_SHADER to use it in parse_param_eval instead of 'PRES_REGTAB_IMMED + 1'. Please note that I introduced PRES_REGTAB_FIRST/_LAST in v3 patch series based on comments on v1, and should issue a possible endless loop warning :). I suggest that if there is still any concern regarding this enum maybe we could confirm its final form at this point? The current enum after my last edits looks like this:
enum pres_reg_tables { PRES_REGTAB_IMMED, /* immediate double constants from CLIT */ PRES_REGTAB_CONST, PRES_REGTAB_OCONST, PRES_REGTAB_OBCONST, PRES_REGTAB_OICONST, PRES_REGTAB_TEMP, PRES_REGTAB_COUNT, PRES_REGTAB_FIRST_SHADER = PRES_REGTAB_CONST, };
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.
Maybe we can leave this in place? I changed this to TRACE("Created param_eval %p.\n", *peval_out). I saw similar traces many times in d3d code and they are extremely helpful when we need to find a creation log for object we see used somewhere.
-
Matteo Bruni -
Paul Gofman