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

VK_EXT_debug_marker should take structs via const pointers #513

Closed
dennis-hamester opened this issue Jun 16, 2017 · 6 comments
Closed

VK_EXT_debug_marker should take structs via const pointers #513

dennis-hamester opened this issue Jun 16, 2017 · 6 comments

Comments

@dennis-hamester
Copy link
Contributor

The following functions from VK_EXT_debug_marker take their structs via non-const pointers:

  • vkDebugMarkerSetObjectTagEXT
  • vkDebugMarkerSetObjectNameEXT
  • vkCmdDebugMarkerBeginEXT
  • vkCmdDebugMarkerInsertEXT

These should probably all be const pointers, since a Vulkan implementation is not supposed to write to any of them.

@oddhack
Copy link
Contributor

oddhack commented Jun 22, 2017

Apparently I can't assign this to @baldurk since he's not in KhronosGroup. Anyway, we will look at this inside Khronos.

@MarkY-LunarG
Copy link
Contributor

One thing I just discovered is the desktop Vulkan loader does translate VkPhysicalDevice and VkSurface handles in place inside vkDebugMarkerSetObjectTagEXT and vkDebugMarkerSetObjectNameEXT because the loader has to maintain its own information about each of those. I'm working on pushing up a change which will work with the new "const" pointers in case this happens, but I'm unsure of what would happen with older loaders. Either way, a newer loader will work as long as it's built after LVL MR !1900 has been integrated.

@dennis-hamester
Copy link
Contributor Author

I think it's totally fine to cast the const away for an implementation detail like this in the loader.

An older loader binary should still work, if the pointers are changed to const. Const/non-const shouldn't have any impact on code generation in C/C++, because of potential pointer aliasing.

Anyway, can I take this as a confirmation, that const is the correct choice here and it will be changed in a future spec revision?

@MarkY-LunarG
Copy link
Contributor

As @oddhack mentioned, we'll discuss it internally in Khronos. I just wanted to make a change which could have caused issues.

@oddhack oddhack assigned courtney-g and MarkY-LunarG and unassigned oddhack Jul 10, 2017
@MarkY-LunarG
Copy link
Contributor

After some discussion internally, Khronos is planning on making this change. There is a very low probability people will hit an issue with this change with an older loader. If they do, they can easily update the desktop loader to a newer version.

@oddhack
Copy link
Contributor

oddhack commented Jul 17, 2017

This should be fixed in the 1.0.55 spec update.

@oddhack oddhack closed this as completed Jul 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants