[PATCH 0/2] MR1340: d3dcompiler: Print message output with ERR().

[Zebediah Figura (＠zfigura)]

Currently it's not obvious when the HLSL compiler fails, either due to a missing
feature or incorrectly coded error check, which can unnecessarily complicate
debugging.

It's legal for well-behaved programs to try to compile invalid shaders, but that
should be a rare enough case that it's worth printing it to Wine debug output
regardless.

[PATCH 1/2] d3dcompiler: Print message output with ERR() in D3DCompile2().

[Zebediah Figura]

From: Zebediah Figura

---
 dlls/d3dcompiler_43/compiler.c | 54 +++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/dlls/d3dcompiler_43/compiler.c b/dlls/d3dcompiler_43/compiler.c
index bb9b7ce54eb..d328e74585b 100644
--- a/dlls/d3dcompiler_43/compiler.c
+++ b/dlls/d3dcompiler_43/compiler.c
@@ -173,6 +173,22 @@ static void close_include(const struct vkd3d_shader_code *code, void *context)
     ID3DInclude_Close(iface, code->code);
 }

+static const char *get_line(const char **ptr)
+{
+    const char *p, *q;
+
+    p = *ptr;
+    if (!(q = strstr(p, "\n")))
+    {
+        if (!*p) return NULL;
+        *ptr += strlen(p);
+        return p;
+    }
+    *ptr = q + 1;
+
+    return p;
+}
+
 static HRESULT preprocess_shader(const void *data, SIZE_T data_size, const char *filename,
         const D3D_SHADER_MACRO *defines, ID3DInclude *include, ID3DBlob **shader_blob,
         ID3DBlob **messages_blob)
@@ -486,8 +502,36 @@ HRESULT WINAPI D3DCompile2(const void *data, SIZE_T data_size, const char *filen
     }

ret = vkd3d_shader_compile(&compile_info, &byte_code, &messages);
+
+    if (!ret)
+    {
+        if (FAILED(hr = D3DCreateBlob(byte_code.size, shader_blob)))
+        {
+            vkd3d_shader_free_shader_code(&byte_code);
+            return hr;
+        }
+        memcpy(ID3D10Blob_GetBufferPointer(*shader_blob), byte_code.code, byte_code.size);
+    }
+    else
+    {
+        ERR("Failed to compile shader, vkd3d result %d.\n", ret);
+    }
+
     if (messages)
     {
+        if (*messages && ERR_ON(d3dcompiler))
+        {
+            const char *ptr = messages;
+            const char *line;
+
+            ERR("Shader log:\n");
+            while ((line = get_line(&ptr)))
+            {
+                ERR("    %.*s", (int)(ptr - line), line);
+            }
+            ERR("\n");
+        }
+
         if (messages_blob)
         {
             size_t size = strlen(messages);
@@ -503,16 +547,6 @@ HRESULT WINAPI D3DCompile2(const void *data, SIZE_T data_size, const char *filen
             vkd3d_shader_free_messages(messages);
     }

-    if (!ret)
-    {
-        if (FAILED(hr = D3DCreateBlob(byte_code.size, shader_blob)))
-        {
-            vkd3d_shader_free_shader_code(&byte_code);
-            return hr;
-        }
-        memcpy(ID3D10Blob_GetBufferPointer(*shader_blob), byte_code.code, byte_code.size);
-    }
-
     return hresult_from_vkd3d_result(ret);
 }

[PATCH 2/2] d3dcompiler: Print message output with ERR() in D3DPreprocess().

[Zebediah Figura]

From: Zebediah Figura

---
 dlls/d3dcompiler_43/compiler.c | 38 +++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/dlls/d3dcompiler_43/compiler.c b/dlls/d3dcompiler_43/compiler.c
index d328e74585b..59020e3d345 100644
--- a/dlls/d3dcompiler_43/compiler.c
+++ b/dlls/d3dcompiler_43/compiler.c
@@ -234,8 +234,36 @@ static HRESULT preprocess_shader(const void *data, SIZE_T data_size, const char
     preprocess_info.include_context = include;

ret = vkd3d_shader_preprocess(&compile_info, &byte_code, &messages);
+
+    if (!ret)
+    {
+        if (FAILED(hr = D3DCreateBlob(byte_code.size, shader_blob)))
+        {
+            vkd3d_shader_free_shader_code(&byte_code);
+            return hr;
+        }
+        memcpy(ID3D10Blob_GetBufferPointer(*shader_blob), byte_code.code, byte_code.size);
+    }
+    else
+    {
+        ERR("Failed to compile shader, vkd3d result %d.\n", ret);
+    }
+
     if (messages)
     {
+        if (*messages && ERR_ON(d3dcompiler))
+        {
+            const char *ptr = messages;
+            const char *line;
+
+            ERR("Shader log:\n");
+            while ((line = get_line(&ptr)))
+            {
+                ERR("    %.*s", (int)(ptr - line), line);
+            }
+            ERR("\n");
+        }
+
         if (messages_blob)
         {
             size_t size = strlen(messages);
@@ -253,16 +281,6 @@ static HRESULT preprocess_shader(const void *data, SIZE_T data_size, const char
         }
     }

-    if (!ret)
-    {
-        if (FAILED(hr = D3DCreateBlob(byte_code.size, shader_blob)))
-        {
-            vkd3d_shader_free_shader_code(&byte_code);
-            return hr;
-        }
-        memcpy(ID3D10Blob_GetBufferPointer(*shader_blob), byte_code.code, byte_code.size);
-    }
-
     return hresult_from_vkd3d_result(ret);
 }

[Matteo Bruni (＠Mystral)]

Matteo Bruni (@Mystral) commented about dlls/d3dcompiler_43/compiler.c:
>  ID3DInclude_Close(iface, code->code);
>
> }
>
> +static const char *get_line(const char **ptr)
> +{
> - const char *p, *q;
> -
> - p = *ptr;
> - if (!(q = strstr(p, "\n")))
> - {
> -
>    if (!*p) return NULL;

Nitpick: could you put the return on its own line?

[Matteo Bruni (＠Mystral)]

Matteo Bruni (@Mystral) commented about dlls/d3dcompiler_43/compiler.c:
>  ret = vkd3d_shader_compile(&compile_info, &byte_code, &messages);
> -
> - if (!ret)
> - {
> -
>    if (FAILED(hr = D3DCreateBlob(byte_code.size, shader_blob)))
> -
>    {
> -
>        vkd3d_shader_free_shader_code(&byte_code);
> -
>        return hr;
> -
>    }
> -
>    memcpy(ID3D10Blob_GetBufferPointer(*shader_blob), byte_code.code, byte_code.size);
> - }
> - else
> - {
> -
>    ERR("Failed to compile shader, vkd3d result %d.\n", ret);
> - }

I wonder if it wouldn't be a bit nicer to just have this branch here (as an `if (ret)`) and leave the `if (!ret) {}` branch in its original place below the `if (messages) {}`

[Matteo Bruni (＠Mystral)]

Matteo Bruni (@Mystral) commented about dlls/d3dcompiler_43/compiler.c:
>  ret = vkd3d_shader_preprocess(&compile_info, &byte_code, &messages);
> -
> - if (!ret)
> - {
> -
>    if (FAILED(hr = D3DCreateBlob(byte_code.size, shader_blob)))
> -
>    {
> -
>        vkd3d_shader_free_shader_code(&byte_code);
> -
>        return hr;
> -
>    }
> -
>    memcpy(ID3D10Blob_GetBufferPointer(*shader_blob), byte_code.code, byte_code.size);
> - }
> - else
> - {
> -
>    ERR("Failed to compile shader, vkd3d result %d.\n", ret);
> - }

Same comment as the other patch.

[Matteo Bruni (＠Mystral)]

Sorry, I did notice this MR right away but then I forgot about it until a few minutes ago...
> It's legal for well-behaved programs to try to compile invalid shaders, but that should be a rare enough case that it's worth printing it to Wine debug output regardless.

I don't know that it's super rare but, at this point in the HLSL compiler's development, it does seem better to be noisy with errors. We can always turn those ERRs() into WARNs() if / when we feel it's too much.

WRT the patches, I have just a couple of nitpicks and I don't feel too strongly about them either.

[Zebediah Figura (＠zfigura)]

On Wed Nov 16 20:22:17 2022 +0000, Matteo Bruni wrote:
> I wonder if it wouldn't be a bit nicer to just have this branch here (as
> an `if (ret)`) and leave the `if (!ret) {}` branch in its original place
> below the `if (messages) {}`

I felt like combining them was nicer, but it's definitely a mild style preference; I'll change it as described.

[Zebediah Figura (＠zfigura)]

On Wed Nov 16 20:22:16 2022 +0000, Matteo Bruni wrote:
> Nitpick: could you put the return on its own line?

Yes, I don't know how I missed that...