[PATCH 2/3] winevulkan: Support use of extensions which musn't be exposed.
Liam Middlebrook
lmiddlebrook at nvidia.com
Tue May 18 02:24:07 CDT 2021
On 5/17/21 1:00 PM, Derek Lesho wrote:
> Signed-off-by: Derek Lesho <dlesho at codeweavers.com>
> ---
> This patch is dead code, being used later for VK_KHR_external_memory_fd. I was informed that commits like this are normal for winevulkan, since exposing an extension w/ stubs can break applications.
I don't agree with the wording here, so let me re-iterate what I said
when you asked me earlier today:
> I personally think that submitting known broken code (which has potential to break apps in unexpected ways) is significantly worse than submitting code is used on a later change in a patchset.
> there’s a difference between submitting something for the unknown future and submitting something where it’s used at the end of the same patchset.
> But you should be asking these questions on #winehackers not in VKx discord
I personally don't consider this dead code. Typically when I think of
dead code, it's something that was once used but is no longer. That
said, unused code with the future promise of use isn't very useful, but
as I quoted from myself above, I think that scaffolding is appropriate
given that you have a later patch in the series which makes use of it.
> ---
> dlls/winevulkan/make_vulkan | 29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
> index 7f76d328fc8..7e1d7c0f043 100755
> --- a/dlls/winevulkan/make_vulkan
> +++ b/dlls/winevulkan/make_vulkan
> @@ -120,6 +120,9 @@ UNSUPPORTED_EXTENSIONS = [
> "VK_NV_external_memory_win32",
> ]
>
> +# Extensions which aren't present on the win32 platform, but which winevulkan may use.
> +UNEXPOSED_EXTENSIONS = []
nit: Could we call this something like WINEVULKAN_INTERNAL_EXTENSIONS?
These extensions are for use only by winevulkan internally. Really the
name is fine either way just a matter of personal preference. I just
picture this in my head as "extensions for use internal to winevulkan"
rather than "extensions that winevulkan must not: expose".
> +
> # The Vulkan loader provides entry-points for core functionality and important
> # extensions. Based on vulkan-1.def this amounts to WSI extensions on 1.0.51.
> CORE_EXTENSIONS = [
> @@ -521,7 +524,7 @@ class VkEnumValue(object):
>
>
> class VkFunction(object):
> - def __init__(self, _type=None, name=None, params=[], extensions=[], alias=None):
> + def __init__(self, _type=None, name=None, params=[], alias=None):
> self.extensions = []
> self.name = name
> self.type = _type
> @@ -665,6 +668,9 @@ class VkFunction(object):
> def needs_private_thunk(self):
> return self.thunk_type == ThunkType.PRIVATE
>
> + def needs_exposed(self):
> + return not any(x for x in self.extensions if x in UNEXPOSED_EXTENSIONS)
Is this syntax correct? I'm used to seeing list comprehensions use []'s
to surround them, like so:
> return not any([x for x in self.extensions if x in UNEXPOSED_EXTENSIONS])
Also this logic is a bit confusing to read as-is. Is the check for
seeing if VkFunction.extensions intersects with UNEXPOSED_EXTENSIONS and
if there is an intersection to not expose the extension? What happens in
the case (although I'm not sure this can exist) where an extension is
marked as "unexposed" and has a function that is shared by say, Vulkan
x.y core?
But it's probably best to add a comment either way clarifying the right
set logic here. Also python has a set() type if that's any easier to use
here.
Thanks,
Liam Middlebrook
> +
> def pfn(self, prefix="p", call_conv=None, conv=False):
> """ Create function pointer. """
>
> @@ -2656,6 +2662,9 @@ class VkGenerator(object):
> if not vk_func.is_required():
> continue
>
> + if not vk_func.needs_exposed():
> + continue
> +
> if vk_func.is_global_func():
> continue
>
> @@ -2676,6 +2685,8 @@ class VkGenerator(object):
> for ext in self.registry.extensions:
> if ext["type"] != "device":
> continue
> + if ext["name"] in UNEXPOSED_EXTENSIONS:
> + continue
>
> f.write(" \"{0}\",\n".format(ext["name"]))
> f.write("};\n\n")
> @@ -2685,6 +2696,8 @@ class VkGenerator(object):
> for ext in self.registry.extensions:
> if ext["type"] != "instance":
> continue
> + if ext["name"] in UNEXPOSED_EXTENSIONS:
> + continue
>
> f.write(" \"{0}\",\n".format(ext["name"]))
> f.write("};\n\n")
> @@ -2746,6 +2759,8 @@ class VkGenerator(object):
> for vk_func in self.registry.funcs.values():
> if not vk_func.is_required():
> continue
> + if not vk_func.needs_exposed():
> + continue
> if vk_func.loader_thunk_type == ThunkType.NONE:
> continue
>
> @@ -2767,6 +2782,8 @@ class VkGenerator(object):
> continue
> if vk_func.needs_thunk() and not vk_func.needs_private_thunk():
> continue
> + if not vk_func.needs_exposed():
> + continue
>
> if vk_func.is_core_func():
> f.write("{0};\n".format(vk_func.prototype("WINAPI", prefix=prefix)))
> @@ -2874,6 +2891,8 @@ class VkGenerator(object):
> for vk_func in self.registry.funcs.values():
> if not vk_func.is_required():
> continue
> + if not vk_func.needs_exposed():
> + continue
> if vk_func.loader_thunk_type != ThunkType.PUBLIC:
> continue
>
> @@ -2883,6 +2902,8 @@ class VkGenerator(object):
> for vk_func in self.registry.device_funcs:
> if not vk_func.is_required():
> continue
> + if not vk_func.needs_exposed():
> + continue
>
> f.write(" {{\"{0}\", &{0}}},\n".format(vk_func.name))
> f.write("};\n\n")
> @@ -2891,6 +2912,8 @@ class VkGenerator(object):
> for vk_func in self.registry.phys_dev_funcs:
> if not vk_func.is_required():
> continue
> + if not vk_func.needs_exposed():
> + continue
>
> f.write(" {{\"{0}\", &{0}}},\n".format(vk_func.name))
> f.write("};\n\n")
> @@ -2899,6 +2922,8 @@ class VkGenerator(object):
> for vk_func in self.registry.instance_funcs:
> if not vk_func.is_required():
> continue
> + if not vk_func.needs_exposed():
> + continue
>
> f.write(" {{\"{0}\", &{0}}},\n".format(vk_func.name))
> f.write("};\n\n")
> @@ -2956,6 +2981,8 @@ class VkGenerator(object):
> for vk_func in self.registry.funcs.values():
> if not vk_func.is_required():
> continue
> + if not vk_func.needs_exposed():
> + continue
> if vk_func.loader_thunk_type == ThunkType.NONE:
> continue
>
>
More information about the wine-devel
mailing list