[PATCH vkd3d 1/4] vkd3d-shader: Do not validate the target type in vkd3d_shader_scan().

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
 libs/vkd3d-shader/vkd3d_shader_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c
index 3982d0730..47fe3d60d 100644
--- a/libs/vkd3d-shader/vkd3d_shader_main.c
+++ b/libs/vkd3d-shader/vkd3d_shader_main.c
@@ -1025,7 +1025,7 @@ int vkd3d_shader_scan(const struct vkd3d_shader_compile_info *compile_info, char
     if (messages)
         *messages = NULL;

-    if ((ret = vkd3d_shader_validate_compile_info(compile_info, true)) < 0)
+    if ((ret = vkd3d_shader_validate_compile_info(compile_info, false)) < 0)
         return ret;

vkd3d_shader_message_context_init(&message_context, compile_info->log_level);

[PATCH vkd3d 2/4] vkd3d-shader: Make shader_extract_from_dxbc() static.

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
 libs/vkd3d-shader/dxbc.c                 | 74 ++++++++++++------------
 libs/vkd3d-shader/vkd3d_shader_private.h |  2 -
 2 files changed, 37 insertions(+), 39 deletions(-)

diff --git a/libs/vkd3d-shader/dxbc.c b/libs/vkd3d-shader/dxbc.c
index b83e26546..0b76a4a35 100644
--- a/libs/vkd3d-shader/dxbc.c
+++ b/libs/vkd3d-shader/dxbc.c
@@ -1669,42 +1669,6 @@ static bool shader_sm4_init(struct vkd3d_shader_sm4_parser *sm4, const uint32_t
     return true;
 }

-int vkd3d_shader_sm4_parser_create(const struct vkd3d_shader_compile_info *compile_info,
-        struct vkd3d_shader_message_context *message_context, struct vkd3d_shader_parser **parser)
-{
-    struct vkd3d_shader_desc *shader_desc;
-    struct vkd3d_shader_sm4_parser *sm4;
-    int ret;
-
-    if (!(sm4 = vkd3d_calloc(1, sizeof(*sm4))))
-    {
-        ERR("Failed to allocate parser.\n");
-        return VKD3D_ERROR_OUT_OF_MEMORY;
-    }
-
-    shader_desc = &sm4->p.shader_desc;
-    if ((ret = shader_extract_from_dxbc(compile_info->source.code, compile_info->source.size,
-            message_context, compile_info->source_name, shader_desc)) < 0)
-    {
-        WARN("Failed to extract shader, vkd3d result %d.\n", ret);
-        vkd3d_free(sm4);
-        return ret;
-    }
-
-    if (!shader_sm4_init(sm4, shader_desc->byte_code, shader_desc->byte_code_size,
-            compile_info->source_name, &shader_desc->output_signature, message_context))
-    {
-        WARN("Failed to initialise shader parser.\n");
-        free_shader_desc(shader_desc);
-        vkd3d_free(sm4);
-        return VKD3D_ERROR_INVALID_ARGUMENT;
-    }
-
-    *parser = &sm4->p;
-
-    return VKD3D_OK;
-}
-
 static bool require_space(size_t offset, size_t count, size_t size, size_t data_size)
 {
     return !count || (data_size - offset) / count >= size;
@@ -2039,7 +2003,7 @@ void free_shader_desc(struct vkd3d_shader_desc *desc)
     vkd3d_shader_free_shader_signature(&desc->patch_constant_signature);
 }

-int shader_extract_from_dxbc(const void *dxbc, size_t dxbc_length,
+static int shader_extract_from_dxbc(const void *dxbc, size_t dxbc_length,
         struct vkd3d_shader_message_context *message_context, const char *source_name, struct vkd3d_shader_desc *desc)
 {
     int ret;
@@ -2063,6 +2027,42 @@ int shader_extract_from_dxbc(const void *dxbc, size_t dxbc_length,
     return ret;
 }

+int vkd3d_shader_sm4_parser_create(const struct vkd3d_shader_compile_info *compile_info,
+        struct vkd3d_shader_message_context *message_context, struct vkd3d_shader_parser **parser)
+{
+    struct vkd3d_shader_desc *shader_desc;
+    struct vkd3d_shader_sm4_parser *sm4;
+    int ret;
+
+    if (!(sm4 = vkd3d_calloc(1, sizeof(*sm4))))
+    {
+        ERR("Failed to allocate parser.\n");
+        return VKD3D_ERROR_OUT_OF_MEMORY;
+    }
+
+    shader_desc = &sm4->p.shader_desc;
+    if ((ret = shader_extract_from_dxbc(compile_info->source.code, compile_info->source.size,
+            message_context, compile_info->source_name, shader_desc)) < 0)
+    {
+        WARN("Failed to extract shader, vkd3d result %d.\n", ret);
+        vkd3d_free(sm4);
+        return ret;
+    }
+
+    if (!shader_sm4_init(sm4, shader_desc->byte_code, shader_desc->byte_code_size,
+            compile_info->source_name, &shader_desc->output_signature, message_context))
+    {
+        WARN("Failed to initialise shader parser.\n");
+        free_shader_desc(shader_desc);
+        vkd3d_free(sm4);
+        return VKD3D_ERROR_INVALID_ARGUMENT;
+    }
+
+    *parser = &sm4->p;
+
+    return VKD3D_OK;
+}
+
 /* root signatures */
 #define VKD3D_ROOT_SIGNATURE_1_0_ROOT_DESCRIPTOR_FLAGS VKD3D_SHADER_ROOT_DESCRIPTOR_FLAG_DATA_VOLATILE

diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h
index 62073cf1b..ffea91d36 100644
--- a/libs/vkd3d-shader/vkd3d_shader_private.h
+++ b/libs/vkd3d-shader/vkd3d_shader_private.h
@@ -1045,8 +1045,6 @@ int vkd3d_shader_sm1_parser_create(const struct vkd3d_shader_compile_info *compi
 int vkd3d_shader_sm4_parser_create(const struct vkd3d_shader_compile_info *compile_info,
         struct vkd3d_shader_message_context *message_context, struct vkd3d_shader_parser **parser);

-int shader_extract_from_dxbc(const void *dxbc, size_t dxbc_length,
-        struct vkd3d_shader_message_context *message_context, const char *source_name, struct vkd3d_shader_desc *desc);
 void free_shader_desc(struct vkd3d_shader_desc *desc);

int shader_parse_input_signature(const void *dxbc, size_t dxbc_length,

[PATCH vkd3d 2/4] vkd3d-shader: Make shader_extract_from_dxbc() static.

[Henri Verbeet]

Signed-off-by: Henri Verbeet

[PATCH vkd3d 3/4] vkd3d-shader: Add support for retrieving the shader version through vkd3d_shader_scan().

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
Besides being the first patch implementing reflection, this patch introduces an
API problem with regard to D3DReflect() that will affect many different fields.
Namely, if the SM4 version token contains a nonsensical version D3DReflect()
will happily return it verbatim. As I see it our options are:

(1) return the token verbatim anyway instead of using struct
    vkd3d_shader_version,

(2) don't use vkd3d_shader_scan() for wine's D3DReflect() [and either diverge
    from native in vkd3d-utils' D3DReflect(), or don't use vkd3d_shader_scan()
    there either]

(3) return the token verbatim in a separate field (and in a separate chained
    structure?)

(4) punt, and use one of the above solutions if we ever do come across a shader
    that contains insane data.

There are some other, similar problems:

* HLSL compile flags would be especially annoying to translate to e.g.
  libvkd3d-shader compile arguments and then back again. (Frankly, it's already
  annoying that we have to do that in the HLSL compiler.) This is basically the
  same problem as the above, but it forms an argument for returning tokens
  verbatim, or at least that one.

* Instruction counts (and I think everything else in the STAT chunk) are always
  taken from the STAT chunk; if the STAT chunk is missing then D3DReflect() will
  return zero, even though it could just count them. Should we preserve that
  behaviour in libvkd3d-shader? in libvkd3d-utils?

include/vkd3d_shader.h                   | 39 ++++++++++++++++++++++
 libs/vkd3d-shader/vkd3d_shader_main.c    | 16 ++++++++-
 libs/vkd3d-shader/vkd3d_shader_private.h | 24 --------------
 tests/vkd3d_shader_api.c                 | 42 ++++++++++++++++++++++++
 4 files changed, 96 insertions(+), 25 deletions(-)

diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h
index 791ac0377..25abf217e 100644
--- a/include/vkd3d_shader.h
+++ b/include/vkd3d_shader.h
@@ -69,6 +69,11 @@ enum vkd3d_shader_structure_type
      * \since 1.3
      */
     VKD3D_SHADER_STRUCTURE_TYPE_DESCRIPTOR_OFFSET_INFO,
+    /**
+     * The structure is a vkd3d_shader_scan_function_info structure.
+     * \since 1.3
+     */
+    VKD3D_SHADER_STRUCTURE_TYPE_SCAN_FUNCTION_INFO,

VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_STRUCTURE_TYPE),
 };
@@ -1267,6 +1272,40 @@ struct vkd3d_shader_scan_descriptor_info
     unsigned int descriptor_count;
 };

+enum vkd3d_shader_type
+{
+    VKD3D_SHADER_TYPE_PIXEL,
+    VKD3D_SHADER_TYPE_VERTEX,
+    VKD3D_SHADER_TYPE_GEOMETRY,
+    VKD3D_SHADER_TYPE_HULL,
+    VKD3D_SHADER_TYPE_DOMAIN,
+    VKD3D_SHADER_TYPE_GRAPHICS_COUNT,
+
+    VKD3D_SHADER_TYPE_COMPUTE = VKD3D_SHADER_TYPE_GRAPHICS_COUNT,
+
+    VKD3D_SHADER_TYPE_EFFECT,
+    VKD3D_SHADER_TYPE_TEXTURE,
+    VKD3D_SHADER_TYPE_LIBRARY,
+    VKD3D_SHADER_TYPE_COUNT,
+
+    VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_TYPE),
+};
+
+struct vkd3d_shader_version
+{
+    enum vkd3d_shader_type type;
+    uint16_t major;
+    uint16_t minor;
+};
+
+struct vkd3d_shader_scan_function_info
+{
+    enum vkd3d_shader_structure_type type;
+    const void *next;
+
+    struct vkd3d_shader_version version;
+};
+
 /**
  * Data type of a shader varying, returned as part of struct
  * vkd3d_shader_signature_element.
diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c
index 47fe3d60d..7e82f18db 100644
--- a/libs/vkd3d-shader/vkd3d_shader_main.c
+++ b/libs/vkd3d-shader/vkd3d_shader_main.c
@@ -933,6 +933,7 @@ static int scan_with_parser(const struct vkd3d_shader_compile_info *compile_info
         struct vkd3d_shader_message_context *message_context, struct vkd3d_shader_parser *parser)
 {
     struct vkd3d_shader_scan_descriptor_info *scan_descriptor_info;
+    struct vkd3d_shader_scan_function_info *func_info;
     struct vkd3d_shader_instruction instruction;
     struct vkd3d_shader_scan_context context;
     int ret;
@@ -943,6 +944,8 @@ static int scan_with_parser(const struct vkd3d_shader_compile_info *compile_info
         scan_descriptor_info->descriptor_count = 0;
     }

+    func_info = vkd3d_find_struct(compile_info->next, SCAN_FUNCTION_INFO);
+
     vkd3d_shader_scan_context_init(&context, compile_info, scan_descriptor_info, message_context);

if (TRACE_ON())
@@ -972,7 +975,18 @@ static int scan_with_parser(const struct vkd3d_shader_compile_info *compile_info
         }
     }

-    ret = parser->failed ? VKD3D_ERROR_INVALID_SHADER : VKD3D_OK;
+    if (parser->failed)
+    {
+        ret = VKD3D_ERROR_INVALID_SHADER;
+        goto done;
+    }
+
+    if (func_info)
+    {
+        func_info->version = parser->shader_version;
+    }
+
+    ret = VKD3D_OK;

done:
     vkd3d_shader_scan_context_cleanup(&context);
diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h
index ffea91d36..c7ffa3c81 100644
--- a/libs/vkd3d-shader/vkd3d_shader_private.h
+++ b/libs/vkd3d-shader/vkd3d_shader_private.h
@@ -594,30 +594,6 @@ enum vkd3d_shader_conditional_op
 #define MAX_IMMEDIATE_CONSTANT_BUFFER_SIZE 4096
 #define MAX_REG_OUTPUT 32

-enum vkd3d_shader_type
-{
-    VKD3D_SHADER_TYPE_PIXEL,
-    VKD3D_SHADER_TYPE_VERTEX,
-    VKD3D_SHADER_TYPE_GEOMETRY,
-    VKD3D_SHADER_TYPE_HULL,
-    VKD3D_SHADER_TYPE_DOMAIN,
-    VKD3D_SHADER_TYPE_GRAPHICS_COUNT,
-
-    VKD3D_SHADER_TYPE_COMPUTE = VKD3D_SHADER_TYPE_GRAPHICS_COUNT,
-
-    VKD3D_SHADER_TYPE_EFFECT,
-    VKD3D_SHADER_TYPE_TEXTURE,
-    VKD3D_SHADER_TYPE_LIBRARY,
-    VKD3D_SHADER_TYPE_COUNT,
-};
-
-struct vkd3d_shader_version
-{
-    enum vkd3d_shader_type type;
-    uint8_t major;
-    uint8_t minor;
-};
-
 struct vkd3d_shader_immediate_constant_buffer
 {
     unsigned int vec4_count;
diff --git a/tests/vkd3d_shader_api.c b/tests/vkd3d_shader_api.c
index 6fb855c26..31eeb17f2 100644
--- a/tests/vkd3d_shader_api.c
+++ b/tests/vkd3d_shader_api.c
@@ -318,6 +318,47 @@ static void test_d3dbc(void)
     ok(rc == VKD3D_ERROR_INVALID_SHADER, "Got unexpected error code %d.\n", rc);
 }

+static void test_scan_dxbc(void)
+{
+    struct vkd3d_shader_scan_function_info func_info = {.type = VKD3D_SHADER_STRUCTURE_TYPE_SCAN_FUNCTION_INFO};
+    struct vkd3d_shader_hlsl_source_info hlsl_info = {.type = VKD3D_SHADER_STRUCTURE_TYPE_HLSL_SOURCE_INFO};
+    struct vkd3d_shader_compile_info compile_info = {.type = VKD3D_SHADER_STRUCTURE_TYPE_COMPILE_INFO};
+    struct vkd3d_shader_code dxbc;
+    int rc;
+
+    static const char vs_source[] =
+        "float4 main(float4 pos : position) : sv_position\n"
+        "{\n"
+        "    return pos;\n"
+        "}";
+
+    compile_info.source.code = vs_source;
+    compile_info.source.size = strlen(vs_source);
+    compile_info.source_type = VKD3D_SHADER_SOURCE_HLSL;
+    compile_info.target_type = VKD3D_SHADER_TARGET_DXBC_TPF;
+    compile_info.log_level = VKD3D_SHADER_LOG_INFO;
+
+    compile_info.next = &hlsl_info;
+    hlsl_info.profile = "vs_4_1";
+
+    rc = vkd3d_shader_compile(&compile_info, &dxbc, NULL);
+    ok(rc == VKD3D_OK, "Got unexpected error code %d.\n", rc);
+
+    compile_info.source_type = VKD3D_SHADER_SOURCE_DXBC_TPF;
+    compile_info.source = dxbc;
+
+    compile_info.next = &func_info;
+
+    rc = vkd3d_shader_scan(&compile_info, NULL);
+    ok(rc == VKD3D_OK, "Got unexpected error code %d.\n", rc);
+
+    ok(func_info.version.type == VKD3D_SHADER_TYPE_VERTEX, "Got shader type %#x.\n", func_info.version.type);
+    ok(func_info.version.major == 4, "Got major version %u.\n", func_info.version.major);
+    ok(func_info.version.minor == 1, "Got minor version %u.\n", func_info.version.minor);
+
+    vkd3d_shader_free_shader_code(&dxbc);
+}
+
 START_TEST(vkd3d_shader_api)
 {
     setlocale(LC_ALL, "");
@@ -326,4 +367,5 @@ START_TEST(vkd3d_shader_api)
     run_test(test_vkd3d_shader_pfns);
     run_test(test_version);
     run_test(test_d3dbc);
+    run_test(test_scan_dxbc);
 }

[PATCH vkd3d 3/4] vkd3d-shader: Add support for retrieving the shader version through vkd3d_shader_scan().

[Henri Verbeet]

On Tue, 28 Dec 2021 at 23:20, Zebediah Figura  wrote:
> Besides being the first patch implementing reflection, this patch introduces an
> API problem with regard to D3DReflect() that will affect many different fields.
> Namely, if the SM4 version token contains a nonsensical version D3DReflect()
> will happily return it verbatim. As I see it our options are:
>
> (1) return the token verbatim anyway instead of using struct
>     vkd3d_shader_version,
>
> (2) don't use vkd3d_shader_scan() for wine's D3DReflect() [and either diverge
>     from native in vkd3d-utils' D3DReflect(), or don't use vkd3d_shader_scan()
>     there either]
>
> (3) return the token verbatim in a separate field (and in a separate chained
>     structure?)
>
> (4) punt, and use one of the above solutions if we ever do come across a shader
>     that contains insane data.

I'd be inclined to go with option 3 here. Somewhat analogous to e.g.
vkd3d_shader_spirv_target_info or vkd3d_shader_hlsl_source_info, we
could perhaps do something like the following:

struct vkd3d_shader_tpf_scan_info
    {
        ...
        uint32_t version_token;
        const char *creator;
        uint32_t flags;
        ...
    };

We could perhaps slightly generalise that to something like
"vkd3d_shader_d3d_scan_info", since the documentation claims the
ID3D12ShaderReflection should be supported for the d3dbc, tpf and dxil
targets, although in case of the dxil target that would normally be
provided by the dxcompiler DLL instead of d3dcompiler.
> There are some other, similar problems:
> - HLSL compile flags would be especially annoying to translate to e.g.
> libvkd3d-shader compile arguments and then back again. (Frankly, it's already
> annoying that we have to do that in the HLSL compiler.) This is basically the
> same problem as the above, but it forms an argument for returning tokens
> verbatim, or at least that one.

Sure.
> - Instruction counts (and I think everything else in the STAT chunk) are always
> taken from the STAT chunk; if the STAT chunk is missing then D3DReflect() will
> return zero, even though it could just count them. Should we preserve that
> behaviour in libvkd3d-shader? in libvkd3d-utils?

If we're going with something like
vkd3d_shader_tpf_scan_info/vkd3d_shader_d3d_scan_info, probably yes.

[PATCH vkd3d 3/4] vkd3d-shader: Add support for retrieving the shader version through vkd3d_shader_scan().

[Giovanni Mascellani]

Hi,

On 06/01/22 11:15, Henri Verbeet wrote:
> On Tue, 28 Dec 2021 at 23:20, Zebediah Figura  wrote:
> > Besides being the first patch implementing reflection, this patch introduces an
> > API problem with regard to D3DReflect() that will affect many different fields.
> > Namely, if the SM4 version token contains a nonsensical version D3DReflect()
> > will happily return it verbatim. As I see it our options are:
> >
> > (1) return the token verbatim anyway instead of using struct
> >      vkd3d_shader_version,
> >
> > (2) don't use vkd3d_shader_scan() for wine's D3DReflect() [and either diverge
> >      from native in vkd3d-utils' D3DReflect(), or don't use vkd3d_shader_scan()
> >      there either]
> >
> > (3) return the token verbatim in a separate field (and in a separate chained
> >      structure?)
> >
> > (4) punt, and use one of the above solutions if we ever do come across a shader
> >      that contains insane data.
>
> I'd be inclined to go with option 3 here.

I don't think I have a complete understanding of the matter, but based 
on what I gather I'd say that it makes sense to report both interpreted 
and uninterpreted data, so that the caller can choose what they believe 
it's best for them. I guess it's option 3 here. No strong opinion of 
whether to using the same or another chained structure (weak opinion is 
to use the same).
> > - Instruction counts (and I think everything else in the STAT chunk) are always
> >  taken from the STAT chunk; if the STAT chunk is missing then D3DReflect() will
> >  return zero, even though it could just count them. Should we preserve that
> >  behaviour in libvkd3d-shader? in libvkd3d-utils?
>
> If we're going with something like
> vkd3d_shader_tpf_scan_info/vkd3d_shader_d3d_scan_info, probably yes.

In the same philosophy, I'd make both the reported and recomputed data 
available, so that the caller can choose. If it is expensive to 
recompute, I'd gate that with a flag.

Giovanni.

[PATCH vkd3d 3/4] vkd3d-shader: Add support for retrieving the shader version through vkd3d_shader_scan().

[Henri Verbeet]

On Fri, 7 Jan 2022 at 11:44, Giovanni Mascellani
 wrote:
> On 06/01/22 11:15, Henri Verbeet wrote:
> > On Tue, 28 Dec 2021 at 23:20, Zebediah Figura  wrote:
> > > Besides being the first patch implementing reflection, this patch introduces an
> > > API problem with regard to D3DReflect() that will affect many different fields.
> > > Namely, if the SM4 version token contains a nonsensical version D3DReflect()
> > > will happily return it verbatim. As I see it our options are:
> > >
> > > (1) return the token verbatim anyway instead of using struct
> > >      vkd3d_shader_version,
> > >
> > > (2) don't use vkd3d_shader_scan() for wine's D3DReflect() [and either diverge
> > >      from native in vkd3d-utils' D3DReflect(), or don't use vkd3d_shader_scan()
> > >      there either]
> > >
> > > (3) return the token verbatim in a separate field (and in a separate chained
> > >      structure?)
> > >
> > > (4) punt, and use one of the above solutions if we ever do come across a shader
> > >      that contains insane data.
> >
> > I'd be inclined to go with option 3 here.
>
> I don't think I have a complete understanding of the matter, but based
> on what I gather I'd say that it makes sense to report both interpreted
> and uninterpreted data, so that the caller can choose what they believe
> it's best for them. I guess it's option 3 here. No strong opinion of
> whether to using the same or another chained structure (weak opinion is
> to use the same).

In the abstract, sure. In practice, I'm not sure how much value there
would be in returning e.g. a vkd3d_shader_version or creator string
for HLSL source, or e.g. SPIR-V source if we ever ended up supporting
that.
> > > - Instruction counts (and I think everything else in the STAT chunk) are always
> > >  taken from the STAT chunk; if the STAT chunk is missing then D3DReflect() will
> > >  return zero, even though it could just count them. Should we preserve that
> > >  behaviour in libvkd3d-shader? in libvkd3d-utils?
> >
> > If we're going with something like
> > vkd3d_shader_tpf_scan_info/vkd3d_shader_d3d_scan_info, probably yes.
>
> In the same philosophy, I'd make both the reported and recomputed data
> available, so that the caller can choose. If it is expensive to
> recompute, I'd gate that with a flag.

I don't think we'd need a flag; if the relevant _scan_info structure
is missing from the chain, we could avoid retrieving the relevant
information, while if the application doesn't need that information,
it should probably avoid adding the relevant structure to the
structure chain.

[PATCH vkd3d 3/4] vkd3d-shader: Add support for retrieving the shader version through vkd3d_shader_scan().

[Zebediah Figura (she/her)]

On 1/6/22 04:15, Henri Verbeet wrote:
> On Tue, 28 Dec 2021 at 23:20, Zebediah Figura  wrote:
> > Besides being the first patch implementing reflection, this patch introduces an
> > API problem with regard to D3DReflect() that will affect many different fields.
> > Namely, if the SM4 version token contains a nonsensical version D3DReflect()
> > will happily return it verbatim. As I see it our options are:
> >
> > (1) return the token verbatim anyway instead of using struct
> >      vkd3d_shader_version,
> >
> > (2) don't use vkd3d_shader_scan() for wine's D3DReflect() [and either diverge
> >      from native in vkd3d-utils' D3DReflect(), or don't use vkd3d_shader_scan()
> >      there either]
> >
> > (3) return the token verbatim in a separate field (and in a separate chained
> >      structure?)
> >
> > (4) punt, and use one of the above solutions if we ever do come across a shader
> >      that contains insane data.
>
> I'd be inclined to go with option 3 here. Somewhat analogous to e.g.
> vkd3d_shader_spirv_target_info or vkd3d_shader_hlsl_source_info, we
> could perhaps do something like the following:
>  struct vkd3d_shader_tpf_scan_info
>  {
>      ...
>      uint32_t version_token;
>      const char *creator;
>      uint32_t flags;
>      ...
>  };
>
> We could perhaps slightly generalise that to something like
> "vkd3d_shader_d3d_scan_info", since the documentation claims the
> ID3D12ShaderReflection should be supported for the d3dbc, tpf and dxil
> targets, although in case of the dxil target that would normally be
> provided by the dxcompiler DLL instead of d3dcompiler.

I think that makes sense to me.
> > There are some other, similar problems:
> > - HLSL compile flags would be especially annoying to translate to e.g.
> >  libvkd3d-shader compile arguments and then back again. (Frankly, it's already
> >  annoying that we have to do that in the HLSL compiler.) This is basically the
> >  same problem as the above, but it forms an argument for returning tokens
> >  verbatim, or at least that one.
>
> Sure.
> > - Instruction counts (and I think everything else in the STAT chunk) are always
> >  taken from the STAT chunk; if the STAT chunk is missing then D3DReflect() will
> >  return zero, even though it could just count them. Should we preserve that
> >  behaviour in libvkd3d-shader? in libvkd3d-utils?
>
> If we're going with something like
> vkd3d_shader_tpf_scan_info/vkd3d_shader_d3d_scan_info, probably yes.

Okay, I think that also makes sense. It then becomes an open question 
whether we should report more accurate information in struct 
vkd3d_shader_scan_function_info (or whatever the generic structure is) 
but that can be deferred until later.

[PATCH vkd3d 4/4] vkd3d-shader: Add support for retrieving the creator string from DXBC shaders.

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
 include/vkd3d_shader.h                   |   4 +
 libs/vkd3d-shader/d3dbc.c                |   7 +-
 libs/vkd3d-shader/dxbc.c                 | 159 +++++++++++++++--------
 libs/vkd3d-shader/vkd3d_shader.map       |   1 +
 libs/vkd3d-shader/vkd3d_shader_main.c    |  20 ++-
 libs/vkd3d-shader/vkd3d_shader_private.h |  11 +-
 tests/vkd3d_shader_api.c                 |   1 +
 7 files changed, 146 insertions(+), 57 deletions(-)

diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h
index 25abf217e..da926b620 100644
--- a/include/vkd3d_shader.h
+++ b/include/vkd3d_shader.h
@@ -1304,6 +1304,8 @@ struct vkd3d_shader_scan_function_info
     const void *next;

struct vkd3d_shader_version version;
+
+    const char *creator;
 };

/**
@@ -1815,6 +1817,8 @@ VKD3D_SHADER_API void vkd3d_shader_free_shader_signature(struct vkd3d_shader_sig
 VKD3D_SHADER_API int vkd3d_shader_preprocess(const struct vkd3d_shader_compile_info *compile_info,
         struct vkd3d_shader_code *out, char **messages);

+VKD3D_SHADER_API void vkd3d_shader_free_scan_function_info(struct vkd3d_shader_scan_function_info *info);
+
 #endif  /* VKD3D_SHADER_NO_PROTOTYPES */

/** Type of vkd3d_shader_get_version(). */
diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c
index c5518752a..eabcc73e9 100644
--- a/libs/vkd3d-shader/d3dbc.c
+++ b/libs/vkd3d-shader/d3dbc.c
@@ -728,6 +728,10 @@ static void shader_sm1_validate_instruction(struct vkd3d_shader_sm1_parser *sm1,
     }
 }

