Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pNext chains unproperly copied in VulkanStateTracker (Issue #1269) #1363

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

marius-pelegrin-arm
Copy link
Contributor

This commit fixes the issue #1269

The first idea was to modify the UnwrapStruct*Handles functions to allow more flexibility on what is unwrapped (Vulkan handles, application owned pointers or both).

But I couldn't find a way to properly modify these functions without either altering their behavior, their performance, or having to dig into deeper fundamental changes.

So instead, this commit simply add two new generated files, generated_vulkan_struct_trackers header and body.

The idea is to implement a function TrackStruct and its overload over all Vulkan structs, that deep-copy the structs by also copying application owned dynamically allocated memory.
This way, the copy returned by TrackStruct can be kept indefinitely without undefined behavior, and not just in the time of a call.

All the other file changes are to integrate this new call.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 87259.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3533 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3533 passed.

@charles-lunarg
Copy link
Contributor

Haven't reviewed the code at all, but at first glance this is the same concept as the 'safe structs' in Vulkan-ValidationLayers. I previously considered moving the safe structs codegen from validation layers over to the Vulkan-Utility-Libraries, but dismissed it as just needless moving of code. But seeing that the same need appears in gfxreconstruct, that makes it clear that its useful for more than just validation layers. Definitely not a priority, but good to know that many repositories could take advantages of a "safe struct" utility library.

@charles-lunarg
Copy link
Contributor

My initial code review says that this change is good, and running it through vulkaninfo didn't find any problems. That said, I haven't tried using the repro case and would like to give that a whirl before any final approvals.

@MarkY-LunarG MarkY-LunarG self-requested a review January 25, 2024 20:33
marius-pelegrin-arm and others added 2 commits January 30, 2024 12:55
This commit fixes the issue LunarG#1269

The first idea was to modify the `UnwrapStruct*Handles` functions
to allow more flexibility on what is unwrapped (Vulkan handles,
application owned pointers or both).

But I couldn't find a way to properly modify these functions
without either altering their behavior, their performance, or having
to dig into deeper changes.

So instead, this commit simply add two new generated files,
`generated_vulkan_struct_trackers` header and body.

The idea is to implement a function `TrackStruct` and its overload
over all Vulkan structs, that deep-copy the structs by also copying
application owned dynamically allocated memory.
This way, the copy returned by `TrackStruct` can be kept indefinitely
without undefined behavior, and not just in the time of a call.

All the other file changes are to integrate this new call.

Change-Id: I25f4408d7d7a8a5f4a3d962f8951672129e88647
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 125116.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3712 running.

@MarkY-LunarG
Copy link
Contributor

Verified that this did fix the issue with the repro case I have.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3712 passed.

@MarkY-LunarG MarkY-LunarG merged commit 871f506 into LunarG:dev Feb 2, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants