Skip to content
Snippets Groups Projects

vkd3d-shader: Add more TPF validations.

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:validate_tpf into master
2 unresolved threads

Introduces an idx_count field to struct vkd3d_shader_register. Another patch set is needed for further validations and to use this field where applicable.

Edited by Conor McCarthy

Merge request reports

Merge request pipeline #10283 skipped

Merge request pipeline skipped for be4a71da

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (May 9, 2023 8:25pm 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
  • Conor McCarthy changed the description

    changed the description

  • diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c
    index 11ba7563..d0dc2978 100644
    --- a/libs/vkd3d-shader/ir.c
    +++ b/libs/vkd3d-shader/ir.c
    @@ -300,11 +300,7 @@ static bool shader_dst_param_normalise_outpointid(struct vkd3d_shader_dst_param
     
         if (normaliser_is_in_control_point_phase(normaliser) && reg->type == VKD3DSPR_OUTPUT)
         {
    -        if (reg->idx[2].offset != ~0u)
    -        {
    -            FIXME("Cannot insert phase id.\n");
    -            return false;
    -        }
    +        assert(reg->idx[2].offset == ~0u);
             if (reg->idx[1].offset != ~0u)
             {
                 WARN("Unexpected address at index 1.\n");

    We could as well compare reg->idx_count against ARRAY_SIZE(reg->idx) here. We should also increase reg->idx_count here because we're adding an extra index.

    This series will need to be adjusted for the signature changes in !197 (merged).

  • Conor McCarthy added 14 commits

    added 14 commits

    • 42ba1dc4...8b57a612 - 10 commits from branch wine:master
    • 9b9d9a22 - vkd3d-shader/tpf: Validate input/output register index counts.
    • f068d76f - vkd3d-shader/tpf: Validate signature element register indices.
    • cc7cc3a8 - vkd3d-shader/tpf: Validate signature element masks.
    • 10d4460c - vkd3d-shader/tpf: Validate input/output registers.

    Compare with previous version

  • Conor McCarthy added 5 commits

    added 5 commits

    • 655cd577 - vkd3d-shader/tpf: Validate input/output register index counts.
    • 6ce8687c - vkd3d-shader/tpf: Validate signature element register indices.
    • 94ca6552 - vkd3d-shader/tpf: Validate signature element masks.
    • 469de967 - vkd3d-shader/tpf: Validate input/output registers.
    • 0787554e - vkd3d-shader/tpf: Validate index range declarations.

    Compare with previous version

  • Conor McCarthy added 1 commit

    added 1 commit

    • 8b4d359f - vkd3d-shader/tpf: Remove an unnecessary carriage return from a parser error message.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • Conor McCarthy added 1 commit

    added 1 commit

    • f043e656 - vkd3d-shader/tpf: Validate input and output index ranges for default control point phases.

    Compare with previous version

  • Conor McCarthy added 1 commit

    added 1 commit

    • 523c3ce7 - vkd3d-shader/tpf: Emit an error if an index range is declared for default...

    Compare with previous version

  • Henri Verbeet unapproved this merge request

    unapproved this merge request

  • Author Developer

    I don't think index range declarations are meaningful for incontrolpoint and outcontrolpoint registers so we should just error out on them.

  • Conor McCarthy added 3 commits

    added 3 commits

    • 8a0b5525 - vkd3d-shader/tpf: Validate index range declarations.
    • 799443c5 - vkd3d-shader/tpf: Remove an unnecessary carriage return from a parser error message.
    • 74dbe3c7 - vkd3d-shader/tpf: Validate input and output index ranges for default control point phases.

    Compare with previous version

  • Author Developer

    I don't think index range declarations are meaningful for incontrolpoint and outcontrolpoint registers

    Actually they do occur, although currently the backend has no support for them. The last normalisation patch adds it.

  • Conor McCarthy added 7 commits

    added 7 commits

    • f03cd243 - vkd3d-shader/tpf: Validate input/output register index counts.
    • 542b3d77 - vkd3d-shader/tpf: Validate signature element register indices.
    • 7d5357e8 - vkd3d-shader/tpf: Validate signature element masks.
    • 98bec075 - vkd3d-shader/tpf: Validate input/output registers.
    • e6b3ca0b - vkd3d-shader/tpf: Validate index range declarations.
    • aea7fe8f - vkd3d-shader/tpf: Remove an unnecessary carriage return from a parser error message.
    • 8f9cbe03 - vkd3d-shader/tpf: Validate input and output index ranges for default control point phases.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on commit f03cd243
1710 1714 * other values up one slot. Normalize to SM5.1. */
1711 1715 param->idx[2] = param->idx[1];
1712 1716 param->idx[1] = param->idx[0];
1717 ++param->idx_count;
  • I don't really know what I'm talking about here, but after reading that my first though is: "Shouldn't we check that idx_count is not already three? And shouldn't we zero idx[0]?".

  • Please register or sign in to reply
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on commit 8f9cbe03
  • 2403 return memcmp(a, b, sizeof(struct sm4_index_range));
    2404 }
    2405
    2406 static void shader_sm4_validate_default_phase_index_ranges(struct vkd3d_shader_sm4_parser *sm4)
    2407 {
    2408 if (!sm4->input_index_ranges.count || !sm4->output_index_ranges.count)
    2409 return;
    2410
    2411 if (sm4->input_index_ranges.count == sm4->output_index_ranges.count)
    2412 {
    2413 qsort(sm4->input_index_ranges.ranges, sm4->input_index_ranges.count, sizeof(sm4->input_index_ranges.ranges[0]),
    2414 index_range_compare);
    2415 qsort(sm4->output_index_ranges.ranges, sm4->output_index_ranges.count, sizeof(sm4->output_index_ranges.ranges[0]),
    2416 index_range_compare);
    2417 if (!memcmp(sm4->input_index_ranges.ranges, sm4->output_index_ranges.ranges,
    2418 sm4->input_index_ranges.count * sizeof(sm4->input_index_ranges.ranges[0])))
    • Comment on lines +2417 to +2418

      I think that's correct, but quite fragile: as soon as, for any reason, sm4_index_range_array is modified in a way that causes it to have some padding, the compare might break.

      No need to change for me, let's just keep this in mind.

    • Please register or sign in to reply
  • I don't really know what I'm talking about here, but after reading that my first though is: "Shouldn't we check that idx_count is not already three? And shouldn't we zero idx[0]?".

    We shouldn't zero idx[0] here; duplicating it is the intended behaviour. It's true that this code could be a bit more robust, although that's not an issue introduced by this series, and probably best addressed by extending the index count validation code to cover the affected register types.

    I think that's correct, but quite fragile: as soon as, for any reason, sm4_index_range_array is modified in a way that causes it to have some padding, the compare might break.

    Yeah.

  • Alexandre Julliard added 35 commits

    added 35 commits

    • 8f9cbe03...b1bc4044 - 28 commits from branch wine:master
    • adf7db02 - vkd3d-shader/tpf: Validate input/output register index counts.
    • b8e64823 - vkd3d-shader/tpf: Validate signature element register indices.
    • 2166088b - vkd3d-shader/tpf: Validate signature element masks.
    • d565fbdc - vkd3d-shader/tpf: Validate input/output registers.
    • 6dd1b012 - vkd3d-shader/tpf: Validate index range declarations.
    • 3e50c4d1 - vkd3d-shader/tpf: Remove an unnecessary carriage return from a parser error message.
    • be4a71da - vkd3d-shader/tpf: Validate input and output index ranges for default control point phases.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading