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

PFN_GetPhysicalDeviceProcAddr has C++ interface. #7

Closed
karl-lunarg opened this issue May 14, 2018 · 3 comments
Closed

PFN_GetPhysicalDeviceProcAddr has C++ interface. #7

karl-lunarg opened this issue May 14, 2018 · 3 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@karl-lunarg
Copy link
Contributor

Issue by stroyan (MIGRATED)
Tuesday Jan 09, 2018 at 16:29 GMT
Originally opened as KhronosGroup/Vulkan-LoaderAndValidationLayers#2330


When PFN_GetPhysicalDeviceProcAddr was added to vk_icd.h and vk_layer.h in commit b5f087a it was done without an extern "C" declaration.
The pfnGetPhysicalDeviceProcAddr member in struct VkNegotiateLayerInterface and the vk_layerGetPhysicalDeviceProcAddr functions in validation layers are all using C++ interfaces.
It will be awkward to change to a C binding now. But it will be troubling to use a C++ binding forever.

$ nm ~/Downloads/VulkanSDK/1.0.46.0/x86_64/lib/libVkLayer_threading.so | grep vk_layerGetPhysicalDeviceProcAddr
000000000001f2b0 T _Z33vk_layerGetPhysicalDeviceProcAddrP12VkInstance_TPKc

@karl-lunarg karl-lunarg added this to the P2 milestone May 14, 2018
@karl-lunarg karl-lunarg added the bug Something isn't working label May 14, 2018
@karl-lunarg
Copy link
Contributor Author

Comment by lenny-lunarg (MIGRATED)
Tuesday Jan 09, 2018 at 17:30 GMT


It looks like there's some weird stuff going on with these functions, in general. I think the missing extern "C" really belongs in the function declaration or definition. I wonder why it isn't a part of VK_LAYER_EXPORT already. vkGetInstanceProcAddr does not get mangled in the layers because it gets declared in vulkan.h, which is then included in threading.cpp. Without any changes to the repo, vkGetInstanceProcAddr does not get mangled:

$ nm libVkLayer_threading.so | grep vkGetInstanceProcAddr
0000000000033641 T vkGetInstanceProcAddr

But when I comment out the declaration in vulkan.h and rebuild, then the function does get mangled:

$ nm libVkLayer_threading.so | grep vkGetInstanceProcAddr
0000000000033651 T _Z21vkGetInstanceProcAddrP12VkInstance_TPKc

As a result, I don't think this has anything to do wih the declaration for PFN_vkGetInstanceProcAddr being wrapped by extern "C". In addition to this, without making any changes to the repo, we somehow wind up with both a mangled and un-mangled version of vkNegotiateLoaderLayerInterfaceVersion:

$ nm libVkLayer_threading.so | grep vkNegotiateLoaderLayerInterfaceVersion
000000000003368b T vkNegotiateLoaderLayerInterfaceVersion
00000000000a1f60 r _ZZ38vkNegotiateLoaderLayerInterfaceVersionE19__PRETTY_FUNCTION__

I have no explanation for why this happens.

The fact that vk_layerGetPhysicalDeviceProcAddr is getting mangled doesn't really matter, since it gets passed as a function pointer when the interface version is negotiated. That being said, we do need to make sure that any symbols that the loader searches for are not mangled (vkNegotiateLoaderLayerInterfaceVersion being the main one). It looks to me like the general solution would really be to add extern "C" to VK_LAYER_EXPORT when using a C++ compiler.

@karl-lunarg
Copy link
Contributor Author

Comment by stroyan (MIGRATED)
Thursday Jan 11, 2018 at 23:31 GMT


There are two different matters that the c++ name mangling brings into question.
Finding symbols by name is important. But the calling convention for C vs C++ may also matter.
Many implementations of C++ have a calling convention that is binary compatible with C for simple types.
But there is no guarantee of that. Both function typedefs and function declarations should be C for vulkan. I see many places where functions themselves have c++ declarations in the layers and mock icd.

The *PRETTY_FUNCTION symbols seem to be harmless. They are string constants that result from function name identifiers like PRETTY_FUNCTION
https://gcc.gnu.org/onlinedocs/gcc/Function-Names.html

@lenny-lunarg
Copy link
Contributor

I'm closing this as this doesn't apply to this repo. If people are interested in this, they can reopen the issue in the Vulkan-Headers repo. As for the immediate issue, a function pointer shouldn't need extern "C" and if you look at the vulkan header, it doesn't use them. At the very least we're being consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants