Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manager: fix debug layers on Android #341

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cebtenzzre
Copy link

I haven't tested this on Android, but on a fork that always uses the dynamic symbol lookup, I ran into this compilation error:

/home/admin/src_clones/gpt4all/gpt4all-backend/llama.cpp-mainline/kompute/src/Manager.cpp:273:36: error: no matching function for call to ‘vk::DispatchLoaderDynamic::init(std::__shared_ptr_access<vk::Instance, __gnu_cxx::_S_atomic, false, false>::element_type&, void (* (**)(VkInstance, const char*))())’
  273 |         this->mDebugDispatcher.init(*this->mInstance, &vkGetInstanceProcAddr);
<snip>
/home/admin/src_clones/gpt4all/gpt4all-backend/build/_deps/vulkan_header-src/include/vulkan/vulkan.hpp:13180:42: note:   no known conversion for argument 2 from ‘void (* (**)(VkInstance, const char*))()’ {aka ‘void (* (**)(VkInstance_T*, const char*))()’} to ‘PFN_vkGetInstanceProcAddr’ {aka ‘void (* (*)(VkInstance_T*, const char*))()’}

It should be possible to reproduce if you build it like this:

$ cmake -B build -DKOMPUTE_OPT_DISABLE_VK_DEBUG_LAYERS=OFF -DKOMPUTE_OPT_ANDROID_BUILD=ON
$ make -C build

@cebtenzzre
Copy link
Author

Actually this isn't enough, because it's crashing while trying to do vk::enumerateInstanceLayerProperties even though it hasn't initialized the dynamic loader yet.

@cebtenzzre cebtenzzre marked this pull request as draft November 6, 2023 23:55
@cebtenzzre cebtenzzre marked this pull request as ready for review November 6, 2023 23:58
@cebtenzzre cebtenzzre changed the title Manager: fix build with debug layers on Android Manager: fix debug layers on Android Nov 6, 2023
@axsaucedo
Copy link
Member

Thank you for submitting - would you be able to test it in the android example? The reason why i ask is because that uses a very particular version of the vulkan libraries for compatibility so it would need to be validated

Signed-off-by: Jared Van Bortel <jared@nomic.ai>
Signed-off-by: Jared Van Bortel <jared@nomic.ai>
@cebtenzzre
Copy link
Author

would you be able to test it in the android example?

Sorry, I don't have the time to attempt something like that. Since the existing code doesn't compile with the latest regular Vulkan headers, I don't see how this PR could be worse. Unless there's some version of Vulkan where vk::DispatchLoaderDynamic::init takes a pointer to a pointer to a function?

@axsaucedo
Copy link
Member

Thank you for the heads up - no worries, we'll have to test it before merging it, we'll test it on our side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants