Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Propagate error instructions instead of making them srcs.

Closed Francisco Casas requested to merge fcasas/vkd3d:fix1 into master
1 unresolved thread

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.

Edited by Francisco Casas

Merge request reports

Closed by Francisco CasasFrancisco Casas 1 week ago (Apr 10, 2025 11:55pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Francisco Casas changed the description

    changed the description

  • Francisco Casas added 3 commits

    added 3 commits

    • fea00af7 - vkd3d-shader/hlsl: Avoid creating srcs to error instructions on attributes.
    • 831a11f0 - vkd3d-shader/hlsl: Skip adding state block entry on error.
    • 5c2c1f81 - vkd3d-shader/hlsl: Avoid creating srcs to error instructions on hlsl_ir_compile.

    Compare with previous version

  • Author Developer

    ↑ tiny improvement to 2/4: I realized instr_array_has_errors() can be used on yet another place.

    • 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.

    • Author Developer

      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:

      1. I think that ctx->error_instr is the only instruction in ctx->static_initializers that can be referenced from another instruction that is not contained in ctx->static_initializers. We could add a special flag to just ctx->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.
      2. 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.
      3. We can aim to always propagate ctx->error_instr, even on memory allocation failures, and get rid of all YYABORTs.
      4. Make sure that ctx->error_instr is never used as src, i.e. this MR.
    • 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:

      1. I think that ctx->error_instr is the only instruction in ctx->static_initializers that can be referenced from another instruction that is not contained in ctx->static_initializers. We could add a special flag to just ctx->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.
      2. 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.
      3. We can aim to always propagate ctx->error_instr, even on memory allocation failures, and get rid of all YYABORTs.
      4. 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.

    • Author Developer

      Thanks! I wrote !1460 (merged) as an alternative using the %destructor declarations.

    • Please register or sign in to reply
  • Francisco Casas mentioned in merge request !1460 (merged)

    mentioned in merge request !1460 (merged)

  • Author Developer

    Closing. Superseded by !1460 (merged).

Please register or sign in to reply
Loading