From 444154c7fdb266391366ac69eeb05360d9bb796f Mon Sep 17 00:00:00 2001 From: spencer-lunarg Date: Thu, 25 Apr 2024 19:42:35 +0900 Subject: [PATCH 1/2] Remove member_count check ParseDescriptorBlockVariableSizes starts with check --- spirv_reflect.c | 78 ++++++++++++++++++++++++------------------------- 1 file changed, 38 insertions(+), 40 deletions(-) diff --git a/spirv_reflect.c b/spirv_reflect.c index 99d71bf..b0ecf53 100644 --- a/spirv_reflect.c +++ b/spirv_reflect.c @@ -2711,59 +2711,57 @@ static SpvReflectResult ParseDescriptorBlockVariableSizes(SpvReflectPrvParser* p } } - if (p_var->member_count > 0) { - // Structs can offset order don't need to match the index order, so first order by offset - // example: - // OpMemberDecorate %struct 0 Offset 4 - // OpMemberDecorate %struct 1 Offset 0 - SpvReflectBlockVariable** pp_member_offset_order = - (SpvReflectBlockVariable**)calloc(p_var->member_count, sizeof(SpvReflectBlockVariable*)); - if (IsNull(pp_member_offset_order)) { - return SPV_REFLECT_RESULT_ERROR_ALLOC_FAILED; - } - - uint32_t bottom_bound = 0; - for (uint32_t i = 0; i < p_var->member_count; ++i) { - uint32_t lowest_offset = UINT32_MAX; - uint32_t member_index = 0; - for (uint32_t j = 0; j < p_var->member_count; ++j) { - const uint32_t offset = p_var->members[j].offset; - if (offset < lowest_offset && offset >= bottom_bound) { - member_index = j; - lowest_offset = offset; - } - } - pp_member_offset_order[i] = &p_var->members[member_index]; - bottom_bound = lowest_offset + 1; // 2 index can't share the same offset - } + // Structs can offset order don't need to match the index order, so first order by offset + // example: + // OpMemberDecorate %struct 0 Offset 4 + // OpMemberDecorate %struct 1 Offset 0 + SpvReflectBlockVariable** pp_member_offset_order = + (SpvReflectBlockVariable**)calloc(p_var->member_count, sizeof(SpvReflectBlockVariable*)); + if (IsNull(pp_member_offset_order)) { + return SPV_REFLECT_RESULT_ERROR_ALLOC_FAILED; + } - // Parse padded size using offset difference for all member except for the last entry... - for (uint32_t i = 0; i < (p_var->member_count - 1); ++i) { - SpvReflectBlockVariable* p_member_var = pp_member_offset_order[i]; - SpvReflectBlockVariable* p_next_member_var = pp_member_offset_order[i + 1]; - p_member_var->padded_size = p_next_member_var->offset - p_member_var->offset; - if (p_member_var->size > p_member_var->padded_size) { - p_member_var->size = p_member_var->padded_size; - } - if (is_parent_rta) { - p_member_var->padded_size = p_member_var->size; + uint32_t bottom_bound = 0; + for (uint32_t i = 0; i < p_var->member_count; ++i) { + uint32_t lowest_offset = UINT32_MAX; + uint32_t member_index = 0; + for (uint32_t j = 0; j < p_var->member_count; ++j) { + const uint32_t offset = p_var->members[j].offset; + if (offset < lowest_offset && offset >= bottom_bound) { + member_index = j; + lowest_offset = offset; } } + pp_member_offset_order[i] = &p_var->members[member_index]; + bottom_bound = lowest_offset + 1; // 2 index can't share the same offset + } - // ...last entry just gets rounded up to near multiple of SPIRV_DATA_ALIGNMENT, which is 16 and - // subtract the offset. - SpvReflectBlockVariable* p_member_var = pp_member_offset_order[p_var->member_count - 1]; - p_member_var->padded_size = RoundUp(p_member_var->offset + p_member_var->size, SPIRV_DATA_ALIGNMENT) - p_member_var->offset; + // Parse padded size using offset difference for all member except for the last entry... + for (uint32_t i = 0; i < (p_var->member_count - 1); ++i) { + SpvReflectBlockVariable* p_member_var = pp_member_offset_order[i]; + SpvReflectBlockVariable* p_next_member_var = pp_member_offset_order[i + 1]; + p_member_var->padded_size = p_next_member_var->offset - p_member_var->offset; if (p_member_var->size > p_member_var->padded_size) { p_member_var->size = p_member_var->padded_size; } if (is_parent_rta) { p_member_var->padded_size = p_member_var->size; } + } - SafeFree(pp_member_offset_order); + // ...last entry just gets rounded up to near multiple of SPIRV_DATA_ALIGNMENT, which is 16 and + // subtract the offset. + SpvReflectBlockVariable* p_member_var = pp_member_offset_order[p_var->member_count - 1]; + p_member_var->padded_size = RoundUp(p_member_var->offset + p_member_var->size, SPIRV_DATA_ALIGNMENT) - p_member_var->offset; + if (p_member_var->size > p_member_var->padded_size) { + p_member_var->size = p_member_var->padded_size; + } + if (is_parent_rta) { + p_member_var->padded_size = p_member_var->size; } + SafeFree(pp_member_offset_order); + // If buffer ref, sizes are same as uint64_t if (is_parent_ref) { p_var->size = p_var->padded_size = 8; From 02b5cc12611659cbf4a2e9e5ef0db83ab06e560e Mon Sep 17 00:00:00 2001 From: spencer-lunarg Date: Thu, 25 Apr 2024 19:48:30 +0900 Subject: [PATCH 2/2] Fix struct variable size with out-of-order offsets --- spirv_reflect.c | 14 ++++++++------ tests/glsl/struct_offset_order.spv.yaml | 4 ++-- .../push_constants/non_zero_block_offset.spv.yaml | 4 ++-- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/spirv_reflect.c b/spirv_reflect.c index b0ecf53..b4f6bc1 100644 --- a/spirv_reflect.c +++ b/spirv_reflect.c @@ -2751,13 +2751,15 @@ static SpvReflectResult ParseDescriptorBlockVariableSizes(SpvReflectPrvParser* p // ...last entry just gets rounded up to near multiple of SPIRV_DATA_ALIGNMENT, which is 16 and // subtract the offset. - SpvReflectBlockVariable* p_member_var = pp_member_offset_order[p_var->member_count - 1]; - p_member_var->padded_size = RoundUp(p_member_var->offset + p_member_var->size, SPIRV_DATA_ALIGNMENT) - p_member_var->offset; - if (p_member_var->size > p_member_var->padded_size) { - p_member_var->size = p_member_var->padded_size; + // last entry == entry with largest offset value + SpvReflectBlockVariable* p_last_member_var = pp_member_offset_order[p_var->member_count - 1]; + p_last_member_var->padded_size = + RoundUp(p_last_member_var->offset + p_last_member_var->size, SPIRV_DATA_ALIGNMENT) - p_last_member_var->offset; + if (p_last_member_var->size > p_last_member_var->padded_size) { + p_last_member_var->size = p_last_member_var->padded_size; } if (is_parent_rta) { - p_member_var->padded_size = p_member_var->size; + p_last_member_var->padded_size = p_last_member_var->size; } SafeFree(pp_member_offset_order); @@ -2769,7 +2771,7 @@ static SpvReflectResult ParseDescriptorBlockVariableSizes(SpvReflectPrvParser* p } // @TODO validate this with assertion - p_var->size = p_var->members[p_var->member_count - 1].offset + p_var->members[p_var->member_count - 1].padded_size; + p_var->size = p_last_member_var->offset + p_last_member_var->padded_size; p_var->padded_size = p_var->size; return SPV_REFLECT_RESULT_SUCCESS; diff --git a/tests/glsl/struct_offset_order.spv.yaml b/tests/glsl/struct_offset_order.spv.yaml index a27b786..98c1d09 100644 --- a/tests/glsl/struct_offset_order.spv.yaml +++ b/tests/glsl/struct_offset_order.spv.yaml @@ -323,8 +323,8 @@ all_block_variables: name: "ubo2" offset: 0 absolute_offset: 0 - size: 16 - padded_size: 16 + size: 32 + padded_size: 32 decorations: 0x00000000 # NONE numeric: scalar: { width: 0, signedness: 0 } diff --git a/tests/push_constants/non_zero_block_offset.spv.yaml b/tests/push_constants/non_zero_block_offset.spv.yaml index de52835..c3ebbf8 100644 --- a/tests/push_constants/non_zero_block_offset.spv.yaml +++ b/tests/push_constants/non_zero_block_offset.spv.yaml @@ -211,8 +211,8 @@ all_block_variables: name: "pc" offset: 4 absolute_offset: 0 - size: 8 - padded_size: 8 + size: 32 + padded_size: 32 decorations: 0x00000000 # NONE numeric: scalar: { width: 0, signedness: 0 }