[PATCH v2 0/5] MR317: vkd3d-shader: Begin validating the generated IR.
The validation code is meant both as a check that the frontend is behaving properly and as a sort of the documentation to establish what is allowed and what is not in the IR. Currently an assertion is thrown if validation fails. I realize this is a rather strong proposal, but it's of course up for debate. In theory asserting here is the right thing, as it is expected that the frontend is generating correct IR code. However vkd3d is already used in production for many programs, and it could very well be that some of those are working properly even if the generated IR is somewhat out of specs; allowing the assertion might cause regressions as soon as enough checks are implemented in the validator. Please let me know your opinions. -- v2: vkd3d-shader: Validate source parameters. vkd3d-shader: Validate destination parameters. vkd3d-shader: Check that registers are valid. vkd3d-shader: Check that instructions are valid. vkd3d-shader: Introduce a boilerplate to validate the generated IR. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> For the moment the validator is trivial, it never fails. Checks will be added incrementally. --- libs/vkd3d-shader/ir.c | 24 ++++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_main.c | 12 ++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 2 ++ 3 files changed, 38 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 705905f7..bc205c5d 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1292,3 +1292,27 @@ enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, return result; } + +struct validation_context +{ + struct vkd3d_shader_parser *parser; + struct vkd3d_string_buffer messages; +}; + +void vkd3d_shader_validate_vsir(struct vkd3d_shader_parser *parser) +{ + struct validation_context ctx = { .parser = parser }; + + if (!TRACE_ON()) + return; + + vkd3d_string_buffer_init(&ctx.messages); + + if (ctx.messages.content_size) + { + ERR("Malformed instruction stream\n"); + vkd3d_string_buffer_trace(&ctx.messages); + } + + vkd3d_string_buffer_cleanup(&ctx.messages); +} diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 38f90966..cfc2e784 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -1218,6 +1218,8 @@ static int scan_dxbc(const struct vkd3d_shader_compile_info *compile_info, return ret; } + vkd3d_shader_validate_vsir(parser); + ret = scan_with_parser(compile_info, message_context, NULL, parser); vkd3d_shader_parser_destroy(parser); @@ -1236,6 +1238,8 @@ static int scan_d3dbc(const struct vkd3d_shader_compile_info *compile_info, return ret; } + vkd3d_shader_validate_vsir(parser); + ret = scan_with_parser(compile_info, message_context, NULL, parser); vkd3d_shader_parser_destroy(parser); @@ -1254,6 +1258,8 @@ static int scan_dxil(const struct vkd3d_shader_compile_info *compile_info, return ret; } + vkd3d_shader_validate_vsir(parser); + ret = scan_with_parser(compile_info, message_context, NULL, parser); vkd3d_shader_parser_destroy(parser); @@ -1370,6 +1376,8 @@ static int compile_dxbc_tpf(const struct vkd3d_shader_compile_info *compile_info return ret; } + vkd3d_shader_validate_vsir(parser); + ret = vkd3d_shader_parser_compile(parser, compile_info, out, message_context); vkd3d_shader_parser_destroy(parser); @@ -1403,6 +1411,8 @@ static int compile_d3d_bytecode(const struct vkd3d_shader_compile_info *compile_ return ret; } + vkd3d_shader_validate_vsir(parser); + vkd3d_shader_dump_shader(compile_info->source_type, parser->shader_version.type, &compile_info->source); if (compile_info->target_type == VKD3D_SHADER_TARGET_D3D_ASM) @@ -1427,6 +1437,8 @@ static int compile_dxbc_dxil(const struct vkd3d_shader_compile_info *compile_inf return ret; } + vkd3d_shader_validate_vsir(parser); + ret = vkd3d_shader_parser_compile(parser, compile_info, out, message_context); vkd3d_shader_parser_destroy(parser); diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 84614a4e..0bcc95a8 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -1281,6 +1281,8 @@ 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 vkd3d_shader_validate_vsir(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/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index bc205c5d..39b6ae60 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1299,15 +1299,35 @@ struct validation_context struct vkd3d_string_buffer messages; }; +static void vkd3d_shader_validate_instruction(struct validation_context *ctx, + size_t instruction_idx) +{ + const struct vkd3d_shader_instruction *instruction = &ctx->parser->instructions.elements[instruction_idx]; + + switch (instruction->handler_idx) + { + case VKD3DSIH_INVALID: + vkd3d_string_buffer_printf(&ctx->messages, "Instruction %zu: invalid handler.\n", instruction_idx); + break; + + default: + break; + } +} + void vkd3d_shader_validate_vsir(struct vkd3d_shader_parser *parser) { struct validation_context ctx = { .parser = parser }; + size_t i; if (!TRACE_ON()) return; vkd3d_string_buffer_init(&ctx.messages); + for (i = 0; i < parser->instructions.count; ++i) + vkd3d_shader_validate_instruction(&ctx, i); + if (ctx.messages.content_size) { ERR("Malformed instruction stream\n"); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 39b6ae60..7c1e9037 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1299,10 +1299,43 @@ struct validation_context struct vkd3d_string_buffer messages; }; +static void vkd3d_shader_validate_register(struct validation_context *ctx, + size_t instruction_idx, const struct vkd3d_shader_register *reg) +{ + switch (reg->type) + { + case VKD3DSPR_COUNT: + case VKD3DSPR_INVALID: + vkd3d_string_buffer_printf(&ctx->messages, "Instruction %zu: invalid register type.\n", instruction_idx); + + default: + break; + } +} + +static void vkd3d_shader_validate_dst_param(struct validation_context *ctx, + size_t instruction_idx, const struct vkd3d_shader_dst_param *dst) +{ + vkd3d_shader_validate_register(ctx, instruction_idx, &dst->reg); +} + +static void vkd3d_shader_validate_src_param(struct validation_context *ctx, + size_t instruction_idx, const struct vkd3d_shader_src_param *src) +{ + vkd3d_shader_validate_register(ctx, instruction_idx, &src->reg); +} + static void vkd3d_shader_validate_instruction(struct validation_context *ctx, size_t instruction_idx) { const struct vkd3d_shader_instruction *instruction = &ctx->parser->instructions.elements[instruction_idx]; + size_t i; + + for (i = 0; i < instruction->dst_count; ++i) + vkd3d_shader_validate_dst_param(ctx, instruction_idx, &instruction->dst[i]); + + for (i = 0; i < instruction->src_count; ++i) + vkd3d_shader_validate_src_param(ctx, instruction_idx, &instruction->src[i]); switch (instruction->handler_idx) { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/d3d_asm.c | 2 +- libs/vkd3d-shader/ir.c | 24 ++++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/d3d_asm.c b/libs/vkd3d-shader/d3d_asm.c index d72402eb..1dde34c0 100644 --- a/libs/vkd3d-shader/d3d_asm.c +++ b/libs/vkd3d-shader/d3d_asm.c @@ -1374,7 +1374,7 @@ static void shader_dump_ins_modifiers(struct vkd3d_d3d_asm_compiler *compiler, if (mmask & VKD3DSPDM_PARTIALPRECISION) shader_addline(buffer, "_pp"); if (mmask & VKD3DSPDM_MSAMPCENTROID) shader_addline(buffer, "_centroid"); - mmask &= ~(VKD3DSPDM_SATURATE | VKD3DSPDM_PARTIALPRECISION | VKD3DSPDM_MSAMPCENTROID); + mmask &= ~VKD3DSPDM_MASK; if (mmask) FIXME("Unrecognised modifier %#x.\n", mmask); } diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 7c1e9037..04c9f31b 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1317,6 +1317,30 @@ static void vkd3d_shader_validate_dst_param(struct validation_context *ctx, size_t instruction_idx, const struct vkd3d_shader_dst_param *dst) { vkd3d_shader_validate_register(ctx, instruction_idx, &dst->reg); + + if (dst->write_mask & ~VKD3DSP_WRITEMASK_ALL) + vkd3d_string_buffer_printf(&ctx->messages, "Instruction %zu: destination has invalid write mask %#x.\n", + instruction_idx, dst->write_mask); + + if (dst->modifiers & ~VKD3DSPDM_MASK) + vkd3d_string_buffer_printf(&ctx->messages, "Instruction %zu: destination has invalid modifiers %#x.\n", + instruction_idx, dst->modifiers); + + switch (dst->shift) + { + case 0: + case 1: + case 2: + case 3: + case 13: + case 14: + case 15: + break; + + default: + vkd3d_string_buffer_printf(&ctx->messages, "Instruction %zu: destination has invalid shift %#x.\n", + instruction_idx, dst->shift); + } } static void vkd3d_shader_validate_src_param(struct validation_context *ctx, diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 0bcc95a8..40d1de27 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -592,6 +592,7 @@ enum vkd3d_shader_dst_modifier VKD3DSPDM_SATURATE = 1, VKD3DSPDM_PARTIALPRECISION = 2, VKD3DSPDM_MSAMPCENTROID = 4, + VKD3DSPDM_MASK = 7, }; enum vkd3d_shader_interpolation_mode -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 8 ++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 9 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 04c9f31b..35e111e6 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1347,6 +1347,14 @@ static void vkd3d_shader_validate_src_param(struct validation_context *ctx, size_t instruction_idx, const struct vkd3d_shader_src_param *src) { vkd3d_shader_validate_register(ctx, instruction_idx, &src->reg); + + if (src->swizzle & ~0x03030303u) + vkd3d_string_buffer_printf(&ctx->messages, "Instruction %zu: source has invalid swizzle %#x.\n", + instruction_idx, src->swizzle); + + if (src->modifiers >= VKD3DSPSM_COUNT) + vkd3d_string_buffer_printf(&ctx->messages, "Instruction %zu: source has invalid modifier %#x.\n", + instruction_idx, src->modifiers); } static void vkd3d_shader_validate_instruction(struct validation_context *ctx, diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 40d1de27..629aea6f 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -578,6 +578,7 @@ enum vkd3d_shader_src_modifier VKD3DSPSM_ABS = 11, VKD3DSPSM_ABSNEG = 12, VKD3DSPSM_NOT = 13, + VKD3DSPSM_COUNT, }; #define VKD3DSP_WRITEMASK_0 0x1u /* .x r */ -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
I agree that `assert()` is not an appropriate way to deal with unexpected or untrusted input. My point is that that `assert()` should never trigger, even on unexpected or untrusted input, because I consider the frontend's job to deal with such input.
Well, yes, but that's what makes it unexpected, as opposed to just invalid.
In other words, my expectation from the frontend is that it's free to reject its input (and in that case no IR is generated nor validated), but if it accepts the input then it takes the responsibility to generate correct and valid IR; so the IR read by the validator is already considered trusted, and the `assert()` call is there just to, well, assert that. If we don't `assert()` we're likely in UB domain anyway, because the backend will assume that the IR is correct and count on it. If the IR is not valid vkd3d could crash anyway, and by that point I believe it's better to crash in a repeatable way with a clear error message rather then ending up tripping whatever UB will bring us any time later.
That's not that choice here though. I have no issue with aborting compilation when validation fails; the issue is with using assert() to do that, because it may either not trigger (i.e., in NDEBUG builds) or potentially escalate the consequences by calling abort() in an inappropriate context.
However, I don't mean to insist too much on this detail. I can remove the assertion and just rely on logging;
See above; I think we should (possibly optionally) fail compilation and return an error.
I can also gate the check with `TRACE_ON()`, which hopefully addresses your performances concern. I should mention that at some point I contemplated making this a `FIXME` rather than a `TRACE`, so that it is more conspicuous on logs posted by users, but that makes sense only if we accept the validator to be always run.
TRACE_ON is a bit inconvenient for one of the usages I had in mind. In particular, assume I have a collection of shaders collected using VKD3D_SHADER_DUMP_PATH, and I'd like to validate that these produce valid output, for example before approving certain MRs or making a release. Ideally I would be able to run vkd3d-compiler on each of these shaders, check its exit code and log its output, and get useful information out of that. This is an issue with the existing vkd3d_spirv_validate() as well, of course; in that particular case the existence of spirv-val helps. Using an environment variable would work; so would introducing a compilation option. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_44159
participants (3)
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)