On Mon, Nov 8, 2021 at 1:30 PM Nikolay Sivov nsivov@codeweavers.com wrote:
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com
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)
+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?
+static HRESULT parse_fx10_fxlc(void *ctx, const char *data, unsigned int data_size) +{
- struct d3d10_preshader_parse_context *context = ctx;
- struct d3d10_effect_preshader *p = context->preshader;
- unsigned int i, offset = 4;
- uint32_t ins_count;
- const char *ptr;
- HRESULT hr;
- if (data_size % sizeof(uint32_t))
- {
WARN("FXLC size misaligned %u.\n", data_size);return E_FAIL;- }
- /* Parse through bytecode copy, so we can patch opcodes. */
This comment is not actual anymore. I think we can just get rid of it.
+static HRESULT parse_fx10_ctab(void *ctx, const char *data, unsigned int data_size) +{
- struct d3d10_preshader_parse_context *context = ctx;
- struct d3d10_effect_preshader *p = context->preshader;
- struct ctab_header
- {
DWORD size;DWORD creator;DWORD version;DWORD constants;DWORD constantinfo;DWORD flags;DWORD target;- } header;
- struct ctab_const_info
- {
DWORD name;WORD register_set;WORD register_index;WORD register_count;WORD reserved;DWORD typeinfo;DWORD default_value;- } *info;
- unsigned int i, cb_reg_count = 0;
- const char *ptr = data;
- const char *name;
- size_t name_len;
- HRESULT hr;
- if (data_size < sizeof(header))
- {
WARN("Invalid constant table size %u.\n", data_size);return E_FAIL;- }
- read_dword(&ptr, &header.size);
- read_dword(&ptr, &header.creator);
- read_dword(&ptr, &header.version);
- read_dword(&ptr, &header.constants);
- read_dword(&ptr, &header.constantinfo);
- read_dword(&ptr, &header.flags);
- read_dword(&ptr, &header.target);
- if (!require_space(header.constantinfo, header.constants, sizeof(*info), data_size))
- {
WARN("Invalid constant info section offset %#x.\n", header.constantinfo);return E_FAIL;- }
- p->vars_count = header.constants;
- TRACE("Variable count %u.\n", p->vars_count);
- if (!(p->vars = heap_calloc(p->vars_count, sizeof(*p->vars))))
return E_OUTOFMEMORY;- /* Collect variables used in expression. */
That's somewhat specific to INDEX_EXPRESSION preshaders. I'd just leave out the comment, the code is pretty self-explanatory anyway.
- info = (struct ctab_const_info *)(data + header.constantinfo);
- for (i = 0; i < p->vars_count; ++i, ++info)
- {
if (!fx10_get_string(data, data_size, info->name, &name, &name_len))return E_FAIL;if (!(p->vars[i].v = d3d10_effect_get_variable_by_name(context->effect, name))){WARN("Couldn't find variable %s.\n", debugstr_a(name));return E_FAIL;}/* 4 components per register */p->vars[i].offset = info->register_index * 4;p->vars[i].length = info->register_count * 4;cb_reg_count = max(cb_reg_count, info->register_index + info->register_count);- }
- /* Allocate contiguous "constant buffer" for all referenced variables. */
- if (FAILED(hr = d3d10_reg_table_allocate(&p->reg_tables[D3D10_REG_TABLE_CB], cb_reg_count * 4)))
- {
WARN("Failed to allocate variables buffer.\n");return hr;- }
- return S_OK;
+}
@@ -2051,14 +2545,75 @@ static HRESULT parse_fx10_property_assignment(const char *data, size_t data_size dep.id = id; dep.idx = idx; dep.operation = operation;
dep.u.var.v = variable;dep.u.var.offset = offset;
dep.var.v = variable;dep.var.offset = offset; return d3d10_effect_add_prop_dependency(d, &dep); } break;case D3D10_EOO_INDEX_EXPRESSION:/* Variable, and an expression for its index. */if (value_offset >= data_size || !require_space(value_offset, 2, sizeof(DWORD), data_size)){WARN("Invalid offset %#x (data size %#lx).\n", value_offset, (long)data_size);return E_FAIL;}data_ptr = data + value_offset;read_dword(&data_ptr, &value_offset);read_dword(&data_ptr, &code_offset);if (!fx10_get_string(data, data_size, value_offset, &name, &name_len)){WARN("Failed to get variable name.\n");return E_FAIL;}TRACE("Variable name %s[<expr>].\n", debugstr_a(name));if (!(variable = d3d10_effect_get_variable_by_name(effect, name))){WARN("Couldn't find variable %s.\n", debugstr_a(name));return E_FAIL;}/* Has to be an array */if (!variable->type->element_count){WARN("Expected array variable.\n");return E_FAIL;}
This comment can also go IMO.
if (!is_object_property(property_info)){WARN("Expected object type property used with indexed expression.\n");return E_FAIL;}if (code_offset >= data_size || !require_space(code_offset, 1, sizeof(DWORD), data_size)){WARN("Invalid offset %#x (data size %#lx).\n", value_offset, (long)data_size);return E_FAIL;}data_ptr = data + code_offset;read_dword(&data_ptr, &blob_size);dep.id = id;dep.idx = idx;dep.operation = operation;dep.index_expr.v = variable;if (FAILED(hr = parse_fx10_preshader(data_ptr, blob_size, effect, &dep.index_expr.index))){WARN("Failed to parse preshader, hr %#x.\n", hr);return hr;}return d3d10_effect_add_prop_dependency(d, &dep);case D3D10_EOO_ANONYMOUS_SHADER: /* Anonymous shader */