KhronosGroup / Vulkan-Ecosystem

Public repository for Vulkan Ecosystem issues
Apache License 2.0
132 stars 15 forks source link

Extension loader work #21

Closed nsubtil closed 3 years ago

nsubtil commented 6 years ago

As a result of the discussion in #4, it became clear that the current extension loader in the Vulkan-Docs repository needs some work. This issue tracks that work.

Copy-pasting a comment from @KarenGhavam-lunarG in #4 as a starting point for this issue:

What commands are directly exported by a Vulkan Implementation/Loader versus which ones do I have to use GIPA/GDPA for?

The loader exports the Vulkan core API entrypoints to simplify Vulkan development when an application links directly to the loader library. The Vulkan loader does not export extensions because extensions can "come and go" and by exporting extensions, you will cause versioning issues. The only exception to this was the WSI extensions. It is defined by the Vulkan spec (via the Normative Reference "Vulkan Loader Specification and Architecture Overview"), that the WSI extensions are to be exported by the loader.

The Normative Reference, "Vulkan Loader Specification and Architecture Overview" documents how to interface with the loader. It is found in the Loader and Validation Layer repo here (see the section on "Interfacing with Vulkan Functions"). This document is for Linux and Windows. For Android, you can reference Android-specific information as referenced earlier by @critsec in this issue (https://developer.android.com/ndk/guides/graphics/index.html)

There has been other discussions about creating a utility that would do the series of vkGetInstanceProcAddr and vkGetDeviceProcAddr calls for the application to determine the address of all functions and extensions available to the application (@mtavenrath started an implementation of such a utility as mentioned in github issue 2143).

One of the challenges with this utility was the problem of WSI extensions being exported twice (once by the utility, and once by the loader), causing conflicts between the utility and the loader (and hence why LunarG hasn't included it as part of the Vulkan SDK). The conflicts occur because the utility detects the WSI extensions as extensions and exports them and the loader also exports the WSI extensions (as defined earlier by the normative reference in the Vulkan specification). Here are two possible solutions to this:

  1. Hard code in the utility the WSI extensions and don't export them (they are documented in the platform's loader interface documentation mentioned earlier). This obviously has the disadvantage that if there were a change to the WSI extensions, the utility could get out of date. This risk is low because the actual liklihood of WSI extension changes is probably low.

  2. Modify the Vulkan header XML to flag the WSI extensions so that the utility can pro-grammatically detect that these extensions shouldn't be exported. A modification to the XML could be sent to the working group for consideration.

nsubtil commented 6 years ago

@mtavenrath, can you document what would be needed to move forward on this issue?

critsec commented 6 years ago

Note that it is not valid for an extension loader library to export any symbols that use unmodified Vulkan function names. It must use different names for them, e.g. a build-time configurable prefix or C++ namespace, or embed them in a client-provided instance of a struct. So there is a larger issue with the current extension loader than just clashes over WSI symbols, and solving that should also solve the WSI extension clash (and clashes with any other extensions), since the extension loader should not be using the same symbol names as the actual Vulkan loader.

The reason for this is that function pointers obtained by vkGetInstanceProcAddr are only valid for use with that instance. Storing them in process-global symbols (including in a singleton instance of a struct) means that other things in the process that use their own VkInstance will collide. This is a major problem for UI toolkits like the Android UI framework or various Linux UI toolkits, which are moving towards being implemented on Vulkan. They'll always use a private VkInstance, separate from any VkInstance(s) used directly by the application code.

Basically, anything that provides process-global Vulkan functions needs to be able to dispatch to a different function pointer based on the first parameter. The Vulkan loader does that. The extension loader does not, so it should not provide process-global symbols.

mtavenrath commented 6 years ago

I fully agree with you Jesse. The loader has been written before we defined the ABI and was based on a still existing bug in Vulkan, the fact that Vulkan declares ALL functions by default, resulting in confused developers on unresolved symbols. We really need to fix that issue.

Vulkan-Hpp now has an extension loader using a struct for the dispatch table and it supports multiple dispatch tables. The inconvenience here is that you have to pass along the dispatch table all over your code because you potentially have more than once instance. I'm okay with this limitation, but some people might argue that it's not perfect.

In an ideal world there'd be a way where one could fetch a fully dispatchable function pointer independent of the instance and the device. Every core symbol in the loader does know how to do that, but the loader is crippled not to be able to export those by the spec. If it'd be possible to fetch a fully dispatchable function pointer for each function an app could have a local static single function pointer for each function instead of keeping multiple dispatch tables and pass them along.

Additionally, we might want to define a second ABI for all the symbols which do not get exported so that libraries can share the exports. Looking at a single engine developer it's okay to have edvk* functions, but for an ecosystem with libraries and tutorials it feels strange that each and every developer might have a different function name for the Vulkan functions.

For the existing loader, I plan to deprecate it and replace it by the functionality I've implemented in Vulkan-Hpp (as C-header of course) to ensure that we don't have a spec violating extension loader anymore.

krOoze commented 6 years ago

The loader library could only import GIPA and do the rest on its own, no? Only thing that needs to be done would be to instruct the user not to statically link against the loader.

If it'd be possible to fetch a fully dispatchable function pointer for each function an app could have a local static single function pointer for each function instead of keeping multiple dispatch tables and pass them along.

Personally I do not see the difference. Either valid VkInstance, or a struct with fptrs have to be provided as a command parameter. Except the second should be more performant (no dispatch using a hashtable). In C++ it can be extra nice, because one can get rid of most of the fluff by using classes.

HansKristian-Work commented 6 years ago

Is it legal (or should it be legal) for an application to use vk*() names as long as those symbols are marked as "hidden" or the equivalent dynamic visibility?

critsec commented 6 years ago

From a practical point of view, as long as they'll only be used to satisfy unresolved symbols in the developer's own code, or code they control, it's okay. The thing to avoid is exposing the symbols in such a way that system libraries (e.g. UI toolkits) or other things the developer doesn't control end up using them instead of the ones those libraries expect to use.

nsubtil commented 6 years ago

I'm a bit worried about https://github.com/zeux/volk/issues/1 as it seems to highlight the kind of problems we're talking about here. The discussion in that issue is worth reading for a more real-world example of the trickiness around loading Vulkan entry points, and how it can lead developers to gloss over some of the spec restrictions for the sake of getting things done.

I'm not sure that relying on developers to control symbol visibility is necessarily a good solution either, especially once middleware gets involved. It's easy to get it wrong and not find out until later.

marty-johnson59 commented 3 years ago

This repository is being archived as it has been replaced with the vulkan.org website and is no longer being maintained (i.e., issues posted here are no longer being addressed ). After reviewing issues posted here, most (if not all) have been resolved or have already been re-opened in Vulkan-Docs (https://github.com/KhronosGroup/Vulkan-Docs) or other repositories for further consideration. Therefore, all issues in this repository will be closed. If you believe your issue has not yet been resolved, please re-open in Vulkan-Docs. Thanks!