vkd3d-shader/hlsl: Propagate error instructions instead of making them srcs.
Currently the ctx->error_instr hlsl_ir_node is used as block->value for the blocks that result from invalid code.
Since this instruction is unique, it is not added to the instruction list of the block itself. Also, when another instruction depends of an error value to be properly defined, e.g. an expression that contains an error as operand, it is considered an error value too instead of keeping the ctx->error_instr as an hlsl_src.
Currently, there are some places where it is still possible that instructions are created normally but with an hlsl_src pointing to the ctx->error_instr. This causes a failed assertion on hlsl_ctx_cleanup() in the example in 1/4 (simplified form of Wine Bug 57686).
These patches take care of propagating the error value (or discarding the instructions) on these cases so that we can assert that no hlsl_src is initialized to point to the error instruction.
OTOH if we want to allow hlsl_srcs to point to the error instruction in some cases, I can reduce this MR to just patch 1/4.
Merge request reports
Activity
The conceit of this series is that error_instr shouldn't be used as an hlsl_src. That's not correct, and if it's causing crashes, there's some reason for that that needs to be debugged. error_instr is literally the first instruction in ctx->static_initializers and so should be the last instruction to be deleted after all others, so it shouldn't have any uses at that time.
Also, when another instruction depends of an error value to be properly defined, e.g. an expression that contains an error as operand, it is considered an error value too instead of keeping the ctx->error_instr as an hlsl_src.
The reason to avoid creating a new instruction isn't to avoid using the error as a src; it's because in such places we can't meaningfully know the new instruction's type. (There may be other reasons to avoid creating an instruction that I'm forgetting, but that's the main one.)
Casts are an exception to this. If we have a statement like, say, "int x = (int2)bogus_identifier", I would argue that should generate an implicit truncation warning. The user clearly intended to type int2 there, or well, perhaps better to say that the mismatch is a separate and unrelated error to the use of a bad identifier and therefore we should report both.
error_instr is literally the first instruction in ctx->static_initializers and so should be the last instruction to be deleted after all others, so it shouldn't have any uses at that time.
The problem are the uses of YYABORT, consider my example again:
float4 main() : sv_target { // statement A int p = foo; // initializer argument is ERROR. // statement B undeclared_fun(); // triggers YYABORT. }
in this case the parser creates a block for statement A which contains a use of the
ctx->error_instr
, then while parsing statement B the YYABORT is hit, and at this point, unless I am missing something, there is no way to reach the block for statement A, it is just lost memory.Then, right after the YYABORT we call hlsl_ctx_cleanup() and we hit the assertion that
ctx->error_instr
has one use before being freed.Under the assumption that in the end we only want to call YYABORT when there is an allocation failure, I have some separate alternatives in mind:
- I think that
ctx->error_instr
is the only instruction inctx->static_initializers
that can be referenced from another instruction that is not contained inctx->static_initializers
. We could add a special flag to justctx->error_instr
that allows it to be freed even if it has some uses left and in case the ctx->result is VKD3D_ERROR_OUT_OF_MEMORY. Then remove all YYABORTS that are not a result of a memory allocation failure. - Skip hlsl_ctx_cleanup() altogether in case of VKD3D_ERROR_OUT_OF_MEMORY since we might have leaks anyways in that case. Then remove all YYABORTS that are not a result of a memory allocation failure.
- We can aim to always propagate ctx->error_instr, even on memory allocation failures, and get rid of all YYABORTs.
- Make sure that
ctx->error_instr
is never used as src, i.e. this MR.
- I think that
in this case the parser creates a block for statement A which contains a use of the
ctx->error_instr
, then while parsing statement B the YYABORT is hit, and at this point, unless I am missing something, there is no way to reach the block for statement A, it is just lost memory.Then, right after the YYABORT we call hlsl_ctx_cleanup() and we hit the assertion that
ctx->error_instr
has one use before being freed.Ah, of course.
Under the assumption that in the end we only want to call YYABORT when there is an allocation failure, I have some separate alternatives in mind:
- I think that
ctx->error_instr
is the only instruction inctx->static_initializers
that can be referenced from another instruction that is not contained inctx->static_initializers
. We could add a special flag to justctx->error_instr
that allows it to be freed even if it has some uses left and in case the ctx->result is VKD3D_ERROR_OUT_OF_MEMORY. Then remove all YYABORTS that are not a result of a memory allocation failure. - Skip hlsl_ctx_cleanup() altogether in case of VKD3D_ERROR_OUT_OF_MEMORY since we might have leaks anyways in that case. Then remove all YYABORTS that are not a result of a memory allocation failure.
- We can aim to always propagate ctx->error_instr, even on memory allocation failures, and get rid of all YYABORTs.
- Make sure that
ctx->error_instr
is never used as src, i.e. this MR.
No, none of those. The right solution is we just shouldn't leak. Bison actually makes this easy, using %destructor. I'm not sure whether or not we need to get rid of the cleanups in error paths in order to use it.
Of course, at the same time, we should indeed reduce YYABORT as much as possible. I don't think we can get rid of every usage, though—not all paths return an instruction.
- I think that
Thanks! I wrote !1460 (merged) as an alternative using the
%destructor
declarations.
mentioned in merge request !1460 (merged)
Closing. Superseded by !1460 (merged).