[PATCH v2 0/5] MR626: vkd3d-utils: Constant buffer reflection without vkd3d-shader.
As per discussion in 606, plumbing constant buffer reflection through vkd3d-shader requires interface design and API documentation for an interface which is inherently quite specific to one format. Instead of wasting time on this interface, just implement RDEF parsing in vkd3d-utils itself. If we change our mind about this, we can always move the implementation to vkd3d-shader anyway. This does not copy the existing implementation from Wine, because: * Wine does not validate offsets; adding this changes the parsing code significantly; * Wine incorrectly handles types, deduplicating them into an rbtree; * Wine skips several fields which I have been able to find the purpose of. The implementation is not that complex to begin with, so reimplementing it from scratch is not much work. -- v2: vkd3d-utils: Implement ID3D12ShaderReflectionType::GetMemberTypeByIndex(). vkd3d-utils: Implement ID3D12ShaderReflectionVariable::GetType(). vkd3d-utils: Implement ID3D12ShaderReflectionConstantBuffer::GetVariableByIndex(). vkd3d-utils: Parse the RD11 section. vkd3d-utils: Implement ID3D12ShaderReflection::GetConstantBufferByIndex(). https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626
From: Zebediah Figura <zfigura(a)codeweavers.com> --- include/private/vkd3d_memory.h | 9 ++ libs/vkd3d-utils/reflection.c | 227 ++++++++++++++++++++++++++++++++- 2 files changed, 230 insertions(+), 6 deletions(-) diff --git a/include/private/vkd3d_memory.h b/include/private/vkd3d_memory.h index 8a2edb100..d89f741b5 100644 --- a/include/private/vkd3d_memory.h +++ b/include/private/vkd3d_memory.h @@ -65,6 +65,15 @@ static inline char *vkd3d_strdup(const char *string) return ptr; } +static inline void *vkd3d_memdup(const void *mem, size_t size) +{ + void *ptr; + + if ((ptr = vkd3d_malloc(size))) + memcpy(ptr, mem, size); + return ptr; +} + bool vkd3d_array_reserve(void **elements, size_t *capacity, size_t element_count, size_t element_size); #endif /* __VKD3D_MEMORY_H */ diff --git a/libs/vkd3d-utils/reflection.c b/libs/vkd3d-utils/reflection.c index 2f604864b..d5935ff5e 100644 --- a/libs/vkd3d-utils/reflection.c +++ b/libs/vkd3d-utils/reflection.c @@ -21,14 +21,79 @@ #include <vkd3d_d3dcommon.h> #include <vkd3d_d3d12shader.h> +struct d3d12_buffer +{ + ID3D12ShaderReflectionConstantBuffer ID3D12ShaderReflectionConstantBuffer_iface; + D3D12_SHADER_BUFFER_DESC desc; +}; + struct d3d12_reflection { ID3D12ShaderReflection ID3D12ShaderReflection_iface; unsigned int refcount; struct vkd3d_shader_scan_signature_info signature_info; + + D3D12_SHADER_DESC desc; + + struct d3d12_buffer *buffers; }; +static struct d3d12_buffer null_buffer; + +static struct d3d12_buffer *impl_from_ID3D12ShaderReflectionConstantBuffer(ID3D12ShaderReflectionConstantBuffer *iface) +{ + return CONTAINING_RECORD(iface, struct d3d12_buffer, ID3D12ShaderReflectionConstantBuffer_iface); +} + +static HRESULT STDMETHODCALLTYPE d3d12_buffer_GetDesc( + ID3D12ShaderReflectionConstantBuffer *iface, D3D12_SHADER_BUFFER_DESC *desc) +{ + struct d3d12_buffer *buffer = impl_from_ID3D12ShaderReflectionConstantBuffer(iface); + + TRACE("iface %p, desc %p.\n", iface, desc); + + if (buffer == &null_buffer) + { + WARN("Null constant buffer, returning E_FAIL.\n"); + return E_FAIL; + } + + if (!desc) + { + WARN("NULL pointer, returning E_FAIL.\n"); + return E_FAIL; + } + + *desc = buffer->desc; + return S_OK; +} + +static ID3D12ShaderReflectionVariable * STDMETHODCALLTYPE d3d12_buffer_GetVariableByIndex( + ID3D12ShaderReflectionConstantBuffer *iface, UINT index) +{ + FIXME("iface %p, index %u, stub!\n", iface, index); + + return NULL; +} + +static ID3D12ShaderReflectionVariable * STDMETHODCALLTYPE d3d12_buffer_GetVariableByName( + ID3D12ShaderReflectionConstantBuffer *iface, const char *name) +{ + FIXME("iface %p, name %s, stub!\n", iface, debugstr_a(name)); + + return NULL; +} + +static const struct ID3D12ShaderReflectionConstantBufferVtbl d3d12_buffer_vtbl = +{ + d3d12_buffer_GetDesc, + d3d12_buffer_GetVariableByIndex, + d3d12_buffer_GetVariableByName, +}; + +static struct d3d12_buffer null_buffer = {{&d3d12_buffer_vtbl}}; + static struct d3d12_reflection *impl_from_ID3D12ShaderReflection(ID3D12ShaderReflection *iface) { return CONTAINING_RECORD(iface, struct d3d12_reflection, ID3D12ShaderReflection_iface); @@ -72,6 +137,10 @@ static ULONG STDMETHODCALLTYPE d3d12_reflection_Release(ID3D12ShaderReflection * if (!refcount) { + for (UINT i = 0; i < reflection->desc.ConstantBuffers; ++i) + vkd3d_free((void *)reflection->buffers[i].desc.Name); + vkd3d_free(reflection->buffers); + vkd3d_shader_free_scan_signature_info(&reflection->signature_info); free(reflection); } @@ -87,9 +156,7 @@ static HRESULT STDMETHODCALLTYPE d3d12_reflection_GetDesc(ID3D12ShaderReflection FIXME("iface %p, desc %p partial stub!\n", iface, desc); - desc->InputParameters = reflection->signature_info.input.element_count; - desc->OutputParameters = reflection->signature_info.output.element_count; - desc->PatchConstantParameters = reflection->signature_info.patch_constant.element_count; + *desc = reflection->desc; return S_OK; } @@ -97,9 +164,17 @@ static HRESULT STDMETHODCALLTYPE d3d12_reflection_GetDesc(ID3D12ShaderReflection static struct ID3D12ShaderReflectionConstantBuffer * STDMETHODCALLTYPE d3d12_reflection_GetConstantBufferByIndex( ID3D12ShaderReflection *iface, UINT index) { - FIXME("iface %p, index %u stub!\n", iface, index); + struct d3d12_reflection *reflection = impl_from_ID3D12ShaderReflection(iface); - return NULL; + TRACE("iface %p, index %u.\n", iface, index); + + if (index > reflection->desc.ConstantBuffers) + { + WARN("Invalid index %u.\n", index); + return &null_buffer.ID3D12ShaderReflectionConstantBuffer_iface; + } + + return &reflection->buffers[index].ID3D12ShaderReflectionConstantBuffer_iface; } static struct ID3D12ShaderReflectionConstantBuffer * STDMETHODCALLTYPE d3d12_reflection_GetConstantBufferByName( @@ -287,9 +362,122 @@ static const struct ID3D12ShaderReflectionVtbl d3d12_reflection_vtbl = d3d12_reflection_GetRequiresFlags, }; +/* Return a pointer to data in a DXBC section, with bounds checking. */ +static const void *get_section_data(const struct vkd3d_shader_dxbc_section_desc *section, + uint32_t offset, uint32_t size) +{ + if (!vkd3d_bound_range(offset, size, section->data.size)) + { + ERR("Offset %#x and size %#x exceeds section size %#zx.\n", offset, size, section->data.size); + return NULL; + } + + return (const char *)section->data.code + offset; +} + +static HRESULT get_string(const struct vkd3d_shader_dxbc_section_desc *section, uint32_t offset, char **ret) +{ + const char *str; + char *end; + + if (offset >= section->data.size) + { + ERR("Offset %#x exceeds section size %#zx.\n", offset, section->data.size); + return E_INVALIDARG; + } + + str = (const char *)section->data.code + offset; + if (!(end = memchr(str, 0, section->data.size - offset))) + { + ERR("String at %#x is not properly zero-terminated.\n", offset); + return E_INVALIDARG; + } + + if (!(*ret = vkd3d_memdup(str, end + 1 - str))) + return E_OUTOFMEMORY; + return S_OK; +} + +struct rdef_header +{ + uint32_t buffer_count; + uint32_t buffers_offset; + uint32_t binding_count; + uint32_t bindings_offset; + uint8_t minor_version; + uint8_t major_version; + uint16_t type; + uint32_t compile_flags; + uint32_t creator_offset; +}; + +struct rdef_buffer +{ + uint32_t name_offset; + uint32_t var_count; + uint32_t vars_offset; + uint32_t size; + uint32_t flags; + uint32_t type; +}; + +static HRESULT d3d12_buffer_init(struct d3d12_buffer *buffer, const struct rdef_buffer *rdef_buffer, + const struct vkd3d_shader_dxbc_section_desc *section) +{ + HRESULT hr; + char *name; + + if ((FAILED(hr = get_string(section, rdef_buffer->name_offset, &name)))) + return hr; + + buffer->ID3D12ShaderReflectionConstantBuffer_iface.lpVtbl = &d3d12_buffer_vtbl; + + buffer->desc.Type = rdef_buffer->type; + buffer->desc.Variables = rdef_buffer->var_count; + buffer->desc.Size = rdef_buffer->size; + buffer->desc.uFlags = rdef_buffer->flags; + buffer->desc.Name = name; + + return S_OK; +} + +static HRESULT parse_rdef(struct d3d12_reflection *reflection, const struct vkd3d_shader_dxbc_section_desc *section) +{ + const struct rdef_header *header; + HRESULT hr; + + if (!(header = get_section_data(section, 0, sizeof(*header)))) + return E_INVALIDARG; + + reflection->desc.ConstantBuffers = header->buffer_count; + + if (header->buffer_count) + { + const struct rdef_buffer *rdef_buffers; + + if (!(rdef_buffers = get_section_data(section, header->buffers_offset, + header->buffer_count * sizeof(*rdef_buffers)))) + return E_INVALIDARG; + + if (!(reflection->buffers = vkd3d_calloc(header->buffer_count, sizeof(*reflection->buffers)))) + return E_OUTOFMEMORY; + + for (uint32_t i = 0; i < header->buffer_count; ++i) + { + if ((hr = d3d12_buffer_init(&reflection->buffers[i], &rdef_buffers[i], section))) + return hr; + } + } + + return S_OK; +} + static HRESULT d3d12_reflection_init(struct d3d12_reflection *reflection, const void *data, size_t data_size) { struct vkd3d_shader_compile_info compile_info = {.type = VKD3D_SHADER_STRUCTURE_TYPE_COMPILE_INFO}; + struct vkd3d_shader_dxbc_desc dxbc_desc; + enum vkd3d_result ret; + HRESULT hr; reflection->ID3D12ShaderReflection_iface.lpVtbl = &d3d12_reflection_vtbl; reflection->refcount = 1; @@ -301,7 +489,34 @@ static HRESULT d3d12_reflection_init(struct d3d12_reflection *reflection, const compile_info.next = &reflection->signature_info; reflection->signature_info.type = VKD3D_SHADER_STRUCTURE_TYPE_SCAN_SIGNATURE_INFO; - return hresult_from_vkd3d_result(vkd3d_shader_scan(&compile_info, NULL)); + if (FAILED(hr = hresult_from_vkd3d_result(vkd3d_shader_scan(&compile_info, NULL)))) + return hr; + + if ((ret = vkd3d_shader_parse_dxbc(&compile_info.source, 0, &dxbc_desc, NULL))) + { + vkd3d_shader_free_scan_signature_info(&reflection->signature_info); + return hresult_from_vkd3d_result(ret); + } + + for (unsigned int i = 0; i < dxbc_desc.section_count; ++i) + { + const struct vkd3d_shader_dxbc_section_desc *section = &dxbc_desc.sections[i]; + + if (section->tag == TAG_RDEF && FAILED(hr = parse_rdef(reflection, section))) + { + vkd3d_shader_free_scan_signature_info(&reflection->signature_info); + vkd3d_shader_free_dxbc(&dxbc_desc); + return hr; + } + } + + reflection->desc.InputParameters = reflection->signature_info.input.element_count; + reflection->desc.OutputParameters = reflection->signature_info.output.element_count; + reflection->desc.PatchConstantParameters = reflection->signature_info.patch_constant.element_count; + + vkd3d_shader_free_dxbc(&dxbc_desc); + + return S_OK; } HRESULT WINAPI D3DReflect(const void *data, SIZE_T data_size, REFIID iid, void **reflection) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626
From: Zebediah Figura <zfigura(a)codeweavers.com> --- include/private/vkd3d_common.h | 2 ++ libs/vkd3d-utils/reflection.c | 47 ++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/include/private/vkd3d_common.h b/include/private/vkd3d_common.h index 01e219b30..7712553a8 100644 --- a/include/private/vkd3d_common.h +++ b/include/private/vkd3d_common.h @@ -71,6 +71,8 @@ #define TAG_XNAP VKD3D_MAKE_TAG('X', 'N', 'A', 'P') #define TAG_XNAS VKD3D_MAKE_TAG('X', 'N', 'A', 'S') +#define TAG_RD11_REVERSE 0x25441313 + static inline uint64_t align(uint64_t addr, size_t alignment) { return (addr + (alignment - 1)) & ~(alignment - 1); diff --git a/libs/vkd3d-utils/reflection.c b/libs/vkd3d-utils/reflection.c index d5935ff5e..db7d226c4 100644 --- a/libs/vkd3d-utils/reflection.c +++ b/libs/vkd3d-utils/reflection.c @@ -411,6 +411,19 @@ struct rdef_header uint32_t creator_offset; }; +struct rdef_rd11 +{ + uint32_t magic; + uint32_t header_size; + uint32_t buffer_size; + uint32_t binding_size; + uint32_t variable_size; + uint32_t type_size; + uint32_t field_size; + /* Always zero. Possibly either padding or a null terminator? */ + uint32_t zero; +}; + struct rdef_buffer { uint32_t name_offset; @@ -444,11 +457,45 @@ static HRESULT d3d12_buffer_init(struct d3d12_buffer *buffer, const struct rdef_ static HRESULT parse_rdef(struct d3d12_reflection *reflection, const struct vkd3d_shader_dxbc_section_desc *section) { const struct rdef_header *header; + const struct rdef_rd11 *rd11; HRESULT hr; if (!(header = get_section_data(section, 0, sizeof(*header)))) return E_INVALIDARG; + if (header->major_version >= 5) + { + if (!(rd11 = get_section_data(section, sizeof(*header), sizeof(*rd11)))) + return E_INVALIDARG; + + /* RD11 is emitted for 5.0, the reversed version for 5.1 and 6.0. + * This corresponds to a difference in the binding_size member, but + * it's not clear why the magic also changed there. */ + if (rd11->magic != TAG_RD11 && rd11->magic != TAG_RD11_REVERSE) + { + FIXME("Unknown tag %#x.\n", rd11->magic); + return E_INVALIDARG; + } + + if (rd11->header_size != sizeof(struct rdef_header) + sizeof(struct rdef_rd11)) + { + FIXME("Unexpected header size %#x.\n", rd11->header_size); + return E_INVALIDARG; + } + + if (rd11->buffer_size != sizeof(struct rdef_buffer)) + { + FIXME("Unexpected buffer size %#x.\n", rd11->buffer_size); + return E_INVALIDARG; + } + + if (rd11->zero) + { + FIXME("Unexpected field %#x.\n", rd11->zero); + return E_INVALIDARG; + } + } + reflection->desc.ConstantBuffers = header->buffer_count; if (header->buffer_count) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-utils/reflection.c | 179 +++++++++++++++++++++++++++++++++- 1 file changed, 174 insertions(+), 5 deletions(-) diff --git a/libs/vkd3d-utils/reflection.c b/libs/vkd3d-utils/reflection.c index db7d226c4..80309885d 100644 --- a/libs/vkd3d-utils/reflection.c +++ b/libs/vkd3d-utils/reflection.c @@ -21,10 +21,19 @@ #include <vkd3d_d3dcommon.h> #include <vkd3d_d3d12shader.h> +struct d3d12_variable +{ + ID3D12ShaderReflectionVariable ID3D12ShaderReflectionVariable_iface; + D3D12_SHADER_VARIABLE_DESC desc; + struct d3d12_buffer *buffer; +}; + struct d3d12_buffer { ID3D12ShaderReflectionConstantBuffer ID3D12ShaderReflectionConstantBuffer_iface; D3D12_SHADER_BUFFER_DESC desc; + + struct d3d12_variable *variables; }; struct d3d12_reflection @@ -40,6 +49,71 @@ struct d3d12_reflection }; static struct d3d12_buffer null_buffer; +static struct d3d12_variable null_variable; + +static struct d3d12_variable *impl_from_ID3D12ShaderReflectionVariable(ID3D12ShaderReflectionVariable *iface) +{ + return CONTAINING_RECORD(iface, struct d3d12_variable, ID3D12ShaderReflectionVariable_iface); +} + +static HRESULT STDMETHODCALLTYPE d3d12_variable_GetDesc( + ID3D12ShaderReflectionVariable *iface, D3D12_SHADER_VARIABLE_DESC *desc) +{ + struct d3d12_variable *variable = impl_from_ID3D12ShaderReflectionVariable(iface); + + TRACE("iface %p, desc %p.\n", iface, desc); + + if (variable == &null_variable) + { + WARN("Null variable, returning E_FAIL.\n"); + return E_FAIL; + } + + if (!desc) + { + WARN("NULL pointer, returning E_FAIL.\n"); + return E_FAIL; + } + + *desc = variable->desc; + return S_OK; +} + +static ID3D12ShaderReflectionType * STDMETHODCALLTYPE d3d12_variable_GetType( + ID3D12ShaderReflectionVariable *iface) +{ + FIXME("iface %p, stub!\n", iface); + + return NULL; +} + +static ID3D12ShaderReflectionConstantBuffer * STDMETHODCALLTYPE d3d12_variable_GetBuffer( + ID3D12ShaderReflectionVariable *iface) +{ + struct d3d12_variable *variable = impl_from_ID3D12ShaderReflectionVariable(iface); + + TRACE("iface %p.\n", iface); + + return &variable->buffer->ID3D12ShaderReflectionConstantBuffer_iface; +} + +static UINT STDMETHODCALLTYPE d3d12_variable_GetInterfaceSlot( + ID3D12ShaderReflectionVariable *iface, UINT index) +{ + FIXME("iface %p, index %u, stub!\n", iface, index); + + return 0; +} + +static const struct ID3D12ShaderReflectionVariableVtbl d3d12_variable_vtbl = +{ + d3d12_variable_GetDesc, + d3d12_variable_GetType, + d3d12_variable_GetBuffer, + d3d12_variable_GetInterfaceSlot, +}; + +static struct d3d12_variable null_variable = {{&d3d12_variable_vtbl}}; static struct d3d12_buffer *impl_from_ID3D12ShaderReflectionConstantBuffer(ID3D12ShaderReflectionConstantBuffer *iface) { @@ -72,9 +146,17 @@ static HRESULT STDMETHODCALLTYPE d3d12_buffer_GetDesc( static ID3D12ShaderReflectionVariable * STDMETHODCALLTYPE d3d12_buffer_GetVariableByIndex( ID3D12ShaderReflectionConstantBuffer *iface, UINT index) { - FIXME("iface %p, index %u, stub!\n", iface, index); + struct d3d12_buffer *buffer = impl_from_ID3D12ShaderReflectionConstantBuffer(iface); - return NULL; + TRACE("iface %p, index %u.\n", iface, index); + + if (index > buffer->desc.Variables) + { + WARN("Invalid index %u.\n", index); + return &null_variable.ID3D12ShaderReflectionVariable_iface; + } + + return &buffer->variables[index].ID3D12ShaderReflectionVariable_iface; } static ID3D12ShaderReflectionVariable * STDMETHODCALLTYPE d3d12_buffer_GetVariableByName( @@ -138,7 +220,19 @@ static ULONG STDMETHODCALLTYPE d3d12_reflection_Release(ID3D12ShaderReflection * if (!refcount) { for (UINT i = 0; i < reflection->desc.ConstantBuffers; ++i) - vkd3d_free((void *)reflection->buffers[i].desc.Name); + { + struct d3d12_buffer *buffer = &reflection->buffers[i]; + + for (UINT j = 0; j < buffer->desc.Variables; ++j) + { + struct d3d12_variable *variable = &buffer->variables[j]; + + vkd3d_free((void *)variable->desc.DefaultValue); + vkd3d_free((void *)variable->desc.Name); + } + vkd3d_free(buffer->variables); + vkd3d_free((void *)buffer->desc.Name); + } vkd3d_free(reflection->buffers); vkd3d_shader_free_scan_signature_info(&reflection->signature_info); @@ -434,12 +528,60 @@ struct rdef_buffer uint32_t type; }; -static HRESULT d3d12_buffer_init(struct d3d12_buffer *buffer, const struct rdef_buffer *rdef_buffer, +struct rdef_variable +{ + uint32_t name_offset; + uint32_t offset; + uint32_t size; + uint32_t flags; + uint32_t type_offset; + uint32_t default_value_offset; + uint32_t resource_binding; + uint32_t resource_count; + uint32_t sampler_binding; + uint32_t sampler_count; +}; + +static HRESULT d3d12_variable_init(struct d3d12_variable *variable, const struct rdef_variable *rdef_variable, const struct vkd3d_shader_dxbc_section_desc *section) { HRESULT hr; char *name; + if (FAILED(hr = get_string(section, rdef_variable->name_offset, &name))) + return hr; + + variable->ID3D12ShaderReflectionVariable_iface.lpVtbl = &d3d12_variable_vtbl; + + variable->desc.Name = name; + variable->desc.StartOffset = rdef_variable->offset; + variable->desc.Size = rdef_variable->size; + variable->desc.uFlags = rdef_variable->flags; + variable->desc.StartTexture = rdef_variable->resource_binding; + variable->desc.TextureSize = rdef_variable->resource_count; + variable->desc.StartSampler = rdef_variable->sampler_binding; + variable->desc.SamplerSize = rdef_variable->sampler_count; + + if (rdef_variable->default_value_offset) + { + const void *default_value; + + if (!(default_value = get_section_data(section, rdef_variable->default_value_offset, rdef_variable->size))) + return E_INVALIDARG; + + if (!(variable->desc.DefaultValue = vkd3d_memdup(default_value, rdef_variable->size))) + return E_OUTOFMEMORY; + } + + return S_OK; +} + +static HRESULT d3d12_buffer_init(struct d3d12_buffer *buffer, const struct rdef_buffer *rdef_buffer, + const struct vkd3d_shader_dxbc_section_desc *section, uint32_t variable_size) +{ + HRESULT hr; + char *name; + if ((FAILED(hr = get_string(section, rdef_buffer->name_offset, &name)))) return hr; @@ -451,11 +593,31 @@ static HRESULT d3d12_buffer_init(struct d3d12_buffer *buffer, const struct rdef_ buffer->desc.uFlags = rdef_buffer->flags; buffer->desc.Name = name; + if (!(buffer->variables = vkd3d_calloc(rdef_buffer->var_count, sizeof(*buffer->variables)))) + return E_OUTOFMEMORY; + + for (uint32_t i = 0; i < rdef_buffer->var_count; ++i) + { + struct rdef_variable normalized_variable = {0}; + const struct rdef_variable *rdef_variable; + + if (!(rdef_variable = get_section_data(section, rdef_buffer->vars_offset + (i * variable_size), variable_size))) + return E_INVALIDARG; + + normalized_variable.resource_binding = ~0u; + normalized_variable.sampler_binding = ~0u; + memcpy(&normalized_variable, rdef_variable, variable_size); + + if ((hr = d3d12_variable_init(&buffer->variables[i], &normalized_variable, section))) + return hr; + } + return S_OK; } static HRESULT parse_rdef(struct d3d12_reflection *reflection, const struct vkd3d_shader_dxbc_section_desc *section) { + uint32_t variable_size = offsetof(struct rdef_variable, resource_binding); const struct rdef_header *header; const struct rdef_rd11 *rd11; HRESULT hr; @@ -489,6 +651,13 @@ static HRESULT parse_rdef(struct d3d12_reflection *reflection, const struct vkd3 return E_INVALIDARG; } + if (rd11->variable_size != sizeof(struct rdef_variable)) + { + FIXME("Unexpected variable size %#x.\n", rd11->variable_size); + return E_INVALIDARG; + } + variable_size = rd11->variable_size; + if (rd11->zero) { FIXME("Unexpected field %#x.\n", rd11->zero); @@ -511,7 +680,7 @@ static HRESULT parse_rdef(struct d3d12_reflection *reflection, const struct vkd3 for (uint32_t i = 0; i < header->buffer_count; ++i) { - if ((hr = d3d12_buffer_init(&reflection->buffers[i], &rdef_buffers[i], section))) + if ((hr = d3d12_buffer_init(&reflection->buffers[i], &rdef_buffers[i], section, variable_size))) return hr; } } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-utils/reflection.c | 206 ++++++++++++++++++++++++++++++++-- 1 file changed, 199 insertions(+), 7 deletions(-) diff --git a/libs/vkd3d-utils/reflection.c b/libs/vkd3d-utils/reflection.c index 80309885d..4741a0507 100644 --- a/libs/vkd3d-utils/reflection.c +++ b/libs/vkd3d-utils/reflection.c @@ -21,11 +21,19 @@ #include <vkd3d_d3dcommon.h> #include <vkd3d_d3d12shader.h> +struct d3d12_type +{ + ID3D12ShaderReflectionType ID3D12ShaderReflectionType_iface; + D3D12_SHADER_TYPE_DESC desc; +}; + struct d3d12_variable { ID3D12ShaderReflectionVariable ID3D12ShaderReflectionVariable_iface; D3D12_SHADER_VARIABLE_DESC desc; struct d3d12_buffer *buffer; + + struct d3d12_type type; }; struct d3d12_buffer @@ -50,6 +58,129 @@ struct d3d12_reflection static struct d3d12_buffer null_buffer; static struct d3d12_variable null_variable; +static struct d3d12_type null_type; + +static struct d3d12_type *impl_from_ID3D12ShaderReflectionType(ID3D12ShaderReflectionType *iface) +{ + return CONTAINING_RECORD(iface, struct d3d12_type, ID3D12ShaderReflectionType_iface); +} + +static HRESULT STDMETHODCALLTYPE d3d12_type_GetDesc( + ID3D12ShaderReflectionType *iface, D3D12_SHADER_TYPE_DESC *desc) +{ + struct d3d12_type *type = impl_from_ID3D12ShaderReflectionType(iface); + + TRACE("iface %p, desc %p.\n", iface, desc); + + if (type == &null_type) + { + WARN("Null type, returning E_FAIL.\n"); + return E_FAIL; + } + + if (!desc) + { + WARN("NULL pointer, returning E_FAIL.\n"); + return E_FAIL; + } + + *desc = type->desc; + return S_OK; +} + +static ID3D12ShaderReflectionType * STDMETHODCALLTYPE d3d12_type_GetMemberTypeByIndex( + ID3D12ShaderReflectionType *iface, UINT index) +{ + FIXME("iface %p, index %u, stub!\n", iface, index); + + return NULL; +} + +static ID3D12ShaderReflectionType * STDMETHODCALLTYPE d3d12_type_GetMemberTypeByName( + ID3D12ShaderReflectionType *iface, const char *name) +{ + FIXME("iface %p, name %s, stub!\n", iface, debugstr_a(name)); + + return NULL; +} + +static const char * STDMETHODCALLTYPE d3d12_type_GetMemberTypeName( + ID3D12ShaderReflectionType *iface, UINT index) +{ + FIXME("iface %p, index %u, stub!\n", iface, index); + + return NULL; +} + +static HRESULT STDMETHODCALLTYPE d3d12_type_IsEqual( + ID3D12ShaderReflectionType *iface, ID3D12ShaderReflectionType *other) +{ + FIXME("iface %p, other %p, stub!\n", iface, other); + return E_NOTIMPL; +} + +static ID3D12ShaderReflectionType * STDMETHODCALLTYPE d3d12_type_GetSubType( + ID3D12ShaderReflectionType *iface) +{ + FIXME("iface %p stub!\n", iface); + + return NULL; +} + +static ID3D12ShaderReflectionType * STDMETHODCALLTYPE d3d12_type_GetBaseClass( + ID3D12ShaderReflectionType *iface) +{ + FIXME("iface %p stub!\n", iface); + + return NULL; +} + +static UINT STDMETHODCALLTYPE d3d12_type_GetNumInterfaces( + ID3D12ShaderReflectionType *iface) +{ + FIXME("iface %p stub!\n", iface); + + return 0; +} + +static ID3D12ShaderReflectionType * STDMETHODCALLTYPE d3d12_type_GetInterfaceByIndex( + ID3D12ShaderReflectionType *iface, UINT index) +{ + FIXME("iface %p, index %u stub!\n", iface, index); + + return NULL; +} + +static HRESULT STDMETHODCALLTYPE d3d12_type_IsOfType( + ID3D12ShaderReflectionType *iface, ID3D12ShaderReflectionType *type) +{ + FIXME("iface %p, type %p stub!\n", iface, type); + + return E_NOTIMPL; +} + +static HRESULT STDMETHODCALLTYPE d3d12_type_ImplementsInterface( + ID3D12ShaderReflectionType *iface, ID3D12ShaderReflectionType *base) +{ + FIXME("iface %p, base %p stub!\n", iface, base); + + return E_NOTIMPL; +} + +static const struct ID3D12ShaderReflectionTypeVtbl d3d12_type_vtbl = +{ + d3d12_type_GetDesc, + d3d12_type_GetMemberTypeByIndex, + d3d12_type_GetMemberTypeByName, + d3d12_type_GetMemberTypeName, + d3d12_type_IsEqual, + d3d12_type_GetSubType, + d3d12_type_GetBaseClass, + d3d12_type_GetNumInterfaces, + d3d12_type_GetInterfaceByIndex, + d3d12_type_IsOfType, + d3d12_type_ImplementsInterface, +}; static struct d3d12_variable *impl_from_ID3D12ShaderReflectionVariable(ID3D12ShaderReflectionVariable *iface) { @@ -82,9 +213,14 @@ static HRESULT STDMETHODCALLTYPE d3d12_variable_GetDesc( static ID3D12ShaderReflectionType * STDMETHODCALLTYPE d3d12_variable_GetType( ID3D12ShaderReflectionVariable *iface) { - FIXME("iface %p, stub!\n", iface); + struct d3d12_variable *variable = impl_from_ID3D12ShaderReflectionVariable(iface); - return NULL; + TRACE("iface %p.\n", iface); + + if (variable == &null_variable) + return &null_type.ID3D12ShaderReflectionType_iface; + + return &variable->type.ID3D12ShaderReflectionType_iface; } static ID3D12ShaderReflectionConstantBuffer * STDMETHODCALLTYPE d3d12_variable_GetBuffer( @@ -210,6 +346,11 @@ static ULONG STDMETHODCALLTYPE d3d12_reflection_AddRef(ID3D12ShaderReflection *i return refcount; } +static void free_type(struct d3d12_type *type) +{ + vkd3d_free((void *)type->desc.Name); +} + static ULONG STDMETHODCALLTYPE d3d12_reflection_Release(ID3D12ShaderReflection *iface) { struct d3d12_reflection *reflection = impl_from_ID3D12ShaderReflection(iface); @@ -227,6 +368,7 @@ static ULONG STDMETHODCALLTYPE d3d12_reflection_Release(ID3D12ShaderReflection * { struct d3d12_variable *variable = &buffer->variables[j]; + free_type(&variable->type); vkd3d_free((void *)variable->desc.DefaultValue); vkd3d_free((void *)variable->desc.Name); } @@ -542,8 +684,50 @@ struct rdef_variable uint32_t sampler_count; }; -static HRESULT d3d12_variable_init(struct d3d12_variable *variable, const struct rdef_variable *rdef_variable, +struct rdef_type +{ + uint16_t class; + uint16_t base_type; + uint16_t row_count; + uint16_t column_count; + uint16_t element_count; + uint16_t field_count; + uint32_t fields_offset; + /* Probably related to interfaces. */ + uint32_t unknown[4]; + uint32_t name_offset; +}; + +static HRESULT d3d12_type_init(struct d3d12_type *type, uint32_t type_offset, uint32_t type_size, const struct vkd3d_shader_dxbc_section_desc *section) +{ + struct rdef_type normalized_type = {0}; + const struct rdef_type *rdef_type; + char *name = NULL; + HRESULT hr; + + if (!(rdef_type = get_section_data(section, type_offset, type_size))) + return E_INVALIDARG; + memcpy(&normalized_type, rdef_type, type_size); + + if (normalized_type.name_offset && FAILED(hr = get_string(section, normalized_type.name_offset, &name))) + return hr; + + type->ID3D12ShaderReflectionType_iface.lpVtbl = &d3d12_type_vtbl; + + type->desc.Class = normalized_type.class; + type->desc.Type = normalized_type.base_type; + type->desc.Rows = normalized_type.row_count; + type->desc.Columns = normalized_type.column_count; + type->desc.Elements = normalized_type.element_count; + type->desc.Members = normalized_type.field_count; + type->desc.Name = name; + + return S_OK; +} + +static HRESULT d3d12_variable_init(struct d3d12_variable *variable, const struct rdef_variable *rdef_variable, + const struct vkd3d_shader_dxbc_section_desc *section, uint32_t type_size) { HRESULT hr; char *name; @@ -573,11 +757,11 @@ static HRESULT d3d12_variable_init(struct d3d12_variable *variable, const struct return E_OUTOFMEMORY; } - return S_OK; + return d3d12_type_init(&variable->type, rdef_variable->type_offset, type_size, section); } static HRESULT d3d12_buffer_init(struct d3d12_buffer *buffer, const struct rdef_buffer *rdef_buffer, - const struct vkd3d_shader_dxbc_section_desc *section, uint32_t variable_size) + const struct vkd3d_shader_dxbc_section_desc *section, uint32_t variable_size, uint32_t type_size) { HRESULT hr; char *name; @@ -608,7 +792,7 @@ static HRESULT d3d12_buffer_init(struct d3d12_buffer *buffer, const struct rdef_ normalized_variable.sampler_binding = ~0u; memcpy(&normalized_variable, rdef_variable, variable_size); - if ((hr = d3d12_variable_init(&buffer->variables[i], &normalized_variable, section))) + if ((hr = d3d12_variable_init(&buffer->variables[i], &normalized_variable, section, type_size))) return hr; } @@ -618,6 +802,7 @@ static HRESULT d3d12_buffer_init(struct d3d12_buffer *buffer, const struct rdef_ static HRESULT parse_rdef(struct d3d12_reflection *reflection, const struct vkd3d_shader_dxbc_section_desc *section) { uint32_t variable_size = offsetof(struct rdef_variable, resource_binding); + uint32_t type_size = offsetof(struct rdef_type, unknown); const struct rdef_header *header; const struct rdef_rd11 *rd11; HRESULT hr; @@ -658,6 +843,13 @@ static HRESULT parse_rdef(struct d3d12_reflection *reflection, const struct vkd3 } variable_size = rd11->variable_size; + if (rd11->type_size != sizeof(struct rdef_type)) + { + FIXME("Unexpected type size %#x.\n", rd11->type_size); + return E_INVALIDARG; + } + type_size = rd11->type_size; + if (rd11->zero) { FIXME("Unexpected field %#x.\n", rd11->zero); @@ -680,7 +872,7 @@ static HRESULT parse_rdef(struct d3d12_reflection *reflection, const struct vkd3 for (uint32_t i = 0; i < header->buffer_count; ++i) { - if ((hr = d3d12_buffer_init(&reflection->buffers[i], &rdef_buffers[i], section, variable_size))) + if ((hr = d3d12_buffer_init(&reflection->buffers[i], &rdef_buffers[i], section, variable_size, type_size))) return hr; } } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-utils/reflection.c | 55 ++++++++++++++++++++++++++++++++--- 1 file changed, 51 insertions(+), 4 deletions(-) diff --git a/libs/vkd3d-utils/reflection.c b/libs/vkd3d-utils/reflection.c index 4741a0507..a9b8d7899 100644 --- a/libs/vkd3d-utils/reflection.c +++ b/libs/vkd3d-utils/reflection.c @@ -25,6 +25,13 @@ struct d3d12_type { ID3D12ShaderReflectionType ID3D12ShaderReflectionType_iface; D3D12_SHADER_TYPE_DESC desc; + + struct d3d12_field *fields; +}; + +struct d3d12_field +{ + struct d3d12_type type; }; struct d3d12_variable @@ -91,9 +98,17 @@ static HRESULT STDMETHODCALLTYPE d3d12_type_GetDesc( static ID3D12ShaderReflectionType * STDMETHODCALLTYPE d3d12_type_GetMemberTypeByIndex( ID3D12ShaderReflectionType *iface, UINT index) { - FIXME("iface %p, index %u, stub!\n", iface, index); + struct d3d12_type *type = impl_from_ID3D12ShaderReflectionType(iface); - return NULL; + TRACE("iface %p, index %u.\n", iface, index); + + if (index > type->desc.Members) + { + WARN("Invalid index %u.\n", index); + return &null_type.ID3D12ShaderReflectionType_iface; + } + + return &type->fields[index].type.ID3D12ShaderReflectionType_iface; } static ID3D12ShaderReflectionType * STDMETHODCALLTYPE d3d12_type_GetMemberTypeByName( @@ -348,6 +363,9 @@ static ULONG STDMETHODCALLTYPE d3d12_reflection_AddRef(ID3D12ShaderReflection *i static void free_type(struct d3d12_type *type) { + for (UINT i = 0; i < type->desc.Members; ++i) + free_type(&type->fields[i].type); + vkd3d_free(type->fields); vkd3d_free((void *)type->desc.Name); } @@ -698,8 +716,15 @@ struct rdef_type uint32_t name_offset; }; +struct rdef_field +{ + uint32_t name_offset; + uint32_t type_offset; + uint32_t offset; +}; + static HRESULT d3d12_type_init(struct d3d12_type *type, uint32_t type_offset, uint32_t type_size, - const struct vkd3d_shader_dxbc_section_desc *section) + const struct vkd3d_shader_dxbc_section_desc *section, uint32_t field_offset) { struct rdef_type normalized_type = {0}; const struct rdef_type *rdef_type; @@ -721,8 +746,30 @@ static HRESULT d3d12_type_init(struct d3d12_type *type, uint32_t type_offset, ui type->desc.Columns = normalized_type.column_count; type->desc.Elements = normalized_type.element_count; type->desc.Members = normalized_type.field_count; + type->desc.Offset = field_offset; type->desc.Name = name; + if (normalized_type.field_count) + { + const struct rdef_field *rdef_fields; + + if (!(rdef_fields = get_section_data(section, normalized_type.fields_offset, + normalized_type.field_count * sizeof(*rdef_fields)))) + return E_INVALIDARG; + + if (!(type->fields = vkd3d_calloc(normalized_type.field_count, sizeof(*type->fields)))) + return false; + + for (uint32_t i = 0; i < normalized_type.field_count; ++i) + { + const struct rdef_field *rdef_field = &rdef_fields[i]; + + if ((hr = d3d12_type_init(&type->fields[i].type, rdef_field->type_offset, + type_size, section, rdef_field->offset))) + return hr; + } + } + return S_OK; } @@ -757,7 +804,7 @@ static HRESULT d3d12_variable_init(struct d3d12_variable *variable, const struct return E_OUTOFMEMORY; } - return d3d12_type_init(&variable->type, rdef_variable->type_offset, type_size, section); + return d3d12_type_init(&variable->type, rdef_variable->type_offset, type_size, section, 0); } static HRESULT d3d12_buffer_init(struct d3d12_buffer *buffer, const struct rdef_buffer *rdef_buffer, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626
Conor McCarthy (@cmccarthy) commented about libs/vkd3d-utils/reflection.c:
+ uint16_t type; + uint32_t compile_flags; + uint32_t creator_offset; +}; + +struct rdef_rd11 +{ + uint32_t magic; + uint32_t header_size; + uint32_t buffer_size; + uint32_t binding_size; + uint32_t variable_size; + uint32_t type_size; + uint32_t field_size; + /* Always zero. Possibly either padding or a null terminator? */ + uint32_t zero; Maybe `field_size` is `uint64_t`? It has an 8-byte alignment.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626#note_60151
On Tue Feb 6 03:56:46 2024 +0000, Conor McCarthy wrote:
Maybe `field_size` is `uint64_t`? It has an 8-byte alignment. It would be very weird, semantically. My best guess is that it's intentionally left as zero to give the structure room to expand (never mind that that's redundant with at least three other things).
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626#note_60153
```diff +static HRESULT parse_rdef(struct d3d12_reflection *reflection, const struct vkd3d_shader_dxbc_section_desc *section) +{ + const struct rdef_header *header; + HRESULT hr; + + if (!(header = get_section_data(section, 0, sizeof(*header)))) + return E_INVALIDARG; + + reflection->desc.ConstantBuffers = header->buffer_count; + + if (header->buffer_count) + { + const struct rdef_buffer *rdef_buffers; + + if (!(rdef_buffers = get_section_data(section, header->buffers_offset, + header->buffer_count * sizeof(*rdef_buffers)))) + return E_INVALIDARG; + + if (!(reflection->buffers = vkd3d_calloc(header->buffer_count, sizeof(*reflection->buffers)))) + return E_OUTOFMEMORY; + + for (uint32_t i = 0; i < header->buffer_count; ++i) + { + if ((hr = d3d12_buffer_init(&reflection->buffers[i], &rdef_buffers[i], section))) + return hr; + } + } + + return S_OK; +} ```
The "header->buffer_count * sizeof(*rdef_buffers)" multiplication can overflow, but when it does, the vkd3d_calloc() call will fail by virtue of sizeof(*reflection->buffers) being the same size as or larger than sizeof(*rdef_buffers). I suppose that works, but it seems a bit unfortunate/fragile. vkd3d-shader's require_space() doesn't have that issue. Perhaps we can do better than require_space(), but I'm not immediately convinced get_section_data() is it. It also seems somewhat undesirable to have to pass vkd3d_shader_dxbc_section_desc structures around, instead of e.g. vkd3d_shader_code structures.
```diff + for (unsigned int i = 0; i < dxbc_desc.section_count; ++i) + { + const struct vkd3d_shader_dxbc_section_desc *section = &dxbc_desc.sections[i]; + + if (section->tag == TAG_RDEF && FAILED(hr = parse_rdef(reflection, section))) + { + vkd3d_shader_free_scan_signature_info(&reflection->signature_info); + vkd3d_shader_free_dxbc(&dxbc_desc); + return hr; + } + } ```
Is that the right behaviour for inputs with multiple RDEF sections? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626#note_60195
The "header->buffer_count \* sizeof(\*rdef_buffers)" multiplication can overflow, but when it does, the vkd3d_calloc() call will fail by virtue of sizeof(\*reflection->buffers) being the same size as or larger than sizeof(\*rdef_buffers). I suppose that works, but it seems a bit unfortunate/fragile.
vkd3d-shader's require_space() doesn't have that issue. Perhaps we can do better than require_space(), but I'm not immediately convinced get_section_data() is it.
Eh, I wasn't aware of require_space(). I'll copy it into get_section_data(). I do like get_section_data() as a helper in general though, which is to say I like having a function that just returns a pointer to a strongly typed struct. If you dislike it I'll revert to the pattern in dxbc.c, though.
It also seems somewhat undesirable to have to pass vkd3d_shader_dxbc_section_desc structures around, instead of e.g. vkd3d_shader_code structures.
Good point.
Is that the right behaviour for inputs with multiple RDEF sections?
No, it was just yet another thing copied directly from Wine. I'll add a FIXME. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626#note_60334
I think I like this better than !606. Could you just add a few tests? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626#note_60446
I do like get_section_data() as a helper in general though, which is to say I like having a function that just returns a pointer to a strongly typed struct. If you dislike it I'll revert to the pattern in dxbc.c, though.
Well, it returns a void pointer, at least in its current form. I think I understand what you mean though. It never feels quite right to me to read/write complete structures like that, but I suppose that at this point there are few enough big endian machines left that perhaps we don't need to be overly concerned with them. In any case, the main objection here is that I think we should either pass a separate count and element size to get_section_data(), or use some kind of helper to do the multiplication safely before passing it to get_section_data(). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626#note_60467
It never feels quite right to me to read/write complete structures like that, but I suppose that at this point there are few enough big endian machines left that perhaps we don't need to be overly concerned with them.
Don't we assume little endian more or less everywhere already? That's not really related reading or writing structures as opposed to individual fields. BTW, I'd totally be in favor of being portable on endianness too, but that probably would have to be a project-wide decision now. Relatedly, and for similar reasons, even using `memcpy()` as suggested in https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/627#note_60196 doesn't feel right to me. IIUC that's not safe for the C standard. You are assuming that two structures with the same prefix have the same layout for those fields (different is the case of a structure having the other one as first field). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626#note_60485
It never feels quite right to me to read/write complete structures like that, but I suppose that at this point there are few enough big endian machines left that perhaps we don't need to be overly concerned with them.
Don't we assume little endian more or less everywhere already? That's not really related reading or writing structures as opposed to individual fields. BTW, I'd totally be in favor of being portable on endianness too, but that probably would have to be a project-wide decision now.
We don't support big endian targets right now, but I think the number of places that would need to be adjusted is relatively modest. In particular, for things that are built on top of read_u32()/put_u32() and so on, it's in principle just a matter of updating those primitives. But well, as I said, perhaps we don't need to be overly concerned with big endian at this point.
Relatedly, and for similar reasons, even using `memcpy()` as suggested in https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/627#note_60196 doesn't feel right to me. IIUC that's not safe for the C standard. You are assuming that two structures with the same prefix have the same layout for those fields (different is the case of a structure having the other one as first field).
Not in general, no, but I think it's fairly well defined when that's safe and when it isn't. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626#note_60498
On Wed Feb 7 17:51:01 2024 +0000, Giovanni Mascellani wrote:
I think I like this better than !606. Could you just add a few tests? I have tests, but they also require 5 patches of HLSL fixes. I didn't really want to send a 15-patch series, but at this point it's probably better than the alternative...
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626#note_60510
On Wed Feb 7 17:51:00 2024 +0000, Zebediah Figura wrote:
I have tests, but they also require 5 patches of HLSL fixes. I didn't really want to send a 15-patch series, but at this point it's probably better than the alternative... I guess that tests were my only objections, if you promise to eventually send them I can accept this right now.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626#note_61260
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/626
participants (5)
-
Conor McCarthy (＠cmccarthy) -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)