[PATCH] d3d9/tests: Add more tests for shader validator.
Related to bug https://bugs.winehq.org/show_bug.cgi?id=47229 Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- 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); } -- 2.29.2
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).
On Fri, 8 Jan 2021 at 15:14, Paul Gofman <pgofman(a)codeweavers.com> 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.
On 1/11/21 20:53, Henri Verbeet wrote:
On Fri, 8 Jan 2021 at 15:14, Paul Gofman <pgofman(a)codeweavers.com> 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?
On Mon, 11 Jan 2021 at 19:02, Paul Gofman <pgofman(a)codeweavers.com> 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.
On 1/11/21 21:18, Henri Verbeet wrote:
On Mon, 11 Jan 2021 at 19:02, Paul Gofman <pgofman(a)codeweavers.com> 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()?
On Mon, 11 Jan 2021 at 20:15, Paul Gofman <pgofman(a)codeweavers.com> wrote:
On 1/11/21 21:18, Henri Verbeet wrote:
On Mon, 11 Jan 2021 at 19:02, Paul Gofman <pgofman(a)codeweavers.com> 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.
Henri Verbeet <hverbeet(a)gmail.com> writes:
On Mon, 11 Jan 2021 at 20:15, Paul Gofman <pgofman(a)codeweavers.com> wrote:
On 1/11/21 21:18, Henri Verbeet wrote:
On Mon, 11 Jan 2021 at 19:02, Paul Gofman <pgofman(a)codeweavers.com> 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... -- Alexandre Julliard julliard(a)winehq.org
On Tue, 12 Jan 2021 at 12:20, Alexandre Julliard <julliard(a)winehq.org> wrote:
Henri Verbeet <hverbeet(a)gmail.com> writes:
On Mon, 11 Jan 2021 at 20:15, Paul Gofman <pgofman(a)codeweavers.com> wrote:
On 1/11/21 21:18, Henri Verbeet wrote:
On Mon, 11 Jan 2021 at 19:02, Paul Gofman <pgofman(a)codeweavers.com> 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.
Henri Verbeet <hverbeet(a)gmail.com> writes:
On Tue, 12 Jan 2021 at 12:20, Alexandre Julliard <julliard(a)winehq.org> wrote:
Henri Verbeet <hverbeet(a)gmail.com> writes:
On Mon, 11 Jan 2021 at 20:15, Paul Gofman <pgofman(a)codeweavers.com> wrote:
On 1/11/21 21:18, Henri Verbeet wrote:
On Mon, 11 Jan 2021 at 19:02, Paul Gofman <pgofman(a)codeweavers.com> 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. -- Alexandre Julliard julliard(a)winehq.org
participants (4)
-
Alexandre Julliard -
Henri Verbeet -
Marvin -
Paul Gofman