+static void shader_sm1_read_metadata(struct vkd3d_shader_parser *parser)
+{
+}
+
 static void shader_sm1_read_instruction(struct vkd3d_shader_parser *parser, struct vkd3d_shader_instruction *ins)
 {
     struct vkd3d_shader_sm1_parser *sm1 = vkd3d_shader_sm1_parser(parser);
@@ -863,6 +867,7 @@ const struct vkd3d_shader_parser_ops shader_sm1_parser_ops =
 {
     .parser_reset = shader_sm1_reset,
     .parser_destroy = shader_sm1_destroy,
+    .parser_read_metadata = shader_sm1_read_metadata,
     .parser_read_instruction = shader_sm1_read_instruction,
     .parser_is_end = shader_sm1_is_end,
 };
@@ -922,7 +927,7 @@ static enum vkd3d_result shader_sm1_init(struct vkd3d_shader_sm1_parser *sm1,
     sm1->start = &code[1];
     sm1->end = &code[token_count];

-    vkd3d_shader_parser_init(&sm1->p, message_context, compile_info->source_name, &version, &shader_sm1_parser_ops);
+    vkd3d_shader_parser_init(&sm1->p, message_context, compile_info, &version, &shader_sm1_parser_ops);
     shader_desc = &sm1->p.shader_desc;
     shader_desc->byte_code = code;
     shader_desc->byte_code_size = code_size;
diff --git a/libs/vkd3d-shader/dxbc.c b/libs/vkd3d-shader/dxbc.c
index 0b76a4a35..9144881e9 100644
--- a/libs/vkd3d-shader/dxbc.c
+++ b/libs/vkd3d-shader/dxbc.c
@@ -96,6 +96,7 @@ struct vkd3d_shader_sm4_parser
     struct list src_free;
     struct list src;
     struct vkd3d_shader_immediate_constant_buffer icb;
+    bool found_rdef;

struct vkd3d_shader_parser p;
 };
@@ -175,11 +176,63 @@ static bool shader_is_sm_5_1(const struct vkd3d_shader_sm4_parser *sm4)
     return version->major >= 5 && version->minor >= 1;
 }

