[PATCH 0/5] MR550: vkd3d-shader/ir: Validate SSA and TEMP usages.
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index bac426919..887782f5a 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1593,26 +1593,32 @@ static void vsir_validate_register(struct validation_context *ctx, { case VKD3DSPR_TEMP: if (reg->idx_count != 1) + { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX_COUNT, "Invalid index count %u for a TEMP register.", reg->idx_count); + break; + } - if (reg->idx_count >= 1 && reg->idx[0].rel_addr) + if (reg->idx[0].rel_addr) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "Non-NULL relative address for a TEMP register."); - if (reg->idx_count >= 1 && reg->idx[0].offset >= temp_count) + if (reg->idx[0].offset >= temp_count) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "TEMP register index %u exceeds the maximum count %u.", reg->idx[0].offset, temp_count); break; case VKD3DSPR_SSA: if (reg->idx_count != 1) + { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX_COUNT, "Invalid index count %u for a SSA register.", reg->idx_count); + break; + } - if (reg->idx_count >= 1 && reg->idx[0].rel_addr) + if (reg->idx[0].rel_addr) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "Non-NULL relative address for a SSA register."); - if (reg->idx_count >= 1 && reg->idx[0].offset >= ctx->parser->shader_desc.ssa_count) + if (reg->idx[0].offset >= ctx->parser->shader_desc.ssa_count) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "SSA register index %u exceeds the maximum count %u.", reg->idx[0].offset, ctx->parser->shader_desc.ssa_count); break; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/550
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 887782f5a..d03e48427 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1900,5 +1900,5 @@ void vsir_validate(struct vkd3d_shader_parser *parser) if (ctx.depth != 0) validator_error(&ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING, "%zu nested blocks were not closed.", ctx.depth); - free(ctx.blocks); + vkd3d_free(ctx.blocks); } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/550
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/d3dbc.c | 11 +++-- libs/vkd3d-shader/dxil.c | 2 +- libs/vkd3d-shader/ir.c | 56 ++++++++++++++++++++++-- libs/vkd3d-shader/vkd3d_shader_private.h | 2 +- 4 files changed, 62 insertions(+), 9 deletions(-) diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 8fec1e637..aa0dd8f4b 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -1360,18 +1360,21 @@ int vkd3d_shader_sm1_parser_create(const struct vkd3d_shader_compile_info *compi sm1->p.shader_desc.flat_constant_count[i].external = get_external_constant_count(sm1, i); if (!sm1->p.failed) - vsir_validate(&sm1->p); + ret = vsir_validate(&sm1->p); - if (sm1->p.failed) + if (sm1->p.failed && ret >= 0) + ret = VKD3D_ERROR_INVALID_SHADER; + + if (ret < 0) { WARN("Failed to parse shader.\n"); shader_sm1_destroy(&sm1->p); - return VKD3D_ERROR_INVALID_SHADER; + return ret; } *parser = &sm1->p; - return VKD3D_OK; + return ret; } bool hlsl_sm1_register_from_semantic(struct hlsl_ctx *ctx, const struct hlsl_semantic *semantic, diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c index c744dfced..1977b96c4 100644 --- a/libs/vkd3d-shader/dxil.c +++ b/libs/vkd3d-shader/dxil.c @@ -6282,7 +6282,7 @@ int vkd3d_shader_sm6_parser_create(const struct vkd3d_shader_compile_info *compi vkd3d_free(byte_code); if (!sm6->p.failed && ret >= 0) - vsir_validate(&sm6->p); + ret = vsir_validate(&sm6->p); if (sm6->p.failed && ret >= 0) ret = VKD3D_ERROR_INVALID_SHADER; diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index d03e48427..18fb1f809 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1511,7 +1511,7 @@ enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, vkd3d_shader_trace(instructions, &parser->shader_version); if (result >= 0 && !parser->failed) - vsir_validate(parser); + result = vsir_validate(parser); if (result >= 0 && parser->failed) result = VKD3D_ERROR_INVALID_SHADER; @@ -1527,6 +1527,12 @@ struct validation_context unsigned int temp_count; enum vkd3d_shader_opcode phase; + struct validation_context_temp_data + { + enum vsir_dimension dimension; + size_t first_seen; + } *temps; + enum vkd3d_shader_opcode *blocks; size_t depth; size_t blocks_capacity; @@ -1592,6 +1598,9 @@ static void vsir_validate_register(struct validation_context *ctx, switch (reg->type) { case VKD3DSPR_TEMP: + { + struct validation_context_temp_data *data; + if (reg->idx_count != 1) { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX_COUNT, "Invalid index count %u for a TEMP register.", @@ -1603,9 +1612,38 @@ static void vsir_validate_register(struct validation_context *ctx, validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "Non-NULL relative address for a TEMP register."); if (reg->idx[0].offset >= temp_count) + { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "TEMP register index %u exceeds the maximum count %u.", reg->idx[0].offset, temp_count); + break; + } + + /* parser->shader_desc.temp_count might be smaller then + * temp_count if the parser made a mistake; we still don't + * want to overflow the array. */ + if (reg->idx[0].offset >= ctx->parser->shader_desc.temp_count) + break; + data = &ctx->temps[reg->idx[0].offset]; + + if (reg->dimension == VSIR_DIMENSION_NONE) + { + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_DIMENSION, "Invalid dimension NONE for a TEMP register."); + break; + } + + if (data->dimension == VSIR_DIMENSION_NONE) + { + data->dimension = reg->dimension; + data->first_seen = ctx->instruction_idx; + } + else if (data->dimension != reg->dimension) + { + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_DIMENSION, "Invalid dimension %#x for a TEMP register: " + "it has already been seen with dimension %#x at instruction %zu.", + reg->dimension, data->dimension, data->first_seen); + } break; + } case VKD3DSPR_SSA: if (reg->idx_count != 1) @@ -1883,7 +1921,7 @@ static void vsir_validate_instruction(struct validation_context *ctx) } } -void vsir_validate(struct vkd3d_shader_parser *parser) +enum vkd3d_result vsir_validate(struct vkd3d_shader_parser *parser) { struct validation_context ctx = { @@ -1892,7 +1930,10 @@ void vsir_validate(struct vkd3d_shader_parser *parser) }; if (!(parser->config_flags & VKD3D_SHADER_CONFIG_FLAG_FORCE_VALIDATION)) - return; + return VKD3D_OK; + + if (!(ctx.temps = vkd3d_calloc(parser->shader_desc.temp_count, sizeof(*ctx.temps)))) + goto fail; for (ctx.instruction_idx = 0; ctx.instruction_idx < parser->instructions.count; ++ctx.instruction_idx) vsir_validate_instruction(&ctx); @@ -1901,4 +1942,13 @@ void vsir_validate(struct vkd3d_shader_parser *parser) validator_error(&ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING, "%zu nested blocks were not closed.", ctx.depth); vkd3d_free(ctx.blocks); + vkd3d_free(ctx.temps); + + return VKD3D_OK; + +fail: + vkd3d_free(ctx.blocks); + vkd3d_free(ctx.temps); + + return VKD3D_ERROR_OUT_OF_MEMORY; } diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 199a47a76..8ef4f3302 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -1448,7 +1448,7 @@ int preproc_lexer_parse(const struct vkd3d_shader_compile_info *compile_info, int hlsl_compile_shader(const struct vkd3d_shader_code *hlsl, const struct vkd3d_shader_compile_info *compile_info, struct vkd3d_shader_code *out, struct vkd3d_shader_message_context *message_context); -void vsir_validate(struct vkd3d_shader_parser *parser); +enum vkd3d_result vsir_validate(struct vkd3d_shader_parser *parser); static inline enum vkd3d_shader_component_type vkd3d_component_type_from_data_type( enum vkd3d_data_type data_type) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/550
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 18fb1f809..924910ea9 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1533,6 +1533,12 @@ struct validation_context size_t first_seen; } *temps; + struct validation_context_ssa_data + { + enum vsir_dimension dimension; + size_t first_seen; + } *ssas; + enum vkd3d_shader_opcode *blocks; size_t depth; size_t blocks_capacity; @@ -1646,6 +1652,9 @@ static void vsir_validate_register(struct validation_context *ctx, } case VKD3DSPR_SSA: + { + struct validation_context_ssa_data *data; + if (reg->idx_count != 1) { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX_COUNT, "Invalid index count %u for a SSA register.", @@ -1657,9 +1666,33 @@ static void vsir_validate_register(struct validation_context *ctx, validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "Non-NULL relative address for a SSA register."); if (reg->idx[0].offset >= ctx->parser->shader_desc.ssa_count) + { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "SSA register index %u exceeds the maximum count %u.", reg->idx[0].offset, ctx->parser->shader_desc.ssa_count); + break; + } + + data = &ctx->ssas[reg->idx[0].offset]; + + if (reg->dimension == VSIR_DIMENSION_NONE) + { + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_DIMENSION, "Invalid dimension NONE for a SSA register."); + break; + } + + if (data->dimension == VSIR_DIMENSION_NONE) + { + data->dimension = reg->dimension; + data->first_seen = ctx->instruction_idx; + } + else if (data->dimension != reg->dimension) + { + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_DIMENSION, "Invalid dimension %#x for a SSA register: " + "it has already been seen with dimension %#x at instruction %zu.", + reg->dimension, data->dimension, data->first_seen); + } break; + } case VKD3DSPR_NULL: if (reg->idx_count != 0) @@ -1935,6 +1968,9 @@ enum vkd3d_result vsir_validate(struct vkd3d_shader_parser *parser) if (!(ctx.temps = vkd3d_calloc(parser->shader_desc.temp_count, sizeof(*ctx.temps)))) goto fail; + if (!(ctx.ssas = vkd3d_calloc(parser->shader_desc.ssa_count, sizeof(*ctx.ssas)))) + goto fail; + for (ctx.instruction_idx = 0; ctx.instruction_idx < parser->instructions.count; ++ctx.instruction_idx) vsir_validate_instruction(&ctx); @@ -1943,12 +1979,14 @@ enum vkd3d_result vsir_validate(struct vkd3d_shader_parser *parser) vkd3d_free(ctx.blocks); vkd3d_free(ctx.temps); + vkd3d_free(ctx.ssas); return VKD3D_OK; fail: vkd3d_free(ctx.blocks); vkd3d_free(ctx.temps); + vkd3d_free(ctx.ssas); return VKD3D_ERROR_OUT_OF_MEMORY; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/550
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> Specifically, they are assigned only once and only assigned components are used. Right now we don't check that the assignment dominates all usages. --- libs/vkd3d-shader/ir.c | 55 +++++++++++++++++++++++- libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 924910ea9..706e09d06 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1523,6 +1523,7 @@ struct validation_context { struct vkd3d_shader_parser *parser; size_t instruction_idx; + bool invalid_instruction_idx; bool dcl_temps_found; unsigned int temp_count; enum vkd3d_shader_opcode phase; @@ -1537,6 +1538,9 @@ struct validation_context { enum vsir_dimension dimension; size_t first_seen; + uint32_t write_mask; + uint32_t read_mask; + size_t first_assigned; } *ssas; enum vkd3d_shader_opcode *blocks; @@ -1556,8 +1560,16 @@ static void VKD3D_PRINTF_FUNC(3, 4) validator_error(struct validation_context *c vkd3d_string_buffer_vprintf(&buf, format, args); va_end(args); - vkd3d_shader_parser_error(ctx->parser, error, "instruction %zu: %s", ctx->instruction_idx + 1, buf.buffer); - ERR("VSIR validation error: instruction %zu: %s\n", ctx->instruction_idx + 1, buf.buffer); + if (ctx->invalid_instruction_idx) + { + vkd3d_shader_parser_error(ctx->parser, error, "instruction %zu: %s", ctx->instruction_idx + 1, buf.buffer); + ERR("VSIR validation error: instruction %zu: %s\n", ctx->instruction_idx + 1, buf.buffer); + } + else + { + vkd3d_shader_parser_error(ctx->parser, error, "%s", buf.buffer); + ERR("VSIR validation error: %s\n", buf.buffer); + } vkd3d_string_buffer_cleanup(&buf); } @@ -1765,6 +1777,23 @@ static void vsir_validate_dst_param(struct validation_context *ctx, validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_SHIFT, "Destination has invalid shift %#x.", dst->shift); } + + if (dst->reg.type == VKD3DSPR_SSA && dst->reg.idx[0].offset < ctx->parser->shader_desc.ssa_count) + { + struct validation_context_ssa_data *data = &ctx->ssas[dst->reg.idx[0].offset]; + + if (data->write_mask == 0) + { + data->write_mask = dst->write_mask; + data->first_assigned = ctx->instruction_idx; + } + else + { + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_SSA_USAGE, + "SSA register is already assigned at instruction %zu.", + data->first_assigned); + } + } } static void vsir_validate_src_param(struct validation_context *ctx, @@ -1783,6 +1812,15 @@ static void vsir_validate_src_param(struct validation_context *ctx, if (src->modifiers >= VKD3DSPSM_COUNT) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_MODIFIERS, "Source has invalid modifiers %#x.", src->modifiers); + + if (src->reg.type == VKD3DSPR_SSA && src->reg.idx[0].offset < ctx->parser->shader_desc.ssa_count) + { + struct validation_context_ssa_data *data = &ctx->ssas[src->reg.idx[0].offset]; + unsigned int i; + + for (i = 0; i < VKD3D_VEC4_SIZE; ++i) + data->read_mask |= (1u << vsir_swizzle_get_component(src->swizzle, i)); + } } static void vsir_validate_dst_count(struct validation_context *ctx, @@ -1961,6 +1999,7 @@ enum vkd3d_result vsir_validate(struct vkd3d_shader_parser *parser) .parser = parser, .phase = VKD3DSIH_INVALID, }; + unsigned int i; if (!(parser->config_flags & VKD3D_SHADER_CONFIG_FLAG_FORCE_VALIDATION)) return VKD3D_OK; @@ -1974,9 +2013,21 @@ enum vkd3d_result vsir_validate(struct vkd3d_shader_parser *parser) for (ctx.instruction_idx = 0; ctx.instruction_idx < parser->instructions.count; ++ctx.instruction_idx) vsir_validate_instruction(&ctx); + ctx.invalid_instruction_idx = true; + if (ctx.depth != 0) validator_error(&ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING, "%zu nested blocks were not closed.", ctx.depth); + for (i = 0; i < parser->shader_desc.ssa_count; ++i) + { + struct validation_context_ssa_data *data = &ctx.ssas[i]; + + if ((data->write_mask | data->read_mask) != data->write_mask) + validator_error(&ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_SSA_USAGE, + "SSA register %u has invalid read mask %#x for write mask %#x.", + i, data->read_mask, data->write_mask); + } + vkd3d_free(ctx.blocks); vkd3d_free(ctx.temps); vkd3d_free(ctx.ssas); diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 8ef4f3302..4de2581ba 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -216,6 +216,7 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_VSIR_INVALID_DCL_TEMPS = 9014, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX = 9015, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING = 9016, + VKD3D_SHADER_ERROR_VSIR_INVALID_SSA_USAGE = 9017, VKD3D_SHADER_WARNING_VSIR_DYNAMIC_DESCRIPTOR_ARRAY = 9300, }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/550
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/550
``` Subject: [PATCH 3/5] vkd3d-shader/ir: Check that TEMP registers have consistent dimensions. ```
I'm not sure I completely understand what we're doing here. In principle VKD3DSPR_TEMP registers are vec4's and would get VSIR_DIMENSION_VEC4, at least before DXIL. Is the idea here that we want to allow (presumably mainly for DXIL) VSIR_DIMENSION_SCALAR temps, as long as we consistently access them that way? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/550#note_57088
I'm not sure I completely understand what we're doing here. In principle VKD3DSPR_TEMP registers are vec4's and would get VSIR_DIMENSION_VEC4, at least before DXIL. Is the idea here that we want to allow (presumably mainly for DXIL) VSIR_DIMENSION_SCALAR temps, as long as we consistently access them that way?
Yeah, that's the idea. Of course it's up for debate (maybe I should have made that explicit; in general, my validator MRs are meant to be proposals for specifying the VSIR syntax). At some point my idea was to mandate that TEMPs are always vec4 and SSAs are always scalar. But Conor's code already violates this, because some SSAs must be vec4 to allow assigning e.g. from sampling instructions; and as I already mentioned I plan to introduce a pass that converts SSAs to TEMPs, and while in principle I could make the TEMPs vec4 and use only `.x`, this feels a bit wasteful: it might be a better idea to explicitly signal to the backend that only the first component is to be used. I think the SPIR-V backend is already able to handle that gracefully. That's why I resolved in favour of allowing both SSAs and TEMPs to be both scalar and vec4, but it makes sense to require that they're consistent so that the semantic is clear. I realize that can be troublesome: for example there are cases in which Conor's code uses temporaries, I don't know exactly why; but I guess it can be an additional burden checking that temporaries are used consistently in different instances. However, I think that having a clear and sensible syntax for VSIR is more important. At any rate, as I said, that's a proposal, and discussion is welcome. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/550#note_57095
That probably makes sense. I imagine it may be helpful to have that clarification somewhere in the code as well though, for future reference. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/550#note_57234
participants (3)
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)