Re: [PATCH] d3dcompiler: Implement D3DReadFileToBlob().

[Matteo Bruni]

On Fri, Apr 26, 2019 at 12:16 PM Jactry Zeng [email protected] wrote:
> Signed-off-by: Jactry Zeng [email protected]
>
> dlls/d3dcompiler_43/blob.c       |  54 +++++++++++++++-
>  dlls/d3dcompiler_43/tests/blob.c | 106 +++++++++++++++++++++++++++++++
>  2 files changed, 158 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/d3dcompiler_43/blob.c b/dlls/d3dcompiler_43/blob.c
> index f22dc7183d..6b1b20317f 100644
> --- a/dlls/d3dcompiler_43/blob.c
> +++ b/dlls/d3dcompiler_43/blob.c
> @@ -466,9 +466,59 @@ HRESULT WINAPI D3DStripShader(const void *data, SIZE_T data_size, UINT flags, ID
>
> HRESULT WINAPI D3DReadFileToBlob(const WCHAR *filename, ID3DBlob **contents)
>  {
> - FIXME("filename %s, contents %p\n", debugstr_w(filename), contents);
> - struct d3dcompiler_blob *object;
> - HANDLE file;
> - SIZE_T data_size;
> - DWORD read_size;
> - HRESULT hr;

Nitpicking, but please sort declarations in reverse Christmas tree
order (i.e. longest line to shortest)
> - return E_NOTIMPL;
> - TRACE("filename %s, contents %p\n", debugstr_w(filename), contents);

Period at the end of the message.
> -
> - file = CreateFileW(filename, GENERIC_READ, FILE_SHARE_READ, NULL,
> -
>                   OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

Please use 8 space indentation for line continuations.
> - if (file == INVALID_HANDLE_VALUE)
> -
>    return HRESULT_FROM_WIN32(GetLastError());
> -
> - data_size = GetFileSize(file, NULL);
> - if (data_size == INVALID_FILE_SIZE)
> - {
> -
>    CloseHandle(file);
> -
>    return HRESULT_FROM_WIN32(GetLastError());
> - }
> -
> - object = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*object));

Please use the heap_alloc_zero() / heap_free() helpers.
> - if (!object)
> - {
> -
>    CloseHandle(file);
> -
>    return E_OUTOFMEMORY;
> - }
> -
> - hr = d3dcompiler_blob_init(object, data_size);
> - if (FAILED(hr))
> - {
> -
>    WARN("Failed to initialize blob, hr %#x.\n", hr);
> -
>    CloseHandle(file);
> -
>    HeapFree(GetProcessHeap(), 0, object);
> -
>    return hr;
> - }

More nitpicking: you can put these two assignments inside the
respective if conditions.
> -
> - if (!ReadFile(file, object->data, data_size, &read_size, NULL) ||
> -
>    (read_size != data_size))

Please put the operator at the start of the line.
> +static void create_cso_file(LPCWSTR pathW, void *data, DWORD data_size)

const WCHAR *filename

There isn't anything specific to .cso files in this function, I'd just
call it create_file() or something like that.
> +{
> - HANDLE file;
> - DWORD written;
> -
> - file = CreateFileW(pathW, GENERIC_READ | GENERIC_WRITE, 0,
> -
>                   NULL, CREATE_ALWAYS, 0, 0);

It doesn't really matter but you don't need GENERIC_READ.
> - ok(file != INVALID_HANDLE_VALUE, "File creation failed, at %s, error 0x%08x.\n",
> -
>   wine_dbgstr_w(pathW), GetLastError());

I don't know that I like failing the test if for whatever reason we
can't create the file. I guess it's okay, although I'd find it
slightly preferable to win_skip instead.
> +/* .cso file compiled by fxc.exe.
> - HLSL source:
> -
> - struct PSInput
> - {
> -
>   float4 value : SV_POSITION;
> - };
> -
> - PSInput main(float4 position : POSITION)
> - {
> -
>   PSInput result;
> -
>   result.value = position;
> -
>   return result;
> - }
> -
> - */

Probably better to use the same style as the d3d11 tests (i.e. inside
an #if 0 - #endif).
> +static byte test_cso_data[] =

You want BYTE here (or even better DWORD, although you need to update
the definition too in that case).
> +static void test_D3DReadFileToBlob(void)
> +{
> - ID3DBlob *blob = NULL;
> - HRESULT hr;
> - static const WCHAR filenameW[] = {'t','e','s','t','.','c','s','o',0};
> - byte *data_enter;

Same here. You can also call it just "data".
> - SIZE_T data_size;
> -
> - hr = pD3DReadFileToBlob(filenameW, NULL);
> - ok(hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND),
> -
>   "D3DReadFileToBlob returned: 0x%08x.\n", hr);

This crashes if you happen to have a "test.cso" file around. Which I
had from a previous run of the test (which in turn happened because I
forgot to rebuild d3dcompiler_47 before running the test).
Since you don't need to use a specific file name, probably better to
just have it generated automatically by GetTempFileName() or similar.

Also, we generally use %#x for HRESULT traces.
> - hr = pD3DReadFileToBlob(filenameW, &blob);
> - ok(hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND),
> -
>   "D3DReadFileToBlob returned: 0x%08x.\n", hr);
> -
> - /* Crash on Windows
> - create_cso_file(filenameW, test_cso_data, sizeof(test_cso_data));
> - pD3DReadFileToBlob(filenameW, NULL);
> - DeleteFileW(filenameW);
> - */

I'd prefer that to be put inside an if (0) {} instead, so that it is
still compiled.
> - create_cso_file(filenameW, NULL, 0);

Please create the test files in the Windows temporary files directory.
> - hr = pD3DReadFileToBlob(filenameW, &blob);
> - ok(hr == S_OK, "D3DReadFileToBlob failed: 0x%08x.\n", hr);
> - data_size = ID3D10Blob_GetBufferSize(blob);
> - ok(data_size == 0, "got wrong data size: %lu, expected %u.\n", data_size, 0);

Please avoid tracing sizeof() results. Also !data_size is preferred.