[PATCH 2/3] winevulkan: Support use of extensions which musn't be exposed.
Derek Lesho
dlesho at codeweavers.com
Tue May 18 09:02:47 CDT 2021
On 5/18/21 3:24 AM, Liam Middlebrook wrote:
>
>
> 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])
I'm just going off what appears in the rest of winevulkan, I don't see
any "any" statement with the brackets in make_vulkan.
>
> 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?
Yep, that was the idea, but it seems wrong now. I just looked at the
code around UNSUPPORTED_EXTENSIONS and it seems we instead only need to
hide if all the extensions are unsupported or internal-only.
> 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?
From what I can see, we don't calculate which core version we support
from the current list of unsupported extensions either. How is this any
different (We just don't expose a core version if any of the required
functions are either unsupported or internal-only).
>
> But it's probably best to add a comment either way clarifying the
> right set logic here.
Sounds good.
> Also python has a set() type if that's any easier to use here.
Yeah, it looks like that would have been ideal if the logic weren't
wrong since I could just checked if the union/intersection of the two
sets were empty. I guess I could still check if the intersection equals
the functions full set (self.extensions | WINEVULKAN_INTERNAL_EXTENSIONS
== self.extensions), but this seems a bit less clear than the any
solution I would have instead (`return self.is_required and any(x for x
in self.extensions if x not in UNEXPOSED_EXTENSIONS)`). Do you disagree?
>
>
> 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