Vulkan function loader feature might conflict with app's similar functionality

[kennyalive]

Version/Branch of Dear ImGui: Version: 1.87 WIP Branch: master

Renderer: imgui_impl_vulkan.cpp

Compiler: VS 2022

My concern: Crash when ImGui calls vulkan function when volk library is used to load function pointers.

~~ImGui defines vulkan function pointers using official names if VK_NO_PROTOTYPES is defined as part of custom function loader feature. For example, vkAllocateCommandBuffers name will be defined if VK_NO_PROTOTYPES is defined.~~

~~This might create a conflict with application's own vulkan function loader functionality. For example, volk library defines VK_NO_PROTOTYPES and it also defines function pointers using official names. vkAllocateCommandBuffers function will be defined both by the application and by the ImGui. This leads to violation of single definition rule and in my use case it leads to runtime crash.~~

~~For my use case I fixed the issue by disabling the part of ImGui code that defines/initialize function pointers. I'm not sure what's the best way to ensure that ImGui vulkan function loader coexist with app's similar functionality and if it's in the scope of ImGui library at all, so I'm writing this mostly with informative purpose. One possible solution is not to declare function pointers globally with official names or to put those pointers into a structure.~~

[ocornut]

The function pointers are defined static so they shouldn’t interfer with equivalent ones used by other compilation unit. Are you trying to include both this .cpp file and eg: volk headers in a same compilation unit ?

[kennyalive]

Here what I see in the debugger.

At first the function from my app is called

And later we get to ImGui initialization. The compiler does not try to dereference vkCreateSampler pointer but calls it directly.

vkCreateSampler pointer value is the same in both screenshots, so compiler does not try to use static vkCreateSampler from ImGui compilation unit. Could it be the problem that even if ImGui compilation unit uses static but the volk version of vkCreateSampler is regular global variable so it does not make any promises and the compiler is free to use it. I'm wondering if there is some shadowing rules for this situation, so 'static' hides other names? It smells like undefined behavior too, non-static variables leak form other modules and we get back to single definition rule violation.

UPDATE: I was wrong in my initial message that VK_NO_PROTOTYPES defined by volk is visible in ImGui compilation units - it does not. Sorry, it took me some time to figure out what's going on. ImGui code sees only vulkan/vulkan.h definitions and because VK_NO_PROTOTYPES is not defined it generates code for statically linked function call - using vkCreateSampler value directly without dereference. But it uses pointer of global variable (vkCreateSampler) which leaks from another compilation unit and violates single definition rule and we end up with code generated for static function that tries to call pointer to a function.

UPDATE2: Volk library also warns about scenario when both volk.h (app logic) and vulkan.h (imgui) are used. Here's a piece from volk documentation:

In summary I can not say that ImGui does something wrong, it's more like inconvenience you encounter when using custom loader. I remember that in the past I had the same problem with volk in my code when if forgot to replace vulkan.h with volk,h in some files. It's easy to fix when it's your own app but harder when you have to make changes in 3rd party code. Again for my personal use I fixed the problem, that's mostly if this will be the issue for other users.

[yelibumeng]

@kennyalive hi i get the same error，how to fix this question(also use volk), help ！！

[kennyalive]

https://github.com/kennyalive/vulkan-base/commit/65113bfffffb317b8c355b7abd13bf3ad361e03b

[yelibumeng]

thanks i will try it

[yelibumeng]

@kennyalive thanks!!!!!, it run now

[artyomd]

I just stumbled into this problem and I think I found a very simple solution. IMGUI_IMPL_VULKAN_NO_PROTOTYPES this definition enables VK_NO_PROTOTYPES for the vulkan.h include of imgui. (Obviously, you can add the VK_NO_PROTOTYPES define for the entire project, but it is a nice touch if you want something specific for imgui). After that, if you try to compile you will get a message like this

imgui-src/backends/imgui_impl_vulkan.cpp:1033: bool ImGui_ImplVulkan_Init(ImGui_ImplVulkan_InitInfo*, VkRenderPass): Assertion `g_FunctionsLoaded && "Need to call ImGui_ImplVulkan_LoadFunctions() if IMGUI_IMPL_VULKAN_NO_PROTOTYPES or VK_NO_PROTOTYPES are set!"' failed.

So I added a call to ImGui_ImplVulkan_LoadFunctions

  ImGui_ImplVulkan_LoadFunctions([](const char *function_name, void *vulkan_instance) {
    return vkGetInstanceProcAddr(*(reinterpret_cast<VkInstance *>(vulkan_instance)), function_name);
  }, &vulkan_instance_);

In this compilation unit, all the vulkan functionality is provided by volk (yes, I am using volk, too :) but this approach is applicable for all the loaders), so we can easily delegate all the loaded function to imgui, without changing anything in imgui sources. Hope this helps somebody, and pls let me know if I missed anything :raised_hands:

[zhivkob]

@artyomd Much more elegant than what I initially did, which was fiddle with the original implementation. Thanks for pointing me in the right direction!

[tim-rex]

The function pointers are defined static so they shouldn’t interfer with equivalent ones used by other compilation unit. Are you trying to include both this .cpp file and eg: volk headers in a same compilation unit ?

In my case, that is precisely what I'm doing and how I landed on this issue just now (unity/jumbo build). Though this approach seems to be creating more friction in other areas.. simple option for me is to revert to a regular build style at least where ImGui is concerned.

[Markyparky56]

To expand on @artyomd's solution, if you are using volk with "optimised device calls" (volkLoadInstanceOnly + volkLoadDevice) you might want to use vkGetDeviceProcAddr rather than vkGetInstanceProcAddr for all requested functions. Except vkGetDeviceProcAddr can't fetch certain functions that are instance-specific!

I found passing in a method to fetch both Instance and Device handles and fetching for both, then selecting the device proc address if available works well enough.

ex:

auto funcLoader = [](const char* funcName, void* engine) 
  {
    FCEngine* fcEngine = reinterpret_cast<FCEngine*>(engine);
    PFN_vkVoidFunction instanceAddr = vkGetInstanceProcAddr(fcEngine->GetVulkanInstance(), funcName);
    PFN_vkVoidFunction deviceAddr = vkGetDeviceProcAddr(fcEngine->GetVulkanDevice(), funcName);
    return deviceAddr ? deviceAddr : instanceAddr;
  };
const bool funcsLoaded = ImGui_ImplVulkan_LoadFunctions(funcLoader, this);

Perhaps there is a more elegant way, but since this is a one-and-done function with a very limited number of functions being requested, I'm happy.

[azer89]

I also got the same error because I volk did not vibe with imgui 🤔

access violation executing location when executing VkCreateSampler()

Fixed using the solution described by @artyomd

Thanks!

[ocornut]

@kennyalive, @adalsteinnh can you clarify why you cannot use the solutions highlighted above, aka using IMGUI_IMPL_VULKAN_NO_PROTOTYPE + calling ImGui_ImplVulkan_LoadFunctions() to make it use Volk?

It seems that this would work out of the box.

[ocornut]

I have pushed b720c0f which adds support for #define IMGUI_IMPL_VULKAN_USE_VOLK Both GLFW+Vulkan and SDL+Vulkan also now supports Volk optionally, which will make it easier to test forward.

See my reasoning in https://github.com/ocornut/imgui/pull/6582#issuecomment-2066582450 after why I didn't merge the IMGUI_IMPL_VULKAN_CUSTOM_HEADER version.

Thanks for your help!

[kennyalive]

can you clarify why you cannot use the solutions highlighted above, aka using IMGUI_IMPL_VULKAN_NO_PROTOTYPE + calling ImGui_ImplVulkan_LoadFunctions() to make it use Volk?

@ocornut I probably never tried it, since my ad-hoc solution worked for me. I updated ImGui with the latest code and IMGUI_IMPL_VULKAN_USE_VOLK option works great for me. Thank you for implementing it.

[renelindsay]

I'm using a custom loader called vulkan_wrapper.h/cpp, instead of volk, but they are very similar, so I'm suffering from the same issue. The problem is, custom loaders come with their own prototypes, which conflicts with the ones in vulkan.h. Luckily, the Vulkan team thought of this, and provided the VK_NO_PROTOTYPES macro, to disable the prototypes in vulkan.h.
Unfortunately, Dear ImGui now detects VK_NO_PROTOTYPES, and proceeds to re-declare these prototypes, which once again conflict with the ones in custom loaders! This makes it impossible to use custom loaders with ImGui, which I think is exactly the opposite of what the new IMGUI_IMPL_VULKAN_NO_PROTOTYPES feature was actually trying to accomplish. 🤦 The easiest solution is to simply roll back to the previous version of the Vulkan backend from imgui v1.80.

Of course this will be a problem for people who actually need features from future versions of the vulkan backend. In that case, they would have to manually delete or comment out the vulkan prototypes in imgui_impl_vulkan.cpp, delete the ImGui_ImplVulkan_LoadFunctions function, as well as the three IM_ASSERT calls that check if g_FunctionsLoaded == true. What a PITA!

If you really want to declare an additional set of prototypes in the imgui_impl_vulkan.cpp backend, at least put them in a namespace, or provide a way to disable them, so they don't conflict with the ones in custom loaders like volk or vulkan_wrapper.

[ocornut]

A PR for this would be good, as well as possibly in the PR please include an extra commit (which I won't merge but will keep aside) that include code for using another loader than Volk. Thank you!

[renelindsay]

When vulkan_wrapper is used, it defines VK_NO_PROTOTYPES, to prevent conflicts with vulkan.h. However, this triggers your loader to define prototypes as well, which reintroduces conflicts. To prevent this, you could check for IMGUI_IMPL_VULKAN_NO_PROTOTYPES instead. While pulling the latest imgui, I noticed you already mostly fixed this in release 1.90.6. As far as I can tell, Just one more change is needed:

In imgui_impl_vulkan.cpp, line 112, I suggest changing #if defined(VK_NO_PROTOTYPES) && !defined(VOLK_H_) to: #if defined(IMGUI_IMPL_VULKAN_NO_PROTOTYPES) && !defined(VOLK_H_)

I'll make a PR once I've done more thorough testing.