ash-rs / ash

Vulkan bindings for Rust
Apache License 2.0
1.76k stars 186 forks source link

Provide mutable access to function-pointer tables #875

Closed Neo-Zhixing closed 3 months ago

Neo-Zhixing commented 4 months ago

Sometimes it may be necessary to patch function pointer values at runtime, for example when integrating third-party SDKs. This PR exposes mutable references to the function pointers, allowing applications to modify their values.

Ralith commented 4 months ago

Can you elaborate on why this might be necessary? Maybe link the docs of an example SDK? Vulkan has existing mechanisms for this sort of thing.

MarijnS95 commented 4 months ago

Besides not understanding what this is useful for in the first place, the same workaround/hack can be achieved with the from_parts constructors and some FRU syntax?

Neo-Zhixing commented 4 months ago

@Ralith One example may be the NVIDIA Streamline SDK: https://github.com/NVIDIAGameWorks/Streamline/blob/main/docs/ProgrammingGuideManualHooking.md

They call for hooking a subset of the Vulkan API from sl.interposer.dll rather than vulkan-1.dll, then dynamically checking which function to call depending on if the Streamline SDK was enabled or not. In my case, it's far easier to do this if I had mutable access to ash's function pointers so that I can simply replace them.


Another case where this might be useful:

Some extensions are promoted into Vulkan core. For example, vkQueueSubmit2 was added in VK_KHR_syncronization2 and later promoted to Vulkan 1.3.

So, if Vulkan 1.3 was enabled, VK_KHR_syncronization2 do not need to be enabled, and one could simply call vkQueueSubmit2 from ash::Device. However, if Vulkan 1.2 was enabled, one will have to enable VK_KHR_syncronization2 and call this function from ash::extensions::khr::Syncronization2. In this case, it's far easier to simply patch the Vulkan 1.3 function table in ash::Device to point to the function pointers obtained from ash::extensions::khr::Syncronization2 instead of having to choose which one to call by checking the enabled Vulkan version.

Neo-Zhixing commented 4 months ago

@MarijnS95 While from_parts could certainly be useful, without into_parts we can't really do much, because we can't obtain the ownership of those function pointer tables from an ash::Device.

Ralith commented 4 months ago

One example may be the NVIDIA Streamline SDK

the best approach is to dynamically load sl.interposer.dll instead of vulkan-1.dll and use vkGetDeviceProcAddr and vkGetInstanceProcAddr provided by the SL.

It sounds like you should call ash::Entry::load_from("sl.interposer.dll"). Then no manual manipulation is required.

it's far easier to simply patch the Vulkan 1.3 function table in ash::Device to point to the function pointers obtained from

Extensions are often changed when they are merged into core. I would strongly discourage doing this.

without into_parts we can't really do much, because we can't obtain the ownership of those function pointer tables from an ash::Device.

You'd load them yourself and call Device::from_parts_1_3.

MarijnS95 commented 4 months ago

sl.interposer.dll seems to behave exactly like a Vulkan layer, having no overhead for non-patched functions beyond initially returning them from the next "layer in the chain" via vkGet*ProcAddr.


Quite a few stabilized extensions seem to have been incorporated as-is, complete with type aliases and identical function signatures. Barring semantical implementation details. Overwriting pointers to extensions might be the go-to solution for an engine here and there, but managing that complete with checking and enabling the right Vulkan version or extensions should be left to higher level Vulkan wrappers.


While from_parts could certainly be useful, without into_parts we can't really do much, because we can't obtain the ownership of those function pointer tables from an ash::Device.

No need to "take ownership" when the Fn structs are Cloneable. Together with #[inline] on the getters, the compiler might be able to "optimize" this in release when deconstructing an owned Device and fully recreating it.

MarijnS95 commented 4 months ago

Also note that we've previously had a contributor painstakingly clean up function ordering across manually-implemented wrappers, and this PR is adding the new fp_mut randomly ordered across impl blocks. We shouldn't undo their hard work.

Neo-Zhixing commented 4 months ago

@Ralith

It sounds like you should call ash::Entry::load_from("sl.interposer.dll").

Loading Vulkan from sl.interposer.dll is just one way for integration, and the NVIDIA docs did recommend loading only the patched functions from sl.interposer.dll. Let's say I'm using another library that patches some other Vulkan function in the exact same way as Streamline. Which DLL would I load then?

Extensions are often changed when they are merged into core. I would strongly discourage doing this. Overwriting pointers to extensions might be the go-to solution for an engine here and there, but managing that complete with checking and enabling the right Vulkan version or extensions should be left to higher level Vulkan wrappers.

Right, I'm not proposing that ash should manage Vulkan extensions. This simply makes it easier for "some hypothetical engines" to use this strategy for managing extensions that were merged into Vulkan core unchanged.

You'd load them yourself and call Device::from_parts_1_3.

Sure, but loading extensions is a problem ash has already solved for me. With just an extra mutable reference, I can do what I need to do and hand it down to the application without writing a bunch of code that already existed within ash.

@MarijnS95

No need to "take ownership" when the Fn structs are Cloneable. Together with #[inline] on the getters, the compiler might be able to "optimize" this in release when deconstructing an owned Device and fully recreating it.

That's a big "might". And yes there are more than 1 way to achieve this. I can also define a "layout-compatible MyDevice" and transmute the ash Device - not very safe but I certainly can.

But here's the thing. There's one easy, obvious, conventional way, and 10 tedious, hard, slow, "might", unsafe, or roundabout ways. And it would be great if ash could make life easier for people by allowing them to do things in the easy and obvious way.

Also note that we've previously had a contributor painstakingly clean up function ordering across manually-implemented wrappers, and this PR is adding the new fp_mut randomly ordered across impl blocks. We shouldn't undo their hard work.

Sure, if we can agree that this is OK I'll reorder where the fp_mut pointers are placed within the impl blocks.

Ralith commented 4 months ago

the NVIDIA docs did recommend loading only the patched functions from sl.interposer.dll

They also document that, if you do what I advised, then functions other than the patched ones will be gracefully passed through for you. You're performing error-prone reimplementation of logic that already exists in the upstream library, in the wrapped GetProcAddr functions.

Let's say I'm using another library that patches some other Vulkan function in the exact same way as Streamline.

Are you? This ad-hoc override scheme is not a recommended or Khronos-supported pattern for extending Vulkan probably for this exact poor composability. Consider the mess you'd have if both libs tried to override the same function, for example. Streamline is the first case I've heard of that does this and I doubt you'll find many others, if any.

With just an extra mutable reference, I can do what I need to do and hand it down to the application without writing a bunch of code that already existed within ash.

You don't need to write a bunch of code. If you refer to the docs, you'll see there's already public associated load fns. Janky and uncomposable as this use case is, it's fully supported by existing API.

Neo-Zhixing commented 3 months ago

I will close this PR for now. We can reopen this if other people also find it necessary and helpful to have mutable access to the function pointers.