I did a fairly detailed review of the first two patches in this series, and found a couple of mostly minor issues. I also did a more cursory review of the subsequent 6 patches, and at first sight those seem fine. With the issues fixed, the first 8 patches in this series are probably ready to go in, although I'd like either @giomasce or @zfigura to approve those as well.
I haven't looked at the last patch in the series in detail yet; it would probably make sense to submit the first 8 patches as a separate MR after fixing the issues.
+static struct vkd3d_shader_param_node *shader_param_allocator_node_create( + struct vkd3d_shader_param_allocator *allocator) +{ + struct vkd3d_shader_param_node *node; + + if (!(node = vkd3d_malloc(sizeof(*node) + allocator->count * allocator->stride))) + return NULL; + node->next = NULL; + return node; +}
It doesn't bother me too much, but we'd normally write that allocation as: ```c vkd3d_malloc(offsetof(struct vkd3d_shader_param_node, param[allocator->count * allocator->stride])) ```
+static bool shader_param_allocator_init(struct vkd3d_shader_param_allocator *allocator, + unsigned int count, unsigned int stride) +{ + allocator->count = max(count, 4); + allocator->stride = stride; + allocator->head = shader_param_allocator_node_create(allocator); + allocator->current = allocator->head; + allocator->index = 0; + + return true; +}
The return value of shader_param_allocator_node_create() is unchecked above.
I think there's an argument for just passing the size to shader_param_allocator_node_create(). I.e.: ```c count = max(count, 4); allocator->count = count; ... allocator->head = shader_param_allocator_node_create(count * stride); ``` mostly because that would avoid passing around a pointer to "allocator" while it's still being constructed. That does slightly complicate shader_param_allocator_get() though, so perhaps the choice is not completely straightforward.
+void *shader_param_allocator_get(struct vkd3d_shader_param_allocator *allocator, unsigned int count) +{ + void *params; + + if (!count) + return NULL; + + if (allocator->index + count > allocator->count) + { + struct vkd3d_shader_param_node *next = shader_param_allocator_node_create(allocator); + allocator->current->next = next; + allocator->current = next; + allocator->index = 0; + } + + params = &allocator->current->param[allocator->index * allocator->stride]; + allocator->index += count; + return params; +}
The expression "allocator->index + count" can overflow. Rewriting the condition as "if (count > allocator->count - allocator->index)" avoids that issue.
The return value of shader_param_allocator_node_create() is unchecked.
+bool shader_instruction_array_init(struct vkd3d_shader_instruction_array *instructions, unsigned int reserve) +{ + memset(instructions, 0, sizeof(*instructions)); + /* Size the parameter initial allocations so they are large enough for most shaders. */ + return shader_instruction_array_reserve(instructions, reserve) + && shader_param_allocator_init(&instructions->dst_params, reserve - reserve / 8u, sizeof(*instructions->elements->dst)) + && shader_param_allocator_init(&instructions->src_params, reserve * 2u, sizeof(*instructions->elements->src)); +}
This would leak "instructions->elements" and "instructions->dst_params.head" if the second shader_param_allocator_init() call were to fail.
+void shader_instruction_array_destroy(struct vkd3d_shader_instruction_array *instructions, bool destroy_instructions) +{ + unsigned int i; + + if (destroy_instructions) + { + for (i = 0; i < instructions->count; ++i) + if (instructions->elements[i].handler_idx == VKD3DSIH_DCL_IMMEDIATE_CONSTANT_BUFFER) + vkd3d_free((void *)instructions->elements[i].declaration.icb); + } + vkd3d_free(instructions->elements); + shader_param_allocator_destroy(&instructions->dst_params); + shader_param_allocator_destroy(&instructions->src_params); +}
The "destroy_instructions" parameter is not used until much later in the series. I also think it's a little ugly; it may be preferable to just copy "icb" in shader_instruction_array_clone_instruction(). Alternatively, we could store a flag in the vkd3d_shader_instruction structure to indicate whether we have a "cloned" instruction or not.
Note that with the current scheme there's also a potential issue with reusing the original "icb" storage for VKD3DSIH_DCL_IMMEDIATE_CONSTANT_BUFFER instructions: creating new VKD3DSIH_DCL_IMMEDIATE_CONSTANT_BUFFER instructions in the normaliser would require new allocations for "icb", which would then be leaked by shader_instruction_array_destroy().
@@ -206,7 +200,8 @@ static bool shader_sm4_read_register_space(struct vkd3d_shader_sm4_parser *priv, static void shader_sm4_read_conditional_op(struct vkd3d_shader_instruction *ins, uint32_t opcode, uint32_t opcode_token, const uint32_t *tokens, unsigned int token_count, struct vkd3d_shader_sm4_parser *priv) { - shader_sm4_read_src_param(priv, &tokens, &tokens[token_count], VKD3D_DATA_UINT, &priv->src_param[0]); + shader_sm4_read_src_param(priv, &tokens, &tokens[token_count], VKD3D_DATA_UINT, + (struct vkd3d_shader_src_param *)&ins->src[0]); ins->flags = (opcode_token & VKD3D_SM4_CONDITIONAL_NZ) ? VKD3D_SHADER_CONDITIONAL_OP_NZ : VKD3D_SHADER_CONDITIONAL_OP_Z; }
Do we need that cast?
@@ -227,16 +223,23 @@ static void shader_sm4_read_shader_data(struct vkd3d_shader_instruction *ins, ui ... + if (!(icb = vkd3d_malloc(sizeof(*icb) + sizeof(*tokens) * icb_size)))
Like further above, we'd typically use `offsetof` to calculate the allocation size here.
```diff @@ -438,8 +441,9 @@ static void shader_sm4_read_dcl_global_flags(struct vkd3d_shader_instruction *in static void shader_sm5_read_fcall(struct vkd3d_shader_instruction *ins, uint32_t opcode, uint32_t opcode_token, const uint32_t *tokens, unsigned int token_count, struct vkd3d_shader_sm4_parser *priv) { - priv->src_param[0].reg.u.fp_body_idx = *tokens++; - shader_sm4_read_src_param(priv, &tokens, &tokens[token_count], VKD3D_DATA_OPAQUE, &priv->src_param[0]); + struct vkd3d_shader_src_param *src_params = (struct vkd3d_shader_src_param *)ins->src; + src_params[0].reg.u.fp_body_idx = *tokens++; + shader_sm4_read_src_param(priv, &tokens, &tokens[token_count], VKD3D_DATA_OPAQUE, &src_params[0]); }
static void shader_sm5_read_dcl_function_body(struct vkd3d_shader_instruction *ins, uint32_t opcode, ```
Like above, do we need this cast?