[PATCH v4 0/6] MR2577: d3dcompiler: Use the vkd3d-shader DXBC API.

[Henri Verbeet (＠hverbeet)]

The main advantage is that this way we're getting valid DXBC checksums for DXBC blobs generated by d3dcompiler. See also https://bugs.winehq.org/show_bug.cgi?id=54464.

--
  v4: d3dcompiler: Use vkd3d_shader_parse_dxbc() in d3dcompiler_shader_reflection_init().
      d3dcompiler: Use vkd3d_shader_parse_dxbc() in d3dcompiler_strip_shader().
      d3dcompiler: Use vkd3d_shader_parse_dxbc() in d3dcompiler_get_blob_part().

https://gitlab.winehq.org/wine/wine/-/merge_requests/2577

[PATCH v4 1/6] d3dcompiler: Store DXBC sections as vkd3d_shader_dxbc_section_desc structures.

[Henri Verbeet]

From: Henri Verbeet

---
 configure.ac                              |  2 +-
 dlls/d3d10/Makefile.in                    |  1 +
 dlls/d3dcompiler_43/blob.c                | 12 ++++++------
 dlls/d3dcompiler_43/compiler.c            |  2 --
 dlls/d3dcompiler_43/d3dcompiler_private.h | 11 +++--------
 dlls/d3dcompiler_43/reflection.c          | 17 +++++++++--------
 dlls/d3dcompiler_43/utils.c               | 16 ++++++++--------
 7 files changed, 28 insertions(+), 33 deletions(-)

diff --git a/configure.ac b/configure.ac
index d6161d7ec3b..2a86fb0c131 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1089,7 +1089,7 @@ then
         if test "$ac_cv_mingw_header_vkd3d_h" = "yes" -a "$ac_cv_mingw_header_vkd3d_shader_h" = "yes"
         then
             WINE_CHECK_MINGW_LIB(vkd3d,vkd3d_set_log_callback,[:],[:],[$VKD3D_PE_LIBS])
-            WINE_CHECK_MINGW_LIB(vkd3d-shader,vkd3d_shader_compile,[:],[:],[$VKD3D_PE_LIBS])
+            WINE_CHECK_MINGW_LIB(vkd3d-shader,vkd3d_shader_serialize_dxbc,[:],[:],[$VKD3D_PE_LIBS])
             if test "$ac_cv_mingw_lib_vkd3d" = "no" -o "$ac_cv_mingw_lib_vkd3d_shader" = "no"
             then
                 VKD3D_PE_CFLAGS=""
diff --git a/dlls/d3d10/Makefile.in b/dlls/d3d10/Makefile.in
index 1b869f47f6c..df46aa3dff4 100644
--- a/dlls/d3d10/Makefile.in
+++ b/dlls/d3d10/Makefile.in
@@ -3,6 +3,7 @@ IMPORTLIB = d3d10
 IMPORTS   = uuid d3d10core d3dcompiler dxgi
 EXTRADEFS = -DD3D_COMPILER_VERSION=0
 PARENTSRC = ../d3dcompiler_43
+EXTRAINCL = $(VKD3D_PE_CFLAGS)

C_SRCS = \
    d3d10_main.c \
diff --git a/dlls/d3dcompiler_43/blob.c b/dlls/d3dcompiler_43/blob.c
index c8c198fb520..216c2794f20 100644
--- a/dlls/d3dcompiler_43/blob.c
+++ b/dlls/d3dcompiler_43/blob.c
@@ -250,11 +250,11 @@ static HRESULT d3dcompiler_get_blob_part(const void *data, SIZE_T data_size, D3D

for (i = 0; i < src_dxbc.count; ++i)
     {
-        struct dxbc_section *section = &src_dxbc.sections[i];
+        const struct vkd3d_shader_dxbc_section_desc *section = &src_dxbc.sections[i];

if (check_blob_part(section->tag, part))
         {
-            hr = dxbc_add_section(&dst_dxbc, section->tag, section->data, section->data_size);
+            hr = dxbc_add_section(&dst_dxbc, section->tag, section->data.code, section->data.size);
             if (FAILED(hr))
             {
                 dxbc_destroy(&src_dxbc);
@@ -304,10 +304,10 @@ static HRESULT d3dcompiler_get_blob_part(const void *data, SIZE_T data_size, D3D
     if (count == 1 && (part == D3D_BLOB_DEBUG_INFO || part == D3D_BLOB_LEGACY_SHADER || part == D3D_BLOB_XNA_PREPASS_SHADER
             || part == D3D_BLOB_XNA_SHADER))
     {
-        hr = D3DCreateBlob(dst_dxbc.sections[0].data_size, blob);
+        hr = D3DCreateBlob(dst_dxbc.sections[0].data.size, blob);
         if (SUCCEEDED(hr))
         {
-            memcpy(ID3D10Blob_GetBufferPointer(*blob), dst_dxbc.sections[0].data, dst_dxbc.sections[0].data_size);
+            memcpy(ID3D10Blob_GetBufferPointer(*blob), dst_dxbc.sections[0].data.code, dst_dxbc.sections[0].data.size);
         }
         else
         {
@@ -391,11 +391,11 @@ static HRESULT d3dcompiler_strip_shader(const void *data, SIZE_T data_size, UINT

for (i = 0; i < src_dxbc.count; ++i)
     {
-        struct dxbc_section *section = &src_dxbc.sections[i];
+        const struct vkd3d_shader_dxbc_section_desc *section = &src_dxbc.sections[i];

if (check_blob_strip(section->tag, flags))
         {
-            hr = dxbc_add_section(&dst_dxbc, section->tag, section->data, section->data_size);
+            hr = dxbc_add_section(&dst_dxbc, section->tag, section->data.code, section->data.size);
             if (FAILED(hr))
             {
                 dxbc_destroy(&src_dxbc);
diff --git a/dlls/d3dcompiler_43/compiler.c b/dlls/d3dcompiler_43/compiler.c
index a7926785184..069ff992c22 100644
--- a/dlls/d3dcompiler_43/compiler.c
+++ b/dlls/d3dcompiler_43/compiler.c
@@ -25,8 +25,6 @@

#include "d3dcompiler_private.h"

-#include <vkd3d_shader.h>
-
 WINE_DEFAULT_DEBUG_CHANNEL(d3dcompiler);

static HRESULT hresult_from_vkd3d_result(int vkd3d_result)
diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
index 6dea5a08a85..0f4cd658ac5 100644
--- a/dlls/d3dcompiler_43/d3dcompiler_private.h
+++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
@@ -38,6 +38,8 @@
 #include <assert.h>
 #include <stdint.h>

+#include <vkd3d_shader.h>
+
 /*
  * This doesn't belong here, but for some functions it is possible to return that value,
  * see http://msdn.microsoft.com/en-us/library/bb205278%28v=VS.85%29.aspx
@@ -549,18 +551,11 @@ void SlDeleteShader(struct bwriter_shader *shader) DECLSPEC_HIDDEN;
 #define TAG_XNAP MAKE_TAG('X', 'N', 'A', 'P')
 #define TAG_XNAS MAKE_TAG('X', 'N', 'A', 'S')

-struct dxbc_section
-{
-    DWORD tag;
-    const char *data;
-    DWORD data_size;
-};
-
 struct dxbc
 {
     UINT size;
     UINT count;
-    struct dxbc_section *sections;
+    struct vkd3d_shader_dxbc_section_desc *sections;
 };

HRESULT dxbc_write_blob(struct dxbc *dxbc, ID3DBlob **blob) DECLSPEC_HIDDEN;
diff --git a/dlls/d3dcompiler_43/reflection.c b/dlls/d3dcompiler_43/reflection.c
index 0fa0dc16839..b83ede073cd 100644
--- a/dlls/d3dcompiler_43/reflection.c
+++ b/dlls/d3dcompiler_43/reflection.c
@@ -1681,12 +1681,13 @@ err_out:
     return hr;
 }

-static HRESULT d3dcompiler_parse_signature(struct d3dcompiler_shader_signature *s, struct dxbc_section *section)
+static HRESULT d3dcompiler_parse_signature(struct d3dcompiler_shader_signature *s,
+        const struct vkd3d_shader_dxbc_section_desc *section)
 {
     enum D3DCOMPILER_SIGNATURE_ELEMENT_SIZE element_size;
+    const char *ptr = section->data.code;
     D3D11_SIGNATURE_PARAMETER_DESC *d;
     unsigned int string_data_offset;
-    const char *ptr = section->data;
     unsigned int string_data_size;
     unsigned int i, count;
     char *string_data;
@@ -1723,7 +1724,7 @@ static HRESULT d3dcompiler_parse_signature(struct d3dcompiler_shader_signature *

/* 2 u32s for the header, element_size for each element. */
     string_data_offset = 2 * sizeof(uint32_t) + count * element_size * sizeof(uint32_t);
-    string_data_size = section->data_size - string_data_offset;
+    string_data_size = section->data.size - string_data_offset;

string_data = HeapAlloc(GetProcessHeap(), 0, string_data_size);
     if (!string_data)
@@ -1732,7 +1733,7 @@ static HRESULT d3dcompiler_parse_signature(struct d3dcompiler_shader_signature *
         HeapFree(GetProcessHeap(), 0, d);
         return E_OUTOFMEMORY;
     }
-    memcpy(string_data, section->data + string_data_offset, string_data_size);
+    memcpy(string_data, (const char *)section->data.code + string_data_offset, string_data_size);

for (i = 0; i < count; ++i)
     {
@@ -1876,12 +1877,12 @@ static HRESULT d3dcompiler_shader_reflection_init(struct d3dcompiler_shader_refl

for (i = 0; i < src_dxbc.count; ++i)
     {
-        struct dxbc_section *section = &src_dxbc.sections[i];
+        const struct vkd3d_shader_dxbc_section_desc *section = &src_dxbc.sections[i];

switch (section->tag)
         {
             case TAG_RDEF:
-                hr = d3dcompiler_parse_rdef(reflection, section->data, section->data_size);
+                hr = d3dcompiler_parse_rdef(reflection, section->data.code, section->data.size);
                 if (FAILED(hr))
                 {
                     WARN("Failed to parse RDEF section.\n");
@@ -1943,7 +1944,7 @@ static HRESULT d3dcompiler_shader_reflection_init(struct d3dcompiler_shader_refl

case TAG_SHEX:
             case TAG_SHDR:
-                hr = d3dcompiler_parse_shdr(reflection, section->data, section->data_size);
+                hr = d3dcompiler_parse_shdr(reflection, section->data.code, section->data.size);
                 if (FAILED(hr))
                 {
                     WARN("Failed to parse SHDR section.\n");
@@ -1952,7 +1953,7 @@ static HRESULT d3dcompiler_shader_reflection_init(struct d3dcompiler_shader_refl
                 break;

case TAG_STAT:
-                hr = d3dcompiler_parse_stat(reflection, section->data, section->data_size);
+                hr = d3dcompiler_parse_stat(reflection, section->data.code, section->data.size);
                 if (FAILED(hr))
                 {
                     WARN("Failed to parse section STAT.\n");
diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
index 9a6be8db923..9a8bdaccb1b 100644
--- a/dlls/d3dcompiler_43/utils.c
+++ b/dlls/d3dcompiler_43/utils.c
@@ -538,7 +538,7 @@ HRESULT dxbc_add_section(struct dxbc *dxbc, DWORD tag, const char *data, size_t

if (dxbc->count >= dxbc->size)
     {
-        struct dxbc_section *new_sections;
+        struct vkd3d_shader_dxbc_section_desc *new_sections;
         DWORD new_size = dxbc->size << 1;

new_sections = HeapReAlloc(GetProcessHeap(), 0, dxbc->sections, new_size * sizeof(*dxbc->sections));
@@ -553,8 +553,8 @@ HRESULT dxbc_add_section(struct dxbc *dxbc, DWORD tag, const char *data, size_t
     }

dxbc->sections[dxbc->count].tag = tag;
-    dxbc->sections[dxbc->count].data_size = data_size;
-    dxbc->sections[dxbc->count].data = data;
+    dxbc->sections[dxbc->count].data.size = data_size;
+    dxbc->sections[dxbc->count].data.code = data;
     ++dxbc->count;

return S_OK;
@@ -670,7 +670,7 @@ HRESULT dxbc_write_blob(struct dxbc *dxbc, ID3DBlob **blob)

for (i = 0; i < dxbc->count; ++i)
     {
-        size += 12 + dxbc->sections[i].data_size;
+        size += 12 + dxbc->sections[i].data.size;
     }

hr = D3DCreateBlob(size, &object);
@@ -703,16 +703,16 @@ HRESULT dxbc_write_blob(struct dxbc *dxbc, ID3DBlob **blob)
     for (i = 0; i < dxbc->count; ++i)
     {
         write_u32(&ptr, offset);
-        offset += 8 + dxbc->sections[i].data_size;
+        offset += 8 + dxbc->sections[i].data.size;
     }

/* write the chunks */
     for (i = 0; i < dxbc->count; ++i)
     {
         write_u32(&ptr, dxbc->sections[i].tag);
-        write_u32(&ptr, dxbc->sections[i].data_size);
-        memcpy(ptr, dxbc->sections[i].data, dxbc->sections[i].data_size);
-        ptr += dxbc->sections[i].data_size;
+        write_u32(&ptr, dxbc->sections[i].data.size);
+        memcpy(ptr, dxbc->sections[i].data.code, dxbc->sections[i].data.size);
+        ptr += dxbc->sections[i].data.size;
     }

TRACE("Created ID3DBlob %p\n", object);

[PATCH v4 2/6] d3dcompiler: Use vkd3d_shader_serialize_dxbc() in d3dcompiler_get_blob_part().

[Henri Verbeet]

From: Henri Verbeet

---
 dlls/d3d10/Makefile.in     |  2 +-
 dlls/d3dcompiler_43/blob.c | 85 +++++++++++++++++---------------------
 dlls/wined3d/wined3d.spec  |  1 +
 3 files changed, 40 insertions(+), 48 deletions(-)

diff --git a/dlls/d3d10/Makefile.in b/dlls/d3d10/Makefile.in
index df46aa3dff4..ed0e52def19 100644
--- a/dlls/d3d10/Makefile.in
+++ b/dlls/d3d10/Makefile.in
@@ -1,6 +1,6 @@
 MODULE    = d3d10.dll
 IMPORTLIB = d3d10
-IMPORTS   = uuid d3d10core d3dcompiler dxgi
+IMPORTS   = uuid d3d10core d3dcompiler dxgi wined3d
 EXTRADEFS = -DD3D_COMPILER_VERSION=0
 PARENTSRC = ../d3dcompiler_43
 EXTRAINCL = $(VKD3D_PE_CFLAGS)
diff --git a/dlls/d3dcompiler_43/blob.c b/dlls/d3dcompiler_43/blob.c
index 216c2794f20..765ab070890 100644
--- a/dlls/d3dcompiler_43/blob.c
+++ b/dlls/d3dcompiler_43/blob.c
@@ -216,9 +216,12 @@ static BOOL check_blob_part(DWORD tag, D3D_BLOB_PART part)

static HRESULT d3dcompiler_get_blob_part(const void *data, SIZE_T data_size, D3D_BLOB_PART part, UINT flags, ID3DBlob **blob)
 {
-    struct dxbc src_dxbc, dst_dxbc;
+    struct vkd3d_shader_dxbc_section_desc *sections;
+    struct vkd3d_shader_code dst_dxbc;
+    unsigned int section_count, i;
+    struct dxbc src_dxbc;
     HRESULT hr;
-    unsigned int i, count;
+    int ret;

if (!data || !data_size || flags || !blob)
     {
@@ -240,33 +243,21 @@ static HRESULT d3dcompiler_get_blob_part(const void *data, SIZE_T data_size, D3D
         return hr;
     }

-    hr = dxbc_init(&dst_dxbc, 0);
-    if (FAILED(hr))
+    if (!(sections = calloc(src_dxbc.count, sizeof(*sections))))
     {
+        ERR("Failed to allocate sections memory.\n");
         dxbc_destroy(&src_dxbc);
-        WARN("Failed to init dxbc\n");
-        return hr;
+        return E_OUTOFMEMORY;
     }

-    for (i = 0; i < src_dxbc.count; ++i)
+    for (i = 0, section_count = 0; i < src_dxbc.count; ++i)
     {
-        const struct vkd3d_shader_dxbc_section_desc *section = &src_dxbc.sections[i];
+        const struct vkd3d_shader_dxbc_section_desc *src_section = &src_dxbc.sections[i];

-        if (check_blob_part(section->tag, part))
-        {
-            hr = dxbc_add_section(&dst_dxbc, section->tag, section->data.code, section->data.size);
-            if (FAILED(hr))
-            {
-                dxbc_destroy(&src_dxbc);
-                dxbc_destroy(&dst_dxbc);
-                WARN("Failed to add section to dxbc\n");
-                return hr;
-            }
-        }
+        if (check_blob_part(src_section->tag, part))
+            sections[section_count++] = *src_section;
     }

-    count = dst_dxbc.count;
-
     switch(part)
     {
         case D3D_BLOB_INPUT_SIGNATURE_BLOB:
@@ -276,15 +267,18 @@ static HRESULT d3dcompiler_get_blob_part(const void *data, SIZE_T data_size, D3D
         case D3D_BLOB_LEGACY_SHADER:
         case D3D_BLOB_XNA_PREPASS_SHADER:
         case D3D_BLOB_XNA_SHADER:
-            if (count != 1) count = 0;
+            if (section_count != 1)
+                section_count = 0;
             break;

case D3D_BLOB_INPUT_AND_OUTPUT_SIGNATURE_BLOB:
-            if (count != 2) count = 0;
+            if (section_count != 2)
+                section_count = 0;
             break;

case D3D_BLOB_ALL_SIGNATURE_BLOB:
-            if (count != 3) count = 0;
+            if (section_count != 3)
+                section_count = 0;
             break;

default:
@@ -292,39 +286,36 @@ static HRESULT d3dcompiler_get_blob_part(const void *data, SIZE_T data_size, D3D
             break;
     }

-    if (count == 0)
+    if (!section_count)
     {
-        dxbc_destroy(&src_dxbc);
-        dxbc_destroy(&dst_dxbc);
-        WARN("Nothing to write into the blob (count = 0)\n");
-        return E_FAIL;
+        WARN("Nothing to write into the blob.\n");
+        hr = E_FAIL;
+        goto done;
     }

/* some parts aren't full DXBCs, they contain only the data */
-    if (count == 1 && (part == D3D_BLOB_DEBUG_INFO || part == D3D_BLOB_LEGACY_SHADER || part == D3D_BLOB_XNA_PREPASS_SHADER
-            || part == D3D_BLOB_XNA_SHADER))
+    if (section_count == 1 && (part == D3D_BLOB_DEBUG_INFO || part == D3D_BLOB_LEGACY_SHADER
+            || part == D3D_BLOB_XNA_PREPASS_SHADER || part == D3D_BLOB_XNA_SHADER))
     {
-        hr = D3DCreateBlob(dst_dxbc.sections[0].data.size, blob);
-        if (SUCCEEDED(hr))
-        {
-            memcpy(ID3D10Blob_GetBufferPointer(*blob), dst_dxbc.sections[0].data.code, dst_dxbc.sections[0].data.size);
-        }
-        else
-        {
-            WARN("Could not create blob\n");
-        }
+        dst_dxbc = sections[0].data;
     }
-    else
+    else if ((ret = vkd3d_shader_serialize_dxbc(section_count, sections, &dst_dxbc, NULL) < 0))
     {
-        hr = dxbc_write_blob(&dst_dxbc, blob);
-        if (FAILED(hr))
-        {
-            WARN("Failed to write blob part\n");
-        }
+        WARN("Failed to serialise DXBC, ret %d.\n", ret);
+        hr = E_FAIL;
+        goto done;
     }

+    if (FAILED(hr = D3DCreateBlob(dst_dxbc.size, blob)))
+        WARN("Failed to create blob, hr %#lx.\n", hr);
+    else
+        memcpy(ID3D10Blob_GetBufferPointer(*blob), dst_dxbc.code, dst_dxbc.size);
+    if (dst_dxbc.code != sections[0].data.code)
+        vkd3d_shader_free_shader_code(&dst_dxbc);
+
+done:
+    free(sections);
     dxbc_destroy(&src_dxbc);
-    dxbc_destroy(&dst_dxbc);

return hr;
 }
diff --git a/dlls/wined3d/wined3d.spec b/dlls/wined3d/wined3d.spec
index 8560c1a1bb6..4838f105872 100644
--- a/dlls/wined3d/wined3d.spec
+++ b/dlls/wined3d/wined3d.spec
@@ -359,4 +359,5 @@
 @ cdecl vkd3d_shader_parse_root_signature(ptr ptr ptr)
 @ cdecl vkd3d_shader_preprocess(ptr ptr ptr)
 @ cdecl vkd3d_shader_scan(ptr ptr)
+@ cdecl vkd3d_shader_serialize_dxbc(long ptr ptr ptr)
 @ cdecl vkd3d_shader_serialize_root_signature(ptr ptr ptr)

[PATCH v4 3/6] d3dcompiler: Use vkd3d_shader_serialize_dxbc() in d3dcompiler_strip_shader().

[Henri Verbeet]

From: Henri Verbeet

Note that this adds a todo_wine to test_get_blob_part2(). It looks like native
d3dcompiler only adds padding between sections, while vkd3d-shader always adds
padding to the end of sections. I've sent a vkd3d-shader patch to fix that,
but the extra padding at the end of the DXBC blob doesn't seem terribly
concerning.
---
 dlls/d3dcompiler_43/blob.c                | 49 ++++++++--------
 dlls/d3dcompiler_43/d3dcompiler_private.h |  7 ---
 dlls/d3dcompiler_43/tests/blob.c          |  2 +-
 dlls/d3dcompiler_43/utils.c               | 70 -----------------------
 4 files changed, 26 insertions(+), 102 deletions(-)

diff --git a/dlls/d3dcompiler_43/blob.c b/dlls/d3dcompiler_43/blob.c
index 765ab070890..2ac087b6815 100644
--- a/dlls/d3dcompiler_43/blob.c
+++ b/dlls/d3dcompiler_43/blob.c
@@ -348,9 +348,12 @@ static BOOL check_blob_strip(DWORD tag, UINT flags)

static HRESULT d3dcompiler_strip_shader(const void *data, SIZE_T data_size, UINT flags, ID3DBlob **blob)
 {
-    struct dxbc src_dxbc, dst_dxbc;
+    struct vkd3d_shader_dxbc_section_desc *sections;
+    struct vkd3d_shader_code dst_dxbc;
+    unsigned int section_count, i;
+    struct dxbc src_dxbc;
     HRESULT hr;
-    unsigned int i;
+    int ret;

if (!blob)
     {
@@ -372,39 +375,37 @@ static HRESULT d3dcompiler_strip_shader(const void *data, SIZE_T data_size, UINT
     }

/* src_dxbc.count >= dst_dxbc.count */
-    hr = dxbc_init(&dst_dxbc, src_dxbc.count);
-    if (FAILED(hr))
+    if (!(sections = calloc(src_dxbc.count, sizeof(*sections))))
     {
+        ERR("Failed to allocate sections memory.\n");
         dxbc_destroy(&src_dxbc);
-        WARN("Failed to init dxbc\n");
-        return hr;
+        return E_OUTOFMEMORY;
     }

-    for (i = 0; i < src_dxbc.count; ++i)
+    for (i = 0, section_count = 0; i < src_dxbc.count; ++i)
     {
-        const struct vkd3d_shader_dxbc_section_desc *section = &src_dxbc.sections[i];
-
-        if (check_blob_strip(section->tag, flags))
-        {
-            hr = dxbc_add_section(&dst_dxbc, section->tag, section->data.code, section->data.size);
-            if (FAILED(hr))
-            {
-                dxbc_destroy(&src_dxbc);
-                dxbc_destroy(&dst_dxbc);
-                WARN("Failed to add section to dxbc\n");
-                return hr;
-            }
-        }
+        const struct vkd3d_shader_dxbc_section_desc *src_section = &src_dxbc.sections[i];
+
+        if (check_blob_strip(src_section->tag, flags))
+            sections[section_count++] = *src_section;
     }

-    hr = dxbc_write_blob(&dst_dxbc, blob);
-    if (FAILED(hr))
+    if ((ret = vkd3d_shader_serialize_dxbc(section_count, sections, &dst_dxbc, NULL) < 0))
     {
-        WARN("Failed to write blob part\n");
+        WARN("Failed to serialise DXBC, ret %d.\n", ret);
+        hr = E_FAIL;
+        goto done;
     }

+    if (FAILED(hr = D3DCreateBlob(dst_dxbc.size, blob)))
+        WARN("Failed to create blob, hr %#lx.\n", hr);
+    else
+        memcpy(ID3D10Blob_GetBufferPointer(*blob), dst_dxbc.code, dst_dxbc.size);
+    vkd3d_shader_free_shader_code(&dst_dxbc);
+
+done:
+    free(sections);
     dxbc_destroy(&src_dxbc);
-    dxbc_destroy(&dst_dxbc);

return hr;
 }
diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
index 0f4cd658ac5..00e76751e0f 100644
--- a/dlls/d3dcompiler_43/d3dcompiler_private.h
+++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
@@ -558,16 +558,9 @@ struct dxbc
     struct vkd3d_shader_dxbc_section_desc *sections;
 };

-HRESULT dxbc_write_blob(struct dxbc *dxbc, ID3DBlob **blob) DECLSPEC_HIDDEN;
 void dxbc_destroy(struct dxbc *dxbc) DECLSPEC_HIDDEN;
 HRESULT dxbc_parse(const char *data, SIZE_T data_size, struct dxbc *dxbc) DECLSPEC_HIDDEN;
 HRESULT dxbc_add_section(struct dxbc *dxbc, DWORD tag, const char *data, size_t data_size) DECLSPEC_HIDDEN;
 HRESULT dxbc_init(struct dxbc *dxbc, unsigned int size) DECLSPEC_HIDDEN;

-static inline void write_u32(char **ptr, uint32_t u32)
-{
-    memcpy(*ptr, &u32, sizeof(u32));
-    *ptr += sizeof(u32);
-}
-
 #endif /* __WINE_D3DCOMPILER_PRIVATE_H */
diff --git a/dlls/d3dcompiler_43/tests/blob.c b/dlls/d3dcompiler_43/tests/blob.c
index 90e4ef3d7da..63e045bd06c 100644
--- a/dlls/d3dcompiler_43/tests/blob.c
+++ b/dlls/d3dcompiler_43/tests/blob.c
@@ -747,7 +747,7 @@ static void test_get_blob_part2(void)
     ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);

size = ID3D10Blob_GetBufferSize(blob);
-    ok(size == 4735, "Got unexpected size %Iu.\n", size);
+    todo_wine ok(size == 4735, "Got unexpected size %Iu.\n", size);

dword = ((DWORD*)ID3D10Blob_GetBufferPointer(blob));
     ok(TAG_DXBC == *dword, "DXBC got %#lx, expected %#lx.\n", *dword, TAG_DXBC);
diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
index 9a8bdaccb1b..7a5a6bbcd61 100644
--- a/dlls/d3dcompiler_43/utils.c
+++ b/dlls/d3dcompiler_43/utils.c
@@ -526,12 +526,6 @@ void skip_u32_unknown(const char **ptr, unsigned int count)
     }
 }

-static void write_u32_unknown(char **ptr, uint32_t u32)
-{
-    FIXME("Writing unknown u32 0x%08x.\n", u32);
-    write_u32(ptr, u32);
-}
-
 HRESULT dxbc_add_section(struct dxbc *dxbc, DWORD tag, const char *data, size_t data_size)
 {
     TRACE("dxbc %p, tag %s, size %#Ix.\n", dxbc, debugstr_an((const char *)&tag, 4), data_size);
@@ -658,70 +652,6 @@ void dxbc_destroy(struct dxbc *dxbc)
     HeapFree(GetProcessHeap(), 0, dxbc->sections);
 }

-HRESULT dxbc_write_blob(struct dxbc *dxbc, ID3DBlob **blob)
-{
-    DWORD size = 32, offset = size + 4 * dxbc->count;
-    ID3DBlob *object;
-    HRESULT hr;
-    char *ptr;
-    unsigned int i;
-
-    TRACE("dxbc %p, blob %p.\n", dxbc, blob);
-
-    for (i = 0; i < dxbc->count; ++i)
-    {
-        size += 12 + dxbc->sections[i].data.size;
-    }
-
-    hr = D3DCreateBlob(size, &object);
-    if (FAILED(hr))
-    {
-        WARN("Failed to create blob\n");
-        return hr;
-    }
-
-    ptr = ID3D10Blob_GetBufferPointer(object);
-
-    write_u32(&ptr, TAG_DXBC);
-
-    /* signature(?) */
-    write_u32_unknown(&ptr, 0);
-    write_u32_unknown(&ptr, 0);
-    write_u32_unknown(&ptr, 0);
-    write_u32_unknown(&ptr, 0);
-
-    /* seems to be always 1 */
-    write_u32_unknown(&ptr, 1);
-
-    /* DXBC size */
-    write_u32(&ptr, size);
-
-    /* chunk count */
-    write_u32(&ptr, dxbc->count);
-
-    /* write the chunk offsets */
-    for (i = 0; i < dxbc->count; ++i)
-    {
-        write_u32(&ptr, offset);
-        offset += 8 + dxbc->sections[i].data.size;
-    }
-
-    /* write the chunks */
-    for (i = 0; i < dxbc->count; ++i)
-    {
-        write_u32(&ptr, dxbc->sections[i].tag);
-        write_u32(&ptr, dxbc->sections[i].data.size);
-        memcpy(ptr, dxbc->sections[i].data.code, dxbc->sections[i].data.size);
-        ptr += dxbc->sections[i].data.size;
-    }
-
-    TRACE("Created ID3DBlob %p\n", object);
-
-    *blob = object;
-
-    return S_OK;
-}
-
 void compilation_message(struct compilation_messages *msg, const char *fmt, va_list args)
 {
     char* buffer;

[PATCH v4 4/6] d3dcompiler: Use vkd3d_shader_parse_dxbc() in d3dcompiler_get_blob_part().

[Henri Verbeet]

From: Henri Verbeet

---
 dlls/d3dcompiler_43/blob.c                | 22 +++++++++++-----------
 dlls/d3dcompiler_43/d3dcompiler_private.h |  2 ++
 dlls/wined3d/wined3d.spec                 |  2 ++
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/dlls/d3dcompiler_43/blob.c b/dlls/d3dcompiler_43/blob.c
index 2ac087b6815..49ec138dc99 100644
--- a/dlls/d3dcompiler_43/blob.c
+++ b/dlls/d3dcompiler_43/blob.c
@@ -216,14 +216,15 @@ static BOOL check_blob_part(DWORD tag, D3D_BLOB_PART part)

static HRESULT d3dcompiler_get_blob_part(const void *data, SIZE_T data_size, D3D_BLOB_PART part, UINT flags, ID3DBlob **blob)
 {
+    const struct vkd3d_shader_code src_dxbc = {.code = data, .size = data_size};
     struct vkd3d_shader_dxbc_section_desc *sections;
+    struct vkd3d_shader_dxbc_desc src_dxbc_desc;
     struct vkd3d_shader_code dst_dxbc;
     unsigned int section_count, i;
-    struct dxbc src_dxbc;
     HRESULT hr;
     int ret;

-    if (!data || !data_size || flags || !blob)
+    if (!data || data_size < DXBC_HEADER_SIZE || flags || !blob)
     {
         WARN("Invalid arguments: data %p, data_size %Iu, flags %#x, blob %p.\n", data, data_size, flags, blob);
         return D3DERR_INVALIDCALL;
@@ -236,23 +237,22 @@ static HRESULT d3dcompiler_get_blob_part(const void *data, SIZE_T data_size, D3D
         return D3DERR_INVALIDCALL;
     }

-    hr = dxbc_parse(data, data_size, &src_dxbc);
-    if (FAILED(hr))
+    if ((ret = vkd3d_shader_parse_dxbc(&src_dxbc, 0, &src_dxbc_desc, NULL)) < 0)
     {
-        WARN("Failed to parse blob part\n");
-        return hr;
+        WARN("Failed to parse source data, ret %d.\n", ret);
+        return E_FAIL;
     }

-    if (!(sections = calloc(src_dxbc.count, sizeof(*sections))))
+    if (!(sections = calloc(src_dxbc_desc.section_count, sizeof(*sections))))
     {
         ERR("Failed to allocate sections memory.\n");
-        dxbc_destroy(&src_dxbc);
+        vkd3d_shader_free_dxbc(&src_dxbc_desc);
         return E_OUTOFMEMORY;
     }

-    for (i = 0, section_count = 0; i < src_dxbc.count; ++i)
+    for (i = 0, section_count = 0; i < src_dxbc_desc.section_count; ++i)
     {
-        const struct vkd3d_shader_dxbc_section_desc *src_section = &src_dxbc.sections[i];
+        const struct vkd3d_shader_dxbc_section_desc *src_section = &src_dxbc_desc.sections[i];

if (check_blob_part(src_section->tag, part))
             sections[section_count++] = *src_section;
@@ -315,7 +315,7 @@ static HRESULT d3dcompiler_get_blob_part(const void *data, SIZE_T data_size, D3D

done:
     free(sections);
-    dxbc_destroy(&src_dxbc);
+    vkd3d_shader_free_dxbc(&src_dxbc_desc);

return hr;
 }
diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
index 00e76751e0f..59eaec5cf9a 100644
--- a/dlls/d3dcompiler_43/d3dcompiler_private.h
+++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
@@ -534,6 +534,8 @@ struct bwriter_shader *SlAssembleShader(const char *text, char **messages) DECLS
 HRESULT shader_write_bytecode(const struct bwriter_shader *shader, uint32_t **result, uint32_t *size) DECLSPEC_HIDDEN;
 void SlDeleteShader(struct bwriter_shader *shader) DECLSPEC_HIDDEN;

+#define DXBC_HEADER_SIZE (8 * sizeof(uint32_t))
+
 #define MAKE_TAG(ch0, ch1, ch2, ch3) \
     ((DWORD)(ch0) | ((DWORD)(ch1) << 8) | \
     ((DWORD)(ch2) << 16) | ((DWORD)(ch3) << 24 ))
diff --git a/dlls/wined3d/wined3d.spec b/dlls/wined3d/wined3d.spec
index 4838f105872..5a406377bf5 100644
--- a/dlls/wined3d/wined3d.spec
+++ b/dlls/wined3d/wined3d.spec
@@ -347,6 +347,7 @@
 @ cdecl vkd3d_shader_compile(ptr ptr ptr)
 @ cdecl vkd3d_shader_convert_root_signature(ptr long ptr)
 @ cdecl vkd3d_shader_find_signature_element(ptr ptr long long)
+@ cdecl vkd3d_shader_free_dxbc(ptr)
 @ cdecl vkd3d_shader_free_messages(ptr)
 @ cdecl vkd3d_shader_free_root_signature(ptr)
 @ cdecl vkd3d_shader_free_scan_descriptor_info(ptr)
@@ -355,6 +356,7 @@
 @ cdecl vkd3d_shader_get_supported_source_types(ptr)
 @ cdecl vkd3d_shader_get_supported_target_types(long ptr)
 @ cdecl vkd3d_shader_get_version(ptr ptr)
+@ cdecl vkd3d_shader_parse_dxbc(ptr long ptr ptr)
 @ cdecl vkd3d_shader_parse_input_signature(ptr ptr ptr)
 @ cdecl vkd3d_shader_parse_root_signature(ptr ptr ptr)
 @ cdecl vkd3d_shader_preprocess(ptr ptr ptr)

[PATCH v4 5/6] d3dcompiler: Use vkd3d_shader_parse_dxbc() in d3dcompiler_strip_shader().

[Henri Verbeet]

From: Henri Verbeet

---
 dlls/d3dcompiler_43/blob.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/dlls/d3dcompiler_43/blob.c b/dlls/d3dcompiler_43/blob.c
index 49ec138dc99..bba1c6f590b 100644
--- a/dlls/d3dcompiler_43/blob.c
+++ b/dlls/d3dcompiler_43/blob.c
@@ -348,10 +348,11 @@ static BOOL check_blob_strip(DWORD tag, UINT flags)

static HRESULT d3dcompiler_strip_shader(const void *data, SIZE_T data_size, UINT flags, ID3DBlob **blob)
 {
+    const struct vkd3d_shader_code src_dxbc = {.code = data, .size = data_size};
     struct vkd3d_shader_dxbc_section_desc *sections;
+    struct vkd3d_shader_dxbc_desc src_dxbc_desc;
     struct vkd3d_shader_code dst_dxbc;
     unsigned int section_count, i;
-    struct dxbc src_dxbc;
     HRESULT hr;
     int ret;

@@ -361,30 +362,29 @@ static HRESULT d3dcompiler_strip_shader(const void *data, SIZE_T data_size, UINT
         return E_FAIL;
     }

-    if (!data || !data_size)
+    if (!data || data_size < DXBC_HEADER_SIZE)
     {
         WARN("Invalid arguments: data %p, data_size %Iu.\n", data, data_size);
         return D3DERR_INVALIDCALL;
     }

-    hr = dxbc_parse(data, data_size, &src_dxbc);
-    if (FAILED(hr))
+    if ((ret = vkd3d_shader_parse_dxbc(&src_dxbc, 0, &src_dxbc_desc, NULL)) < 0)
     {
-        WARN("Failed to parse blob part\n");
-        return hr;
+        WARN("Failed to parse source data, ret %d.\n", ret);
+        return E_FAIL;
     }

/* src_dxbc.count >= dst_dxbc.count */
-    if (!(sections = calloc(src_dxbc.count, sizeof(*sections))))
+    if (!(sections = calloc(src_dxbc_desc.section_count, sizeof(*sections))))
     {
         ERR("Failed to allocate sections memory.\n");
-        dxbc_destroy(&src_dxbc);
+        vkd3d_shader_free_dxbc(&src_dxbc_desc);
         return E_OUTOFMEMORY;
     }

-    for (i = 0, section_count = 0; i < src_dxbc.count; ++i)
+    for (i = 0, section_count = 0; i < src_dxbc_desc.section_count; ++i)
     {
-        const struct vkd3d_shader_dxbc_section_desc *src_section = &src_dxbc.sections[i];
+        const struct vkd3d_shader_dxbc_section_desc *src_section = &src_dxbc_desc.sections[i];

if (check_blob_strip(src_section->tag, flags))
             sections[section_count++] = *src_section;
@@ -405,7 +405,7 @@ static HRESULT d3dcompiler_strip_shader(const void *data, SIZE_T data_size, UINT

done:
     free(sections);
-    dxbc_destroy(&src_dxbc);
+    vkd3d_shader_free_dxbc(&src_dxbc_desc);

return hr;
 }

[PATCH v4 6/6] d3dcompiler: Use vkd3d_shader_parse_dxbc() in d3dcompiler_shader_reflection_init().

[Henri Verbeet]

From: Henri Verbeet

---
 dlls/d3dcompiler_43/d3dcompiler_private.h |  12 ---
 dlls/d3dcompiler_43/reflection.c          |  21 ++--
 dlls/d3dcompiler_43/utils.c               | 126 ----------------------
 3 files changed, 11 insertions(+), 148 deletions(-)

diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
index 59eaec5cf9a..12db34891e0 100644
--- a/dlls/d3dcompiler_43/d3dcompiler_private.h
+++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
@@ -553,16 +553,4 @@ void SlDeleteShader(struct bwriter_shader *shader) DECLSPEC_HIDDEN;
 #define TAG_XNAP MAKE_TAG('X', 'N', 'A', 'P')
 #define TAG_XNAS MAKE_TAG('X', 'N', 'A', 'S')

-struct dxbc
-{
-    UINT size;
-    UINT count;
-    struct vkd3d_shader_dxbc_section_desc *sections;
-};
-
-void dxbc_destroy(struct dxbc *dxbc) DECLSPEC_HIDDEN;
-HRESULT dxbc_parse(const char *data, SIZE_T data_size, struct dxbc *dxbc) DECLSPEC_HIDDEN;
-HRESULT dxbc_add_section(struct dxbc *dxbc, DWORD tag, const char *data, size_t data_size) DECLSPEC_HIDDEN;
-HRESULT dxbc_init(struct dxbc *dxbc, unsigned int size) DECLSPEC_HIDDEN;
-
 #endif /* __WINE_D3DCOMPILER_PRIVATE_H */
diff --git a/dlls/d3dcompiler_43/reflection.c b/dlls/d3dcompiler_43/reflection.c
index b83ede073cd..e4080da8c21 100644
--- a/dlls/d3dcompiler_43/reflection.c
+++ b/dlls/d3dcompiler_43/reflection.c
@@ -1862,22 +1862,23 @@ static HRESULT d3dcompiler_parse_shdr(struct d3dcompiler_shader_reflection *r, c
 static HRESULT d3dcompiler_shader_reflection_init(struct d3dcompiler_shader_reflection *reflection,
         const void *data, SIZE_T data_size)
 {
-    struct dxbc src_dxbc;
-    HRESULT hr;
+    const struct vkd3d_shader_code src_dxbc = {.code = data, .size = data_size};
+    struct vkd3d_shader_dxbc_desc src_dxbc_desc;
+    HRESULT hr = S_OK;
     unsigned int i;
+    int ret;

wine_rb_init(&reflection->types, d3dcompiler_shader_reflection_type_compare);

-    hr = dxbc_parse(data, data_size, &src_dxbc);
-    if (FAILED(hr))
+    if ((ret = vkd3d_shader_parse_dxbc(&src_dxbc, 0, &src_dxbc_desc, NULL)) < 0)
     {
-        WARN("Failed to parse reflection\n");
-        return hr;
+        WARN("Failed to parse reflection, ret %d.\n", ret);
+        return E_FAIL;
     }

-    for (i = 0; i < src_dxbc.count; ++i)
+    for (i = 0; i < src_dxbc_desc.section_count; ++i)
     {
-        const struct vkd3d_shader_dxbc_section_desc *section = &src_dxbc.sections[i];
+        const struct vkd3d_shader_dxbc_section_desc *section = &src_dxbc_desc.sections[i];

switch (section->tag)
         {
@@ -1967,13 +1968,13 @@ static HRESULT d3dcompiler_shader_reflection_init(struct d3dcompiler_shader_refl
         }
     }

-    dxbc_destroy(&src_dxbc);
+    vkd3d_shader_free_dxbc(&src_dxbc_desc);

return hr;

err_out:
     reflection_cleanup(reflection);
-    dxbc_destroy(&src_dxbc);
+    vkd3d_shader_free_dxbc(&src_dxbc_desc);

return hr;
 }
diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
index 7a5a6bbcd61..ceeb41048e2 100644
--- a/dlls/d3dcompiler_43/utils.c
+++ b/dlls/d3dcompiler_43/utils.c
@@ -526,132 +526,6 @@ void skip_u32_unknown(const char **ptr, unsigned int count)
     }
 }

-HRESULT dxbc_add_section(struct dxbc *dxbc, DWORD tag, const char *data, size_t data_size)
-{
-    TRACE("dxbc %p, tag %s, size %#Ix.\n", dxbc, debugstr_an((const char *)&tag, 4), data_size);
-
-    if (dxbc->count >= dxbc->size)
-    {
-        struct vkd3d_shader_dxbc_section_desc *new_sections;
-        DWORD new_size = dxbc->size << 1;
-
-        new_sections = HeapReAlloc(GetProcessHeap(), 0, dxbc->sections, new_size * sizeof(*dxbc->sections));
-        if (!new_sections)
-        {
-            ERR("Failed to allocate dxbc section memory\n");
-            return E_OUTOFMEMORY;
-        }
-
-        dxbc->sections = new_sections;
-        dxbc->size = new_size;
-    }
-
-    dxbc->sections[dxbc->count].tag = tag;
-    dxbc->sections[dxbc->count].data.size = data_size;
-    dxbc->sections[dxbc->count].data.code = data;
-    ++dxbc->count;
-
-    return S_OK;
-}
-
-HRESULT dxbc_init(struct dxbc *dxbc, unsigned int size)
-{
-    TRACE("dxbc %p, size %u.\n", dxbc, size);
-
-    /* use a good starting value for the size if none specified */
-    if (!size) size = 2;
-
-    dxbc->sections = HeapAlloc(GetProcessHeap(), 0, size * sizeof(*dxbc->sections));
-    if (!dxbc->sections)
-    {
-        ERR("Failed to allocate dxbc section memory\n");
-        return E_OUTOFMEMORY;
-    }
-
-    dxbc->size = size;
-    dxbc->count = 0;
-
-    return S_OK;
-}
-
-HRESULT dxbc_parse(const char *data, SIZE_T data_size, struct dxbc *dxbc)
-{
-    uint32_t tag, total_size, chunk_count;
-    const char *ptr = data;
-    unsigned int i;
-    HRESULT hr;
-
-    if (!data)
-    {
-        WARN("No data supplied.\n");
-        return E_FAIL;
-    }
-
-    tag = read_u32(&ptr);
-    TRACE("tag: %s.\n", debugstr_an((const char *)&tag, 4));
-
-    if (tag != TAG_DXBC)
-    {
-        WARN("Wrong tag.\n");
-        return E_FAIL;
-    }
-
-    /* checksum? */
-    skip_u32_unknown(&ptr, 4);
-
-    skip_u32_unknown(&ptr, 1);
-
-    total_size = read_u32(&ptr);
-    TRACE("total size: %#x\n", total_size);
-
-    if (data_size != total_size)
-    {
-        WARN("Wrong size supplied.\n");
-        return D3DERR_INVALIDCALL;
-    }
-
-    chunk_count = read_u32(&ptr);
-    TRACE("chunk count: %#x\n", chunk_count);
-
-    hr = dxbc_init(dxbc, chunk_count);
-    if (FAILED(hr))
-    {
-        WARN("Failed to init dxbc\n");
-        return hr;
-    }
-
-    for (i = 0; i < chunk_count; ++i)
-    {
-        uint32_t chunk_tag, chunk_size;
-        const char *chunk_ptr;
-        uint32_t chunk_offset;
-
-        chunk_offset = read_u32(&ptr);
-        TRACE("chunk %u at offset %#x\n", i, chunk_offset);
-
-        chunk_ptr = data + chunk_offset;
-
-        chunk_tag = read_u32(&chunk_ptr);
-        chunk_size = read_u32(&chunk_ptr);
-
-        hr = dxbc_add_section(dxbc, chunk_tag, chunk_ptr, chunk_size);
-        if (FAILED(hr))
-        {
-            WARN("Failed to add section to dxbc\n");
-            return hr;
-        }
-    }
-
-    return hr;
-}
-
-void dxbc_destroy(struct dxbc *dxbc)
-{
-    TRACE("dxbc %p.\n", dxbc);
-
-    HeapFree(GetProcessHeap(), 0, dxbc->sections);
-}
-
 void compilation_message(struct compilation_messages *msg, const char *fmt, va_list args)
 {
     char* buffer;

[Henri Verbeet (＠hverbeet)]

> -    if (!data || !data_size || flags || !blob)
> +    if (!data || data_size < DXBC_HEADER_SIZE || flags || !blob)
>
> Ideally this would be a separate change with a test to validate its correctness (AFAICS it is correct but there isn't a test already for it).

Well, it more or less preserves existing behaviour. Previously this case would simply read beyond "data_size" and (typically) either hit the "data_size != total_size" condition in dxbc_parse() and return D3DERR_INVALIDCALL, or hit the "tag != TAG_DXBC" condition and return E_FAIL. None of that is particularly great, of course.

In vkd3d-shader's parse_dxbc(), we instead hit the "data_size < VKD3D_DXBC_HEADER_SIZE" condition, which returns VKD3D_ERROR_INVALID_ARGUMENT. That's the same error as returned for the "tag != TAG_DXBC" case. None of this seems particularly worth worrying about on first sight, but in the case of D3DStripShader() we happen to have a test that hits these paths. (I.e., the earlier "blob.c:364: Test failed: Got unexpected hr 0x80004005." failure.)

There are a couple of different ways we could address this of course, including:

- Explicitly check the tag in d3dcompiler_strip_shader().
  - Explicitly check the size in d3dcompiler_strip_shader().
  - Return a different error for these two cases in vkd3d-shader.
  - Remove the offending test.

These aren't mutually exclusive options, of course. I chose the second option because it seemed the easiest, particularly because there are already existing "data_size" checks in d3dcompiler_strip_shader() and d3dcompiler_get_blob_part(), but I could certainly be convinced to take a different approach.

The d3dcompiler_get_blob_part() change is just intended to keep the behaviour consistent with d3dcompiler_strip_shader(), which generally seems desirable, but in principle it could be dropped.
> -    /* some parts aren't full DXBCs, they contain only the data */
> +    /* some parts aren't full DXBCs, they contain only the data. */
>
> Inane nitpick: this change could be moved 2 patches back where nearby code is already being touched.

Actually, I didn't intend to touch that line at all; I've dropped it in v4.
> Similar to the previous patch here. Also can get rid of the || !data_size part.

Right, not removing the "!data_size" check was an oversight; I've dropped it in v4. For the DXBC_HEADER_SIZE check, see the explanation above.

[Matteo Bruni (＠Mystral)]

On Tue Apr 11 07:23:35 2023 +0000, Henri Verbeet wrote:
> > -    if (!data || !data_size || flags || !blob)
> > +    if (!data || data_size < DXBC_HEADER_SIZE || flags || !blob)
> >
> > Ideally this would be a separate change with a test to validate its
>
> correctness (AFAICS it is correct but there isn't a test already for it).
> Well, it more or less preserves existing behaviour. Previously this case
> would simply read beyond "data_size" and (typically) either hit the
> "data_size != total_size" condition in dxbc_parse() and return
> D3DERR_INVALIDCALL, or hit the "tag != TAG_DXBC" condition and return
> E_FAIL. None of that is particularly great, of course.
> In vkd3d-shader's parse_dxbc(), we instead hit the "data_size <
> VKD3D_DXBC_HEADER_SIZE" condition, which returns
> VKD3D_ERROR_INVALID_ARGUMENT. That's the same error as returned for the
> "tag != TAG_DXBC" case. None of this seems particularly worth worrying
> about on first sight, but in the case of D3DStripShader() we happen to
> have a test that hits these paths. (I.e., the earlier "blob.c:364: Test
> failed: Got unexpected hr 0x80004005." failure.)
> There are a couple of different ways we could address this of course, including:
> - Explicitly check the tag in d3dcompiler_strip_shader().
> - Explicitly check the size in d3dcompiler_strip_shader().
> - Return a different error for these two cases in vkd3d-shader.
> - Remove the offending test.
>
> These aren't mutually exclusive options, of course. I chose the second
> option because it seemed the easiest, particularly because there are
> already existing "data_size" checks in d3dcompiler_strip_shader() and
> d3dcompiler_get_blob_part(), but I could certainly be convinced to take
> a different approach.
> The d3dcompiler_get_blob_part() change is just intended to keep the
> behaviour consistent with d3dcompiler_strip_shader(), which generally
> seems desirable, but in principle it could be dropped.
> > -    /* some parts aren't full DXBCs, they contain only the data */
> > +    /* some parts aren't full DXBCs, they contain only the data. */
> >
> > Inane nitpick: this change could be moved 2 patches back where nearby
>
> code is already being touched.
> Actually, I didn't intend to touch that line at all; I've dropped it in v4.
> > Similar to the previous patch here. Also can get rid of the ||
>
> !data_size part.
> Right, not removing the "!data_size" check was an oversight; I've
> dropped it in v4. For the DXBC_HEADER_SIZE check, see the explanation above.

Right, I guess I'm saying that I'd slightly prefer if the problematic behavior was fixed beforehand, introducing these minimum size checks (both to d3dcompiler_get_blob_part() and d3dcompiler_strip_shader()) in a separate patch. It could be a new "patch 1" for this MR.

Think of it as a bonus fix that you get to do simply because you happened to be the first to spot it :D

Even better if the patch comes with tests (it doesn't need to be anything fancy, something like my earlier attachment would do just fine), especially for D3DGetBlobPart(), given that it doesn't happen to have anything affected by the specific behavior, or the fix, right now.

[PATCH v4 0/6] MR2577: d3dcompiler: Use the vkd3d-shader DXBC API. - closed

[Henri Verbeet (＠hverbeet)]

This merge request was closed by Henri Verbeet.

[Henri Verbeet (＠hverbeet)]

I'll be leaving that as an exercise for the interested maintainer then.