[PATCH 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. -- 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 | 23 +++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_main.c | 12 ++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 2 ++ 3 files changed, 37 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 705905f7..2a334db5 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1292,3 +1292,26 @@ 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 }; + + vkd3d_string_buffer_init(&ctx.messages); + + if (ctx.messages.content_size) + { + ERR("Malformed instruction stream\n"); + vkd3d_string_buffer_trace(&ctx.messages); + } + + assert(!ctx.messages.content_size); + + 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 2a334db5..4debbbdf 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1299,12 +1299,32 @@ 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; 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 4debbbdf..cbf64b51 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 cbf64b51..58d09893 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 58d09893..cfcd753a 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
Couple of thoughts: - How do we intend to position this? Is this intended as a debug tool that someone could enable with e.g. VKD3D_CONFIG? Or should it be a compilation option or even a separate API? How do we want this to relate to e.g. IDirect3DShaderValidator9? This has consequences for some of the other choices we might make, of course. - In general, assert() seems like a mistake to me. In NDEBUG builds it does nothing, while on the other hand aborting usually isn't the most useful thing to do either. We could get a backtrace, but that'll just point to the validator instead of the place that introduced the issue. - This ties a bit into the question of positioning, but note that the compile functions generally (should) do a scan first. On the other hand, we might want to revalidate after e.g. vkd3d_shader_normalise() as well. - I think we want to use the existing location information in the validator instead of reinventing it, and we should probably just use the message context from struct vkd3d_shader_parser for outputting messages. - Do we want to validate against the limits of the device or shader model here? E.g., the number of available constant registers or input/output registers depends on the shader model. Some features like stencil export require extension when targetting e.g. SPIR-V. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_43889
* How do we intend to position this? Is this intended as a debug tool that someone could enable with e.g. VKD3D_CONFIG? Or should it be a compilation option or even a separate API? How do we want this to relate to e.g. IDirect3DShaderValidator9? This has consequences for some of the other choices we might make, of course.
My intention is to have this essentially as a debug and documentation tool. As the `assert()` implies, ideally in no case an IR error should ever be detected, because it is assumed that the frontend already generates correct code, or fails if some error in its own input is detected. The problem is that a number of different IR frontends and backends are either being developed or planned, and the IR itself is relatively complicated, which might bring us in a situation in which even for the developers it's not obvious which are the IR rules, i.e., when an IR program is syntactically correct or not. This can lead to subtle bugs if the frontend and backend used for a given translation disagree on what is allowed and what is not in the IR. So the first reason to have an IR validator is to bless it as a source of truth for what is correct IR and what is not, i.e., consider it the documentation for the IR, or at least for its syntax. The second reason is that you can actually run it on each shader translation. If it flags an error, it means that your quest to find a bug might be much shorter than otherwise, because you know that at least a mistake is in the frontend and you also have some coordinates to find it. Since under no case this validator should produce an error, there is little sense to expose it to the library client, with whatever API. If the validator flags something, it is a developer's job to pick it up, not a user's.
* In general, assert() seems like a mistake to me. In NDEBUG builds it does nothing, while on the other hand aborting usually isn't the most useful thing to do either. We could get a backtrace, but that'll just point to the validator instead of the place that introduced the issue.
I think we already discussed this in the past, but I disagree: I find that using `assert()` is useful to document the assumptions of some piece of code. If those assumptions end up being violated, there are two sensible behaviors: 1) do nothing, hoping for the best; or 2) make the world explode, so that a developer is more likely to check why something that should have never happened really happened. You sound like you're from camp 1, so you should probably just use `NDEBUG` (and notice that my code still gives some potentially useful information). But camp 2 makes sense to: it trades some short terms crashes for the ability to spot a bug before you have to spend a lot of time on debugging some less obvious symptoms. At any rate, defining or not `NDEBUG` makes it easy for everybody to choose and possibly change their own camp. As I already mentioned, I admit that this case is a bit more special, so I can see some more compelling reason than usual to remove the `assert()`.
* This ties a bit into the question of positioning, but note that the compile functions generally (should) do a scan first. On the other hand, we might want to revalidate after e.g. vkd3d_shader_normalise() as well.
Yeah, that makes sense.
* I think we want to use the existing location information in the validator instead of reinventing it, and we should probably just use the message context from struct vkd3d_shader_parser for outputting messages.
Right, I don't know that in detail, but I will study it.
* Do we want to validate against the limits of the device or shader model here? E.g., the number of available constant registers or input/output registers depends on the shader model. Some features like stencil export require extension when targetting e.g. SPIR-V.
Yeah, I would say that most of these checks sound sensible. The idea is that the backend should be able to rely on the validator to be sure of what its input IR looks like. So it makes sense to have more or less validation strictness depending on what the backend can handle. Also, it would make sense to have a stricter check after normalisation (i.e., check that the IR post-normalisation is indeed normalised). Hopefully all of this can be done is such a way to leave the validator as easy to read as possible, so that it's an effective documentation useful for the backend programmer. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_43903
My intention is to have this essentially as a debug and documentation tool. As the `assert()` implies, ideally in no case an IR error should ever be detected, because it is assumed that the frontend already generates correct code, or fails if some error in its own input is detected.
So we'd probably want to skip it when not developing/debugging then, right? That may also make it a bit more acceptable to do something drastic like calling abort() as the result of a validation failure, or doing potentially more expensive kinds of validation.
I think we already discussed this in the past, but I disagree: I find that using `assert()` is useful to document the assumptions of some piece of code. If those assumptions end up being violated, there are two sensible behaviors: 1) do nothing, hoping for the best; or 2) make the world explode, so that a developer is more likely to check why something that should have never happened really happened. You sound like you're from camp 1,
Not exactly, I'd like more options than those two. (In particular, I think "reject the specific input" is the right choice here. We can make it an ICE if you like.) Fundamentally though, my arguments are this: - assert() is not a robust way to abort on unexpected input, even if we would like to abort the process instead of just aborting compilation of that specific shader. Calling abort() would already be better in that regard, although I think it's rarely appropriate for libraries to call abort() as well. - We're a library, we might get invalid or malicious input, and we can't possibly oversee all the consequences of aborting the calling process. (And I'll point out that that's just as true for d3d11 or d3dcompiler as it is for vkd3d-shader.) And it shouldn't be terribly hard to think up scenario's where "abort the process on unexpected input" isn't the most appropriate course of action. E.g., imagine we're running some kind of shader translation web service. People submit their shaders to our web/application server, the server translates those shaders to the desired output format, and sends the results back. It might not be a terribly robust design to run shader translation in the context of the web/application server, but I don't think it's necessarily unreasonable, and who am I to judge. In this scenario, it would seem more appropriate to me to fail compilation when someone submits a shader that manages to trip our checks than to make the server explode. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_43956
On Thu Aug 31 12:50:17 2023 +0000, Henri Verbeet wrote: > > My intention is to have this essentially as a debug and documentation > tool. As the `assert()` implies, ideally in no case an IR error should > ever be detected, because it is assumed that the frontend already > generates correct code, or fails if some error in its own input is detected. > So we'd probably want to skip it when not developing/debugging then, > right? That may also make it a bit more acceptable to do something > drastic like calling abort() as the result of a validation failure, or > doing potentially more expensive kinds of validation. > > I think we already discussed this in the past, but I disagree: I find > that using `assert()` is useful to document the assumptions of some > piece of code. If those assumptions end up being violated, there are two > sensible behaviors: 1) do nothing, hoping for the best; or 2) make the > world explode, so that a developer is more likely to check why something > that should have never happened really happened. You sound like you're > from camp 1, > Not exactly, I'd like more options than those two. (In particular, I > think "reject the specific input" is the right choice here. We can make > it an ICE if you like.) Fundamentally though, my arguments are this: > - assert() is not a robust way to abort on unexpected input, even if > we would like to abort the process instead of just aborting compilation > of that specific shader. Calling abort() would already be better in that > regard, although I think it's rarely appropriate for libraries to call > abort() as well. > - We're a library, we might get invalid or malicious input, and we > can't possibly oversee all the consequences of aborting the calling > process. (And I'll point out that that's just as true for d3d11 or > d3dcompiler as it is for vkd3d-shader.) > And it shouldn't be terribly hard to think up scenario's where "abort > the process on unexpected input" isn't the most appropriate course of > action. E.g., imagine we're running some kind of shader translation web > service. People submit their shaders to our web/application server, the > server translates those shaders to the desired output format, and sends > the results back. It might not be a terribly robust design to run shader > translation in the context of the web/application server, but I don't > think it's necessarily unreasonable, and who am I to judge. In this > scenario, it would seem more appropriate to me to fail compilation when > someone submits a shader that manages to trip our checks than to make > the server explode. 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. 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. However, I don't mean to insist too much on this detail. I can remove the assertion and just rely on logging; 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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_43963
participants (3)
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)