+static int parse_dxbc(const char *data, size_t data_size,
+        struct vkd3d_shader_message_context *message_context, const char *source_name,
+        int (*chunk_handler)(const char *data, DWORD data_size, DWORD tag, void *ctx), void *ctx);
 static bool shader_sm4_read_src_param(struct vkd3d_shader_sm4_parser *priv, const uint32_t **ptr,
         const uint32_t *end, enum vkd3d_data_type data_type, struct vkd3d_shader_src_param *src_param);
 static bool shader_sm4_read_dst_param(struct vkd3d_shader_sm4_parser *priv, const uint32_t **ptr,
         const uint32_t *end, enum vkd3d_data_type data_type, struct vkd3d_shader_dst_param *dst_param);

+static bool require_space(size_t offset, size_t count, size_t size, size_t data_size)
+{
+    return !count || (data_size - offset) / count >= size;
+}
+
+static void read_dword(const char **ptr, DWORD *d)
+{
+    memcpy(d, *ptr, sizeof(*d));
+    *ptr += sizeof(*d);
+}
+
+static void read_float(const char **ptr, float *f)
+{
+    STATIC_ASSERT(sizeof(float) == sizeof(DWORD));
+    read_dword(ptr, (DWORD *)f);
+}
+
+static void skip_dword_unknown(const char **ptr, unsigned int count)
+{
+    unsigned int i;
+    DWORD d;
+
+    WARN("Skipping %u unknown DWORDs:\n", count);
+    for (i = 0; i < count; ++i)
+    {
+        read_dword(ptr, &d);
+        WARN("\t0x%08x\n", d);
+    }
+}
+
+static const char *shader_get_string(const char *data, size_t data_size, DWORD offset)
+{
+    size_t len, max_len;
+
+    if (offset >= data_size)
+    {
+        WARN("Invalid offset %#x (data size %#lx).\n", offset, (long)data_size);
+        return NULL;
+    }
+
+    max_len = data_size - offset;
+    len = strnlen(data + offset, max_len);
+
+    if (len == max_len)
+        return NULL;
+
+    return data + offset;
+}
+
 static bool shader_sm4_read_register_space(struct vkd3d_shader_sm4_parser *priv,
         const uint32_t **ptr, const uint32_t *end, unsigned int *register_space)
 {
@@ -1441,6 +1494,54 @@ static void shader_sm4_read_instruction_modifier(DWORD modifier, struct vkd3d_sh
     }
 }

