Fix struct variable size with out-of-order offsets (#264)

* Remove member_count check ParseDescriptorBlockVariableSizes starts with check * Fix struct variable size with out-of-order offsets
KhronosGroup · Apr 27, 2024 · ee5b57f · ee5b57f
1 parent 8cac559
commit ee5b57f
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 45 deletions.
diff --git a/spirv_reflect.c b/spirv_reflect.c
@@ -2711,67 +2711,67 @@ 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.
+  // 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_last_member_var->padded_size = p_last_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;
     return SPV_REFLECT_RESULT_SUCCESS;
   }
 
   // @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
@@ -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
@@ -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 }