[PATCH 3/4] winevulkan: Remove unnecessary checks in wine_vkCmdExecuteCommands
The Vulkan spec states commandBufferCount must be greater than zero and pCommandBuffers must be a valid pointer. Signed-off-by: Joshua Ashton <joshua(a)froggi.es> --- dlls/winevulkan/vulkan.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index 1cc1b4f61f..f767b86dc4 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -517,9 +517,6 @@ void WINAPI wine_vkCmdExecuteCommands(VkCommandBuffer buffer, uint32_t count, TRACE("%p %u %p\n", buffer, count, buffers); - if (!buffers || !count) - return; - tmp_buffers = WINEVULKAN_ALLOCA(count * sizeof(*tmp_buffers)); for (i = 0; i < count; i++) -- 2.25.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=66442 Your paranoid android. === debiant (build log) === error: patch failed: dlls/winevulkan/vulkan_private.h:34 error: patch failed: dlls/winevulkan/make_vulkan:666 error: patch failed: dlls/winevulkan/vulkan.c:520 error: patch failed: dlls/winevulkan/vulkan.c:517 Task: Patch failed to apply === debiant (build log) === error: patch failed: dlls/winevulkan/vulkan_private.h:34 error: patch failed: dlls/winevulkan/make_vulkan:666 error: patch failed: dlls/winevulkan/vulkan.c:520 error: patch failed: dlls/winevulkan/vulkan.c:517 Task: Patch failed to apply
I'm not sure that we can remove this if we keep both the alloca and heap_alloc implementations in-place, to my understanding RtlAllocateHeap would run into issues when given an input size of zero. Thanks, Liam Middlebrook On 3/5/20 9:03 AM, Joshua Ashton wrote:
The Vulkan spec states commandBufferCount must be greater than zero and pCommandBuffers must be a valid pointer.
Signed-off-by: Joshua Ashton <joshua(a)froggi.es> --- dlls/winevulkan/vulkan.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index 1cc1b4f61f..f767b86dc4 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -517,9 +517,6 @@ void WINAPI wine_vkCmdExecuteCommands(VkCommandBuffer buffer, uint32_t count,
TRACE("%p %u %p\n", buffer, count, buffers);
- if (!buffers || !count) - return; - tmp_buffers = WINEVULKAN_ALLOCA(count * sizeof(*tmp_buffers));
for (i = 0; i < count; i++)
----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
A size of zero violates the Vulkan spec so it should be a non-issue. - Josh 🐸 ---- On Fri, 06 Mar 2020 00:10:40 +0000 Liam Middlebrook <mailto:lmiddlebrook(a)nvidia.com> wrote ---- I'm not sure that we can remove this if we keep both the alloca and heap_alloc implementations in-place, to my understanding RtlAllocateHeap would run into issues when given an input size of zero. Thanks, Liam Middlebrook On 3/5/20 9:03 AM, Joshua Ashton wrote:
The Vulkan spec states commandBufferCount must be greater than zero and pCommandBuffers must be a valid pointer.
Signed-off-by: Joshua Ashton <mailto:joshua(a)froggi.es> --- dlls/winevulkan/vulkan.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index 1cc1b4f61f..f767b86dc4 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -517,9 +517,6 @@ void WINAPI wine_vkCmdExecuteCommands(VkCommandBuffer buffer, uint32_t count,
TRACE("%p %u %p\n", buffer, count, buffers);
- if (!buffers || !count) - return; - tmp_buffers = WINEVULKAN_ALLOCA(count * sizeof(*tmp_buffers));
for (i = 0; i < count; i++)
----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
Sure, so the implementation can choose to handle this undefined behavior how it likes. I usually like to err on the side of failing gracefully rather than catastrophically (that being said I don't think the cases I outline below would reasonably be classified as catastrophic failure, which I'd normally attribute to hangs, or system-level crashes, nothing that winevulkan should hopefully be getting itself into). The decision to be made is whether the assumed boost in performance is worth the downsides of calling RtlAllocateHeap with a size of zero. I haven't taken the time to inspect RtlAllocateHeap carefully but I would presume there are two cases which could happen here: 1. We successfully allocate the memory, leave tmp_buffers as uninitialized, and then call to the driver, getting back whatever undefined behavior the driver has chosen is appropriate in this case. 2. We fail to allocate the memory and an ERR is logged indicated the failure to allocate memory. While I think it's less-likely I'm more concerned about case 2, because it could confuse an end-user or developer into thinking that a legitimate out-of-memory case, or some WINE bug is happening, rather than invalid usage of the Vulkan API on the application's part. Thanks, Liam Middlebrook On 3/5/20 4:19 PM, Joshua Ashton wrote:
A size of zero violates the Vulkan spec so it should be a non-issue.
- Josh 🐸
---- On Fri, 06 Mar 2020 00:10:40 +0000 *Liam Middlebrook <lmiddlebrook(a)nvidia.com <mailto:lmiddlebrook(a)nvidia.com>>* wrote ----
I'm not sure that we can remove this if we keep both the alloca and heap_alloc implementations in-place, to my understanding RtlAllocateHeap would run into issues when given an input size of zero.
Thanks,
Liam Middlebrook
On 3/5/20 9:03 AM, Joshua Ashton wrote: > The Vulkan spec states commandBufferCount must be greater than zero and pCommandBuffers must be a valid pointer. > > Signed-off-by: Joshua Ashton <joshua(a)froggi.es <mailto:joshua(a)froggi.es>> > --- > dlls/winevulkan/vulkan.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c > index 1cc1b4f61f..f767b86dc4 100644 > --- a/dlls/winevulkan/vulkan.c > +++ b/dlls/winevulkan/vulkan.c > @@ -517,9 +517,6 @@ void WINAPI wine_vkCmdExecuteCommands(VkCommandBuffer buffer, uint32_t count, > > TRACE("%p %u %p\n", buffer, count, buffers); > > - if (!buffers || !count) > - return; > - > tmp_buffers = WINEVULKAN_ALLOCA(count * sizeof(*tmp_buffers)); > > for (i = 0; i < count; i++) >
----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
participants (3)
-
Joshua Ashton -
Liam Middlebrook -
Marvin