scripts: Improve SafePNextCopy for unknown structs

[spatel2-samsung]

If a pNext chain contains unknown structure, the current code will skip it by not updating prev_pNext. When a known structure is encountered later, the chain will be re-sewn to exclude the custom structure. However, if the unknown structure is at the very end, the current code will not repair the pNext pointer of prev_pNext. Thus the safe copy of the chain will still contain an unsafe pNext pointer at the end.

This commit fixes this issue by setting prev_pNext->pNext on every iteration, even if prev_pNext itself is not updated. If safe_pNext is itself a nullptr, we should still overwrite prev_pNext->pNext so as not to leave an unsafe value there. If later a known structure is seen, this nullptr will be overwritten and the current functionality will remain the same.

[CLAassistant]

All committers have signed the CLA.

[spencer-lunarg]

this is just the python script, you need to re-generate the code to apply it

[spencer-lunarg]

@spatel2-samsung it would be nice to have a test, which happy to write, could you just provide a small example (pseudocode is fine) of 1 or 2 function calls with their pNext struct that would demonstrate this issue

[spatel2-samsung]

@spatel2-samsung it would be nice to have a test, which happy to write, could you just provide a small example (pseudocode is fine) of 1 or 2 function calls with their pNext struct that would demonstrate this issue

The following pseudocode should demonstrate this. The example I used to find the issue isn't exactly this (I didn't test by calling SafePNextCopy directly, I used a debugger during the vkCreateInstance call to see the difference). I can create a full minimal example if that's better!

const PRIVATE_VK_STYPE = 1000700400 // Pick something unknown to the SafePNextCopy function

vkInstanceCreateInfo = {
    .sType = VK_STRUCTURE_TYPE_INSTANCE_CREATE_INFO,
    .pNext = nullptr,
    ...
}

vkPrivateType = {
    .sType = PRIVATE_VK_STYPE,
    .pNext = nullptr,
    ...
}

vkValidationFeatures = {
    .sType = VK_STRUCTURE_TYPE_VALIDATION_FEATURES_EXT,
    .pNext = nullptr,
    ...
}

// Test 1: Private stype is in the middle. This passes even without my patch
vkInstanceCreateInfo.pNext = &vkPrivateType;
vkPrivateType.pNext = &vkValidationFeatures;
vkValidationFeatures.pNext = nullptr;
safe_pNext = SafePNextCopy(&vkInstanceCreateInfo)
assert safe_pNext->sType == VK_STRUCTURE_TYPE_INSTANCE_CREATE_INFO
assert safe_pNext->pNext->sType == VK_STRUCTURE_TYPE_VALIDATION_FEATURES_EXT
assert safe_pNext->pNext->pNext == nullptr

// Test 2: Private stype is at the end. This will fail without my patch
vkInstanceCreateInfo.pNext = &vkValidationFeatures;
vkValidationFeatures.pNext = &vkPrivateType;
vkPrivateType.pNext = nullptr;
safe_pNext = SafePNextCopy(&vkInstanceCreateInfo)
assert safe_pNext->sType == VK_STRUCTURE_TYPE_INSTANCE_CREATE_INFO
assert safe_pNext->pNext->sType == VK_STRUCTURE_TYPE_VALIDATION_FEATURES_EXT
assert safe_pNext->pNext->pNext == nullptr // This will fail. It will be equal to &vkPrivateType currently

[spencer-lunarg]

@spatel2-samsung looks good (and I tested it)

1. Please add the comment

+        // need to make sure pass through a private struct
+        // https://github.com/KhronosGroup/Vulkan-Utility-Libraries/pull/385#issuecomment-4490691826
         if (prev_pNext) {
             prev_pNext->pNext = (VkBaseOutStructure*)safe_pNext;
         }

2. Run the code gen (or cheat and copy and paste the fix into src/vulkan/vk_safe_struct_utils.cpp and then I can merge this in

[spatel2-samsung]

Thanks for the review @spencer-lunarg! I've done as you asked. Side note: I tried to run the code gen properly, but following the directions on the doc you linked earlier did not work for me. The example there says to use external/Vulkan-Headers/registry as the registry path, but my actual path was the build-specific external/Debug/64/Vulkan-Headers/registry. And the contents of that folder was not in the proper format that the script seems to prefer. It wanted vk.xml to have a sibling directory registry which contains reg.py inside, but the actual structure I had was that vk.xml and reg.py were siblings. I did the cheat you suggested instead, but thought I should mention

[spencer-lunarg]

ya, I work on Linux, realize the Windows build instructions are not up 100%... but also we don't need to make changes to VUL that often

[spatel2-samsung]

ya, I work on Linux, realize the Windows build instructions are not up 100%... but also we don't need to make changes to VUL that often

Actually I was trying this on Linux too. But agreed that it's not that important either way 😅