[PATCH 3/4] winevulkan: Remove unnecessary checks in wine_vkCmdExecuteCommands

List overview All Threads

newer

older

[PATCH 1/6] include: Use public...

[PATCH 4/4] winevulkan: Regenerate...

Joshua Ashton

5 Mar 2020 5 Mar '20

5:03 p.m.

The Vulkan spec states commandBufferCount must be greater than zero and pCommandBuffers must be a valid pointer.

Signed-off-by: Joshua Ashton joshua@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

Attachments:

0003-winevulkan-Remove-unnecessary-checks-in-wine_vkCmdEx.patch (application/octet-stream — 949 bytes)

Show replies by date

Marvin

5 Mar 5 Mar

5:46 p.m.

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) ===

Liam Middlebrook

6 Mar 6 Mar

12:10 a.m.

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@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. -----------------------------------------------------------------------------------

Joshua Ashton

12:19 a.m.

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@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@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++) 

Liam Middlebrook

12:35 a.m.

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@nvidia.com mailto:lmiddlebrook@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@froggi.es <mailto:joshua@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.
-----------------------------------------------------------------------------------

1770

Age (days ago)

1771

Last active (days ago)

wine-devel@winehq.org

4 comments

3 participants

tags (0)

participants (3)

Joshua Ashton
Liam Middlebrook
Marvin