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. Solved in favor of a softer failure, and only when validation is enabled
Merge request reports
Activity
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.
-
- 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 useNDEBUG
(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 notNDEBUG
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.
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 thatassert()
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 theassert()
call is there just to, well, assert that. If we don'tassert()
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 aFIXME
rather than aTRACE
, 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.
added 5 commits
- 8556671a - vkd3d-shader: Introduce a boilerplate to validate the generated IR.
- a7099079 - vkd3d-shader: Check that instructions are valid.
- 6c97a1db - vkd3d-shader: Check that registers are valid.
- 99edd59e - vkd3d-shader: Validate destination parameters.
- bf390519 - vkd3d-shader: Validate source parameters.
Toggle commit listI agree that
assert()
is not an appropriate way to deal with unexpected or untrusted input. My point is that thatassert()
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'tassert()
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 aFIXME
rather than aTRACE
, 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.
added 24 commits
-
bf390519...f525e9e9 - 19 commits from branch
wine:master
- 4fb58107 - vkd3d-shader: Introduce a boilerplate to validate the generated IR.
- 0bab0827 - vkd3d-shader: Check that instructions are valid.
- 7489521d - vkd3d-shader: Check that registers are valid.
- d97aa7fe - vkd3d-shader: Validate destination parameters.
- d53ed4e4 - vkd3d-shader: Validate source parameters.
Toggle commit list-
bf390519...f525e9e9 - 19 commits from branch
added 9 commits
- c63675b2 - vkd3d-shader: Destroy the SM1 parser on parsing errors.
- 46682ae1 - vkd3d-shader: Destroy the SM4 parser on parsing errors.
- a982b195 - vkd3d-shader: Destroy the SM6 parser on parsing errors.
- 98a70189 - vkd3d-shader: Embed the parsing location in vkd3d_shader_instruction.
- a12d8a32 - vkd3d-shader: Introduce a boilerplate to validate the generated IR.
- 41152fa1 - vkd3d-shader: Validator instruction handlers.
- bc4a2c32 - vkd3d-shader: Validate register types.
- 4752884a - vkd3d-shader: Validate destination parameters.
- b0736b9e - vkd3d-shader: Validate source parameters.
Toggle commit listThe new revision should address all the concerns discussed above. It could very well introduce new concerns, though! :-) I rebased on top of current master so that I can also update the GitLab CI pipeline appropriately.
Also, I fixed a couple of related memory leaks which I discovered while working on this MR.
added 5 commits
Toggle commit listThis seems generally fine to me. Couple of points:
-void shader_instruction_init(struct vkd3d_shader_instruction *ins, enum vkd3d_shader_opcode handler_idx) +void shader_instruction_init(struct vkd3d_shader_instruction *ins, const struct vkd3d_shader_location *location, + enum vkd3d_shader_opcode handler_idx)
If we're touching it anyway, we might as well rename it to vsir_instruction_init(), IMO.
@@ -1062,7 +1068,10 @@ static void shader_instruction_normalise_io_params(struct vkd3d_shader_instructi } if (!keep) - shader_instruction_init(ins, VKD3DSIH_NOP); + { + location = ins->location; + shader_instruction_init(ins, &location, VKD3DSIH_NOP); + } } static enum vkd3d_result instruction_array_normalise_io_registers(struct vkd3d_shader_instruction_array *instructions,
This is not introduced by this series, so in that regard it's fine, but this looks odd. In particular:
- Why shader_instruction_init(..., VKD3DSIH_NOP) instead of vkd3d_shader_instruction_make_nop()?
- Why the "keep" variable if we could just nop out the instruction in the two places that might set it to false?
+void vkd3d_shader_validate_vsir(struct vkd3d_shader_parser *parser)
I.e., vsir_validate()? :)
+static void validator_error(struct validation_context *ctx, enum vkd3d_shader_error error, + const char *format, ...) VKD3D_PRINTF_FUNC(3, 4); + +static void validator_error(struct validation_context *ctx, enum vkd3d_shader_error error, + const char *format, ...) +{
I.e.,
static void VKD3D_PRINTF_FUNC(3, 4) validator_error(struct validation_context *ctx, enum vkd3d_shader_error error, const char *format, ...) { ...
right?
+ case VKD3DSIH_INVALID: + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_HANDLER, "Invalid handler.\n");
Note that vkd3d_shader_verror() already adds a newline, so validator_error() shouldn't need to add an extra one. That happens in a few other places in this series as well.
+static void vkd3d_shader_validate_register(struct validation_context *ctx, + const struct vkd3d_shader_register *reg) +{ + switch (reg->type) + { + case VKD3DSPR_COUNT: + case VKD3DSPR_INVALID: + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_REGISTER_TYPE, "Invalid register type.\n"); + break; + + default: + break; + } +}
Should we just check for "reg->type >= VKD3DSPR_COUNT" and print the actual value here?
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.
I feel like we've probably had this discussion before, but do you see any place for assert()? I see it as a way to communicate and/or double-check the internal assumptions the code is making; in that sense it makes sense that it's disabled on NDEBUG, and also that it aborts the program entirely, since it's only meant to be a developer tool. I get the impression you don't see that as sensible, though—and that any internal assumptions should either just not be written out, or should be validated always, even for "release" builds, and shouldn't crash the program?
added 11 commits
- 2ae9f824 - 1 earlier commit
- b0328e77 - vkd3d-shader/d3dbc: Ignore DCL source parameters.
- 3f0793bd - vkd3d-shader/d3dbc: Destroy the SM1 parser on parsing errors.
- 1599f288 - vkd3d-shader/tpf: Destroy the SM4 parser on parsing errors.
- d13f462a - vkd3d-shader/dxil: Destroy the SM6 parser on parsing errors.
- 53f383cc - vkd3d-shader: Embed the parsing location in vkd3d_shader_instruction.
- 38df2b43 - vkd3d-shader/ir: Introduce a boilerplate to validate the generated IR.
- 36e0d81e - vkd3d-shader/ir: Validate instruction handlers.
- e7064bf9 - vkd3d-shader/ir: Validate register types.
- 1487f74e - vkd3d-shader/ir: Validate destination parameters.
- e1ed3c1b - vkd3d-shader/ir: Validate source parameters.
Toggle commit listThe new revision should address the latest batch of comments.
Also, the validator found the first potential bug: in SM1, opcode DCL apparently expects two sources, but they were not being parsed (nor, evidently, used). The outcome was that the corresponding entries in
src
were not initialised, and had random values which triggered the validator. I am not sure what the correct fix for that it. For the moment I just pretend that DCL in VSIR doesn't have any source (I see the validator as an internal thing, so if those sources are eventually found out to mean something, we can change the convention and the validator).1033 1032 /* fall through */ 1034 1033 case VKD3DSIH_DCL_INPUT_PS: 1035 1034 case VKD3DSIH_DCL_OUTPUT: 1036 keep = shader_dst_param_io_normalise(&ins->declaration.dst, true, normaliser); 1035 if (!shader_dst_param_io_normalise(&ins->declaration.dst, true, normaliser)) 1036 vkd3d_shader_instruction_make_nop(ins); 1037 1037 break; 1038 1038 case VKD3DSIH_DCL_INPUT_SGV: 1039 1039 case VKD3DSIH_DCL_INPUT_SIV: 1040 1040 case VKD3DSIH_DCL_INPUT_PS_SGV: 1041 1041 case VKD3DSIH_DCL_INPUT_PS_SIV: 1042 1042 case VKD3DSIH_DCL_OUTPUT_SIV: 1043 keep = shader_dst_param_io_normalise(&ins->declaration.register_semantic.reg, true, 1044 normaliser); 1043 if (!shader_dst_param_io_normalise(&ins->declaration.register_semantic.reg, true, normaliser)) 1044 vkd3d_shader_instruction_make_nop(ins); I wish the documentation for VSIR was clearer on this (i.e., it existed, at least; which is part of why I began my effort to write the validator), but my understanding is that once the handler is NOP, no other field of the VSIR instruction should even be read. But the disassembler might be an exception and, even more in view of the validator, it's better to be precise. I'll change so that the whole instruction is reset to zero, except for the location.
313 315 reg->immconst_type = VKD3D_IMMCONST_SCALAR; 314 316 } 315 317 316 void shader_instruction_init(struct vkd3d_shader_instruction *ins, enum vkd3d_shader_opcode handler_idx) 318 void vsir_instruction_init(struct vkd3d_shader_instruction *ins, const struct vkd3d_shader_location *location, 233 234 234 235 if (ins->handler_idx == VKD3DSIH_RET) 235 236 { 237 normaliser->last_ret_location = ins->location; I don't understand ir.c very well yet. That said, I am not sure if flattener_eliminate_phase_related_dcls() is the right place to initialize
normaliser.last_ret_location
.Maybe initializing this value deserves a separated function, lest we forget that we are charging flattener_eliminate_phase_related_dcls() with this additional responsibility.
Also, it seems to me that it could be a local variable in instruction_array_flatten_hull_shader_phases() instead of a member of the normalizer, unless there is another place where it could be used later.
Edited by Francisco CasasI don't see this as very problematic:
flattener_eliminate_phase_related_dcls()
is just a helper forinstruction_array_flatten_hull_shader_phases()
, so both functions are basically in the same "domain of responsibility". Same thing forstruct hull_flattener
: it's just a helper structure to share variables between the main function and its helpers without passing a lot of arguments to each call.
added 13 commits
- e1ed3c1b...7127a195 - 3 earlier commits
- 1b00a3f4 - vkd3d-shader/d3dbc: Ignore DCL source parameters.
- 6281c789 - vkd3d-shader/d3dbc: Destroy the SM1 parser on parsing errors.
- 4751071d - vkd3d-shader/tpf: Destroy the SM4 parser on parsing errors.
- 55721aa7 - vkd3d-shader/dxil: Destroy the SM6 parser on parsing errors.
- 22bc85fb - vkd3d-shader: Embed the parsing location in vkd3d_shader_instruction.
- 445d161d - vkd3d-shader/ir: Introduce a boilerplate to validate the generated IR.
- aa5b475d - vkd3d-shader/ir: Validate instruction handlers.
- 0a89ece9 - vkd3d-shader/ir: Validate register types.
- 537d2e3e - vkd3d-shader/ir: Validate destination parameters.
- 0e97789a - vkd3d-shader/ir: Validate source parameters.
Toggle commit listI feel like we've probably had this discussion before, but do you see any place for assert()?
I think it's fine in programs like vkd3d-compiler, vkd3d-dxbc, or the demos. I generally don't think assert() is appropriate in libraries like libvkd3d or libvkd3d-shader, no.
shader_sm1_read_semantic(sm1, &p, &ins->declaration.semantic); + /* DCL has two source parameters, but they seem to be + * unused. We ignore them. */ + ins->src_count = 0;
They're not source parameters. These are the "usage_token" and "dst_token" read by shader_sm1_read_semantic(). src_count is set to 2 in order to make shader_sm1_skip_opcode() do the right thing for 1.x shaders, but arguably that function has outlived its usefulness.
I think setting src_count to 0 like this is fine if the comment is updated, but I wouldn't be opposed to instead setting src_count to 0 in the opcode info tables and adjusting sm1->ptr in either shader_sm1_skip_opcode() or shader_sm1_read_semantic() either.
I think setting src_count to 0 like this is fine if the comment is updated, but I wouldn't be opposed to instead setting src_count to 0 in the opcode info tables and adjusting sm1->ptr in either shader_sm1_skip_opcode() or shader_sm1_read_semantic() either.
I implemented skipping in
shader_sm1_skip_opcode()
.