[PATCH] d3d9/tests: Add more tests for shader validator.

[Paul Gofman]

Related to bug https://bugs.winehq.org/show_bug.cgi?id=47229

Signed-off-by: Paul Gofman 
---
 dlls/d3d9/tests/device.c | 155 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 153 insertions(+), 2 deletions(-)

diff --git a/dlls/d3d9/tests/device.c b/dlls/d3d9/tests/device.c
index ff024176b3b..eb08a4eb262 100644
--- a/dlls/d3d9/tests/device.c
+++ b/dlls/d3d9/tests/device.c
@@ -14243,18 +14243,71 @@ struct IDirect3DShaderValidator9
     const struct IDirect3DShaderValidator9Vtbl *vtbl;
 };

+#define MAX_VALIDATOR_CB_CALL_COUNT 5
+
+struct test_shader_validator_cb_context
+{
+    unsigned int call_count;
+    const char *file[MAX_VALIDATOR_CB_CALL_COUNT];
+    int line[MAX_VALIDATOR_CB_CALL_COUNT];
+    DWORD_PTR message_id[MAX_VALIDATOR_CB_CALL_COUNT];
+    const char *message[MAX_VALIDATOR_CB_CALL_COUNT];
+};
+
 HRESULT WINAPI test_shader_validator_cb(const char *file, int line, DWORD_PTR arg3,
         DWORD_PTR message_id, const char *message, void *context)
 {
-    ok(0, "Unexpected call.\n");
+    if (context)
+    {
+        struct test_shader_validator_cb_context *c = context;
+
+        c->file[c->call_count] = file;
+        c->line[c->call_count] = line;
+        c->message_id[c->call_count] = message_id;
+        c->message[c->call_count] = message;
+        ++c->call_count;
+    }
+    else
+    {
+        ok(0, "Unexpected call.\n");
+    }
     return S_OK;
 }

static void test_shader_validator(void)
 {
+    static const unsigned int dcl_texcoord_9_9[] = {0x0200001f, 0x80090005, 0x902f0009};   /* dcl_texcoord9_pp v9 */
+    static const unsigned int dcl_texcoord_9_10[] = {0x0200001f, 0x80090005, 0x902f000a};  /* dcl_texcoord9_pp v10 */
+    static const unsigned int dcl_texcoord_10_9[] = {0x0200001f, 0x800a0005, 0x902f0009};  /* dcl_texcoord10_pp v9 */
+    static const unsigned int mov_r2_v9[] = {0x02000001, 0x80220002, 0x90ff0009};          /* mov_pp r2.y, v9.w */
+    static const unsigned int mov_r2_v10[] = {0x02000001, 0x80220002, 0x90ff000a};         /* mov_pp r2.y, v10.w */
+
+    static const unsigned int ps_3_0 = 0xffff0300;
+    static const unsigned int end_token = 0x0000ffff;
+    static const char *test_file_name = "test_file";
+    static const struct instruction_test
+    {
+        unsigned int shader_version;
+        const unsigned int *instruction;
+        unsigned int instruction_length;
+        DWORD_PTR message_id;
+        const unsigned int *decl;
+        unsigned int decl_length;
+    }
+    instruction_tests[] =
+    {
+        {0xffff0300, dcl_texcoord_9_9, ARRAY_SIZE(dcl_texcoord_9_9)},
+        {0xffff0300, dcl_texcoord_9_10, ARRAY_SIZE(dcl_texcoord_9_10), 0x12c},
+        {0xffff0300, dcl_texcoord_10_9, ARRAY_SIZE(dcl_texcoord_10_9)},
+        {0xffff0300, mov_r2_v9, ARRAY_SIZE(mov_r2_v9), 0, dcl_texcoord_9_9, ARRAY_SIZE(dcl_texcoord_9_9)},
+        {0xffff0300, mov_r2_v10, ARRAY_SIZE(mov_r2_v10), 0x167},
+    };
+
+    struct test_shader_validator_cb_context context;
     IDirect3DShaderValidator9 *validator;
+    HRESULT expected_hr, hr;
+    unsigned int i;
     ULONG refcount;
-    HRESULT hr;

validator = Direct3DShaderValidatorCreate9();

@@ -14277,6 +14330,104 @@ static void test_shader_validator(void)
     hr = validator->vtbl->End(validator);
     ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);

+    hr = validator->vtbl->Begin(validator, test_shader_validator_cb, &context, 0);
+    ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
+
+    memset(&context, 0, sizeof(context));
+    hr = validator->vtbl->Instruction(validator, test_file_name, 28, &simple_vs[1], 3);
+    todo_wine
+    {
+        ok(hr == E_FAIL, "Got unexpected hr %#x.\n", hr);
+        ok(context.call_count == 2, "Got unexpected call_count %u.\n", context.call_count);
+        ok(!!context.message[0], "Got NULL message.\n");
+        ok(!!context.message[1], "Got NULL message.\n");
+        ok(context.message_id[0] == 0xef, "Got unexpected message_id[0] %p.\n", (void *)context.message_id[0]);
+        ok(context.message_id[1] == 0xf0, "Got unexpected message_id[1] %p.\n", (void *)context.message_id[1]);
+        ok(context.line[0] == -1, "Got unexpected line %d.\n", context.line[0]);
+    }
+    ok(!context.file[0], "Got unexpected file[0] %s.\n", context.file[0]);
+    ok(!context.file[1], "Got unexpected file[0] %s.\n", context.file[1]);
+    ok(!context.line[1], "Got unexpected line %d.\n", context.line[1]);
+
+    memset(&context, 0, sizeof(context));
+    hr = validator->vtbl->End(validator);
+    todo_wine ok(hr == E_FAIL, "Got unexpected hr %#x.\n", hr);
+    ok(!context.call_count, "Got unexpected call_count %u.\n", context.call_count);
+
+    memset(&context, 0, sizeof(context));
+    hr = validator->vtbl->Begin(validator, test_shader_validator_cb, &context, 0);
+    todo_wine
+    {
+        ok(hr == E_FAIL, "Got unexpected hr %#x.\n", hr);
+        ok(context.call_count == 1, "Got unexpected call_count %u.\n", context.call_count);
+        ok(context.message_id[0] == 0xeb, "Got unexpected message_id[0] %p.\n", (void *)context.message_id[0]);
+        ok(!!context.message[0], "Got NULL message.\n");
+    }
+    ok(!context.file[0], "Got unexpected file[0] %s.\n", context.file[0]);
+    ok(!context.line[0], "Got unexpected line %d.\n", context.line[0]);
+
+    hr = validator->vtbl->Begin(validator, NULL, &context, 0);
+    todo_wine ok(hr == E_FAIL, "Got unexpected hr %#x.\n", hr);
+
+    refcount = validator->vtbl->Release(validator);
+    todo_wine ok(!refcount, "Validator has %u references left.\n", refcount);
+    validator = Direct3DShaderValidatorCreate9();
+
+    hr = validator->vtbl->Begin(validator, NULL, &context, 0);
+    ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
+    hr = validator->vtbl->Instruction(validator, test_file_name, 1, &ps_3_0, 1);
+    ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
+    hr = validator->vtbl->Instruction(validator, test_file_name, 5, &end_token, 1);
+    ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
+    hr = validator->vtbl->End(validator);
+    ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
+
+    for (i = 0; i < ARRAY_SIZE(instruction_tests); ++i)
+    {
+        const struct instruction_test *test = &instruction_tests[i];
+
+        hr = validator->vtbl->Begin(validator, test_shader_validator_cb, &context, 0);
+        ok(hr == S_OK, "Got unexpected hr %#x, test %u.\n", hr, i);
+
+        hr = validator->vtbl->Instruction(validator, test_file_name, 1, &test->shader_version, 1);
+        ok(hr == S_OK, "Got unexpected hr %#x, test %u.\n", hr, i);
+
+        if (test->decl)
+        {
+            memset(&context, 0, sizeof(context));
+            hr = validator->vtbl->Instruction(validator, test_file_name, 3, test->decl, test->decl_length);
+            ok(hr == S_OK, "Got unexpected hr %#x, test %u.\n", hr, i);
+            ok(!context.call_count, "Got unexpected call_count %u, test %u.\n", context.call_count, i);
+        }
+
+        memset(&context, 0, sizeof(context));
+        hr = validator->vtbl->Instruction(validator, test_file_name, 3, test->instruction, test->instruction_length);
+        ok(hr == S_OK, "Got unexpected hr %#x, test %u.\n", hr, i);
+        if (test->message_id)
+        {
+            todo_wine
+            {
+                ok(context.call_count == 1, "Got unexpected call_count %u, test %u.\n", context.call_count, i);
+                ok(!!context.message[0], "Got NULL message, test %u.\n", i);
+                ok(context.message_id[0] == test->message_id, "Got unexpected message_id[0] %p, test %u.\n",
+                        (void *)context.message_id[0], i);
+                ok(context.file[0] == test_file_name, "Got unexpected file[0] %s, test %u.\n", context.file[0], i);
+                ok(context.line[0] == 3, "Got unexpected line %d, test %u.\n", context.line[0], i);
+            }
+        }
+        else
+        {
+            ok(!context.call_count, "Got unexpected call_count %u, test %u.\n", context.call_count, i);
+        }
+
+        hr = validator->vtbl->Instruction(validator, test_file_name, 5, &end_token, 1);
+        ok(hr == S_OK, "Got unexpected hr %#x, test %u.\n", hr, i);
+
+        hr = validator->vtbl->End(validator);
+        expected_hr = test->message_id ? E_FAIL : S_OK;
+        todo_wine_if(expected_hr) ok(hr == expected_hr, "Got unexpected hr %#x, test %u.\n", hr, i);
+    }
+
     refcount = validator->vtbl->Release(validator);
     todo_wine ok(!refcount, "Validator has %u references left.\n", refcount);
 }

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=83874

Your paranoid android.

=== w10pro64 (32 bit report) ===

d3d9:
device.c:5260: Test failed: Didn't receive MOUSEMOVE 4 (150, 150).

[Henri Verbeet]

On Fri, 8 Jan 2021 at 15:14, Paul Gofman  wrote:
> - static const struct instruction_test
> - {
> -
>    unsigned int shader_version;
> -
>    const unsigned int *instruction;
> -
>    unsigned int instruction_length;
> -
>    DWORD_PTR message_id;
> -
>    const unsigned int *decl;
> -
>    unsigned int decl_length;
> - }
> - instruction_tests[] =
> - {
> -
>    {0xffff0300, dcl_texcoord_9_9, ARRAY_SIZE(dcl_texcoord_9_9)},
> -
>    {0xffff0300, dcl_texcoord_9_10, ARRAY_SIZE(dcl_texcoord_9_10), 0x12c},
> -
>    {0xffff0300, dcl_texcoord_10_9, ARRAY_SIZE(dcl_texcoord_10_9)},
> -
>    {0xffff0300, mov_r2_v9, ARRAY_SIZE(mov_r2_v9), 0, dcl_texcoord_9_9, ARRAY_SIZE(dcl_texcoord_9_9)},
> -
>    {0xffff0300, mov_r2_v10, ARRAY_SIZE(mov_r2_v10), 0x167},
> - };

D3DPS_VERSION(3, 0) instead of 0xffff0300, I'd say.

As an aside, I'm not sure how much thought you've already given to the
implementation of the shader validator, but we'll probably want most
of that to live in vkd3d-shader. Perhaps that can be part of
vkd3d_shader_scan(), although in its current form that requires
complete shaders.

[Paul Gofman]

On 1/11/21 20:53, Henri Verbeet wrote:
> On Fri, 8 Jan 2021 at 15:14, Paul Gofman  wrote:
> > - static const struct instruction_test
> > - {
> > -
> >    unsigned int shader_version;
> > -
> >    const unsigned int *instruction;
> > -
> >    unsigned int instruction_length;
> > -
> >    DWORD_PTR message_id;
> > -
> >    const unsigned int *decl;
> > -
> >    unsigned int decl_length;
> > - }
> > - instruction_tests[] =
> > - {
> > -
> >    {0xffff0300, dcl_texcoord_9_9, ARRAY_SIZE(dcl_texcoord_9_9)},
> > -
> >    {0xffff0300, dcl_texcoord_9_10, ARRAY_SIZE(dcl_texcoord_9_10), 0x12c},
> > -
> >    {0xffff0300, dcl_texcoord_10_9, ARRAY_SIZE(dcl_texcoord_10_9)},
> > -
> >    {0xffff0300, mov_r2_v9, ARRAY_SIZE(mov_r2_v9), 0, dcl_texcoord_9_9, ARRAY_SIZE(dcl_texcoord_9_9)},
> > -
> >    {0xffff0300, mov_r2_v10, ARRAY_SIZE(mov_r2_v10), 0x167},
> > - };
>
> D3DPS_VERSION(3, 0) instead of 0xffff0300, I'd say.
>
> As an aside, I'm not sure how much thought you've already given to the
> implementation of the shader validator, but we'll probably want most
> of that to live in vkd3d-shader. Perhaps that can be part of
> vkd3d_shader_scan(), although in its current form that requires
> complete shaders.

As soon as there is just one game known so far which happens to depend
on this validation, and we are unlikely to see more of such, I thought
of implementing only the minimum checks required in place (based on how
that is done in d3dx9 for, e. g., D3DX9GetShaderSemantics()). I've
attached some draft patches to the bug here:
https://bugs.winehq.org/attachment.cgi?id=69101. Do you think that is a
possible way to go? Or are there any plans for d3d compiler to use d3d9
shader validator, the same way as native maybe does?

[Henri Verbeet]

On Mon, 11 Jan 2021 at 19:02, Paul Gofman  wrote:
> As soon as there is just one game known so far which happens to depend
> on this validation, and we are unlikely to see more of such, I thought
> of implementing only the minimum checks required in place (based on how
> that is done in d3dx9 for, e. g., D3DX9GetShaderSemantics()). I've
> attached some draft patches to the bug here:
> https://bugs.winehq.org/attachment.cgi?id=69101. Do you think that is a
> possible way to go? Or are there any plans for d3d compiler to use d3d9
> shader validator, the same way as native maybe does?

The long-term plan is for pretty much all of the d3dx9 and d3dcompiler
shader handling code to be implemented in vkd3d-shader. For new code
it probably makes the most sense to implement it there in the first
place, instead of moving it over later.

[Paul Gofman]

On 1/11/21 21:18, Henri Verbeet wrote:
> On Mon, 11 Jan 2021 at 19:02, Paul Gofman  wrote:
> > As soon as there is just one game known so far which happens to depend
> > on this validation, and we are unlikely to see more of such, I thought
> > of implementing only the minimum checks required in place (based on how
> > that is done in d3dx9 for, e. g., D3DX9GetShaderSemantics()). I've
> > attached some draft patches to the bug here:
> > https://bugs.winehq.org/attachment.cgi?id=69101. Do you think that is a
> > possible way to go? Or are there any plans for d3d compiler to use d3d9
> > shader validator, the same way as native maybe does?
>
> The long-term plan is for pretty much all of the d3dx9 and d3dcompiler
> shader handling code to be implemented in vkd3d-shader. For new code
> it probably makes the most sense to implement it there in the first
> place, instead of moving it over later.

I will check that but I am pretty sure doing the actual check in the end
through vkd3d_shader_scan() should do for this bug. I am not sure which
is the best way to interface that d3d9, should we add a helper to
wined3d for vkd3d_shader_scan()?

[Henri Verbeet]

On Mon, 11 Jan 2021 at 20:15, Paul Gofman  wrote:
> On 1/11/21 21:18, Henri Verbeet wrote:
> > On Mon, 11 Jan 2021 at 19:02, Paul Gofman  wrote:
> > > As soon as there is just one game known so far which happens to depend
> > > on this validation, and we are unlikely to see more of such, I thought
> > > of implementing only the minimum checks required in place (based on how
> > > that is done in d3dx9 for, e. g., D3DX9GetShaderSemantics()). I've
> > > attached some draft patches to the bug here:
> > > https://bugs.winehq.org/attachment.cgi?id=69101. Do you think that is a
> > > possible way to go? Or are there any plans for d3d compiler to use d3d9
> > > shader validator, the same way as native maybe does?
> >
> > The long-term plan is for pretty much all of the d3dx9 and d3dcompiler
> > shader handling code to be implemented in vkd3d-shader. For new code
> > it probably makes the most sense to implement it there in the first
> > place, instead of moving it over later.
>
> I will check that but I am pretty sure doing the actual check in the end
> through vkd3d_shader_scan() should do for this bug. I am not sure which
> is the best way to interface that d3d9, should we add a helper to
> wined3d for vkd3d_shader_scan()?

I'm not sure there's much of a point to going through wined3d for
this; as far as I'm concerned it would be fine to call
vkd3d_shader_scan() directly from d3d9.

[Alexandre Julliard]

Henri Verbeet  writes:
> On Mon, 11 Jan 2021 at 20:15, Paul Gofman  wrote:
> > On 1/11/21 21:18, Henri Verbeet wrote:
> > > On Mon, 11 Jan 2021 at 19:02, Paul Gofman  wrote:
> > > > As soon as there is just one game known so far which happens to depend
> > > > on this validation, and we are unlikely to see more of such, I thought
> > > > of implementing only the minimum checks required in place (based on how
> > > > that is done in d3dx9 for, e. g., D3DX9GetShaderSemantics()). I've
> > > > attached some draft patches to the bug here:
> > > > https://bugs.winehq.org/attachment.cgi?id=69101. Do you think that is a
> > > > possible way to go? Or are there any plans for d3d compiler to use d3d9
> > > > shader validator, the same way as native maybe does?
> > >
> > > The long-term plan is for pretty much all of the d3dx9 and d3dcompiler
> > > shader handling code to be implemented in vkd3d-shader. For new code
> > > it probably makes the most sense to implement it there in the first
> > > place, instead of moving it over later.
> >
> > I will check that but I am pretty sure doing the actual check in the end
> > through vkd3d_shader_scan() should do for this bug. I am not sure which
> > is the best way to interface that d3d9, should we add a helper to
> > wined3d for vkd3d_shader_scan()?
>
> I'm not sure there's much of a point to going through wined3d for
> this; as far as I'm concerned it would be fine to call
> vkd3d_shader_scan() directly from d3d9.

You'd need a PE build of vkd3d-shader then...

[Henri Verbeet]

On Tue, 12 Jan 2021 at 12:20, Alexandre Julliard  wrote:
> Henri Verbeet  writes:
> > On Mon, 11 Jan 2021 at 20:15, Paul Gofman  wrote:
> > > On 1/11/21 21:18, Henri Verbeet wrote:
> > > > On Mon, 11 Jan 2021 at 19:02, Paul Gofman  wrote:
> > > > > As soon as there is just one game known so far which happens to depend
> > > > > on this validation, and we are unlikely to see more of such, I thought
> > > > > of implementing only the minimum checks required in place (based on how
> > > > > that is done in d3dx9 for, e. g., D3DX9GetShaderSemantics()). I've
> > > > > attached some draft patches to the bug here:
> > > > > https://bugs.winehq.org/attachment.cgi?id=69101. Do you think that is a
> > > > > possible way to go? Or are there any plans for d3d compiler to use d3d9
> > > > > shader validator, the same way as native maybe does?
> > > >
> > > > The long-term plan is for pretty much all of the d3dx9 and d3dcompiler
> > > > shader handling code to be implemented in vkd3d-shader. For new code
> > > > it probably makes the most sense to implement it there in the first
> > > > place, instead of moving it over later.
> > >
> > > I will check that but I am pretty sure doing the actual check in the end
> > > through vkd3d_shader_scan() should do for this bug. I am not sure which
> > > is the best way to interface that d3d9, should we add a helper to
> > > wined3d for vkd3d_shader_scan()?
> >
> > I'm not sure there's much of a point to going through wined3d for
> > this; as far as I'm concerned it would be fine to call
> > vkd3d_shader_scan() directly from d3d9.
>
> You'd need a PE build of vkd3d-shader then...

We do support PE builds of vkd3d, but I was under the impression we
would be able to interface with host libraries using the
__wine_init_unix_lib() mechanism.

[Alexandre Julliard]

Henri Verbeet  writes:
> On Tue, 12 Jan 2021 at 12:20, Alexandre Julliard  wrote:
> > Henri Verbeet  writes:
> > > On Mon, 11 Jan 2021 at 20:15, Paul Gofman  wrote:
> > > > On 1/11/21 21:18, Henri Verbeet wrote:
> > > > > On Mon, 11 Jan 2021 at 19:02, Paul Gofman  wrote:
> > > > > > As soon as there is just one game known so far which happens to depend
> > > > > > on this validation, and we are unlikely to see more of such, I thought
> > > > > > of implementing only the minimum checks required in place (based on how
> > > > > > that is done in d3dx9 for, e. g., D3DX9GetShaderSemantics()). I've
> > > > > > attached some draft patches to the bug here:
> > > > > > https://bugs.winehq.org/attachment.cgi?id=69101. Do you think that is a
> > > > > > possible way to go? Or are there any plans for d3d compiler to use d3d9
> > > > > > shader validator, the same way as native maybe does?
> > > > >
> > > > > The long-term plan is for pretty much all of the d3dx9 and d3dcompiler
> > > > > shader handling code to be implemented in vkd3d-shader. For new code
> > > > > it probably makes the most sense to implement it there in the first
> > > > > place, instead of moving it over later.
> > > >
> > > > I will check that but I am pretty sure doing the actual check in the end
> > > > through vkd3d_shader_scan() should do for this bug. I am not sure which
> > > > is the best way to interface that d3d9, should we add a helper to
> > > > wined3d for vkd3d_shader_scan()?
> > >
> > > I'm not sure there's much of a point to going through wined3d for
> > > this; as far as I'm concerned it would be fine to call
> > > vkd3d_shader_scan() directly from d3d9.
> >
> > You'd need a PE build of vkd3d-shader then...
>
> We do support PE builds of vkd3d, but I was under the impression we
> would be able to interface with host libraries using the
> __wine_init_unix_lib() mechanism.

We could of course, but it would be better to avoid it, especially for
libraries that we control. The more Unix interfaces we have, the more
trouble we'll have down the road.