Closed exaexa closed 1 year ago
Hi, thanks for raising the issue.
The correct usage is to restrict the memory_types
field according to the memory_type_count
field of the PhysicalDeviceMemoryProperties
. That is because internally the specification uses a tuple of fixed size into which it writes elements, along with count values (both memory_type_count
and memory_heap_count
for heaps). This checks out with the output from vulkaninfo
.
While the workaround is clear, it would make for less confusion to just special-case this struct/API call to have variable-size Vector
s for heaps and memory types that are indexed with the right count from the start (such that you can rely on the command you pasted without having to check for the counts manually).
hi! thanks for quick reply. I kindof figured I should simply not read more items than what's specified in memory_type/heap_count
, otoh it is certainly a bit surprising that there's actual vector with uninitialized memory getting returned. Perhaps a small warning in documentation would suffice.
(Btw how is this different from other array-returning functions (such as the queries for queue families) that seem to behave just right?)
(Btw how is this different from other array-returning functions (such as the queries for queue families) that seem to behave just right?)
The difference is in the API. vkGetPhysicalDeviceMemoryProperties
does not allow you to query a count, initialize the array, and query for data; it just retrieves the data in one go. I don't know why they adopted this particular design, but it's confusing for sure.
As of 1.3.207, it seems that the issue with bounded arrays containing garbage concerns the following structures:
VkDeviceGroupPresentCapabilitiesKHR
VkPhysicalDeviceGroupProperties
VkQueueFamilyGlobalPriorityPropertiesKHR
VkPhysicalDeviceMemoryBudgetPropertiesEXT
VkPhysicalDeviceMemoryProperties
Ideally, instead of having docs about this (since there are lots of things described in the docs already) we should replace these fixed-size tuples with Vector
s that contain only valid elements.
Hi, thanks for the detailed explanation, actually when compared to vulkan API this makes much more sense now. I guess the methods in question aren't particularly performance-center-ish so a bit of extra processing for making the vectors look nice won't be a problem right? (Perhaps returning a correct "view" into the vector should be sufficient?)
That's what I think as well, as these structures look like they'll be used mostly in application setup/initialization. Returning a view assumes that the original vector exists somewhere, so I think it would be easier to remove the original fields (count + fixed-size tuple) to replace that with a vector. Though there are several possibilities, we should probably take whichever is easiest on the generator code.
:thinking: looks like the structures aren't static, at least SwiftShader is copying the data out: https://github.com/google/swiftshader/blob/master/src/Vulkan/libVulkan.cpp#L672-L677
I guess that depends what the vk::PhysicalDevice::GetMemoryProperties()
does, pMemoryProperties
is intended to be a pointer filled with new data. Or maybe I didn't understand your comment?
Ah yes I thought that the data would be laying somewhere indefinitely and we could just safely pick a view for them. Unfortunately not the case :sweat_smile:
Closing this to avoid hanging issues, thanks!
Hi, I'm getting what seems to be an overflowing buffer reads when trying to enumerate the available physical device memory types.
Reduced to the simplest reproducer I know the problem looks like this:
The command lists a few valid memory types, but these are followed by garbage that looks like random data picked from uninitialized memory, such as:
The beginning of the listing checks with the
vulkaninfo
, the rest seems to be garbage.I'll try to debug what's wrong and report back. Is there any guess on whether this is a possible "premature GC" issue?
Thanks! -mk
Relevant vulkaninfo section: