Skip to content
Snippets Groups Projects

vkd3d-shader: Add a stub constant buffers tracing for fx -> text path.

Merged Nikolay Sivov requested to merge nsivov/vkd3d:fx_disasm into master
1 unresolved thread

Signed-off-by: Nikolay Sivov nsivov@codeweavers.com

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • +    /**
      +     * Binary format used by Direct3D 9/10.x/11 effects.
      +     * For fx_4_x binaries both raw sections and dxbc containers are accepted. \since 1.14
      +     */
      +    VKD3D_SHADER_SOURCE_FX,

      Do we have to accept DXBC containers here? I notice the MR doesn't actually implement that functionality. If this is mainly a matter of convenience for vkd3d-compiler, I think I'd prefer vkd3d-compiler handling that by itself.

      +struct fx_parser
      +{
      +    const char *ptr, *start, *end;
      +    struct vkd3d_shader_message_context *message_context;
      +    struct vkd3d_string_buffer buffer;
      +    struct
      +    {
      +        const char *ptr;
      +        const char *end;
      +        uint32_t size;
      +    } unstructured;
      +    uint32_t buffer_count;
      +    bool failed;
      +};

      It's perhaps largely cosmetic, but "uint8_t" seems more appropriate than "char".

      +static int fx_2_parse(struct fx_parser *parser)
      +{
      +    WARN("Parsing of fx_2_0 binaries is not implemented.\n");
      +
      +    return -1;
      +}

      Using WARN here isn't wrong, but it seems slightly nicer to use vkd3d_shader_error() to return a message to the user. That would also mean the "message_context" field of struct fx_parser would actually get used.

      +static int fx_4_parse(struct fx_parser *parser)
      +{
      +    struct fx_4_header
      +    {
      +        uint32_t version;
      +        uint32_t buffer_count;
      +        uint32_t numeric_variable_count;
      +        uint32_t object_variable_count;
      +        uint32_t shared_buffer_count;
      +        uint32_t shared_numeric_variable_count;
      +        uint32_t shared_object_count;
      +        uint32_t technique_count;
      +        uint32_t unstructured_size;
      +        uint32_t string_count;
      +        uint32_t texture_count;
      +        uint32_t depth_stencil_state_count;
      +        uint32_t blend_state_count;
      +        uint32_t rasterizer_state_count;
      +        uint32_t sampler_state_count;
      +        uint32_t rtv_count;
      +        uint32_t dsv_count;
      +        uint32_t shader_count;
      +        uint32_t inline_shader_count;
      +    } header;
      +
      +    fx_parser_read_u32s(parser, &header, sizeof(header));
      +    parser->buffer_count = header.buffer_count;
      +
      +    parser->unstructured.ptr = parser->ptr;
      +    parser->unstructured.end = parser->ptr + header.unstructured_size;
      +    parser->unstructured.size = header.unstructured_size;
      +
      +    if (parser->unstructured.end > parser->end)
      +    {
      +        parser->failed = true;
      +        return -1;
      +    }

      The addition of "parser->ptr" and "header.unstructured_size" can overflow in principle, although it's perhaps harder to make that actually happen on 64-bit builds. It seems more robust to write the check as "if (parser->end - parser->unstructured.ptr < header.unstructured_size)", or something similar.

      +static void fx_parser_read_u32s_unstructured(struct fx_parser *parser, void *dst, uint32_t offset, size_t size)
      +{
      +    const char *ptr = parser->unstructured.ptr;
      +
      +    memset(dst, 0, size);
      +    if (offset >= parser->unstructured.size
      +            || size > parser->unstructured.size
      +            || offset > parser->unstructured.size - size)
      +    {
      +        parser->failed = true;
      +        return;
      +    }
      +
      +    ptr += offset;
      +    memcpy(dst, ptr, size);
      +}

      Doesn't that read bytes instead of u32's? Also, if we know that "offset < parser->unstructured.size", we can just check that "size <= parser->unstructured.size - offset".

      Edited by Henri Verbeet
    • Author Developer

      I think all the points are addressed now.

    • Please register or sign in to reply
  • Author Developer

    Do we have to accept DXBC containers here? I notice the MR doesn't actually implement that functionality. If this is mainly a matter of convenience for vkd3d-compiler, I think I'd prefer vkd3d-compiler handling that by itself.

    That would mean to do the same in D3DDisassemble(). But yes, I can see why it's better to have consistent input.

    I'll fix up the rest.

  • Do we have to accept DXBC containers here? I notice the MR doesn't actually implement that functionality. If this is mainly a matter of convenience for vkd3d-compiler, I think I'd prefer vkd3d-compiler handling that by itself.

    That would mean to do the same in D3DDisassemble(). But yes, I can see why it's better to have consistent input.

    Yeah, I think we're fine with that. We could have an internal helper to avoid writing that more that once; shouldn't be more complicated than vkd3d_shader_parse_dxbc_source_type().

  • Author Developer

    I have to mention also that output text format does not have to strictly follow d3dcompiler. This output can't be used to assemble effect back, but it's very useful to visually inspect contents.

  • Nikolay Sivov added 44 commits

    added 44 commits

    • 1d788e20...cd74461d - 41 commits from branch wine:master
    • a338ed9c - vkd3d-shader: Add a stub constant buffers tracing for fx -> text path.
    • 2d4815a8 - vkd3d-shader/fx: Add support for parsing constant buffer elements.
    • c9dd7150 - vkd3d-shader/fx: Add support for tracing string variables.

    Compare with previous version

  • +static uint32_t fx_parser_read_u32(struct fx_parser *parser)
    +{
    +    uint32_t ret;
    +
    +    if (parser->ptr == parser->end || (parser->end - parser->ptr) < sizeof(uint32_t))
    +    {

    Do we need the "parser->ptr == parser->end" check above? The other check would evaluate to "0 < sizeof(uint32_t)", wouldn't it?

    +static void fx_parser_skip(struct fx_parser *parser, size_t size)
    +{
    +    if ((parser->end - parser->ptr) < size)
    +        parser->failed = true;
    +    parser->ptr += size;
    +}

    It may be safer to set "parser->ptr" to "parser->end" on failure (or just don't touch it at all) here, instead of moving it past the end anyway.

    +static int fx_2_parse(struct fx_parser *parser)
    +{
    +    vkd3d_shader_error(parser->message_context, NULL, VKD3D_SHADER_ERROR_FX_NOT_IMPLEMENTED,
    +            "Parsing fx_2_0 binaries is not implemented.\n");
    +
    +    return -1;
    +}

    Just a note, doesn't have to be in this MR, but you could instead implement a "fx_parser_error()" function on top of vkd3d_shader_verror(), which would then set "parser->failed = true", much like e.g. vkd3d_shader_parser_error() and hlsl_error().

    +static int fx_5_parse(struct fx_parser *parser)
    +{
    +    struct fx_5_header
    +    {
    +        uint32_t version;
    +        uint32_t buffer_count;
    +        uint32_t numeric_variable_count;
    +        uint32_t object_variable_count;
    +        uint32_t shared_buffer_count;
    +        uint32_t shared_numeric_variable_count;
    +        uint32_t shared_object_count;
    +        uint32_t technique_count;
    +        uint32_t unstructured_size;
    +        uint32_t string_count;
    +        uint32_t texture_count;
    +        uint32_t depth_stencil_state_count;
    +        uint32_t blend_state_count;
    +        uint32_t rasterizer_state_count;
    +        uint32_t sampler_state_count;
    +        uint32_t rtv_count;
    +        uint32_t dsv_count;
    +        uint32_t shader_count;
    +        uint32_t inline_shader_count;
    +        uint32_t group_count;
    +        uint32_t uav_count;
    +        uint32_t interface_variable_count;
    +        uint32_t interface_variable_element_count;
    +        uint32_t class_instance_element_count;
    +    } header;
    +
    +    fx_parser_read_u32s(parser, &header, sizeof(header));
    +    parser->buffer_count = header.buffer_count;
    +
    +    parser->unstructured.ptr = parser->ptr;
    +    parser->unstructured.end = parser->ptr + header.unstructured_size;
    +    parser->unstructured.size = header.unstructured_size;
    +
    +    if (parser->unstructured.end > parser->end)
    +    {
    +        parser->failed = true;
    +        return -1;
    +    }
    +    fx_parser_skip(parser, header.unstructured_size);
    +
    +    return fx_parse_buffers(parser);
    +}

    This still has the same issue that fx_4_parse() used to have. (I.e., "parser->ptr + header.unstructured_size" potentially overflowing.)

    +            unsigned int base_type, comp_count;
    +            size_t j;
    +
    +            if (type.class == 1)
    +                base_type = (type.typeinfo >> 3) & 0xf;
    +            else
    +                base_type = 0;

    "3" above is NUMERIC_BASE_TYPE_SHIFT, right?

    +            for (j = 0; j < comp_count; ++j)
    +            {
    +                union hlsl_constant_value_component value;
    +
    +                fx_parser_read_unstructured(parser, &value, var.value + j * sizeof(uint32_t), sizeof(uint32_t));
    +
    +                if (base_type == 1)
    +                    vkd3d_string_buffer_printf(&parser->buffer, "%f", value.f);
    +                else if (base_type == 2)
    +                    vkd3d_string_buffer_printf(&parser->buffer, "%d", value.i);
    +                else if (base_type == 3)
    +                    vkd3d_string_buffer_printf(&parser->buffer, "%u", value.u);
    +                else if (base_type == 4)
    +                    vkd3d_string_buffer_printf(&parser->buffer, "%s", value.u ? "true" : "false" );

    And I imagine 1/2/3/4 above are the values from fx_4_numeric_base_type[]. Might not be bad to have some constants for these.

  • Nikolay Sivov added 37 commits

    added 37 commits

    • c9dd7150...f0e31dd6 - 34 commits from branch wine:master
    • 7ebe72b4 - vkd3d-shader: Add a stub constant buffers tracing for fx -> text path.
    • 998cc77d - vkd3d-shader/fx: Add support for parsing constant buffer elements.
    • 873e9a5a - vkd3d-shader/fx: Add support for tracing string variables.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • Henri Verbeet added 11 commits

    added 11 commits

    • 873e9a5a...cd249a47 - 8 commits from branch wine:master
    • 6801ad9b - vkd3d-shader/fx: Introduce a parser/disassembler.
    • 907b6705 - vkd3d-shader/fx: Add support for parsing constant buffer elements.
    • 35d2df14 - vkd3d-shader/fx: Add support for tracing string variables.

    Compare with previous version

Please register or sign in to reply
Loading