+static void shader_parse_rdef(uint32_t tag, const char *data, size_t data_size, struct vkd3d_shader_sm4_parser *sm4)
+{
+    DWORD cb_count, cb_offset, resource_count, resource_offset, target, flags, creator_offset;
+    const char *ptr = data;
+
+    read_dword(&ptr, &cb_count);
+    read_dword(&ptr, &cb_offset);
+    read_dword(&ptr, &resource_count);
+    read_dword(&ptr, &resource_offset);
+    read_dword(&ptr, &target);
+    read_dword(&ptr, &flags);
+    read_dword(&ptr, &creator_offset);
+
+    /* TODO: Parse RD11 chunks, resources, constant buffers. */
+
+    sm4->p.creator = shader_get_string(data, data_size, creator_offset);
+}
+
+static int metadata_handler(const char *data, DWORD data_size, DWORD tag, void *ctx)
+{
+    struct vkd3d_shader_sm4_parser *sm4 = ctx;
+
+    switch (tag)
+    {
+        case TAG_RDEF:
+        case TAG_RD11:
+            if (sm4->found_rdef)
+                FIXME("Multiple reflection chunks.\n");
+            shader_parse_rdef(tag, data, data_size, sm4);
+            break;
+
+        default:
+            TRACE("Skipping chunk %#x.\n", tag);
+            break;
+    }
+
+    return VKD3D_OK;
+}
+
+static void shader_sm4_read_metadata(struct vkd3d_shader_parser *parser)
+{
+    struct vkd3d_shader_sm4_parser *sm4 = vkd3d_shader_sm4_parser(parser);
+
+    if (parse_dxbc(parser->shader->code, parser->shader->size, parser->message_context,
+            parser->location.source_name, metadata_handler, sm4))
+        ERR("Failed to parse metadata.\n");
+}
+
 static void shader_sm4_read_instruction(struct vkd3d_shader_parser *parser, struct vkd3d_shader_instruction *ins)
 {
     struct vkd3d_shader_sm4_parser *sm4 = vkd3d_shader_sm4_parser(parser);
@@ -1582,12 +1683,13 @@ static const struct vkd3d_shader_parser_ops shader_sm4_parser_ops =
 {
     .parser_reset = shader_sm4_reset,
     .parser_destroy = shader_sm4_destroy,
+    .parser_read_metadata = shader_sm4_read_metadata,
     .parser_read_instruction = shader_sm4_read_instruction,
     .parser_is_end = shader_sm4_is_end,
 };

-static bool shader_sm4_init(struct vkd3d_shader_sm4_parser *sm4, const uint32_t *byte_code,
-        size_t byte_code_size, const char *source_name, const struct vkd3d_shader_signature *output_signature,
+static bool shader_sm4_init(struct vkd3d_shader_sm4_parser *sm4, const uint32_t *byte_code, size_t byte_code_size,
+        const struct vkd3d_shader_compile_info *compile_info, const struct vkd3d_shader_signature *output_signature,
         struct vkd3d_shader_message_context *message_context)
 {
     struct vkd3d_shader_version version;
@@ -1646,7 +1748,7 @@ static bool shader_sm4_init(struct vkd3d_shader_sm4_parser *sm4, const uint32_t
     version.major = VKD3D_SM4_VERSION_MAJOR(version_token);
     version.minor = VKD3D_SM4_VERSION_MINOR(version_token);

-    vkd3d_shader_parser_init(&sm4->p, message_context, source_name, &version, &shader_sm4_parser_ops);
+    vkd3d_shader_parser_init(&sm4->p, message_context, compile_info, &version, &shader_sm4_parser_ops);
     sm4->p.ptr = sm4->start;

memset(sm4->output_map, 0xff, sizeof(sm4->output_map));
@@ -1669,55 +1771,6 @@ static bool shader_sm4_init(struct vkd3d_shader_sm4_parser *sm4, const uint32_t
     return true;
 }

-static bool require_space(size_t offset, size_t count, size_t size, size_t data_size)
-{
-    return !count || (data_size - offset) / count >= size;
-}
-
-static void read_dword(const char **ptr, DWORD *d)
-{
-    memcpy(d, *ptr, sizeof(*d));
-    *ptr += sizeof(*d);
-}
-
-static void read_float(const char **ptr, float *f)
-{
-    STATIC_ASSERT(sizeof(float) == sizeof(DWORD));
-    read_dword(ptr, (DWORD *)f);
-}
-
-static void skip_dword_unknown(const char **ptr, unsigned int count)
-{
-    unsigned int i;
-    DWORD d;
-
-    WARN("Skipping %u unknown DWORDs:\n", count);
-    for (i = 0; i < count; ++i)
-    {
-        read_dword(ptr, &d);
-        WARN("\t0x%08x\n", d);
-    }
-}
-
-static const char *shader_get_string(const char *data, size_t data_size, DWORD offset)
-{
-    size_t len, max_len;
-
-    if (offset >= data_size)
-    {
-        WARN("Invalid offset %#x (data size %#lx).\n", offset, (long)data_size);
-        return NULL;
-    }
-
-    max_len = data_size - offset;
-    len = strnlen(data + offset, max_len);
-
-    if (len == max_len)
-        return NULL;
-
-    return data + offset;
-}
-
 static int parse_dxbc(const char *data, size_t data_size,
         struct vkd3d_shader_message_context *message_context, const char *source_name,
         int (*chunk_handler)(const char *data, DWORD data_size, DWORD tag, void *ctx), void *ctx)
@@ -2050,7 +2103,7 @@ int vkd3d_shader_sm4_parser_create(const struct vkd3d_shader_compile_info *compi
     }

if (!shader_sm4_init(sm4, shader_desc->byte_code, shader_desc->byte_code_size,
-            compile_info->source_name, &shader_desc->output_signature, message_context))
+            compile_info, &shader_desc->output_signature, message_context))
     {
         WARN("Failed to initialise shader parser.\n");
         free_shader_desc(shader_desc);
diff --git a/libs/vkd3d-shader/vkd3d_shader.map b/libs/vkd3d-shader/vkd3d_shader.map
index 2e49fe248..1127391cf 100644
--- a/libs/vkd3d-shader/vkd3d_shader.map
+++ b/libs/vkd3d-shader/vkd3d_shader.map
@@ -7,6 +7,7 @@ global:
     vkd3d_shader_free_messages;
     vkd3d_shader_free_root_signature;
     vkd3d_shader_free_scan_descriptor_info;
+    vkd3d_shader_free_scan_function_info;
     vkd3d_shader_free_shader_code;
     vkd3d_shader_free_shader_signature;
     vkd3d_shader_get_supported_source_types;
diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c
index 7e82f18db..307ffce70 100644
--- a/libs/vkd3d-shader/vkd3d_shader_main.c
+++ b/libs/vkd3d-shader/vkd3d_shader_main.c
@@ -365,11 +365,12 @@ void vkd3d_shader_dump_shader(enum vkd3d_shader_source_type source_type,
 }

void vkd3d_shader_parser_init(struct vkd3d_shader_parser *parser,
-        struct vkd3d_shader_message_context *message_context, const char *source_name,
+        struct vkd3d_shader_message_context *message_context, const struct vkd3d_shader_compile_info *compile_info,
         const struct vkd3d_shader_version *version, const struct vkd3d_shader_parser_ops *ops)
 {
+    parser->shader = &compile_info->source;
     parser->message_context = message_context;
-    parser->location.source_name = source_name;
+    parser->location.source_name = compile_info->source_name;
     parser->location.line = 1;
     parser->location.column = 0;
     parser->shader_version = *version;
@@ -983,7 +984,15 @@ static int scan_with_parser(const struct vkd3d_shader_compile_info *compile_info

if (func_info)
     {
+        vkd3d_shader_parser_read_metadata(parser);
+
         func_info->version = parser->shader_version;
+        func_info->creator = NULL;
+        if (parser->creator && !(func_info->creator = vkd3d_strdup(parser->creator)))
+        {
+            ret = VKD3D_ERROR_OUT_OF_MEMORY;
+            goto done;
+        }
     }

ret = VKD3D_OK;
@@ -1251,6 +1260,13 @@ void vkd3d_shader_free_scan_descriptor_info(struct vkd3d_shader_scan_descriptor_
     vkd3d_free(scan_descriptor_info->descriptors);
 }

+void vkd3d_shader_free_scan_function_info(struct vkd3d_shader_scan_function_info *info)
+{
+    TRACE("info %p.\n", info);
+
+    vkd3d_free((char *)info->creator);
+}
+
 void vkd3d_shader_free_shader_code(struct vkd3d_shader_code *shader_code)
 {
     TRACE("shader_code %p.\n", shader_code);
diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h
index c7ffa3c81..b4d856948 100644
--- a/libs/vkd3d-shader/vkd3d_shader_private.h
+++ b/libs/vkd3d-shader/vkd3d_shader_private.h
@@ -880,6 +880,7 @@ struct vkd3d_shader_location

struct vkd3d_shader_parser
 {
+    const struct vkd3d_shader_code *shader;
     struct vkd3d_shader_message_context *message_context;
     struct vkd3d_shader_location location;
     bool failed;
@@ -888,12 +889,15 @@ struct vkd3d_shader_parser
     struct vkd3d_shader_version shader_version;
     const uint32_t *ptr;
     const struct vkd3d_shader_parser_ops *ops;
+
+    const char *creator;
 };

struct vkd3d_shader_parser_ops
 {
     void (*parser_reset)(struct vkd3d_shader_parser *parser);
     void (*parser_destroy)(struct vkd3d_shader_parser *parser);
+    void (*parser_read_metadata)(struct vkd3d_shader_parser *parser);
     void (*parser_read_instruction)(struct vkd3d_shader_parser *parser, struct vkd3d_shader_instruction *instruction);
     bool (*parser_is_end)(struct vkd3d_shader_parser *parser);
 };
@@ -901,7 +905,7 @@ struct vkd3d_shader_parser_ops
 void vkd3d_shader_parser_error(struct vkd3d_shader_parser *parser,
         enum vkd3d_shader_error error, const char *format, ...) VKD3D_PRINTF_FUNC(3, 4);
 void vkd3d_shader_parser_init(struct vkd3d_shader_parser *parser,
-        struct vkd3d_shader_message_context *message_context, const char *source_name,
+        struct vkd3d_shader_message_context *message_context, const struct vkd3d_shader_compile_info *compile_info,
         const struct vkd3d_shader_version *version, const struct vkd3d_shader_parser_ops *ops);
 void vkd3d_shader_parser_warning(struct vkd3d_shader_parser *parser,
         enum vkd3d_shader_error error, const char *format, ...) VKD3D_PRINTF_FUNC(3, 4);
@@ -916,6 +920,11 @@ static inline bool vkd3d_shader_parser_is_end(struct vkd3d_shader_parser *parser
     return parser->ops->parser_is_end(parser);
 }

+static inline void vkd3d_shader_parser_read_metadata(struct vkd3d_shader_parser *parser)
+{
+    parser->ops->parser_read_metadata(parser);
+}
+
 static inline void vkd3d_shader_parser_read_instruction(struct vkd3d_shader_parser *parser,
         struct vkd3d_shader_instruction *instruction)
 {
diff --git a/tests/vkd3d_shader_api.c b/tests/vkd3d_shader_api.c
index 31eeb17f2..89455b627 100644
--- a/tests/vkd3d_shader_api.c
+++ b/tests/vkd3d_shader_api.c
@@ -356,6 +356,7 @@ static void test_scan_dxbc(void)
     ok(func_info.version.major == 4, "Got major version %u.\n", func_info.version.major);
     ok(func_info.version.minor == 1, "Got minor version %u.\n", func_info.version.minor);

+    vkd3d_shader_free_scan_function_info(&func_info);
     vkd3d_shader_free_shader_code(&dxbc);
 }

[PATCH vkd3d 4/4] vkd3d-shader: Add support for retrieving the creator string from DXBC shaders.

[Henri Verbeet]

On Tue, 28 Dec 2021 at 23:20, Zebediah Figura  wrote:
> struct vkd3d_shader_parser_ops
>  {
>      void (*parser_reset)(struct vkd3d_shader_parser *parser);
>      void (*parser_destroy)(struct vkd3d_shader_parser *parser);
> - void (*parser_read_metadata)(struct vkd3d_shader_parser *parser);
> void (*parser_read_instruction)(struct vkd3d_shader_parser *parser, struct vkd3d_shader_instruction *instruction);
> bool (*parser_is_end)(struct vkd3d_shader_parser *parser);
>
> };

I don't feel strongly one way or another at this point, but should
parser_read_metadata() perhaps just return the metadata to the caller,
instead of storing it in the vkd3d_shader_parser structure?

[PATCH vkd3d 4/4] vkd3d-shader: Add support for retrieving the creator string from DXBC shaders.

[Zebediah Figura (she/her)]

On 1/6/22 04:15, Henri Verbeet wrote:
> On Tue, 28 Dec 2021 at 23:20, Zebediah Figura  wrote:
> > struct vkd3d_shader_parser_ops
> >   {
> >       void (*parser_reset)(struct vkd3d_shader_parser *parser);
> >       void (*parser_destroy)(struct vkd3d_shader_parser *parser);
> > - void (*parser_read_metadata)(struct vkd3d_shader_parser *parser);
> >  void (*parser_read_instruction)(struct vkd3d_shader_parser *parser, struct vkd3d_shader_instruction *instruction);
> >  bool (*parser_is_end)(struct vkd3d_shader_parser *parser);
> > };
>
> I don't feel strongly one way or another at this point, but should
> parser_read_metadata() perhaps just return the metadata to the caller,
> instead of storing it in the vkd3d_shader_parser structure?

I haven't written much more than these patches so far, but I think that 
makes sense.

[Henri Verbeet]

Signed-off-by: Henri Verbeet